[PATCH] D57105: [ELF] Return the section name when calling getSymbolName on a section symbol.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

My instinct is that the place you've made the change is too deep in the hierarchy, and that's why you have the problem in llvm-objdump. My feeling is that Symbol::getName() should probably return the name of the symbol, and leave it up to the caller to fall back and get the section name for section symbols.

That being said, I see that llvm-nm, at least in places, is assuming that the section name IS being returned when looking up symbol names for section symbols (see the bottom of `getSymblNMTypeChar` for `ELFObjectFileBase`, which is currently broken because of this incorrect assumption - presumably this piece of code is untested!).

I think before putting this in as is, it's probably worth reviewing all the call sites and seeing if there is anywhere where returning the section name would be harmful. I suspect it's probably mostly fine, given the lack of test failures.



================
Comment at: include/llvm/Object/ELFObjectFile.h:448
+    StringRef SecName;
+    auto Sec = getSymbolSection(Sym);
+    if (Sec && !(*Sec)->getName(SecName))
----------------
I'm not sure what the type of Sec here is, so this probably shouldn't be auto (but feel free to point to a similar call in this area that uses auto, and I'll back down).


================
Comment at: test/Object/nm-trivial-object.test:21-22
 RUN:         | FileCheck %s -check-prefix ABSOLUTE-ELF64
+RUN: llvm-nm -a %p/Inputs/IsNAN.o \
+RUN:         | FileCheck %s -check-prefix ELF64-DEBUG-SYMS
 RUN: llvm-nm %p/Inputs/trivial-object-test.macho-i386 \
----------------
I'm not sure you're using the right input for this sort of test. I'd expect it to be something like `%p/Inputs/trivial-object-test.elf-x86-64`, which allows you to then compare the two sets of symbols to see what's different.


================
Comment at: test/Object/nm-trivial-object.test:123-124
+ELF64-DEBUG-SYMS: 00000000 N .debug_line
+ELF64-DEBUG-SYMS: 00000000 N .debug_pubnames
+ELF64-DEBUG-SYMS: 00000000 n .note.GNU-stack
+
----------------
Aside: Why are the majority of these 'N', not 'n'? I wouldn't expect to see global section symbols... but that's my understanding of the difference between the two, based on the GNU nm man page (although I note GNU nm does the same thing).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57105/new/

https://reviews.llvm.org/D57105