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

[Matt Davis via Phabricator via llvm-commits]

mattd marked 2 inline comments as done.
mattd 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.

Interesting point.  I was going to fix this up closer to the callsite (llvm-nm), but thought that I was fixing the wrong problem.  Therefore, I made the change deeper.  The change only uses a section name as a last resort: If there is no symbol name  or an empty symbol name.  I'm not sure we want to put the responsibility for using a section name in the caller.  Ideally, the caller (llvm-nm, etc) calls `SymbolRef::getName` and gets an Expected<StringRef>.  The caller already has to check if the current name is valid.  For a symbol representing a section, the caller would need to add more ceremony to get the SectionName, after obtaining the SectionRef and validating that the value is not an error.



================
Comment at: include/llvm/Object/ELFObjectFile.h:448
+    StringRef SecName;
+    auto Sec = getSymbolSection(Sym);
+    if (Sec && !(*Sec)->getName(SecName))
----------------
jhenderson wrote:
> 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).
That's a  fair point.  This routine uses a variety of auto, I figured I could get away with it in this case.  I'll update this to std::error_code, which is what getSymbolSection returns.


================
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 \
----------------
jhenderson wrote:
> 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.
IsNAN.o has  both .debug sections and a .note section.  Testing IsNAN will exercise both of those cases in llvm-nm.cpp `getSymbolNMTypeChar.`  The test you mention (`trivial-object-test.elf-x86-64`) only has a .note section.  I wanted to test the .debug and .note cases. 


================
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
+
----------------
jhenderson wrote:
> 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).
That's the decision of `getSymbolNMTypeChar` in llvm-nm.cpp.  As you point out, the 'N' representing the names for the debug sections  is treated the same in GNU nm.  According to the GNU nm manual:
> "N" The symbol is a debugging symbol.
Since this is a section containing debug data, I assume 'N' is close enough.


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

https://reviews.llvm.org/D57105