Do not look up symbol names when n_strx == 0

[mtrent]

Summary

Historical tools for working with mach-o binaries verify the nlist field
n_strx has a non-zero value before using that value to retrieve symbol names.
Under some cirumstances, llvm-nm will attempt to display the symbol name at
position 0, even though symbol names at that position are not well defined.
This change addresses this problem by returning an empty string when n_strx
is zero.

rdar://problem/35750548

[davide]

I wonder whether you can use something like yaml2obj to craft the object? (or, assuming it's valid, llvm-mc)?
That would improve the readability a lot IMHO.
If not, can you at least add comments to the test (e.g. source file + compiler/linker version etc..) in case we need to regenerate this in the future?

lib/Object/MachOObjectFile.cpp

1662–1665

No {} around single line ifs.

test/tools/llvm-nm/X86/macho-dwarf.test

2

Do you need cat ? Can't you just pipe the nm output to FileCheck?

[mtrent]

In D41657#965799, @davide wrote:

I wonder whether you can use something like yaml2obj to craft the object? (or, assuming it's valid, llvm-mc)?
That would improve the readability a lot IMHO.
If not, can you at least add comments to the test (e.g. source file + compiler/linker version etc..) in case we need to regenerate this in the future?

llvm-objdump and llvm-nm commonly use binary tests when working with bad binaries, so this test isn't unusual. It's not clear to me how you would regenerate this mach-o with lld.

lib/Object/MachOObjectFile.cpp

1662–1665

will fix.

test/tools/llvm-nm/X86/macho-dwarf.test

2

Good point, will fix.

[mtrent]

Updating change request with review feedback.

Main change is to rewrite the lit test to exactly match whole lines of input.
Also added a comment explaining how the test binary was formed.

[davide]

In D41657#966001, @mtrent wrote:

In D41657#965799, @davide wrote:

I wonder whether you can use something like yaml2obj to craft the object? (or, assuming it's valid, llvm-mc)?
That would improve the readability a lot IMHO.
If not, can you at least add comments to the test (e.g. source file + compiler/linker version etc..) in case we need to regenerate this in the future?

llvm-objdump and llvm-nm commonly use binary tests when working with bad binaries, so this test isn't unusual. It's not clear to me how you would regenerate this mach-o with lld.

It happened in the past that changes break tests. If you have a binary checked in, it's harder to understand what was the original intent, IMHO.
That's why I was recommending to use YAML, if possible at all. That said, I don't think it's critical, and your comment is explicative enough.

This LGTM after the two minors are fixed.

[enderby]

Looks good to me with the update of the one comment.

lib/Object/MachOObjectFile.cpp

1662

Mach-O back in 1988 when I created it, was based on 4.2 BSD Unix. In there there it states "A n_strx value of 0 indicates that no name is associated with a particular symbol table entry". Also the a.out(5) format at that time had the size of the string table as the first 4 bytes of the string table, so valid string table indexes where 4 or more. In the early days of Mach-O since the load command had the string table size, tools still reserved these 4 bytes. And generally put nulls in them so incase some tool did not correctly understand index 0 was special it would "just work".

So I would update or remove this comment.

[mtrent]

Updating comment based upon review feedback.

lib/Object/MachOObjectFile.cpp

unsigned SymbolTableEntrySize = is64Bit() ?
     sizeof(MachO::nlist_64) :
     sizeof(MachO::nlist);
   Symb.p += SymbolTableEntrySize;
 }
 
 Expected<StringRef> MachOObjectFile::getSymbolName(DataRefImpl Symb) const {
   StringRef StringTable = getStringTableData();
   MachO::nlist_base Entry = getSymbolTableEntryBase(*this, Symb);
+  if (Entry.n_strx == 0) {
+    // contents of the StringTable at n_strx 0 are not defined
+    return StringRef();
+  }
   const char *Start = &StringTable.data()[Entry.n_strx];
   if (Start < getData().begin() || Start >= getData().end()) {
     return malformedError("bad string index: " + Twine(Entry.n_strx) +
                           " for symbol at index " + Twine(getSymbolIndex(Symb)));
   }
   return StringRef(Start);
 }
 

test/tools/llvm-nm/X86/macho-dwarf.test

+# RUN: llvm-nm -ap %p/Inputs/macho-dwarf-x86_64 | cat -e | FileCheck %s
+
+# CHECK-NOT: ENSYM  $
+# CHECK: ENSYM $
+