Page MenuHomePhabricator

Do not look up symbol names when n_strx == 0
ClosedPublic

Authored by mtrent on Jan 1 2018, 2:13 PM.

Details

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

Event Timeline

mtrent created this revision.Jan 1 2018, 2:13 PM
davide added a subscriber: davide.Jan 2 2018, 6:24 AM

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
3

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

davide requested changes to this revision.Jan 2 2018, 6:25 AM
This revision now requires changes to proceed.Jan 2 2018, 6:25 AM
mtrent marked 2 inline comments as done.Jan 2 2018, 11:36 AM

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
3

Good point, will fix.

mtrent marked 2 inline comments as done.Jan 2 2018, 11:37 AM
mtrent updated this revision to Diff 128451.Jan 2 2018, 1:04 PM

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 accepted this revision.Jan 2 2018, 2:52 PM

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.

This revision is now accepted and ready to land.Jan 2 2018, 2:52 PM
enderby accepted this revision.Jan 3 2018, 2:34 PM

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 updated this revision to Diff 128573.Jan 3 2018, 3:11 PM
mtrent marked an inline comment as done.

Updating comment based upon review feedback.

mtrent closed this revision.Jan 3 2018, 3:29 PM