Skip to content

[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

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Nov 5, 2024

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 #114605

@llvmbot llvmbot added compiler-rt PGO Profile Guided Optimizations labels Nov 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 5, 2024

@llvm/pr-subscribers-pgo

Author: Josh Stone (cuviper)

Changes

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 #114605


Full diff: https://github.com/llvm/llvm-project/pull/114907.diff

1 Files Affected:

  • (modified) compiler-rt/lib/profile/InstrProfilingPlatformLinux.c (+16-24)
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);
Copy link
Member Author

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.

@cuviper
Copy link
Member Author

cuviper commented Nov 19, 2024

Pinging for review, please!

Copy link
Contributor

@david-xl david-xl left a 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?

@cuviper
Copy link
Member Author

cuviper commented Nov 20, 2024

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.

Copy link

github-actions bot commented Nov 21, 2024

✅ 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
@cuviper
Copy link
Member Author

cuviper commented Nov 21, 2024

I figured out how to force the error on any arch in a test with -Wl,--section-start, whereas we were only getting crashes on aarch64 in Fedora rawhide for #114605. The fix does work, with correct build-ids confirmed in the test.

@cuviper
Copy link
Member Author

cuviper commented Nov 21, 2024

@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.

@gulfemsavrun
Copy link
Contributor

@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.

@gulfemsavrun gulfemsavrun merged commit 667e1fa into llvm:main Nov 21, 2024
7 checks passed
@cuviper
Copy link
Member Author

cuviper commented Nov 22, 2024

It seems the section-start trick failed on a couple buildbots:

@amy-kwan
Copy link
Contributor

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?

@cuviper
Copy link
Member Author

cuviper commented Nov 25, 2024

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.

@cuviper
Copy link
Member Author

cuviper commented Nov 25, 2024

Maybe just the test case can be reverted though? I think the code fix itself is not problematic.

@maryammo
Copy link
Contributor

maryammo commented Nov 25, 2024

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.

@cuviper
Copy link
Member Author

cuviper commented Nov 25, 2024

@maryammo that's a link error while compiling the test with --section-start, not at runtime.

@cuviper
Copy link
Member Author

cuviper commented Nov 26, 2024

See #117647 for my attempt at improving that test.

amy-kwan pushed a commit that referenced this pull request Nov 27, 2024
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.
milkice233 pushed a commit to fedora-riscv/llvm that referenced this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler-rt PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SIGSEGV in __llvm_write_binary_ids due to bad NOTE pointers
6 participants