[PATCH] D80354: [lld][WebAssembly] Do not emit initialization for .bss segments

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 2 inline comments as done.
tlively added inline comments.


================
Comment at: lld/test/wasm/data-segments.ll:65
+; PASSIVE-NEXT:        Body:            41B4D60041004101FE480200044041B4D6004101427FFE0102001A054180084100410DFC08000041900841004114FC08010041B4D6004102FE17020041B4D600417FFE0002001A0BFC0900FC09010B
+
 ; PASSIVE-NEXT:  - Index:           2
----------------
sbc100 wrote:
> tlively wrote:
> > There used to be 3 data.drop (FC09) instructions at the end, but now you can see that there are only two, as expected.
> So we doulbly dropping a segment before?
No, the previous dropping code was "FC09**00**FC09**01**FC09**02**", so it was trying to drop segments 0, 1, and 2. But segment 2 did not exist, so this test was previously producing an invalid binary.


================
Comment at: lld/wasm/Writer.cpp:832
           writeUleb128(os, WASM_OPCODE_MEMORY_INIT, "memory.init");
           writeUleb128(os, s->index, "segment index immediate");
           writeU8(os, 0, "memory index immediate");
----------------
sbc100 wrote:
> So was the crash was happening when there was only a bss segment?  And somehow having more than one segment was masking the crash?  
> 
> Should we add a specific test case for that?  An example that would have previously caused and invalid binary would be good.
> 
> I'm stil now clear why we didn't see this is more of our emscripten test cases,
The crash was happening when there were any bss segments and the memory was exported, since .bss segments are not `isBss` if the memory is imported. That's why we never saw this in Emscripten. The existing test case was already invalid, so I don't think we need another one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80354/new/

https://reviews.llvm.org/D80354