Page MenuHomePhabricator

[llvm-objdump][WebAssembly] Fix llvm-objdump on files without symbols
ClosedPublic

Authored by wingo on Jul 7 2021, 2:16 AM.

Details

Summary

If a file has no symbols, perhaps because it is a linked executable,
synthesize some symbols by walking the code section. Otherwise the
disassembler will try to treat the whole code section as a function,
which won't parse. Fixes https://bugs.llvm.org/show_bug.cgi?id=50957.

Diff Detail

Event Timeline

wingo created this revision.Jul 7 2021, 2:16 AM
wingo requested review of this revision.Jul 7 2021, 2:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2021, 2:16 AM

Thanks for look into this! I've been meaning fix this forever but didn't realize the solution could be this simple. I'm excited to starting objdump in more of our test code once this lands.

Do you think it would make sense to do this at the libObject level rather than at the objdump level? i.e. have libObject guarantee that each function has a symbol? Or is it really only objdump and benefits from these fake symbols?

llvm/test/tools/llvm-objdump/wasm/executable-without-symbols.test
2

If we can we should try to find a way to avoid checking in the binary here.

Also, it would be good to have a test for a file that also lacks a name sections, in which case I would expect to see either empty names, or synthetic/fake names (which do you think make the most sense).

llvm/tools/llvm-objdump/llvm-objdump.cpp
724

Can we keep this function as is, and call it from addFallbackSymbols instead? This would keep the diff smaller and avoid extra nesting.

749

Perhaps assert Function->SymbolName.empty() then?

751

Looking at createSymbolInfo it looks like we always use STT_NOTYPE for wasm symbols.

1165

I think maybe we should leave these two lines alone and add a separate block:

if (Obj->isWam())
   addMissingWasmCodeSymbols(..);

The reasons is because we want to ensure that every function has a symbol don't we? So we might want to handle the case where some functions have symbols and some don't?

There's an awful lot of WebAssembly test failures in pre-merge. Are they related (they might not be)?

llvm/test/tools/llvm-objdump/wasm/executable-without-symbols.test
2

Would yaml2obj allow you to avoid the canned binary? Alternatively, presumably there's some way of building this from assembly?

4

Nit: not looked at wasm tests, but in most other llvm-objdump tests, we usually add comment markers (i.e. '#') for CHECK and RUN lines, even if they're not strictly necessary.

May be moot if you switch to yaml2obj or similar, as you'll need them anyway in that case.

wingo updated this revision to Diff 359239.Jul 16 2021, 12:36 AM

Simplify test

Test changes look good. I'll leave @sbc100 to review things more thoroughly.

wingo updated this revision to Diff 359244.Jul 16 2021, 1:01 AM

Rebase; will fix spurious presubmit errors

wingo updated this revision to Diff 359247.Jul 16 2021, 1:25 AM
wingo marked 3 inline comments as done.

Add wasm symbols for any function without a symbol

wingo updated this revision to Diff 359248.Jul 16 2021, 1:29 AM
wingo marked 4 inline comments as done.

Use STT_NOTYPE for synthesized symbols

wingo added a comment.Jul 16 2021, 1:30 AM

Thanks all for the feedback! I think the updated patch addresses the feedback; PTAL :)

wingo updated this revision to Diff 359249.Jul 16 2021, 1:32 AM

Tighten up tests

wingo updated this revision to Diff 359250.Jul 16 2021, 1:38 AM

Prettify tests

sbc100 accepted this revision.Jul 16 2021, 9:37 AM

Great! This will allow for far more use of objdump in our testing elsewhere. Thanks for working on this.

This revision is now accepted and ready to land.Jul 16 2021, 9:37 AM
This revision was landed with ongoing or failed builds.Jul 19 2021, 12:22 AM
This revision was automatically updated to reflect the committed changes.