DebugInfo/Symbolize: Allow STT_NOTYPE/STT_GNU_IFUNC symbols for .symtab symbolization

[MaskRay]

Summary

In assembly files, omitting .type foo,@function is common. Such functions have
type STT_NOTYPE and llvm-symbolizer reports ?? for them.

An ifunc symbol usually has an associated resolver symbol which is defined at
the same address. Returning either one is fine for symbolization. The resolver
symbol may not end up in the symbol table if (object file) .L is used (linked
image) .symtab is stripped while .dynsym is retained.

This patch allows non SymbolRef::ST_Data non SymbolRef::ST_Function symbols,
thus allowing ELF STT_NOTYPE/STT_GNU_IFUNC symbols for .symtab symbolization.

I have left TODO in the test files for an unimplemented STT_FILE heuristic.

[MaskRay]

Note: There are some interesting tests which can be made easy by allowing ld.lld, e.g. multiple STT_FILE. It is possible to construct such extra tests with yaml2obj but they can be really verbose.
https://lists.llvm.org/pipermail/llvm-dev/2021-January/148048.html the debuginfo-test idea

[dblaikie]

In D95916#2538260, @MaskRay wrote:

Note: There are some interesting tests which can be made easy by allowing ld.lld, e.g. multiple STT_FILE.

Couldn't that be done with an assembly file with multiple .file directives? (as much as possible, it'd be good to keep test coverage in the project where the code is - with the cross-project lit testing area more for end-to-end/extra integration (though targeted/narrowly scoped) coverage)

llvm/test/DebugInfo/Symbolize/ELF/symtab-notype.s

14–15

what are these two lines of output from? (if the gap is covered by the checks on 18 and 19?

27–35

Is this consistent with gnu addr2line behavior? Seems slightly weird to symbolize addresses outside the size of the section, I'd have thought?

45–46

any reason for the variety of instructions? I'd have thought nops might be the clearest/most uninteresting instructions to use here (& in the other test)

[MaskRay]

comment

[MaskRay]

Couldn't that be done with an assembly file with multiple .file directives? (as much as possible, it'd be good to keep test coverage in the project where the code is - with the cross-project lit testing area more for end-to-end/extra integration (though targeted/narrowly scoped) coverage)

In GNU as, the placement of .file appears to affect its symbol table position. However, MC ELFObjectWriter places STT_FILE before all other symbols.

llvm/test/DebugInfo/Symbolize/ELF/symtab-notype.s

14–15

(I forgot to write | FileCheck...)

27–35

Added a TODO.

[dblaikie]

In D95916#2538304, @MaskRay wrote:

Couldn't that be done with an assembly file with multiple .file directives? (as much as possible, it'd be good to keep test coverage in the project where the code is - with the cross-project lit testing area more for end-to-end/extra integration (though targeted/narrowly scoped) coverage)

In GNU as, the placement of .file appears to affect its symbol table position. However, MC ELFObjectWriter places STT_FILE before all other symbols.

Sounds like that might be worth fixing for correctness and testability.

llvm/test/DebugInfo/Symbolize/ELF/symtab-notype.s

50

retq that could be a nop so as not to draw undue attention/questions (could cause a reader to wonder if there's some importance to this difference - if there is no important difference, it'd be more readable if it weren't different)

[MaskRay]

Improve test for the next patch

[jhenderson]

Some time ago, I filed https://bugs.llvm.org/show_bug.cgi?id=45729, which I think this will address? Is there a particular reason to pay any attention to the symbol type at all, other than to pick the "best" one if there are multiple candidates? Even if someone is using DATA or CODE, I'm not sure we should pay any attention to the symbol type - just pick the symbol that matches the requested address, possibly restricting it to one in the matching symbol. (I haven't checked GNU behaviour to make sure what I'm saying makes sense compared to addr2line, mind you).

llvm/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp

163

202

The last line of this comment seems messed up? I've suggested what I think is intended.

llvm/test/DebugInfo/Symbolize/ELF/symtab-file.yaml

1 ↗

(On Diff #321008)

Top-level test comment saying what this test is trying to show? Same in the other tests.

[MaskRay]

Simplify
Allow all symbol tables

[MaskRay]

In D95916#2538703, @jhenderson wrote:

Some time ago, I filed https://bugs.llvm.org/show_bug.cgi?id=45729, which I think this will address?

Seems so.

Is there a particular reason to pay any attention to the symbol type at all, other than to pick the "best" one if there are multiple candidates? Even if someone is using DATA or CODE, I'm not sure we should pay any attention to the symbol type - just pick the symbol that matches the requested address, possibly restricting it to one in the matching symbol. (I haven't checked GNU behaviour to make sure what I'm saying makes sense compared to addr2line, mind you).

I think ST_Function/ST_Data were designed for BFD types. They are not good use outside of tools such as llvm-objdump/llvm-nm. I only kept ST_Data just to not duplicate Objects entries in Functions.
I am mildly confident that being more permissive won't regress other binary formats' behavior. For ELF there is some interesting case to consider, the relevant test coverage can be added separately.

[jhenderson]

llvm/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp

209

Looks like some of my previous comment wasn't addressed.

llvm/test/DebugInfo/Symbolize/ELF/symtab-file.yaml

1 ↗

(On Diff #321008)

Ping this in this particular file?

[MaskRay]

comments

[jhenderson]

So I just thought about this - in the event we have two symbols at the same location with different types, e.g. STT_NOTYPE and STT_FUNC, I think it would make more sense to use the STT_FUNC symbols. I haven't checked GNU behaviour for this though. What does GNU do in that situation?

[MaskRay]

In D95916#2547955, @jhenderson wrote:

So I just thought about this - in the event we have two symbols at the same location with different types, e.g. STT_NOTYPE and STT_FUNC, I think it would make more sense to use the STT_FUNC symbols. I haven't checked GNU behaviour for this though. What does GNU do in that situation?

addrline just picks the first symbol. I don't think this heuristic is useful. In the case where several aliases are defined at the same address, it is rare that they have different symbol types. (Even for symbol assignments, see lld D86263)

An arguably slightly more useful heuristic is to prefer a non-local symbol to a local symbol, but personally I don't think it is worthy to implement, either. addrline does not implement this heuristic.

[jhenderson]

In D95916#2547979, @MaskRay wrote:

In D95916#2547955, @jhenderson wrote:

So I just thought about this - in the event we have two symbols at the same location with different types, e.g. STT_NOTYPE and STT_FUNC, I think it would make more sense to use the STT_FUNC symbols. I haven't checked GNU behaviour for this though. What does GNU do in that situation?

addrline just picks the first symbol. I don't think this heuristic is useful. In the case where several aliases are defined at the same address, it is rare that they have different symbol types. (Even for symbol assignments, see lld D86263)

An arguably slightly more useful heuristic is to prefer a non-local symbol to a local symbol, but personally I don't think it is worthy to implement, either. addrline does not implement this heuristic.

Having given it more thought, I agree that it probably isn't a useful heuristic - if two symbols are at the same location, it doesn't really matter which is picked, as the information provided should be plenty for the situation.

There is one other situation related to this, however: symbols with address 0. There could be both defined and undefined symbols at address 0. We probably want to restrict it to defined symbols only (I also wondered about absolute symbols, as they could be confused with the real function, but in some limited cases, absolute symbols could actually be the real function). Probably this should be a separate change though?

[MaskRay]

In D95916#2548230, @jhenderson wrote:

In D95916#2547979, @MaskRay wrote:

In D95916#2547955, @jhenderson wrote:

So I just thought about this - in the event we have two symbols at the same location with different types, e.g. STT_NOTYPE and STT_FUNC, I think it would make more sense to use the STT_FUNC symbols. I haven't checked GNU behaviour for this though. What does GNU do in that situation?

addrline just picks the first symbol. I don't think this heuristic is useful. In the case where several aliases are defined at the same address, it is rare that they have different symbol types. (Even for symbol assignments, see lld D86263)

An arguably slightly more useful heuristic is to prefer a non-local symbol to a local symbol, but personally I don't think it is worthy to implement, either. addrline does not implement this heuristic.

Having given it more thought, I agree that it probably isn't a useful heuristic - if two symbols are at the same location, it doesn't really matter which is picked, as the information provided should be plenty for the situation.

There is one other situation related to this, however: symbols with address 0. There could be both defined and undefined symbols at address 0. We probably want to restrict it to defined symbols only (I also wondered about absolute symbols, as they could be confused with the real function, but in some limited cases, absolute symbols could actually be the real function). Probably this should be a separate change though?

Yes. STT_SECTION can have 0 address. STT_TLS can have a small address overlapping other addresses. But I think these cases which could regress things are rare enough to be worried in this patch for now.

I also started a topic with internal sanitizer folks why the "DATA" command was needed and whether it is still useful today. It'd be best if we can make data & functions behave the same way and drop "DATA" all together.

[MaskRay]

Add ignored symbol type test
Hard code ELF only. Allowing ST_Debug/ST_Other can break Windows tests.

[jhenderson]

In D95916#2549306, @MaskRay wrote:

I also started a topic with internal sanitizer folks why the "DATA" command was needed and whether it is still useful today. It'd be best if we can make data & functions behave the same way and drop "DATA" all together.

Please don't remove publicly facing features like this. We document this to our downstream users, and whilst I cannot say for certain whether we have any who use the DATA command, removing an option like this could potentially break people's workflows in non-trivial ways.

[zatrazz]

This change broke arm32 llvm-symbolizer which triggered some sanitizer regression. Previously, ARM mapping symbols [1] were ignored but after this change they are shown as the function name.

For instance:

$ ./bin/llvm-symbolizer --obj=projects/compiler-rt/test/asan/ARMHFLinuxConfig/TestCases/Output/invalid-pointer-pairs.cpp.tmp --functions 0xcbf48
f(char, char*, char*)
[...]/llvm/llvm-project/compiler-rt/test/asan/TestCases/invalid-pointer-pairs.cpp:17:14

After this:

./bin/llvm-symbolizer --obj=projects/compiler-rt/test/asan/ARMHFLinuxConfig/TestCases/Output/invalid-pointer-pairs.cpp.tmp --functions 0xcbf48
$a.2
/home/adhemerval.zanella/llvm/llvm-project/compiler-rt/test/asan/TestCases/invalid-pointer-pairs.cpp:17:14

[MaskRay]

In D95916#2560392, @zatrazz wrote:

This change broke arm32 llvm-symbolizer which triggered some sanitizer regression. Previously, ARM mapping symbols [1] were ignored but after this change they are shown as the function name.

For instance:

$ ./bin/llvm-symbolizer --obj=projects/compiler-rt/test/asan/ARMHFLinuxConfig/TestCases/Output/invalid-pointer-pairs.cpp.tmp --functions 0xcbf48
f(char, char*, char*)
[...]/llvm/llvm-project/compiler-rt/test/asan/TestCases/invalid-pointer-pairs.cpp:17:14

After this:

./bin/llvm-symbolizer --obj=projects/compiler-rt/test/asan/ARMHFLinuxConfig/TestCases/Output/invalid-pointer-pairs.cpp.tmp --functions 0xcbf48
$a.2
/home/adhemerval.zanella/llvm/llvm-project/compiler-rt/test/asan/TestCases/invalid-pointer-pairs.cpp:17:14

Fix D96617

[luismarques]

This broke debug info for RISC-V. Test case with broken symbolization:

$ cat test.c
void _start() {}
$ clang --target=riscv64 --gcc-toolchain=<path> -nostdlib -g test.c
$ addr=$((0x`readelf -Ws a.out | grep " _start" | awk '{print $2}'` + 4))
$ hexaddr=`printf '%x\n' $addr`
$ r=`llvm-addr2line -f -e a.out $hexaddr | head -n 1`
[ -z "$r" ] && echo "bad" || echo "good"

Apologies for the terrible shell script, it was the most expedient thing I could come up with.
That test case as written needs the GNU toolchain because LLD will complain about the R_RISCV_ALIGN relaxation relocation, and using -mno-relax breaks the test.

[MaskRay]

In D95916#2627565, @luismarques wrote:

This broke debug info for RISC-V. Test case with broken symbolization:

$ cat test.c
void _start() {}
$ clang --target=riscv64 --gcc-toolchain=<path> -nostdlib -g test.c
$ addr=$((0x`readelf -Ws a.out | grep " _start" | awk '{print $2}'` + 4))
$ hexaddr=`printf '%x\n' $addr`

(llvm-symbolizer works with a decimal address. So $hexaddr is not needed. However, addr2line parses the value in hexadecimal even if there is no 0x prefix.)

$ r=llvm-addr2line -f -e a.out $hexaddr | head -n 1
[ -z "$r" ] && echo "bad" || echo "good"

Apologies for the terrible shell script, it was the most expedient thing I could come up with.
That test case as written needs the GNU toolchain because LLD will complain about the `R_RISCV_ALIGN` relaxation relocation, and using `-mno-relax` breaks the test.

It can be argued that addr2line has the same problem.

With -g or -fasynchronous-unwind-tables, .debug_frame/.eh_frame has a label below _start. Its address is _start + 2 and its name is empty.
Neither addr2line nor llvm-symbolizer skips such a symbol with empty name.

% ~/Dev/binutils-gdb/out/riscv64/binutils/addr2line -fe a.out 0x100e8        
_start
??:?
% ~/Dev/binutils-gdb/out/riscv64/binutils/addr2line -fe a.out 0x100ea
??
test.c:?

In ARM, such symbols have names ($a $t $d and they are modeled as SF_FormatSpecific.
In binutils, such symbols are not printed by nm.
I will file a feature request about empty symbols on binutils and probably make such symbols SF_FormatSpecific.

[luismarques]

In D95916#2627640, @MaskRay wrote:

With -g or -fasynchronous-unwind-tables, .debug_frame/.eh_frame has a label below _start. Its address is _start + 2 and its name is empty.
Neither addr2line nor llvm-symbolizer skips such a symbol with empty name.

But D98669 doesn't fix my original test case...?

[MaskRay]

In D95916#2628775, @luismarques wrote:

In D95916#2627640, @MaskRay wrote:

With -g or -fasynchronous-unwind-tables, .debug_frame/.eh_frame has a label below _start. Its address is _start + 2 and its name is empty.
Neither addr2line nor llvm-symbolizer skips such a symbol with empty name.

But D98669 doesn't fix my original test case...?

Could you recheck? (I am quite certain it fixes your case)

addr2line PR: https://sourceware.org/bugzilla/show_bug.cgi?id=27585

Without that patch (my fllvm-symbolizer is an alias)

% fllvm-symbolizer -fe a.out $(nm a.out | awk '/ _start/{printf "%#x",strtonum("0x"$1)+4}')                                 

a.c:0:0

With that patch

% fllvm-symbolizer -fe a.out $(nm a.out | awk '/ _start/{printf "%#x",strtonum("0x"$1)+4}')
_start
??:0:0

[luismarques]

In D95916#2629534, @MaskRay wrote:

Could you recheck? (I am quite certain it fixes your case)

My apologies, I must have messed up something. It does fix it. Thanks for the quick turn-around!

llvm/lib/DebugInfo/Symbolize/SymbolizableObjectFile.cpp

Error SymbolizableObjectFile::addCoffExportSymbols(
   return Error::success();
 }
 
 Error SymbolizableObjectFile::addSymbol(const SymbolRef &Symbol,
                                         uint64_t SymbolSize,
                                         DataExtractor *OpdExtractor,
                                         uint64_t OpdAddress) {
   // Avoid adding symbols from an unknown/undefined section.
-  const ObjectFile *Obj = Symbol.getObject();
+  const ObjectFile &Obj = *Symbol.getObject();
   Expected<section_iterator> Sec = Symbol.getSection();
-  if (!Sec || (Obj && Obj->section_end() == *Sec))
+  if (!Sec || Obj.section_end() == *Sec)
     return Error::success();
+
   Expected<SymbolRef::Type> SymbolTypeOrErr = Symbol.getType();
   if (!SymbolTypeOrErr)
     return SymbolTypeOrErr.takeError();
   SymbolRef::Type SymbolType = *SymbolTypeOrErr;
-  if (SymbolType != SymbolRef::ST_Function && SymbolType != SymbolRef::ST_Data)
+  if (Obj.isELF()) {
+    // Allow function and data symbols. Additionally allow STT_NONE, which are
+    // common for functions defined in assembly.
+    uint8_t Type = ELFSymbolRef(Symbol).getELFType();
+    if (Type != ELF::STT_NOTYPE && Type != ELF::STT_FUNC &&
+        Type != ELF::STT_OBJECT && Type != ELF::STT_GNU_IFUNC)
+      return Error::success();
+  } else if (SymbolType != SymbolRef::ST_Function &&
+             SymbolType != SymbolRef::ST_Data) {
     return Error::success();
+  }
+
   Expected<uint64_t> SymbolAddressOrErr = Symbol.getAddress();
   if (!SymbolAddressOrErr)
     return SymbolAddressOrErr.takeError();
   uint64_t SymbolAddress = *SymbolAddressOrErr;
   if (UntagAddresses) {
     // For kernel addresses, bits 56-63 need to be set, so we sign extend bit 55
     // into bits 56-63 instead of masking them out.
     SymbolAddress &= (1ull << 56) - 1;
Error SymbolizableObjectFile::addSymbol(const SymbolRef &Symbol,
   }
   Expected<StringRef> SymbolNameOrErr = Symbol.getName();
   if (!SymbolNameOrErr)
     return SymbolNameOrErr.takeError();
   StringRef SymbolName = *SymbolNameOrErr;
   // Mach-O symbol table names have leading underscore, skip it.
   if (Module->isMachO() && !SymbolName.empty() && SymbolName[0] == '_')
     SymbolName = SymbolName.drop_front();
-  // FIXME: If a function has alias, there are two entries in symbol table
-  // with same address size. Make sure we choose the correct one.
-  auto &M = SymbolType == SymbolRef::ST_Function ? Functions : Objects;
-  SymbolDesc SD = { SymbolAddress, SymbolSize };
-  M.emplace_back(SD, SymbolName);
+
+  SymbolDesc SD = {SymbolAddress, SymbolSize};
+
+  // DATA command symbolizes just ST_Data (ELF STT_OBJECT) symbols as an
+  // optimization. Treat everything else (e.g. ELF STT_NOTYPE, STT_FUNC and
+  // STT_GNU_IFUNC) as function symbols which can be used to symbolize
+  // addresses.
+  if (SymbolType == SymbolRef::ST_Data)
+    Objects.emplace_back(SD, SymbolName);
+  else
+    Functions.emplace_back(SD, SymbolName);
   return Error::success();
 }
 
 // Return true if this is a 32-bit x86 PE COFF module.
 bool SymbolizableObjectFile::isWin32Module() const {
   auto *CoffObject = dyn_cast<COFFObjectFile>(Module);
   return CoffObject && CoffObject->getMachine() == COFF::IMAGE_FILE_MACHINE_I386;
 }

llvm/test/DebugInfo/Symbolize/ELF/symtab-file.s

+## When locating a local symbol, we can obtain the filename according to the
+## preceding STT_FILE symbol.
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t
+# RUN: llvm-symbolizer --obj=%t 0 1 2 | FileCheck %s
+
+## TODO Find the preceding STT_FILE symbol as the filename of a local symbol.
+# CHECK:       local1
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+# CHECK-NEXT:  local2
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+# CHECK-NEXT:  local3
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+
+.file "1.c"
+local1:
+  nop
+
+.file "2.c"
+local2:
+  nop
+
+.file "3.c"
+local3:
+  nop

llvm/test/DebugInfo/Symbolize/ELF/symtab-ifunc.s

+## Test we can symbolize STT_GNU_IFUNC symbols.
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t
+# RUN: llvm-symbolizer --obj=%t 0 1
+
+# CHECK:       g_ifunc
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+# CHECK-NEXT:  l_ifunc
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+
+## TODO Find the preceding STT_FILE symbol as the filename of l_ifunc.
+.file "symtab-ifunc.s"
+
+.Lg_resolver:
+  ret
+.size .Lg_resolver, 1
+
+.globl g_ifunc
+.set g_ifunc, .Lg_resolver
+.type g_ifunc, @gnu_indirect_function
+
+.Ll_resolver:
+  ret
+.size .Ll_resolver, 1
+
+.set l_ifunc, .Ll_resolver
+.type l_ifunc, @gnu_indirect_function

llvm/test/DebugInfo/Symbolize/ELF/symtab-ignored.s

+## Ignore STT_SECTION and STT_TLS symbols for .symtab symbolization.
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t
+# RUN: llvm-symbolizer --obj=%t 0 | FileCheck %s
+
+# CHECK:       b
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+
+.file "1.c"
+
+.section a,"a",@progbits
+b:
+  .reloc ., R_X86_64_NONE, a
+.section c,"a",@progbits
+  .reloc ., R_X86_64_NONE, c
+
+.section .tbss,"awT",@nobits
+.globl tls
+tls:

llvm/test/DebugInfo/Symbolize/ELF/symtab-notype.s

+## STT_NOTYPE symbols are common in assembly files. Test we can symbolize them.
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t
+# RUN: llvm-symbolizer --obj=%t --inlines 0 1 2 3 4 5 6 7 | FileCheck %s
+# RUN: llvm-symbolizer --obj=%t --no-inlines 0 1 2 3 4 5 6 7 | FileCheck %s
+
+# CHECK:       _start
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+# CHECK-NEXT:  g_notype
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+# CHECK-NEXT:  g_notype
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+
+## This is a gap.
+# CHECK-NEXT:  ??
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+
+# CHECK-NEXT:  l_notype
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+
+## TODO addr2line does not symbolize the last two out-of-bounds addresses.
+# CHECK-NEXT:  l_notype_nosize
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+# CHECK-NEXT:  l_notype_nosize
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+# CHECK-NEXT:  l_notype_nosize
+# CHECK-NEXT:  ??:0:0
+# CHECK-EMPTY:
+
+## TODO Find the preceding STT_FILE symbol as the filename of a local symbol.
+.file "symtab-notype.s"
+
+.globl _start, g_notype
+_start:
+  retq
+
+g_notype:
+  nop
+  nop
+.size g_notype, . - g_notype
+
+  nop
+
+l_notype:
+  nop
+.size l_notype, . - l_notype
+
+l_notype_nosize:
+  nop