This is an archive of the discontinued LLVM Phabricator instance.

[lld][WebAssembly] Fix resolveIndirectFunctionTable for relocatable output
ClosedPublic

Authored by wingo on Feb 16 2021, 4:32 AM.

Details

Summary

For relocatable output that needs the indirect function table, identify
the well-known function table. This allows us to properly fix the
limits on the imported table, and in a followup will allow the element
section to reference the indirect function table even if it's not
assigned to table number 0. Adapt tests for import reordering.

Diff Detail

Event Timeline

wingo created this revision.Feb 16 2021, 4:32 AM
wingo requested review of this revision.Feb 16 2021, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 4:32 AM
wingo updated this revision to Diff 323957.Feb 16 2021, 4:39 AM

Fix typo

Does this mean the --relocatable always produces output with an entry in the symbol table for __indirect_function_table? i.e. does the linker always produce non-MVP object files?

lld/wasm/Driver.cpp
824

Would it be better to ensure that importTable is always set when config->relocatable. Anything else would be nonsensical I think.

We should also error out if -r is used with any of the table control options (--export-table, --import-table, --growable-table all make no sense with --relocatable.

Also why the extra existing here? In --relocatable mode we never want to end up calling createDefinedIndirectFunctionTable so we need to make sure that this branch of the if I think, no?

wingo added a comment.Feb 17 2021, 1:24 AM

Does this mean the --relocatable always produces output with an entry in the symbol table for __indirect_function_table? i.e. does the linker always produce non-MVP object files?

Yeah apparently so. Though if no input needs an indirect function table, specifying --relocatable won't cause one to be produced. Otherwise yes a symtab entry for __indirect_function_table is residualized. Note however that the file is still conforming to the MVP spec (i.e no 5-byte relocs for call_indirect).

wingo added inline comments.Feb 17 2021, 1:26 AM
lld/wasm/Driver.cpp
824

I can make this change, I just thought you wanted a precise mapping between command-line args and the importTable config flag.

The extra "existing" is because both importTable and exportTable force the table to be present, even if not needed. That's not what we want with relocatable.

wingo added inline comments.Feb 17 2021, 1:32 AM
lld/wasm/Driver.cpp
824

Sorry, following up here -- can't just set importTable when relocatable because that could cause the function table to be created when not needed. Advice welcome; as is I don't see an obvious change to make.

sbc100 added inline comments.Feb 17 2021, 7:33 AM
lld/wasm/Driver.cpp
824

I think if fine for --import-table to mean "import table if you need a table at all".

Export table is different because the embedded may want access to the exported table. For the import, there no point in imported a table one does not user.

sbc100 added inline comments.Feb 17 2021, 7:37 AM
lld/wasm/Driver.cpp
824

I think if fine for --import-table to mean "import table if you need a table at all".

Export table is different because the embedded may want access to the exported table. For the import, there no point in imported a table one does not user.

So this would become:

// No point in importing a table if we don't have any references to one.
if (config->importTable && existing)

And --relocatable could set config->importTable internally?

826

Can an error when --export-table is passed in --relocatable mode so we know we never get here.
Perhaps even an assert on line 838 too. In fact, perhaps we can land a separate change that no table
flags should be passed in --relocatable mode?

wingo updated this revision to Diff 324329.Feb 17 2021, 8:54 AM

Rebase on top of D96872

wingo added a comment.Feb 17 2021, 8:55 AM

Thanks for feedback :) Fixed nits, I think. Tests pass here.

sbc100 accepted this revision.Feb 17 2021, 11:09 AM

lgtm with a test for the new error(s).

lld/wasm/Driver.cpp
475

Can you do the same for growableTable? Probably good to have a test too.

This revision is now accepted and ready to land.Feb 17 2021, 11:09 AM
wingo updated this revision to Diff 324553.Feb 18 2021, 12:34 AM

Add error tests, and mark -r incompatible with --growable-table

This revision was landed with ongoing or failed builds.Feb 18 2021, 12:37 AM
This revision was automatically updated to reflect the committed changes.