The ubsan (undefined behavior sanitizer) version of perf throws an
error on the 'x86 instruction decoder - new instructions' function
of perf test.
To reproduce this run:
make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
then run: tools/perf/perf test 62 -v
The error occurs in the __get_next macro (line 34) where an int is
read from a potentially unaligned address. Using memcpy instead of
assignment from an unaligned pointer.
Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
---
tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
index ca983e2bea8b..de1944c60aa9 100644
--- a/tools/perf/util/intel-pt-decoder/insn.c
+++ b/tools/perf/util/intel-pt-decoder/insn.c
@@ -31,7 +31,8 @@
((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
#define __get_next(t, insn) \
- ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
+ ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
+ insn->next_byte += sizeof(t); r; })
#define __peek_nbyte_next(t, insn, n) \
({ t r = *(t*)((insn)->next_byte + n); r; })
--
2.22.0.657.g960e92d24f-goog
From: Numfor Mbiziwo-Tiapo
> Sent: 24 July 2019 19:45
>
> The ubsan (undefined behavior sanitizer) version of perf throws an
> error on the 'x86 instruction decoder - new instructions' function
> of perf test.
>
> To reproduce this run:
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
>
> then run: tools/perf/perf test 62 -v
>
> The error occurs in the __get_next macro (line 34) where an int is
> read from a potentially unaligned address. Using memcpy instead of
> assignment from an unaligned pointer.
...
> #define __get_next(t, insn) \
> - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> + insn->next_byte += sizeof(t); r; })
Isn't there a get_unaligned_u32() (or similar) that can be used?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Thu, Jul 25, 2019 at 6:06 AM David Laight <[email protected]> wrote:
>
> From: Numfor Mbiziwo-Tiapo
> > Sent: 24 July 2019 19:45
> >
> > The ubsan (undefined behavior sanitizer) version of perf throws an
> > error on the 'x86 instruction decoder - new instructions' function
> > of perf test.
> >
> > To reproduce this run:
> > make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >
> > then run: tools/perf/perf test 62 -v
> >
> > The error occurs in the __get_next macro (line 34) where an int is
> > read from a potentially unaligned address. Using memcpy instead of
> > assignment from an unaligned pointer.
> ...
> > #define __get_next(t, insn) \
> > - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> > + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> > + insn->next_byte += sizeof(t); r; })
>
> Isn't there a get_unaligned_u32() (or similar) that can be used?
memcpy is a compiler intrinsic. get_unaligned_u32 would mean either a
'if (sizeof(t) == sizeof(u32)) get_unaligned_u32(.. ' for all sizes or
changing all call sites of __get_next. Numfor's change feels right as
it is the least invasive.
Thanks,
Ian Rogers
(resent to make plain text)
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> The ubsan (undefined behavior sanitizer) version of perf throws an
> error on the 'x86 instruction decoder - new instructions' function
> of perf test.
>
> To reproduce this run:
> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
>
> then run: tools/perf/perf test 62 -v
>
> The error occurs in the __get_next macro (line 34) where an int is
> read from a potentially unaligned address. Using memcpy instead of
> assignment from an unaligned pointer.
Since this came from the kernel, don't we have to fix it there as well?
Masami, Adrian?
[acme@quaco perf]$ find . -name insn.c
./arch/x86/lib/insn.c
./arch/arm/kernel/insn.c
./arch/arm64/kernel/insn.c
./tools/objtool/arch/x86/lib/insn.c
./tools/perf/util/intel-pt-decoder/insn.c
[acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
--- ./tools/perf/util/intel-pt-decoder/insn.c 2019-07-06 16:59:05.734265998 -0300
+++ ./arch/x86/lib/insn.c 2019-07-06 16:59:01.369202998 -0300
@@ -10,8 +10,8 @@
#else
#include <string.h>
#endif
-#include "inat.h"
-#include "insn.h"
+#include <asm/inat.h>
+#include <asm/insn.h>
/* Verify next sizeof(t) bytes can be on the same instruction */
#define validate_next(t, insn, n) \
[acme@quaco perf]$
- Arnaldo
> Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> ---
> tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
> index ca983e2bea8b..de1944c60aa9 100644
> --- a/tools/perf/util/intel-pt-decoder/insn.c
> +++ b/tools/perf/util/intel-pt-decoder/insn.c
> @@ -31,7 +31,8 @@
> ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>
> #define __get_next(t, insn) \
> - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> + insn->next_byte += sizeof(t); r; })
>
> #define __peek_nbyte_next(t, insn, n) \
> ({ t r = *(t*)((insn)->next_byte + n); r; })
> --
> 2.22.0.657.g960e92d24f-goog
--
- Arnaldo
On Fri, 26 Jul 2019 16:38:06 -0300
Arnaldo Carvalho de Melo <[email protected]> wrote:
> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> > The ubsan (undefined behavior sanitizer) version of perf throws an
> > error on the 'x86 instruction decoder - new instructions' function
> > of perf test.
> >
> > To reproduce this run:
> > make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >
> > then run: tools/perf/perf test 62 -v
> >
> > The error occurs in the __get_next macro (line 34) where an int is
> > read from a potentially unaligned address. Using memcpy instead of
> > assignment from an unaligned pointer.
>
> Since this came from the kernel, don't we have to fix it there as well?
> Masami, Adrian?
I guess we don't need it, since x86 can access "unaligned address" and
x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
that part. Maybe if we run it on other arch as cross-arch application,
it may cause unaligned pointer issue.
Thank you,
>
> [acme@quaco perf]$ find . -name insn.c
> ./arch/x86/lib/insn.c
> ./arch/arm/kernel/insn.c
> ./arch/arm64/kernel/insn.c
> ./tools/objtool/arch/x86/lib/insn.c
> ./tools/perf/util/intel-pt-decoder/insn.c
> [acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
> --- ./tools/perf/util/intel-pt-decoder/insn.c 2019-07-06 16:59:05.734265998 -0300
> +++ ./arch/x86/lib/insn.c 2019-07-06 16:59:01.369202998 -0300
> @@ -10,8 +10,8 @@
> #else
> #include <string.h>
> #endif
> -#include "inat.h"
> -#include "insn.h"
> +#include <asm/inat.h>
> +#include <asm/insn.h>
>
> /* Verify next sizeof(t) bytes can be on the same instruction */
> #define validate_next(t, insn, n) \
> [acme@quaco perf]$
>
>
> - Arnaldo
>
> > Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> > ---
> > tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
> > index ca983e2bea8b..de1944c60aa9 100644
> > --- a/tools/perf/util/intel-pt-decoder/insn.c
> > +++ b/tools/perf/util/intel-pt-decoder/insn.c
> > @@ -31,7 +31,8 @@
> > ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
> >
> > #define __get_next(t, insn) \
> > - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> > + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> > + insn->next_byte += sizeof(t); r; })
> >
> > #define __peek_nbyte_next(t, insn, n) \
> > ({ t r = *(t*)((insn)->next_byte + n); r; })
> > --
> > 2.22.0.657.g960e92d24f-goog
>
> --
>
> - Arnaldo
--
Masami Hiramatsu <[email protected]>
On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
> On Fri, 26 Jul 2019 16:38:06 -0300
> Arnaldo Carvalho de Melo <[email protected]> wrote:
>
>> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
>>> The ubsan (undefined behavior sanitizer) version of perf throws an
>>> error on the 'x86 instruction decoder - new instructions' function
>>> of perf test.
>>>
>>> To reproduce this run:
>>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
>>>
>>> then run: tools/perf/perf test 62 -v
>>>
>>> The error occurs in the __get_next macro (line 34) where an int is
>>> read from a potentially unaligned address. Using memcpy instead of
>>> assignment from an unaligned pointer.
>>
>> Since this came from the kernel, don't we have to fix it there as well?
>> Masami, Adrian?
>
> I guess we don't need it, since x86 can access "unaligned address" and
> x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
> that part. Maybe if we run it on other arch as cross-arch application,
> it may cause unaligned pointer issue.
Yes, theoretically Intel PT decoding can be done on any arch.
But the memcpy is probably sub-optimal for x86, so the patch as it stands
does not seem suitable. I notice the kernel has get_unaligned() and
put_unaligned().
Obviously it would be better for a patch to be accepted to
arch/x86/lib/insn.c also.
>
> Thank you,
>
>>
>> [acme@quaco perf]$ find . -name insn.c
>> ./arch/x86/lib/insn.c
>> ./arch/arm/kernel/insn.c
>> ./arch/arm64/kernel/insn.c
>> ./tools/objtool/arch/x86/lib/insn.c
>> ./tools/perf/util/intel-pt-decoder/insn.c
>> [acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
>> --- ./tools/perf/util/intel-pt-decoder/insn.c 2019-07-06 16:59:05.734265998 -0300
>> +++ ./arch/x86/lib/insn.c 2019-07-06 16:59:01.369202998 -0300
>> @@ -10,8 +10,8 @@
>> #else
>> #include <string.h>
>> #endif
>> -#include "inat.h"
>> -#include "insn.h"
>> +#include <asm/inat.h>
>> +#include <asm/insn.h>
>>
>> /* Verify next sizeof(t) bytes can be on the same instruction */
>> #define validate_next(t, insn, n) \
>> [acme@quaco perf]$
>>
>>
>> - Arnaldo
>>
>>> Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
>>> ---
>>> tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
>>> index ca983e2bea8b..de1944c60aa9 100644
>>> --- a/tools/perf/util/intel-pt-decoder/insn.c
>>> +++ b/tools/perf/util/intel-pt-decoder/insn.c
>>> @@ -31,7 +31,8 @@
>>> ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
>>>
>>> #define __get_next(t, insn) \
>>> - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
>>> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
>>> + insn->next_byte += sizeof(t); r; })
>>>
>>> #define __peek_nbyte_next(t, insn, n) \
>>> ({ t r = *(t*)((insn)->next_byte + n); r; })
>>> --
>>> 2.22.0.657.g960e92d24f-goog
>>
>> --
>>
>> - Arnaldo
>
>
On Mon, Jul 29, 2019 at 1:24 AM Adrian Hunter <[email protected]> wrote:
>
> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
> > On Fri, 26 Jul 2019 16:38:06 -0300
> > Arnaldo Carvalho de Melo <[email protected]> wrote:
> >
> >> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> >>> The ubsan (undefined behavior sanitizer) version of perf throws an
> >>> error on the 'x86 instruction decoder - new instructions' function
> >>> of perf test.
> >>>
> >>> To reproduce this run:
> >>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >>>
> >>> then run: tools/perf/perf test 62 -v
> >>>
> >>> The error occurs in the __get_next macro (line 34) where an int is
> >>> read from a potentially unaligned address. Using memcpy instead of
> >>> assignment from an unaligned pointer.
> >>
> >> Since this came from the kernel, don't we have to fix it there as well?
> >> Masami, Adrian?
> >
> > I guess we don't need it, since x86 can access "unaligned address" and
> > x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
> > that part. Maybe if we run it on other arch as cross-arch application,
> > it may cause unaligned pointer issue.
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
"A pointer to an object or incomplete type may be converted to a
pointer to a different object or incomplete type. If the resulting
pointer is not correctly aligned for the pointed-to type, the behavior
is undefined."
I agree the code will generally run on x86.
> Yes, theoretically Intel PT decoding can be done on any arch.
>
> But the memcpy is probably sub-optimal for x86, so the patch as it stands
> does not seem suitable. I notice the kernel has get_unaligned() and
> put_unaligned().
Why is a fixed sized memcpy suboptimal? The compiler can should turn
into a load.
Thanks,
Ian
> Obviously it would be better for a patch to be accepted to
> arch/x86/lib/insn.c also.
>
> >
> > Thank you,
> >
> >>
> >> [acme@quaco perf]$ find . -name insn.c
> >> ./arch/x86/lib/insn.c
> >> ./arch/arm/kernel/insn.c
> >> ./arch/arm64/kernel/insn.c
> >> ./tools/objtool/arch/x86/lib/insn.c
> >> ./tools/perf/util/intel-pt-decoder/insn.c
> >> [acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
> >> --- ./tools/perf/util/intel-pt-decoder/insn.c 2019-07-06 16:59:05.734265998 -0300
> >> +++ ./arch/x86/lib/insn.c 2019-07-06 16:59:01.369202998 -0300
> >> @@ -10,8 +10,8 @@
> >> #else
> >> #include <string.h>
> >> #endif
> >> -#include "inat.h"
> >> -#include "insn.h"
> >> +#include <asm/inat.h>
> >> +#include <asm/insn.h>
> >>
> >> /* Verify next sizeof(t) bytes can be on the same instruction */
> >> #define validate_next(t, insn, n) \
> >> [acme@quaco perf]$
> >>
> >>
> >> - Arnaldo
> >>
> >>> Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> >>> ---
> >>> tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
> >>> index ca983e2bea8b..de1944c60aa9 100644
> >>> --- a/tools/perf/util/intel-pt-decoder/insn.c
> >>> +++ b/tools/perf/util/intel-pt-decoder/insn.c
> >>> @@ -31,7 +31,8 @@
> >>> ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
> >>>
> >>> #define __get_next(t, insn) \
> >>> - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> >>> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> >>> + insn->next_byte += sizeof(t); r; })
> >>>
> >>> #define __peek_nbyte_next(t, insn, n) \
> >>> ({ t r = *(t*)((insn)->next_byte + n); r; })
> >>> --
> >>> 2.22.0.657.g960e92d24f-goog
> >>
> >> --
> >>
> >> - Arnaldo
> >
> >
>
On Mon, 29 Jul 2019 11:22:34 +0300
Adrian Hunter <[email protected]> wrote:
> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
> > On Fri, 26 Jul 2019 16:38:06 -0300
> > Arnaldo Carvalho de Melo <[email protected]> wrote:
> >
> >> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> >>> The ubsan (undefined behavior sanitizer) version of perf throws an
> >>> error on the 'x86 instruction decoder - new instructions' function
> >>> of perf test.
> >>>
> >>> To reproduce this run:
> >>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >>>
> >>> then run: tools/perf/perf test 62 -v
> >>>
> >>> The error occurs in the __get_next macro (line 34) where an int is
> >>> read from a potentially unaligned address. Using memcpy instead of
> >>> assignment from an unaligned pointer.
> >>
> >> Since this came from the kernel, don't we have to fix it there as well?
> >> Masami, Adrian?
> >
> > I guess we don't need it, since x86 can access "unaligned address" and
> > x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
> > that part. Maybe if we run it on other arch as cross-arch application,
> > it may cause unaligned pointer issue.
>
> Yes, theoretically Intel PT decoding can be done on any arch.
>
> But the memcpy is probably sub-optimal for x86, so the patch as it stands
> does not seem suitable. I notice the kernel has get_unaligned() and
> put_unaligned().
>
> Obviously it would be better for a patch to be accepted to
> arch/x86/lib/insn.c also.
Hmm, then I rather like memcpy() for arch/x86/lib/insn.c, because it runs only
on x86.
Thank you,
>
> >
> > Thank you,
> >
> >>
> >> [acme@quaco perf]$ find . -name insn.c
> >> ./arch/x86/lib/insn.c
> >> ./arch/arm/kernel/insn.c
> >> ./arch/arm64/kernel/insn.c
> >> ./tools/objtool/arch/x86/lib/insn.c
> >> ./tools/perf/util/intel-pt-decoder/insn.c
> >> [acme@quaco perf]$ diff -u ./tools/perf/util/intel-pt-decoder/insn.c ./arch/x86/lib/insn.c
> >> --- ./tools/perf/util/intel-pt-decoder/insn.c 2019-07-06 16:59:05.734265998 -0300
> >> +++ ./arch/x86/lib/insn.c 2019-07-06 16:59:01.369202998 -0300
> >> @@ -10,8 +10,8 @@
> >> #else
> >> #include <string.h>
> >> #endif
> >> -#include "inat.h"
> >> -#include "insn.h"
> >> +#include <asm/inat.h>
> >> +#include <asm/insn.h>
> >>
> >> /* Verify next sizeof(t) bytes can be on the same instruction */
> >> #define validate_next(t, insn, n) \
> >> [acme@quaco perf]$
> >>
> >>
> >> - Arnaldo
> >>
> >>> Signed-off-by: Numfor Mbiziwo-Tiapo <[email protected]>
> >>> ---
> >>> tools/perf/util/intel-pt-decoder/insn.c | 3 ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/tools/perf/util/intel-pt-decoder/insn.c b/tools/perf/util/intel-pt-decoder/insn.c
> >>> index ca983e2bea8b..de1944c60aa9 100644
> >>> --- a/tools/perf/util/intel-pt-decoder/insn.c
> >>> +++ b/tools/perf/util/intel-pt-decoder/insn.c
> >>> @@ -31,7 +31,8 @@
> >>> ((insn)->next_byte + sizeof(t) + n <= (insn)->end_kaddr)
> >>>
> >>> #define __get_next(t, insn) \
> >>> - ({ t r = *(t*)insn->next_byte; insn->next_byte += sizeof(t); r; })
> >>> + ({ t r; memcpy(&r, insn->next_byte, sizeof(t)); \
> >>> + insn->next_byte += sizeof(t); r; })
> >>>
> >>> #define __peek_nbyte_next(t, insn, n) \
> >>> ({ t r = *(t*)((insn)->next_byte + n); r; })
> >>> --
> >>> 2.22.0.657.g960e92d24f-goog
> >>
> >> --
> >>
> >> - Arnaldo
> >
> >
>
--
Masami Hiramatsu <[email protected]>
On 29/07/19 10:32 PM, Ian Rogers wrote:
> On Mon, Jul 29, 2019 at 1:24 AM Adrian Hunter <[email protected]> wrote:
>>
>> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
>>> On Fri, 26 Jul 2019 16:38:06 -0300
>>> Arnaldo Carvalho de Melo <[email protected]> wrote:
>>>
>>>> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
>>>>> The ubsan (undefined behavior sanitizer) version of perf throws an
>>>>> error on the 'x86 instruction decoder - new instructions' function
>>>>> of perf test.
>>>>>
>>>>> To reproduce this run:
>>>>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
>>>>>
>>>>> then run: tools/perf/perf test 62 -v
>>>>>
>>>>> The error occurs in the __get_next macro (line 34) where an int is
>>>>> read from a potentially unaligned address. Using memcpy instead of
>>>>> assignment from an unaligned pointer.
>>>>
>>>> Since this came from the kernel, don't we have to fix it there as well?
>>>> Masami, Adrian?
>>>
>>> I guess we don't need it, since x86 can access "unaligned address" and
>>> x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
>>> that part. Maybe if we run it on other arch as cross-arch application,
>>> it may cause unaligned pointer issue.
>
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
> "A pointer to an object or incomplete type may be converted to a
> pointer to a different object or incomplete type. If the resulting
> pointer is not correctly aligned for the pointed-to type, the behavior
> is undefined."
> I agree the code will generally run on x86.
>
>> Yes, theoretically Intel PT decoding can be done on any arch.
>>
>> But the memcpy is probably sub-optimal for x86, so the patch as it stands
>> does not seem suitable. I notice the kernel has get_unaligned() and
>> put_unaligned().
>
> Why is a fixed sized memcpy suboptimal? The compiler can should turn
> into a load.
True, I didn't click it was fixed size.
From: Adrian Hunter
> Sent: 30 July 2019 08:53
> On 30/07/19 3:47 AM, Masami Hiramatsu wrote:
> > On Mon, 29 Jul 2019 11:22:34 +0300
> > Adrian Hunter <[email protected]> wrote:
> >
> >> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
> >>> On Fri, 26 Jul 2019 16:38:06 -0300
> >>> Arnaldo Carvalho de Melo <[email protected]> wrote:
> >>>
> >>>> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
> >>>>> The ubsan (undefined behavior sanitizer) version of perf throws an
> >>>>> error on the 'x86 instruction decoder - new instructions' function
> >>>>> of perf test.
> >>>>>
> >>>>> To reproduce this run:
> >>>>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
> >>>>>
> >>>>> then run: tools/perf/perf test 62 -v
> >>>>>
> >>>>> The error occurs in the __get_next macro (line 34) where an int is
> >>>>> read from a potentially unaligned address. Using memcpy instead of
> >>>>> assignment from an unaligned pointer.
> >>>>
> >>>> Since this came from the kernel, don't we have to fix it there as well?
> >>>> Masami, Adrian?
> >>>
> >>> I guess we don't need it, since x86 can access "unaligned address" and
> >>> x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
> >>> that part. Maybe if we run it on other arch as cross-arch application,
> >>> it may cause unaligned pointer issue.
> >>
> >> Yes, theoretically Intel PT decoding can be done on any arch.
> >>
> >> But the memcpy is probably sub-optimal for x86, so the patch as it stands
> >> does not seem suitable. I notice the kernel has get_unaligned() and
> >> put_unaligned().
> >>
> >> Obviously it would be better for a patch to be accepted to
> >> arch/x86/lib/insn.c also.
> >
> > Hmm, then I rather like memcpy() for arch/x86/lib/insn.c, because it runs only
> > on x86.
>
> Yes, I was wrong about memcpy, and it is simpler for perf tools than
> dragging out get_unaligned().
It may well make the generated code worse because some optimisations
won't happen because they would need to be done before memcpy()
gets inlined.
I've certainly seen cases where a #define generates significantly
better code than an inline function.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 30/07/19 3:47 AM, Masami Hiramatsu wrote:
> On Mon, 29 Jul 2019 11:22:34 +0300
> Adrian Hunter <[email protected]> wrote:
>
>> On 27/07/19 12:46 PM, Masami Hiramatsu wrote:
>>> On Fri, 26 Jul 2019 16:38:06 -0300
>>> Arnaldo Carvalho de Melo <[email protected]> wrote:
>>>
>>>> Em Wed, Jul 24, 2019 at 11:45:12AM -0700, Numfor Mbiziwo-Tiapo escreveu:
>>>>> The ubsan (undefined behavior sanitizer) version of perf throws an
>>>>> error on the 'x86 instruction decoder - new instructions' function
>>>>> of perf test.
>>>>>
>>>>> To reproduce this run:
>>>>> make -C tools/perf USE_CLANG=1 EXTRA_CFLAGS="-fsanitize=undefined"
>>>>>
>>>>> then run: tools/perf/perf test 62 -v
>>>>>
>>>>> The error occurs in the __get_next macro (line 34) where an int is
>>>>> read from a potentially unaligned address. Using memcpy instead of
>>>>> assignment from an unaligned pointer.
>>>>
>>>> Since this came from the kernel, don't we have to fix it there as well?
>>>> Masami, Adrian?
>>>
>>> I guess we don't need it, since x86 can access "unaligned address" and
>>> x86 insn decoder in kernel runs only on x86. I'm not sure about perf's
>>> that part. Maybe if we run it on other arch as cross-arch application,
>>> it may cause unaligned pointer issue.
>>
>> Yes, theoretically Intel PT decoding can be done on any arch.
>>
>> But the memcpy is probably sub-optimal for x86, so the patch as it stands
>> does not seem suitable. I notice the kernel has get_unaligned() and
>> put_unaligned().
>>
>> Obviously it would be better for a patch to be accepted to
>> arch/x86/lib/insn.c also.
>
> Hmm, then I rather like memcpy() for arch/x86/lib/insn.c, because it runs only
> on x86.
Yes, I was wrong about memcpy, and it is simpler for perf tools than
dragging out get_unaligned().