This is an archive of the discontinued LLVM Phabricator instance.

[lld][WebAssembly] -Bsymbolic creates indirect function table if needed
ClosedPublic

Authored by wingo on Mar 3 2021, 2:16 AM.

Details

Summary

It can be that while processing relocations, we realize that in the end
we need an indirect function table. Ensure that one is present, in that
case, to avoid writing invalid object files.

Fixes https://bugs.llvm.org/show_bug.cgi?id=49397.

Diff Detail

Event Timeline

wingo created this revision.Mar 3 2021, 2:16 AM
wingo requested review of this revision.Mar 3 2021, 2:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 2:16 AM
sbc100 added a comment.Mar 3 2021, 7:30 AM

lgtm % comments / nits

lld/wasm/Driver.cpp
800

This function can probably just be removed now.

lld/wasm/SymbolTable.cpp
683

Remove this line?

lld/wasm/SyntheticSections.cpp
304

you can do resolveIndirectFunctionTable(/*required =*/true); to avoid the extra local

Perhaps the early return is then overkill if if the body is just one line anyway?

315

How about calling this ensureIndirectFunctionTable ?

lld/wasm/Writer.cpp
748

This kind of ordering violation kind of makes me sad, but I'm not sure I have any better ideas.

748

Can this be replaced by:

// In some cases indirectFunctionTable is added during scanRelocations and therefore was not
// added to the import list at the normal time. 
if (shouldImport(WasmSym::indirectFunctionTable) && !WasmSym::indirectFunctionTable.hasTableNumber()) {
    ...
`

This would avoid the extra indirectFunctionTablePresentWhenComputingImports variable which would simplify the patch a little I think (you could revert the conversion of finalizeIndirectFunctionTable to a member for example).

Its still ugly but less intrusive maybe.

wingo marked 6 inline comments as done.Mar 4 2021, 12:23 AM
wingo added inline comments.
lld/wasm/Writer.cpp
748

Yeah I feel the pain here -- I did try to add a clause in the earlier resolveIndirectFunctionTable routine, but at that point it's too hard to know if there are GOT function pointers. Sad indeed.

Thanks for the review!

wingo updated this revision to Diff 328051.Mar 4 2021, 12:26 AM
wingo marked an inline comment as done.

Address feedback

This revision was not accepted when it landed; it landed in state Needs Review.Mar 4 2021, 12:30 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.