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

[petrhosek]

Reverts #114907

[llvmbot]

@llvm/pr-subscribers-pgo

Author: Petr Hosek (petrhosek)

Changes

Reverts llvm/llvm-project#114907

---

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

2 Files Affected:

• (modified) compiler-rt/lib/profile/InstrProfilingPlatformLinux.c (+24-16)
• (removed) compiler-rt/test/profile/Linux/binary-id-offset.c (-33)

diff --git a/compiler-rt/lib/profile/InstrProfilingPlatformLinux.c b/compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
index 5b230c1b200623..613cfb60857cf3 100644
--- a/compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
+++ b/compiler-rt/lib/profile/InstrProfilingPlatformLinux.c
@@ -194,33 +194,41 @@ 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 (uint32_t I = 0; I < ElfHeader->e_phnum; I++) {
+  for (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) *)(Base + ProgramHeader[I].p_vaddr);
-    const ElfW(Nhdr) *NotesEnd =
-        (const ElfW(Nhdr) *)((const char *)(Note) + ProgramHeader[I].p_memsz);
+    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);
+    }
 
     int BinaryIdsSize = WriteBinaryIds(Writer, Note, NotesEnd);
     if (TotalBinaryIdsSize == -1)
diff --git a/compiler-rt/test/profile/Linux/binary-id-offset.c b/compiler-rt/test/profile/Linux/binary-id-offset.c
deleted file mode 100644
index c66fe82d714ce9..00000000000000
--- a/compiler-rt/test/profile/Linux/binary-id-offset.c
+++ /dev/null
@@ -1,33 +0,0 @@
-// REQUIRES: linux
-//
-// Make sure the build-id can be found in both EXEC and DYN (PIE) files,
-// even when the note's section-start is forced to a weird address.
-// (The DYN case would also apply to libraries, not explicitly tested here.)
-
-// DEFINE: %{cflags} =
-// DEFINE: %{check} = (                                             \
-// DEFINE:     %clang_profgen -Wl,--build-id -o %t %s %{cflags}  && \
-// DEFINE:     env LLVM_PROFILE_FILE=%t.profraw %run %t          && \
-// DEFINE:     llvm-readelf --notes %t                           && \
-// DEFINE:     llvm-profdata show --binary-ids %t.profraw           \
-// DEFINE:   ) | FileCheck %s
-
-// REDEFINE: %{cflags} = -no-pie
-// RUN: %{check}
-
-// REDEFINE: %{cflags} = -pie -fPIE
-// RUN: %{check}
-
-// REDEFINE: %{cflags} = -no-pie -Wl,--section-start=.note.gnu.build-id=0x1000000
-// RUN: %{check}
-
-// REDEFINE: %{cflags} = -pie -fPIE -Wl,--section-start=.note.gnu.build-id=0x1000000
-// RUN: %{check}
-
-// CHECK-LABEL{LITERAL}: .note.gnu.build-id
-// CHECK: Build ID: [[ID:[0-9a-f]+]]
-
-// CHECK-LABEL{LITERAL}: Binary IDs:
-// CHECK-NEXT: [[ID]]
-
-int main() { return 0; }

[cuviper]

What do you think about my followup comment that we could just revert the test case for now? The buildbot failures were just because of its --section-start manipulation, but I'm pretty sure of the library fix itself, and that still passes other pre-existing tests.

[cuviper]

See #117647 for improvements to that test, so hopefully we don't need to revert.