Do not reference PTRACE_{G,S}ETFPXREGS when undefined

[umarcor]

Issue: #3381
Issue: #3399

Signed-off-by: Unai Martinez-Corral [email protected]

---

Building on armv7l or armv8l fails with:

[ 64%] Building C object core/CMakeFiles/drinjectlib.dir/config.c.o
/dynamorio/core/unix/injector.c:866:50: error: 'PTRACE_GETFPXREGS' undeclared here (not in a function)
                                                { PTRACE_GETFPXREGS, "PTRACE_GETFPXREGS" },
                                                  ^~~~~~~~~~~~~~~~~
/dynamorio/core/unix/injector.c:867:50: error: 'PTRACE_SETFPXREGS' undeclared here (not in a function)
                                                { PTRACE_SETFPXREGS, "PTRACE_SETFPXREGS" },
                                                  ^~~~~~~~~~~~~~~~~
make[2]: *** [core/CMakeFiles/drinjectlib.dir/unix/injector.c.o] Error 1

This PR avoids using PTRACE_GETFPXREGS and PTRACE_SETFPXREGS on ARM, as it is currently done on AARCH64.

[derekbruening]

Certainly our ARM build on Travis works fine with the ptrace defines as they are, so this seems more like a toolchain version issue. Please specify which toolchain needs this change.

[umarcor]

I am not sure about how to do that. I found the problem on Ubuntu 18.04 (apt install -y cmake g++-6 gcc).

[egrimley]

I get the impression that PTRACE_GETFPXREGS and PTRACE_SETFPXREGS are perhaps obsolete and not available on newer systems, though I can't find an explanation of why. Online man pages for ptrace don't mention those symbols.

If you want to build with GCC 7, replace || with | in line 1888 of opnd_shared.c. (That bug should be fixed in any case. I thought it had been.)

[egrimley]

This patch looks good to me, though the commit message should be improved.

There's the obvious alternative of replacing the architecture test with a direct test for the macro in question:

#    ifdef PTRACE_GETFPXREGS
                                               { PTRACE_GETFPXREGS, "PTRACE_GETFPXREGS" },
#    endif

I tried grepping the Linux kernel source and found FPXREGS mentioned only under arch/mips/ and arch/x86/, so perhaps the test should be #ifdef X86, if it is going to be an architecture test.

[umarcor]

@egrimley I was trying to find some "solid" proof that PTRACE_GETFPXREGS and PTRACE_SETFPXREGS are specific to Intel (and probably to i386, not x86_64). In the sources of the latest kernel they are only found under arch/x86 (https://elixir.bootlin.com/linux/v4.20.13/ident/PTRACE_GETFPXREGS), but I don't know if that is enough to defend merging the PR as is.

It is very surprising for me that the Travis builds and tests are passing, because it seems that those variables have never existed on ARM.

[umarcor]

This patch looks good to me, though the commit message should be improved.

I changed it.

There's the obvious alternative of replacing the architecture test with a direct test for the macro in question:

I implemented this now. If both macros exist, both are added. Otherwise, none.

[umarcor]

@derekbruening, which should the commit/title if there is no specific issue number to fix by this PR?

[derekbruening]

@derekbruening, which should the commit/title if there is no specific issue number to fix by this PR?

Just a short description. "Do not reference PTRACE_{G,S}ETFPXREGS when undefined" or some such.

umarcor force-pushed the umarcor:fix-ptrace-fpxregs branch 3 times

Please don't force-push: it deletes past review comments and makes it impossible for the reviewer to see what changed since the last review. The squash-and-merge cleans up the final commit. (See https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews#submitting-a-change-as-a-non-project-member, https://github.com/DynamoRIO/dynamorio/wiki/Contributing)

[umarcor]

Please don't force-push: it deletes past review comments and makes it impossible for the reviewer to see what changed since the last review. The squash-and-merge cleans up the final commit. (See DynamoRIO/dynamorio/wiki/Code-Reviews#submitting-a-change-as-a-non-project-member, DynamoRIO/dynamorio/wiki/Contributing)

Ok. Sorry for the inconvenience. Just to make it clear:

• You don't mind the actual commit messages, because you are going to squash and use the title of the PR as the final message. Therefore:
  • I should not edit already pushed commits to fix the message.
  • I should neither rebase my branches on top of master.

Is it correct?

[derekbruening]

Please don't force-push: it deletes past review comments and makes it impossible for the reviewer to see what changed since the last review. The squash-and-merge cleans up the final commit. (See DynamoRIO/dynamorio/wiki/Code-Reviews#submitting-a-change-as-a-non-project-member, DynamoRIO/dynamorio/wiki/Contributing)

Ok. Sorry for the inconvenience. Just to make it clear:

• You don't mind the actual commit messages, because you are going to squash and use the title of the PR as the final message. Therefore:

The PR title would be the default commit title (== first line), yes, but the default final commit body (== lines beyond the first line) comes from the commit message bodies concatenated together. Whoever does the squash merges those messages together in some fashion, usually by discarding later bug fix or tweak commit messages.

• I should not edit already pushed commits to fix the message.
• I should neither rebase my branches on top of master.

Right, in general do not change the history of a shared git branch: rebase/history rewrite is for private branches.

I wish Github would have first-class support for the final commit message and title being some separate field that would take review comments and be editable, but given that support not being there, one method is to make the next commit's message be the new full message for the final squash. The squasher would then take that one and discard the earlier one.

[derekbruening]

run arm tests

[derekbruening]

Looks like #2898 but sigsuspend has never been flaky on x86 before.