[PATCH] D37869: [WebAssembly] Remove default -fdata-sections

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

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.



================
Comment at: include/llvm/BinaryFormat/Wasm.h:100
   ArrayRef<uint8_t> Content;
+  StringRef Name;
 };
----------------
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.


================
Comment at: lib/MC/WasmObjectWriter.cpp:99
 
 struct WasmDataSegment {
   MCSectionWasm *Section;
----------------
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


================
Comment at: lib/MC/WasmObjectWriter.cpp:502
 
+static void addData(SmallVector<char, 0> &DataBytes, MCSectionWasm &DataSection,
+                    uint32_t &DataAlignment) {
----------------
As mentioned in the other review, this can be SmallVectorImpl


================
Comment at: lib/MC/WasmObjectWriter.cpp:1149
 
+  for (MCSection &Sec : Asm) {
+    auto &Section = static_cast<MCSectionWasm &>(Sec);
----------------
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.


https://reviews.llvm.org/D37869