[lld][WebAssembly] Allow data symbols to extend past end of segment

[sbc100]

Summary

This fixes a bug with string merging which assumes that string symbols
are NULL terminated and therefore can end up trimming unused bytes.
Indeed this is exactly what happens the merge-string.s test.

I verified that this can happen in ELF too given the right conditions.

[sbc100]

add test

[dschuff]

I'm not sure I understand what the description means, in particular how null-termination is related to the symbol extending past the end of the segment. (more generally I guess I don't understand why we want to allow symbols to extend past the end).

[sbc100]

In D102281#2752454, @dschuff wrote:

I'm not sure I understand what the description means, in particular how null-termination is related to the symbol extending past the end of the segment. (more generally I guess I don't understand why we want to allow symbols to extend past the end).

Each data symbol has a give size, but string tail merging ignores that an will treat each string as NULL terminated. In this example the symbol bar is of size 6 string merging removes the trailing 3 chars which are essentially unreachable but the symbol still has a size of 6 in the symbol table. Since its at the end of the section it end up being reports has overlapping the end of the section by 3 bytes... at least according the the size in the symbol table. I've verified the ELF does the same thing.

[sbc100]

In D102281#2752637, @sbc100 wrote:

In D102281#2752454, @dschuff wrote:

I'm not sure I understand what the description means, in particular how null-termination is related to the symbol extending past the end of the segment. (more generally I guess I don't understand why we want to allow symbols to extend past the end).

Each data symbol has a give size, but string tail merging ignores that an will treat each string as NULL terminated. In this example the symbol bar is of size 6 string merging removes the trailing 3 chars which are essentially unreachable but the symbol still has a size of 6 in the symbol table. Since its at the end of the section it end up being reports has overlapping the end of the section by 3 bytes... at least according the the size in the symbol table. I've verified the ELF does the same thing.

So now just only check that a symbol starts within the segment it is defined in.

[dschuff]

lld/test/wasm/merge-string.s

19

does .asciz mean that it puts a NUL after each one of the "bc" substrings here? (in other words, is it 'b' 'c' \0 'b' 'c' \0 ?

[sbc100]

lld/test/wasm/merge-string.s

19

Yes, exactly. So the size here was actually wrong to begin with.

lld/test/wasm/merge-string.s

 // RUN: llvm-mc -filetype=obj -triple=wasm32-unknown-unknown %s -o %t.o
 // RUN: wasm-ld -O1 %t.o -o %t.wasm --no-gc-sections --no-entry
 // RUN: obj2yaml %t.wasm | FileCheck %s --check-prefixes=COMMON,MERGE
 
 // Check that the default is the same as -O1 (since we default to -O1)
 // RUN: wasm-ld %t.o -o %t.wasm --no-gc-sections --no-entry
 // RUN: obj2yaml %t.wasm | FileCheck %s --check-prefixes=COMMON,MERGE
 
 // Check that -O0 disables merging
 // RUN: wasm-ld -O0 %t.o -o %t2.wasm --no-gc-sections --no-entry
 // RUN: obj2yaml %t2.wasm | FileCheck --check-prefixes=COMMON,NOMERGE %s
 
+// Check relocatable
+// RUN: wasm-ld -r %t.o -o %t2.o
+// RUN: obj2yaml %t2.o | FileCheck --check-prefixes=RELOC %s
+
         .section .rodata1,"S",@
         .asciz "abc"
 foo:
         .ascii "a"
         .size foo, 1
 bar:
         .asciz "bc"
         .asciz "bc"
-        .size bar, 4
+        .size bar, 6
 
         .section .rodata_relocs,"",@
 negative_addend:
         .int32 foo-10
         .size negative_addend, 4
 
 .globl foo
 .globl bar

 //  COMMON-NEXT:    Segments:
 //  COMMON-NEXT:      - SectionOffset:   7
 //  COMMON-NEXT:        InitFlags:       0
 //  COMMON-NEXT:        Offset:
 //  COMMON-NEXT:          Opcode:          I32_CONST
 //  COMMON-NEXT:          Value:           1024
 //   MERGE-NEXT:          Content:         '61626300'
 // NOMERGE-NEXT:          Content:         '6162630061626300626300'
+
+
+//      RELOC:  - Type:            DATA
+// RELOC-NEXT:    Relocations:
+// RELOC-NEXT:      - Type:            R_WASM_MEMORY_ADDR_I32
+// RELOC-NEXT:        Index:           0
+// RELOC-NEXT:        Offset:          0xF
+// RELOC-NEXT:        Addend:          -10
+// RELOC-NEXT:    Segments:
+// RELOC-NEXT:      - SectionOffset:   6
+// RELOC-NEXT:        InitFlags:       0
+// RELOC-NEXT:        Offset:
+// RELOC-NEXT:          Opcode:          I32_CONST
+// RELOC-NEXT:          Value:           0
+// RELOC-NEXT:        Content:         '61626300'
+// RELOC-NEXT:      - SectionOffset:   15
+// RELOC-NEXT:        InitFlags:       0
+// RELOC-NEXT:        Offset:
+// RELOC-NEXT:          Opcode:          I32_CONST
+// RELOC-NEXT:          Value:           4
+// RELOC-NEXT:        Content:         F6FFFFFF

llvm/lib/Object/WasmObjectFile.cpp

case wasm::WASM_SYMBOL_TYPE_DATA:
       Info.Name = readString(Ctx);
       if (IsDefined) {
         auto Index = readVaruint32(Ctx);
         if (Index >= DataSegments.size())
           return make_error<GenericBinaryError>("invalid data symbol index",
                                                 object_error::parse_failed);
         auto Offset = readVaruint64(Ctx);
         auto Size = readVaruint64(Ctx);
-        if (Offset + Size > DataSegments[Index].Data.Content.size())
-          return make_error<GenericBinaryError>("invalid data symbol offset",
-                                                object_error::parse_failed);
+        size_t SegmentSize = DataSegments[Index].Data.Content.size();
+        if (Offset > SegmentSize)
+          return make_error<GenericBinaryError>(
+              "invalid data symbol offset: `" + Info.Name + "` (offset: " +
+                  Twine(Offset) + " segment size: " + Twine(SegmentSize) + ")",
         Info.DataRef = wasm::WasmDataReference{Index, Offset, Size};
       }
       break;
 
     case wasm::WASM_SYMBOL_TYPE_SECTION: {
       if ((Info.Flags & wasm::WASM_SYMBOL_BINDING_MASK) !=
           wasm::WASM_SYMBOL_BINDING_LOCAL)
         return make_error<GenericBinaryError>(

llvm/test/Object/wasm-bad-data-symbol.yaml

+# RUN: yaml2obj %s | not llvm-objdump -s - 2>&1 | FileCheck %s
+
+# Check that data symbols must have and offset that is within the
+# bounds of the containing segment
+
+# CHECK: invalid data symbol offset: `foo` (offset: 42 segment size: 5)
+
+--- !WASM
+FileHeader:
+  Version:         0x00000001
+Sections:
+  - Type:            DATA
+    Segments:
+      - SectionOffset:   0
+        InitFlags:       0
+        Offset:
+          Opcode:          I32_CONST
+          Value:           0
+        Content:         '6401020304'
+  - Type:            CUSTOM
+    Name:            linking
+    Version:         2
+    SymbolTable:
+      - Index:           0
+        Kind:            DATA
+        Name:            foo
+        Flags:           [ ]
+        Segment:         0
+        Offset:          42
+        Size:            1
+...