[BOLT] Reject symbols pointing to section end

[jobnoorman]

Summary

Sometimes, symbols are present that point to the end of a section (i.e.,
one-past the highest valid address). Currently, BOLT either rejects
those symbols when they don't point to another existing section, or errs
when they do and the other section is not executable. I suppose BOLT
would accept the symbol when it points to an executable section.

In any case, these symbols should not be considered while discovering
functions and should not result in an error. This patch implements that.

Note that this patch checks explicitly for symbols whose value equals
the end of their section. It might make more sense to verify that the
symbol's value is within [section start, section end). However, I'm not
sure if this could every happen *and* its value does not equal the end.

Another way to implement this is to verify that the BinarySection we
find at the symbol's address actually corresponds to the symbol's
section. I'm not sure what the best approach is so feedback is welcome.

[rafauler]

Makes sense, but where did you see this in practice?

[yota9]

LGTM, but small nit - I would swap this new condition with the " if (!Section->isText()) " condition above, since this one is more "univesal" and might appear not inly in text section. And although currently it does not really matter it probably better from the logical standpoint.

[jobnoorman]

Swap check with the if (!Section->isText()) check as suggested by @yota9.

[jobnoorman]

In D146215#4202973, @rafauler wrote:

Makes sense, but where did you see this in practice?

This happens when using the RISC-V GNU toolchain in debug mode and emitting relocations:

$ cat test.c
int main() {}

$ riscv64-linux-gnu-gcc -g -Wl,--no-relax,--emit-relocs -o test test.c
$ readelf -s test | grep etext
    73: 000000000000064a     0 NOTYPE  LOCAL  DEFAULT   12 .Letext0

In D146215#4203049, @yota9 wrote:

LGTM, but small nit - I would swap this new condition with the " if (!Section->isText()) " condition above, since this one is more "univesal" and might appear not inly in text section. And although currently it does not really matter it probably better from the logical standpoint.

Makes sense, done!

[yota9]

LGTM, but let's wait next week plz, maybe @rafauler have some other thoughts :)

[rafauler]

I don't have anything else to add, I think it makes sense.

[jobnoorman]

Thanks for reviewing @yota9, @rafauler! Please note the I don't have commit access so I cannot land this myself.

My author details are Job Noorman <[e-mail]>

[akhuang]

bolt/test/X86/section-end-sym.s

8

This flag doesn't work if LLVM is built without asserts enabled -- test probably needs a # REQUIRES: asserts ?

[akhuang]

bolt/test/X86/section-end-sym.s

8

added the line in https://reviews.llvm.org/rGfd47ab05e5ab

[jobnoorman]

bolt/test/X86/section-end-sym.s

8

TIL :) Thanks for fixing this!

bolt/lib/Rewrite/RewriteInstance.cpp

if (Section == InputFile->section_end()) {
       });
       registerName(SymbolSize);
       continue;
     }
 
     LLVM_DEBUG(dbgs() << "BOLT-DEBUG: considering symbol " << UniqueName
                       << " for function\n");
 
+    if (Address == Section->getAddress() + Section->getSize()) {
+      assert(SymbolSize == 0 &&
+             "unexpect non-zero sized symbol at end of section");
+      LLVM_DEBUG(
+          dbgs()
+          << "BOLT-DEBUG: rejecting as symbol points to end of its section\n");
+      registerName(SymbolSize);
+      continue;
+    }
+
     if (!Section->isText()) {
       assert(SymbolType != SymbolRef::ST_Function &&
              "unexpected function inside non-code section");
       LLVM_DEBUG(dbgs() << "BOLT-DEBUG: rejecting as symbol is not in code\n");
       registerName(SymbolSize);
       continue;
     }
 

bolt/test/X86/section-end-sym.s

+## Check that BOLT doesn't consider end-of-section symbols (e.g., _etext) as
+## functions.
+
+# REQUIRES: system-linux
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-linux %s -o %t.o
+# RUN: ld.lld %t.o -o %t.exe -q
+# RUN: llvm-bolt %t.exe -o /dev/null --print-cfg --debug-only=bolt 2>&1 \
+# RUN:   | FileCheck %s
+
+# CHECK: considering symbol etext for function
+# CHECK-NEXT: rejecting as symbol points to end of its section
+# CHECK-NOT: Binary Function "etext{{.*}}" after building cfg
+
+
+  .text
+  .globl _start
+  .type _start,@function
+_start:
+  retq
+  .size _start, .-_start
+
+  .align 0x1000
+  .globl etext
+etext:
+
+  .data
+.Lfoo:
+  .word 0