This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Remove default -fdata-sections
AbandonedPublic

Authored by sbc100 on Sep 14 2017, 2:27 PM.

Details

Reviewers
dschuff
Summary

This reduces the number of wasm data sections that clang
will generate by default. -fdata-sections can still be
used to produce one segement per global variable.

This change also adds support for naming data segments
which is useful for the linker so that it can merge similar
sections.

Event Timeline

sbc100 created this revision.Sep 14 2017, 2:27 PM
sbc100 added a subscriber: llvm-commits.
dschuff edited edge metadata.Sep 14 2017, 2:59 PM

Do wer have any tests with global vars that have an explicit section? We should have one, that tests that they get merged into a segment in the object file.

include/llvm/BinaryFormat/Wasm.h
100

StringRefs generally shouldn't have lifetimes that extend beyond a callstack since they don't own their memory.
Edit: OK I tracked down where it actually comes from; I guess here (and below) can probably just have a comment like the one in MCSectionWasm's SectionName.

lib/MC/WasmObjectWriter.cpp
99

I guess this depends on D37834? I didn't notice before but it's kind of sad that we are duplicating this data structure from wasm.h

502

As mentioned in the other review, this can be SmallVectorImpl

1149

This bit implements the new behavior that each data section from the object file gets put into a separate segment, correct? IMO that is really the fundamental change of this patch, and the commit description should reflect that.

Yes, this change is a WIP that depends on D37834.

I apologize if I missed something, but what's the motivation for not using -fdata-sections by default?

I guess I was just trying to match the other native toolchains. One can still pass -fdata-sections, but why make it the default?

I can remove the change in the default from this change to make it less controversial perhaps?

I just have the same thought as @dschuff here. We're not even the first ones to do so, because CloudABI already does it. I also don't want to block you though.

I created a more minimal version of this change https://reviews.llvm.org/D37876. Abandoning this one.

sbc100 abandoned this revision.Sep 14 2017, 5:00 PM