Add usdt support for ARM64 #1414

yonghong-song · 2017-10-26T18:38:49Z

Specifically the following argument patterns will be
supported:

   [-]<size>@<value>
   [-]<size>@<reg>
   [-]<size>@[<reg>]
   [-]<size>@[<reg>,<offset>]

I did not use std regex library as the old gcc (e.g. 4.8.5)
does not have a good support for it.

Signed-off-by: Yonghong Song yhs@fb.com

yonghong-song · 2017-10-26T18:39:11Z

cc @palmtenor

4ast · 2017-10-26T21:48:02Z

tests/cc/test_usdt_args.cc

@@ -113,7 +121,7 @@ TEST_CASE("test usdt argument parsing", "[usdt]") {
    verify_register(parser, 2, 1097);
    verify_register(parser, 4, "gpr[30]", 108);
    verify_register(parser, -2, "gpr[31]", -4);
-#elif defined(__x86_64__)


why remove defined(x86_64) ?
Other than this nit it all looks good to me.
cc @goldshtn

This is for consistency. In all other places, we have x86_64 as the default. So I changed a couple of places here to have x86_64 as the default.

The x86_64 as the default in the core source files makes build easier for not-fully-supported architectures. Although some tests/tools may not work as expected, at least build can go through without change.

palmtenor · 2017-10-26T22:06:39Z

tests/cc/test_usdt_args.cc

@@ -54,9 +54,11 @@ static void verify_register(USDT::ArgumentParser &parser, int arg_size,

 TEST_CASE("test usdt argument parsing", "[usdt]") {
  SECTION("parse failure") {
-#ifdef __powerpc64__
+#ifdef __aarch64__


Theoritically these tests doesn't rely on architecture. We just put in fake argument format strings, call specific parser, and see if it outputs correct results. It would be nice to always test all architectures all the time, right?

Okay. I will add the arch guard back to the test.

Theoritically these tests doesn't rely on architecture. We just put in fake argument format strings, call specific parser, and see if it outputs correct results. It would be nice to always test all architectures all the time, right?

The test input needs to be different for different architecture. That is why we have different "ifdef"s.

It feels to me that, say, the code ArgumentParser_aarch64 itself, can also run on x86, is that right?

In that case, we can always invoke ArgumentParser_aarch64 on arm64 test inputs, USDT::ArgumentParser_powerpc64 on Power PC inputs, etc.?

Otherwise, since our testbot only runs on x86, we won't be able to catch regressions and breakage for other architectures at PR time?

I still prefer that it is guarded by different architecture. The reason is usdt parsing code is arch specific. Even if we run some other arch inputs on x86, this still won't catch regression
on powerpc since powerpc parsing code is not exercised on x86.

There is a chance that illegal input in one arch is legal in a different one.
For example 4@4 is illegal on x86 but it is legal in aarch64. Mixing together
will prevent put 4@4 in negative test since it is legal in aarch64.

After the second thought, I know what you mean. Yes, we could exercise non x86 parsing code on x86 host. I will make a revision to reflect that.

Sorry, I cannot make the change because powerpc64 uses the regex library which old compiler likes gcc 4.8.5 does not have a good support (as I mentioned in my commit message as well).

....
REQUIRE( !parser.parse(&arg) )
due to unexpected exception with message:
regex_error
....

Therefore, let us stick to arch. specific usdt parser tests for now. Supposedly, each arch parser change should have run this test on their respective arch to catch all possible errors.

Specifically the following argument patterns will be supported: ``` [-]<size>@<value> [-]<size>@<reg> [-]<size>@[<reg>] [-]<size>@[<reg>,<offset>] ``` I did not use std regex library as the old gcc (e.g. 4.8.5) does not have a good support for it. Signed-off-by: Yonghong Song <yhs@fb.com>

yonghong-song · 2017-10-26T22:31:34Z

updated the patch to add "#elif x86_64" back to test_usdt_args.c.

yonghong-song · 2017-10-26T22:43:07Z

Just a little more information. The following is the output of
readelf -n /lib64/libc.so.6

......
  stapsdt              0x00000048       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: memory_mallopt_arena_max
    Location: 0x000000000007e720, Base: 0x00000000001366b8, Semaphore: 0x0000000000000000
    Arguments: -4@x20 8@[x22,32]
  stapsdt              0x00000054       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: memory_mallopt_mmap_max
    Location: 0x000000000007e740, Base: 0x00000000001366b8, Semaphore: 0x0000000000000000
    Arguments: -4@x20 -4@[x22,44] -4@[x22,52]
  stapsdt              0x00000037       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: memory_malloc_retry
    Location: 0x000000000007ec18, Base: 0x00000000001366b8, Semaphore: 0x0000000000000000
    Arguments: 8@x20
  stapsdt              0x0000003e       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: memory_realloc_retry
    Location: 0x000000000007efa0, Base: 0x00000000001366b8, Semaphore: 0x0000000000000000
    Arguments: 8@x23 8@x19
  stapsdt              0x0000003f       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: memory_memalign_retry
    Location: 0x000000000007f120, Base: 0x00000000001366b8, Semaphore: 0x0000000000000000
    Arguments: 8@x21 8@x20
  stapsdt              0x00000037       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: memory_valloc_retry
    Location: 0x000000000007f7cc, Base: 0x00000000001366b8, Semaphore: 0x0000000000000000
    Arguments: 8@x20
  stapsdt              0x00000038       NT_STAPSDT (SystemTap probe descriptors)
    Provider: libc
    Name: memory_pvalloc_retry
    Location: 0x000000000007f9d8, Base: 0x00000000001366b8, Semaphore: 0x0000000000000000
    Arguments: 8@x21
......

palmtenor

That's unfortunate:( Makes sense to keep it how it is now, we can move this later when we have higher version of GCC

yonghong-song requested review from 4ast and drzaeus77 as code owners October 26, 2017 18:38

4ast reviewed Oct 26, 2017

View reviewed changes

4ast approved these changes Oct 26, 2017

View reviewed changes

palmtenor reviewed Oct 26, 2017

View reviewed changes

yonghong-song force-pushed the yhs_dev branch from 5bb3b91 to c2d9880 Compare October 26, 2017 22:30

palmtenor approved these changes Oct 27, 2017

View reviewed changes

4ast merged commit 740c407 into master Oct 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add usdt support for ARM64 #1414

Add usdt support for ARM64 #1414

yonghong-song commented Oct 26, 2017 •

edited

Loading

yonghong-song commented Oct 26, 2017 •

edited

Loading

4ast Oct 26, 2017

yonghong-song Oct 26, 2017

palmtenor Oct 26, 2017 •

edited

Loading

yonghong-song Oct 26, 2017

yonghong-song Oct 26, 2017

palmtenor Oct 27, 2017

yonghong-song Oct 27, 2017

yonghong-song Oct 27, 2017

yonghong-song Oct 27, 2017

yonghong-song commented Oct 26, 2017

yonghong-song commented Oct 26, 2017

palmtenor left a comment

Add usdt support for ARM64 #1414

Add usdt support for ARM64 #1414

Conversation

yonghong-song commented Oct 26, 2017 • edited Loading

yonghong-song commented Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palmtenor Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonghong-song commented Oct 26, 2017

yonghong-song commented Oct 26, 2017

palmtenor left a comment

Choose a reason for hiding this comment

yonghong-song commented Oct 26, 2017 •

edited

Loading

yonghong-song commented Oct 26, 2017 •

edited

Loading

palmtenor Oct 26, 2017 •

edited

Loading