This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] ensure .functype applies to right label in assembler
ClosedPublic

Authored by aardappel on Feb 5 2021, 11:13 AM.

Details

Summary

We used to require .functype immediately follows the label it sets the type of, but not all Clang output follows this rule.

Now we simply allow it on any symbol, but only assume its a function start for a defined symbol, which is simpler and more general.

Fixes (part of) https://bugs.llvm.org/show_bug.cgi?id=49036

Diff Detail

Event Timeline

aardappel created this revision.Feb 5 2021, 11:13 AM
aardappel requested review of this revision.Feb 5 2021, 11:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 11:13 AM
sbc100 added inline comments.Feb 5 2021, 12:52 PM
lld/test/wasm/command-exports.s
44

This looks a little worrying since I imagine what we are looking at here output from a compiler pass.

Can you check that WebAssemblyLowerGlobalDtors doesn't generate functions with local names like this? I think you can just build anything with static C++ ctor/dtor to see it in action.

aardappel added inline comments.Feb 5 2021, 2:12 PM
lld/test/wasm/command-exports.s
44

Good catch, it generates functions starting with register_call_dtors with Function::PrivateLinkage, which I guess at an asm level turns into .L labels.

aardappel updated this revision to Diff 321886.Feb 5 2021, 3:12 PM
aardappel retitled this revision from [WebAssembly] ensure .functype applies to last non-local label in assembler to [WebAssembly] ensure .functype applies to right label in assembler.
aardappel edited the summary of this revision. (Show Details)

simplified and generalized .functype handing

sbc100 accepted this revision.Feb 5 2021, 3:26 PM

Wow that is way better. Assuming all the tests pass LGTM!

This revision is now accepted and ready to land.Feb 5 2021, 3:26 PM
sbc100 added inline comments.Feb 5 2021, 3:26 PM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
218

This comment seems like it might still be relevant to the remaining LastFunctionLabel?

aardappel added inline comments.Feb 5 2021, 3:35 PM
llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp
218

It doesn't, because we don't require .functype to follow labels at all anymore.