This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objdump] Change symbol name/PLT decoding errors to warnings
ClosedPublic

Authored by MaskRay on Aug 9 2020, 11:15 PM.

Details

Summary

If the referenced symbol of a J[U]MP_SLOT is invalid (e.g. symbol index 0), llvm-objdump -d will bail out:

error: 'a': st_name (0x326600) is past the end of the string table of size 0x7

where 0x326600 is the st_name field of the first entry past the end of .symtab

Change it to a warning to continue dumping. Noticed when trying an LLD patch.
X86/plt.test uses a prebuilt executable, so I pick ELF/AArch64/plt.test
which has a YAML input and can be easily modified.

Diff Detail

Event Timeline

MaskRay created this revision.Aug 9 2020, 11:15 PM
MaskRay requested review of this revision.Aug 9 2020, 11:15 PM
grimar added inline comments.Aug 10 2020, 2:07 AM
llvm/test/tools/llvm-objdump/ELF/AArch64/plt.test
21

Probably it might be a bit better to be explicit here:

yaml2obj %s -D SYM=0 -o %t.warn
48

I'd do:

[[SYM=f1]]

I.e. then we have a valid YAML input by default and break it with -D SYM=0 for a particular test.

Otherwise, with [[SYM=<none>]] the input is initially broken, what prevents the possible futher extensions
(e.g. testing the second symbol) for this test without fixing values back to normal ones in each following
yaml2obj invocation.

llvm/tools/llvm-objdump/llvm-objdump.cpp
1406

Will it be better not to consume the error and to report something like:
"warning: unable to read the name of a symbol for PLT entry at ....: <error message with the reason>"?

MaskRay updated this revision to Diff 284536.Aug 10 2020, 5:29 PM
MaskRay marked 3 inline comments as done.

Improve test

llvm/test/tools/llvm-objdump/ELF/AArch64/plt.test
48

Thanks. Did not know Symbol: 0 works..

llvm/tools/llvm-objdump/llvm-objdump.cpp
1406

The message is completely bogus (snowball effect after a decoding error): `st_name (0x326600) is past the end of the string tab
le of size 0x7`

jhenderson added inline comments.Aug 11 2020, 2:34 AM
llvm/tools/llvm-objdump/llvm-objdump.cpp
1406

Sorry, I'm not sure I follow. Are you saying the PLT entries are bogus? In what context? Presumably that's not always the case when you get here, right?

MaskRay updated this revision to Diff 284776.Aug 11 2020, 9:13 AM

Change the signature of getPltAddresses to return Optional<DataRefImpl>

Add another test

MaskRay edited the summary of this revision. (Show Details)Aug 11 2020, 9:14 AM
MaskRay updated this revision to Diff 284780.Aug 11 2020, 9:18 AM
MaskRay retitled this revision from [llvm-objdump] Change a PLT decoding error to a warning to [llvm-objdump] Change symbol name/PLT decoding errors to warnings.
MaskRay edited the summary of this revision. (Show Details)

Fix a Mach-O test

Harbormaster completed remote builds in B67937: Diff 284780.
jhenderson added inline comments.Aug 12 2020, 12:35 AM
llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp
571–574

This is either unrelated and/or untested, I think?

grimar added inline comments.Aug 12 2020, 2:33 AM
llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp
571–574

Previously

std::vector<std::pair<DataRefImpl, uint64_t>>
ELFObjectFileBase::getPltAddresses() const

had the following logic:

if (PltEntryIter != GotToPlt.end())
  Result.push_back(std::make_pair(
    Relocation.getSymbol()->getRawDataRefImpl(), PltEntryIter->second));

i.e Relocation.getSymbol() could return symbol_end() and
Addr.first could be broken here. Now Addr.first is Optional<> and can be None.
So I think this code is related, but indeed untested.

Perhaps we should just exit (like above code does):

if (!KV.second) {
  errs() << "Failed to add instruction at address "
         << format_hex(Instruction.VMAddress, 2)
         << ": Instruction at this address already exists.\n";
  exit(EXIT_FAILURE);
}

or return a error instead.

MaskRay marked an inline comment as done.Aug 12 2020, 7:24 AM
MaskRay added inline comments.
llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp
571–574

The existing code does consumeError(SymNameOrErr.takeError()); - an intention that it does not want to recover errors, so I think it is fine leaving it ignored. The maintainers may want to fix it, though, but it is out of the scope of this patch.

grimar added inline comments.Aug 12 2020, 7:26 AM
llvm/tools/llvm-cfi-verify/lib/FileAnalysis.cpp
571–574

Sounds OK to me.

jhenderson accepted this revision.Aug 13 2020, 12:01 AM

Looks good from my point of view.

This revision is now accepted and ready to land.Aug 13 2020, 12:01 AM