[profile] Use base+vaddr for `__llvm_write_binary_ids` note pointers #114907

cuviper · 2024-11-05T01:25:43Z

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 · 2024-11-05T01:26:24Z

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

cuviper · 2024-11-05T02:06:58Z

compiler-rt/lib/profile/InstrProfilingPlatformLinux.c

-          (const ElfW(Nhdr) *)((const char *)(Note) + ProgramHeader[I].p_memsz);
-    }
+    const ElfW(Nhdr) *Note =
+        (const ElfW(Nhdr) *)(Base + ProgramHeader[I].p_vaddr);


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 · 2024-11-19T19:08:11Z

Pinging for review, please!

david-xl

possible to add a test case?

cuviper · 2024-11-20T19:15:40Z

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.

github-actions · 2024-11-21T00:27:12Z

✅ 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 · 2024-11-21T00:30:22Z

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 · 2024-11-21T18:08:04Z

@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 · 2024-11-21T18:14:02Z

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

cuviper · 2024-11-22T17:31:51Z

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

https://lab.llvm.org/buildbot/#/builders/145/builds/3327

ld.lld: error: output file too large: 18446744073457995232 bytes
- That's 0xfffffffff1018de0 -- probably a subtraction overflow?
https://lab.llvm.org/buildbot/#/builders/42/builds/1996

/usr/bin/ld: section .interp LMA [0000000001000270,000000000100027e] overlaps section .dynsym LMA [0000000001000150,00000000010004f7]
- I didn't directly manipulate either of those sections, but I guess the linker still got confused?

amy-kwan · 2024-11-25T15:41:19Z

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

https://lab.llvm.org/buildbot/#/builders/145/builds/3327

ld.lld: error: output file too large: 18446744073457995232 bytes

That's 0xfffffffff1018de0 -- probably a subtraction overflow?

https://lab.llvm.org/buildbot/#/builders/42/builds/1996

/usr/bin/ld: section .interp LMA [0000000001000270,000000000100027e] overlaps section .dynsym LMA [0000000001000150,00000000010004f7]

I didn't directly manipulate either of those sections, but I guess the linker still got confused?

@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 · 2024-11-25T19:49:15Z

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 · 2024-11-25T19:50:17Z

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

maryammo · 2024-11-25T20:27:33Z

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 · 2024-11-25T20:31:21Z

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

cuviper · 2024-11-26T00:13:03Z

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

llvmbot added compiler-rt PGO Profile Guided Optimizations labels Nov 5, 2024

cuviper commented Nov 5, 2024

View reviewed changes

nikic requested review from petrhosek, MaskRay, gulfemsavrun and david-xl November 12, 2024 14:45

cuviper force-pushed the profile-elf-base branch from ccc2b79 to 6250641 Compare November 19, 2024 19:07

gulfemsavrun approved these changes Nov 20, 2024

View reviewed changes

david-xl reviewed Nov 20, 2024

View reviewed changes

cuviper force-pushed the profile-elf-base branch from 6250641 to 818715b Compare November 21, 2024 00:23

cuviper force-pushed the profile-elf-base branch from 818715b to ad27983 Compare November 21, 2024 00:29

david-xl approved these changes Nov 21, 2024

View reviewed changes

gulfemsavrun merged commit 667e1fa into llvm:main Nov 21, 2024
7 checks passed

petrhosek mentioned this pull request Nov 25, 2024

Revert "[profile] Use base+vaddr for __llvm_write_binary_ids note pointers" #117621

Closed

cuviper mentioned this pull request Nov 26, 2024

[profile] Make the binary-id-offset.c test more robust #117647

Merged

milkice233 pushed a commit to fedora-riscv/llvm that referenced this pull request Jan 22, 2025

Fix profiling after a binutils NOTE change

680f925

Resolves: rhbz#2322754 Ref: llvm/llvm-project#114605 Ref: llvm/llvm-project#114907

[profile] Use base+vaddr for __llvm_write_binary_ids note pointers #114907

[profile] Use base+vaddr for __llvm_write_binary_ids note pointers #114907

Uh oh!

Conversation

cuviper commented Nov 5, 2024

Uh oh!

llvmbot commented Nov 5, 2024

Uh oh!

cuviper Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

cuviper commented Nov 19, 2024

Uh oh!

david-xl left a comment

Choose a reason for hiding this comment

Uh oh!

cuviper commented Nov 20, 2024

Uh oh!

github-actions bot commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cuviper commented Nov 21, 2024

Uh oh!

cuviper commented Nov 21, 2024

Uh oh!

gulfemsavrun commented Nov 21, 2024

Uh oh!

Uh oh!

cuviper commented Nov 22, 2024

Uh oh!

amy-kwan commented Nov 25, 2024

Uh oh!

cuviper commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cuviper commented Nov 25, 2024

Uh oh!

maryammo commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cuviper commented Nov 25, 2024

Uh oh!

cuviper commented Nov 26, 2024

Uh oh!

Uh oh!

[profile] Use base+vaddr for `__llvm_write_binary_ids` note pointers #114907

[profile] Use base+vaddr for `__llvm_write_binary_ids` note pointers #114907

github-actions bot commented Nov 21, 2024 •

edited

Loading

cuviper commented Nov 25, 2024 •

edited

Loading

maryammo commented Nov 25, 2024 •

edited

Loading