-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[profile] Use base+vaddr for __llvm_write_binary_ids
note pointers
#114907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-pgo Author: Josh Stone (cuviper) ChangesThis function is always examining its own ELF headers in memory, but it Now we do a first scan of the program headers to figure out the runtime Fixes #114605 Full diff: https://github.com/llvm/llvm-project/pull/114907.diff 1 Files Affected:
diff --git a/compiler-rt/lib/profile/InstrProfilingPlatformLinux.c b/compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
index e2c06d51e0c67c..c365129a076847 100644
--- a/compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
+++ b/compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
@@ -194,41 +194,33 @@ static int WriteBinaryIds(ProfDataWriter *Writer, const ElfW(Nhdr) * Note,
*/
COMPILER_RT_VISIBILITY int __llvm_write_binary_ids(ProfDataWriter *Writer) {
extern const ElfW(Ehdr) __ehdr_start __attribute__((visibility("hidden")));
+ extern ElfW(Dyn) _DYNAMIC[] __attribute__((weak, visibility("hidden")));
+
const ElfW(Ehdr) *ElfHeader = &__ehdr_start;
const ElfW(Phdr) *ProgramHeader =
(const ElfW(Phdr) *)((uintptr_t)ElfHeader + ElfHeader->e_phoff);
+ /* Compute the added base address in case of position-independent code. */
+ uintptr_t Base = 0;
+ for (uint32_t I = 0; I < ElfHeader->e_phnum; I++) {
+ if (ProgramHeader[I].p_type == PT_PHDR)
+ Base = (uintptr_t)ProgramHeader - ProgramHeader[I].p_vaddr;
+ if (ProgramHeader[I].p_type == PT_DYNAMIC && _DYNAMIC)
+ Base = (uintptr_t)_DYNAMIC - ProgramHeader[I].p_vaddr;
+ }
+
int TotalBinaryIdsSize = 0;
- uint32_t I;
/* Iterate through entries in the program header. */
- for (I = 0; I < ElfHeader->e_phnum; I++) {
+ for (uint32_t I = 0; I < ElfHeader->e_phnum; I++) {
/* Look for the notes segment in program header entries. */
if (ProgramHeader[I].p_type != PT_NOTE)
continue;
/* There can be multiple notes segment, and examine each of them. */
- const ElfW(Nhdr) * Note;
- const ElfW(Nhdr) * NotesEnd;
- /*
- * When examining notes in file, use p_offset, which is the offset within
- * the elf file, to find the start of notes.
- */
- if (ProgramHeader[I].p_memsz == 0 ||
- ProgramHeader[I].p_memsz == ProgramHeader[I].p_filesz) {
- Note = (const ElfW(Nhdr) *)((uintptr_t)ElfHeader +
- ProgramHeader[I].p_offset);
- NotesEnd = (const ElfW(Nhdr) *)((const char *)(Note) +
- ProgramHeader[I].p_filesz);
- } else {
- /*
- * When examining notes in memory, use p_vaddr, which is the address of
- * section after loaded to memory, to find the start of notes.
- */
- Note =
- (const ElfW(Nhdr) *)((uintptr_t)ElfHeader + ProgramHeader[I].p_vaddr);
- NotesEnd =
- (const ElfW(Nhdr) *)((const char *)(Note) + ProgramHeader[I].p_memsz);
- }
+ const ElfW(Nhdr) *Note =
+ (const ElfW(Nhdr) *)(Base + ProgramHeader[I].p_vaddr);
+ const ElfW(Nhdr) *NotesEnd =
+ (const ElfW(Nhdr) *)((const char *)(Note) + ProgramHeader[I].p_memsz);
int BinaryIdsSize = WriteBinaryIds(Writer, Note, NotesEnd);
if (TotalBinaryIdsSize == -1)
|
(const ElfW(Nhdr) *)((const char *)(Note) + ProgramHeader[I].p_memsz); | ||
} | ||
const ElfW(Nhdr) *Note = | ||
(const ElfW(Nhdr) *)(Base + ProgramHeader[I].p_vaddr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may need to clarify: this is close to the former ElfHeader + p_vaddr
, but in the case of non-PIE, the vaddr already includes the fixed base address. The Base
computation above works either way since it's finding the difference between the runtime address and the header's vaddr, so it's just 0 for non-PIE.
Also, I don't think the code was ever reaching the old else
case at all. At least in all the files I looked at, p_filesz
and p_memsz
were always the same, so we would always be in the "examining notes in file" case.
ccc2b79
to
6250641
Compare
Pinging for review, please! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible to add a test case?
I'm not sure, but I'll look at the existing binary-id tests and see if I can coerce particular NOTE layouts. |
6250641
to
818715b
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
This function is always examining its own ELF headers in memory, but it was trying to use conditions between examining files or memory, and it wasn't accounting for LOAD offsets at runtime. This is especially bad if a loaded segment has additional padding that's not in the file offsets. Now we do a first scan of the program headers to figure out the runtime base address based on `PT_PHDR` and/or `PT_DYNAMIC` (else assume zero), similar to libc's `do_start`. Then each `PT_NOTE` pointer is simply the base plus the segments's `pt_vaddr`, which includes LOAD offsets. Fixes llvm#114605
818715b
to
ad27983
Compare
I figured out how to force the error on any arch in a test with |
@gulfemsavrun @david-xl Thank you both for reviews! I don't have write access to the repo myself, so if you think this is ready, please do merge it. |
Ok, I'll merge this for you. Thanks for fixing it. |
It seems the section-start trick failed on a couple buildbots:
|
@cuviper The buildbots are still failing for this test case. Could we revert this patch in the mean time so the test case failures can be investigated? |
Without write access, I can't revert it myself, but if someone else wants to do that they can. I'll continue looking for a more robust testing approach either way. |
Maybe just the test case can be reverted though? I think the code fix itself is not problematic. |
The failure on ppc64le bot suggests an overflow which might be a bug in the fix itself, if so then reverting just the test hides the actual problem I think. |
@maryammo that's a link error while compiling the test with |
See #117647 for my attempt at improving that test. |
Using a `--section-start` address in the test was causing link errors on some targets. Now it uses a linker script to move the note after `.bss`, which should still have the kind of memory offset we're looking for. This is a follow-up to #114907 to fix buildbot errors.
Resolves: rhbz#2322754 Ref: llvm/llvm-project#114605 Ref: llvm/llvm-project#114907
This function is always examining its own ELF headers in memory, but it
was trying to use conditions between examining files or memory, and it
wasn't accounting for LOAD offsets at runtime. This is especially bad if
a loaded segment has additional padding that's not in the file offsets.
Now we do a first scan of the program headers to figure out the runtime
base address based on
PT_PHDR
and/orPT_DYNAMIC
(else assume zero),similar to libc's
do_start
. Then eachPT_NOTE
pointer is simply thebase plus the segments's
pt_vaddr
, which includes LOAD offsets.Fixes #114605