[WebAssembly] MC: Don't allow zero sized data segments

[sbc100]

Summary

This ensures that each segment has a unique address.
Without this, consecutive zero sized symbols would
end up with the same address and the linker cannot
map symbols to unique data segments.

[dschuff]

Should symbols actually be allowed to have the same value? How do we handle aliases?

[sbc100]

Symbols can have the same value. The problem I was running into was when a zero sized symbol was at the end of segment its not possible know if its pointing to the end one of segment of the beginning on the next. An alternative would be store the segment index for each symbol.

[dschuff]

lib/MC/WasmObjectWriter.cpp

526 ↗

(On Diff #119615)

should we set LastFragmentSize to the fill size here and/or for align frags above? I guess if the answer is "no" it's because they do not represent symbols?

[sbc100]

lib/MC/WasmObjectWriter.cpp

526 ↗

(On Diff #119615)

Basically yes. Zero sized fill fragments do seem to exist but don't represent symbols. They seems to be generated to inject padded between elements of global arrays. So I don't think its possible to have a symbol point to them.

I do think we might need a better solution to this issue in the long run though. Perhaps adding specifying explicitly which segment a symbol belongs too?

[dschuff]

Yeah, this seems OK for now.
I think at some point we may want to revisit the overall goals of the representation that we discussed originally (i.e. not having to encode a symbol table outside of wasm globals, a wasm object file possibly being directly loadable, etc), once we have a prototype and the constraints are clearer. Probably then it will be more clear what to do in cases like this.

llvm/trunk/lib/MC/WasmObjectWriter.cpp

 }
 
 static void addData(SmallVectorImpl<char> &DataBytes,
                     MCSectionWasm &DataSection) {
   DEBUG(errs() << "addData: " << DataSection.getSectionName() << "\n");
 
   DataBytes.resize(alignTo(DataBytes.size(), DataSection.getAlignment()));
 
+  size_t LastFragmentSize = 0;
   for (const MCFragment &Frag : DataSection) {
     if (Frag.hasInstructions())
       report_fatal_error("only data supported in data sections");
 
     if (auto *Align = dyn_cast<MCAlignFragment>(&Frag)) {
       if (Align->getValueSize() != 1)
         report_fatal_error("only byte values supported for alignment");
       // If nops are requested, use zeros, as this is the data section.
       uint8_t Value = Align->hasEmitNops() ? 0 : Align->getValue();
       uint64_t Size = std::min<uint64_t>(alignTo(DataBytes.size(),
                                                  Align->getAlignment()),
                                          DataBytes.size() +
                                              Align->getMaxBytesToEmit());
       DataBytes.resize(Size, Value);
     } else if (auto *Fill = dyn_cast<MCFillFragment>(&Frag)) {
       DataBytes.insert(DataBytes.end(), Fill->getSize(), Fill->getValue());
     } else {
       const auto &DataFrag = cast<MCDataFragment>(Frag);
       const SmallVectorImpl<char> &Contents = DataFrag.getContents();
 
       DataBytes.insert(DataBytes.end(), Contents.begin(), Contents.end());
+      LastFragmentSize = Contents.size();
     }
   }
 
+  // Don't allow empty segments, or segments that end with zero-sized
+  // fragment, otherwise the linker cannot map symbols to a unique
+  // data segment.  This can be triggered by zero-sized structs
+  // See: test/MC/WebAssembly/bss.ll
+  if (LastFragmentSize == 0)
+    DataBytes.resize(DataBytes.size() + 1);
   DEBUG(dbgs() << "addData -> " << DataBytes.size() << "\n");
 }
 
 uint32_t WasmObjectWriter::getRelocationIndexValue(
     const WasmRelocationEntry &RelEntry) {
   switch (RelEntry.Type) {
   case wasm::R_WEBASSEMBLY_TABLE_INDEX_SLEB:
   case wasm::R_WEBASSEMBLY_TABLE_INDEX_I32:

llvm/trunk/test/MC/WebAssembly/bss.ll

 ; RUN: llc -mtriple wasm32-unknown-unknown-wasm -filetype=obj %s -o - | obj2yaml | FileCheck %s
 
 @g0 = global i8* null, align 4
+@g1 = global i32 0, align 4
 
-; CHECK:        - Type:            DATA
+%union.u1 = type {}
+@foo = global %union.u1 zeroinitializer, align 1
+@bar = global %union.u1 zeroinitializer, align 1
+
+; CHECK:        - Type:            GLOBAL
+; CHECK-NEXT:     Globals:         
+; CHECK-NEXT:       - Type:            I32
+; CHECK-NEXT:         Mutable:         false
+; CHECK-NEXT:         InitExpr:        
+; CHECK-NEXT:           Opcode:          I32_CONST
+; CHECK-NEXT:           Value:           0
+; CHECK-NEXT:       - Type:            I32
+; CHECK-NEXT:         Mutable:         false
+; CHECK-NEXT:         InitExpr:        
+; CHECK-NEXT:           Opcode:          I32_CONST
+; CHECK-NEXT:           Value:           4
+; CHECK-NEXT:       - Type:            I32
+; CHECK-NEXT:         Mutable:         false
+; CHECK-NEXT:         InitExpr:        
+; CHECK-NEXT:           Opcode:          I32_CONST
+; CHECK-NEXT:           Value:           8
+; CHECK-NEXT:       - Type:            I32
+; CHECK-NEXT:         Mutable:         false
+; CHECK-NEXT:         InitExpr:        
+; CHECK-NEXT:           Opcode:          I32_CONST
+; CHECK-NEXT:           Value:           9
+; CHECK-NEXT:   - Type:            EXPORT
+; CHECK-NEXT:     Exports:         
+; CHECK-NEXT:       - Name:            g0
+; CHECK-NEXT:         Kind:            GLOBAL
+; CHECK-NEXT:         Index:           0
+; CHECK-NEXT:       - Name:            g1
+; CHECK-NEXT:         Kind:            GLOBAL
+; CHECK-NEXT:         Index:           1
+; CHECK-NEXT:       - Name:            foo
+; CHECK-NEXT:         Kind:            GLOBAL
+; CHECK-NEXT:         Index:           2
+; CHECK-NEXT:       - Name:            bar
+; CHECK-NEXT:         Kind:            GLOBAL
+; CHECK-NEXT:         Index:           3
+; CHECK-NEXT:   - Type:            DATA
 ; CHECK-NEXT:     Segments:        
 ; CHECK-NEXT:       - SectionOffset:   6
 ; CHECK-NEXT:         MemoryIndex:     0
 ; CHECK-NEXT:         Offset:          
 ; CHECK-NEXT:           Opcode:          I32_CONST
 ; CHECK-NEXT:           Value:           0
 ; CHECK-NEXT:         Content:         '00000000'
+; CHECK-NEXT:       - SectionOffset:   15
+; CHECK-NEXT:         MemoryIndex:     0
+; CHECK-NEXT:         Offset:          
+; CHECK-NEXT:           Opcode:          I32_CONST
+; CHECK-NEXT:           Value:           4
+; CHECK-NEXT:         Content:         '00000000'
+; CHECK-NEXT:       - SectionOffset:   24
+; CHECK-NEXT:         MemoryIndex:     0
+; CHECK-NEXT:         Offset:          
+; CHECK-NEXT:           Opcode:          I32_CONST
+; CHECK-NEXT:           Value:           8
+; CHECK-NEXT:         Content:         '00'
+; CHECK-NEXT:       - SectionOffset:  30 
+; CHECK-NEXT:         MemoryIndex:     0
+; CHECK-NEXT:         Offset:          
+; CHECK-NEXT:           Opcode:          I32_CONST
+; CHECK-NEXT:           Value:           9
+; CHECK-NEXT:         Content:         '00'
 ; CHECK-NEXT:   - Type:            CUSTOM
 ; CHECK-NEXT:     Name:            linking
-; CHECK-NEXT:     DataSize:        4
+; CHECK-NEXT:     DataSize:        10
 ; CHECK-NEXT:     SegmentInfo:    
 ; CHECK-NEXT:       - Index:           0
 ; CHECK-NEXT:         Name:            .bss.g0
 ; CHECK-NEXT:         Alignment:       4
 ; CHECK-NEXT:         Flags:           0
+; CHECK-NEXT:       - Index:           1
+; CHECK-NEXT:         Name:            .bss.g1
+; CHECK-NEXT:         Alignment:       4
+; CHECK-NEXT:         Flags:           0
+; CHECK-NEXT:       - Index:           2
+; CHECK-NEXT:         Name:            .bss.foo
+; CHECK-NEXT:         Alignment:       1
+; CHECK-NEXT:         Flags:           0
+; CHECK-NEXT:       - Index:           3
+; CHECK-NEXT:         Name:            .bss.bar
+; CHECK-NEXT:         Alignment:       1
+; CHECK-NEXT:         Flags:           0
 ; CHECK-NEXT: ...