Minor changes to permit building with older toolchains. #6482
Conversation
Make some code conditional on EM_RISCV or PR_SVE_GET_VL being defined. This was needed on RHE7, which will be around for a few more years. We also check for another older macro not being defined in case all the macros get replaced by an enum. Change-Id: I1314ca2fed6bbee0428a5aa7413d574c68c89d0d
egrimley-arm
force-pushed
the
pre-sve
branch
from
f31f253
to
5e59187
Compare
|
The x86 failures look like #6417 |
| @@ -114,8 +114,11 @@ is_elf_so_header_common(app_pc base, size_t size, bool memory) | |||
| (memory && elf_header.e_ehsize != sizeof(ELF_HEADER_TYPE)) || | |||
There was a problem hiding this comment.
egrimley-arm force-pushed the pre-sve branch from f31f253 to 5e59187
5 days ago
Please do not force-push shared branches. In PR's this wreaks havoc on the review process, messes up the comment history, prevents the reviewer from seeing just the new changes, etc. This is documented in several places (https://dynamorio.org/page_contributing.html, https://dynamorio.org/page_code_reviews.html).
| elf_header.e_machine != EM_RISCV | ||
| elf_header.e_machine != EM_X86_64 && elf_header.e_machine != EM_AARCH64 | ||
| /* XXX: The test for defined(EM_RISCV) was still needed in 2023 on RHE7. */ | ||
| # if defined(EM_RISCV) || !defined(EM_386) |
There was a problem hiding this comment.
Why is !defined(EM_386) here? OK I see in another change and the PR description it says you are checking some older define. Please put that comment here too, as otherwise this just looks like a bug and someone is likely to delete this part of it.
Ditto in the other instances.
If this were not a different architecture, we would not want ifdefs but would instead want to provide our own define for the new value, since we'd want our library to work on other platform variations than the one it's built on. (It happens quite a bit that we need to deal with system calls that are not listed in the local toolchain headers, so generally we are leery of limiting a build to what's in the local headers.). I suppose a different arch is a little different. Even so, it might be safer in some respects to remove this exclusion and replace it with #ifndef EM_RISCV\n#define EM_RISCV whatever-it-is\n#endif in a DR private header. Hardcoding the value is a slight ugliness but outweighed by having the code enabled for all builds. Imagine the standalone decoder: what if I want to build that and decode RISC-V byte streams on my RHEL7 machine? Maybe it doesn't need this ELF code but the idea applies generally. What if I have a RISC-V binary and want to look at the symbols with drsyms or something.
|
On RHEL 7, |
Submodules used to be optional but we did make elfutils required on Linux so yes that seems reasonable. |
@egrimley-arm do you want to try this approach if you are still planning to work on this issue |
Make some code conditional on EM_RISCV or PR_SVE_GET_VL being defined. This was needed on RHE7, which will be around for a few more years.
We also check for another older macro not being defined in case all the macros get replaced by an enum.