This is an archive of the discontinued LLVM Phabricator instance.

Support dwarf fission for wasm object files
ClosedPublic

Authored by dschuff on Aug 10 2020, 1:24 PM.

Details

Summary

Initial support for dwarf fission sections (-gsplit-dwarf) on wasm.
The most interesting change is support for writing 2 files (.o and .dwo) in the
wasm object writer. My approach moves object-writing logic into its own function
and calls it twice, swapping out the endian::Writer (W) in between calls.
It also splits the import-preparation step into its own function (and skips it when writing a dwo).

Diff Detail

Event Timeline

dschuff created this revision.Aug 10 2020, 1:24 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 10 2020, 1:24 PM
dschuff requested review of this revision.Aug 10 2020, 1:24 PM
dschuff updated this revision to Diff 284522.Aug 10 2020, 4:08 PM

Fix bad diff

aardappel accepted this revision.Aug 10 2020, 5:13 PM

Nice, fairly unintrusive actually. If you desperately wanted to fix the need for changing W dynamically, you could instead make it allocate a second WasmObjectWriter to write the dwo version? :)

llvm/lib/MC/MCAsmBackend.cpp
56–57

change error message

This revision is now accepted and ready to land.Aug 10 2020, 5:13 PM
dschuff updated this revision to Diff 292055.Sep 15 2020, 4:39 PM

Get the right sections in the objects, add tests

dschuff updated this revision to Diff 292056.Sep 15 2020, 4:41 PM

upload the correct diff

dschuff edited the summary of this revision. (Show Details)Sep 15 2020, 4:42 PM
dschuff edited the summary of this revision. (Show Details)Sep 15 2020, 4:49 PM
dschuff retitled this revision from [WIP] Support dwarf fission for wasm object files to Support dwarf fission for wasm object files.

OK, I think this is ready to review for real. Can you take another look?

Seems reasonable. Do you think this way is cleaner than the way elf does it? Looks like ELF creates two different ELFWriter inside the ELFDwoObjectWriter subclass right?

Are we going need to wasm-ld tests to followup or is this really independent of the linker?

llvm/lib/MC/WasmObjectWriter.cpp
224

Kind of a same this change means changing so many line in this file ..

Seems reasonable. Do you think this way is cleaner than the way elf does it? Looks like ELF creates two different ELFWriter inside the ELFDwoObjectWriter subclass right?

Yeah, ELF splits ELFWriter out from ELFObjectWriter, and then instantiates it twice. It's all because ELFObjectWriter has to derive from MCObjectWriter which was clearly not designed with this in mind. I found the class split to be a bit awkward, but I don't really have strong feelings about it either way.

Are we going need to wasm-ld tests to followup or is this really independent of the linker?

It should be independent of the linker because the dwo files don't get linked by the linker. They can be used independently (or combined by the dwp tool but AFAIK it's simpler than the linker). And the object files are just the same as usual from the linker's perspective.

Yeah, ELF splits ELFWriter out from ELFObjectWriter, and then instantiates it twice. It's all because ELFObjectWriter has to derive from MCObjectWriter which was clearly not designed with this in mind. I found the class split to be a bit awkward, but I don't really have strong feelings about it either way.

I should add that what I do instead is just have one instance, and just reset the relevant state before calling writeOneObject again. So the structure is cleaner but the downside of that is that the state has to be manually reset.

sbc100 accepted this revision.Sep 16 2020, 3:42 PM

I don't really grok the TargetFrameLowering::DwarfFrameBase part but everything else LGTM

aardappel added inline comments.Sep 17 2020, 9:23 AM
llvm/lib/MC/WasmObjectWriter.cpp
1335

Nit picking on this because I can't find anything else to complain about:
This line would be more readable inside an else.
I know LLVM prefers "early out" (and so do I), but here IsSplitDwarf is really not an early out case since it is the most complex case, and !IsSplitDwarf is not great for early out either because it is the common case.

dschuff updated this revision to Diff 292624.Sep 17 2020, 2:41 PM

rebase, address comment

This revision was landed with ongoing or failed builds.Sep 17 2020, 2:44 PM
This revision was automatically updated to reflect the committed changes.