[WebAssembly] Prevent data inside text sections in assembly

[aardappel]

Summary

This is not supported in Wasm, unless the data was encoded instructions, but that wouldn't work with the assembler's other functionality (enforcing nesting etc.).

Fixes: https://bugs.llvm.org/show_bug.cgi?id=48971

[aardappel]

llvm/lib/MC/WasmObjectWriter.cpp

494

Note this check may strictly not be necessary anymore, but the unchecked dereference that was there before seemed a bit fragile for an invariant established elsewhere. Maybe better as an assert?

[sbc100]

lgtm with a test for the parse error.

llvm/lib/MC/MCParser/WasmAsmParser.cpp

224 ↗

(On Diff #320643)

CurrentSection?

235 ↗

(On Diff #320643)

I think it would be good to have a test for this user-facing error.. partly because I think we could use more .s file tests in general and the tests kind of act as documentation for what is and isn't possible with our .s format.

[aardappel]

• Moved checking of type to AsmParser label parsing.
• Improved MCSymbolWasm default type handling slightly.
• Added error test case.

[sbc100]

llvm/include/llvm/MC/MCSymbolWasm.h

52

Any reason to keep the deprecated method around? Why not just remove it? I guess a followup that removes it would be fine too.

llvm/test/MC/WebAssembly/basic-assembly-errors.s

7

wasm or Wasm rather than WASM.

[aardappel]

Further refactor the symbol type accessors

[aardappel]

llvm/include/llvm/MC/MCSymbolWasm.h

52

Was able to get rid of the deprecated method.

[sbc100]

llvm/lib/MC/WasmObjectWriter.cpp

1706

So we still can end up with symbol that don't have exploit type and in that case we default to data symbols?

llvm/include/llvm/MC/MCSymbolWasm.h

 //===- MCSymbolWasm.h -  ----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_MC_MCSYMBOLWASM_H
 #define LLVM_MC_MCSYMBOLWASM_H
 
 #include "llvm/BinaryFormat/Wasm.h"
 #include "llvm/MC/MCSymbol.h"
 
 namespace llvm {
 
 class MCSymbolWasm : public MCSymbol {
-  wasm::WasmSymbolType Type = wasm::WASM_SYMBOL_TYPE_DATA;
+  Optional<wasm::WasmSymbolType> Type;
   bool IsWeak = false;
   bool IsHidden = false;
   bool IsComdat = false;
   mutable bool IsUsedInInitArray = false;
   mutable bool IsUsedInGOT = false;
   Optional<StringRef> ImportModule;
   Optional<StringRef> ImportName;
   Optional<StringRef> ExportName;

 public:
   MCSymbolWasm(const StringMapEntry<bool> *Name, bool isTemporary)
       : MCSymbol(SymbolKindWasm, Name, isTemporary) {}
   static bool classof(const MCSymbol *S) { return S->isWasm(); }
 
   const MCExpr *getSize() const { return SymbolSize; }
   void setSize(const MCExpr *SS) { SymbolSize = SS; }
 
-  bool isFunction() const { return Type == wasm::WASM_SYMBOL_TYPE_FUNCTION; }
-  bool isData() const { return Type == wasm::WASM_SYMBOL_TYPE_DATA; }
-  bool isGlobal() const { return Type == wasm::WASM_SYMBOL_TYPE_GLOBAL; }
-  bool isTable() const { return Type == wasm::WASM_SYMBOL_TYPE_TABLE; }
-  bool isSection() const { return Type == wasm::WASM_SYMBOL_TYPE_SECTION; }
-  bool isEvent() const { return Type == wasm::WASM_SYMBOL_TYPE_EVENT; }
-  wasm::WasmSymbolType getType() const { return Type; }
+  bool isFunction() const { return getType() == wasm::WASM_SYMBOL_TYPE_FUNCTION; }
+  bool isData() const { return getType() == wasm::WASM_SYMBOL_TYPE_DATA; }
+  bool isGlobal() const { return getType() == wasm::WASM_SYMBOL_TYPE_GLOBAL; }
+  bool isTable() const { return getType() == wasm::WASM_SYMBOL_TYPE_TABLE; }
+  bool isSection() const { return getType() == wasm::WASM_SYMBOL_TYPE_SECTION; }
+  bool isEvent() const { return getType() == wasm::WASM_SYMBOL_TYPE_EVENT; }
+
+  // Deprecated: For backwards compatibility, WASM_SYMBOL_TYPE_DATA used to be
+  // the default. If possible, call getDefinedType instead, and/or ensure
+  // symbols always have the correct type set when constructed.
+  wasm::WasmSymbolType getType() const {
+    return Type.getValueOr(wasm::WASM_SYMBOL_TYPE_DATA);
+  }
+
+  Optional<wasm::WasmSymbolType> getDefinedType() const { return Type; }
+
   void setType(wasm::WasmSymbolType type) { Type = type; }
 
   bool isExported() const {
     return getFlags() & wasm::WASM_SYMBOL_EXPORTED;
   }
   void setExported() const {
     modifyFlags(wasm::WASM_SYMBOL_EXPORTED, wasm::WASM_SYMBOL_EXPORTED);
   }

llvm/lib/MC/WasmObjectWriter.cpp

if (Type == wasm::R_WASM_FUNCTION_OFFSET_I32 ||
       Type == wasm::R_WASM_FUNCTION_OFFSET_I64 ||
       Type == wasm::R_WASM_SECTION_OFFSET_I32) {
     if (!FixupSection.getKind().isMetadata())
       report_fatal_error("relocations for function or section offsets are "
                          "only supported in metadata sections");
 
     const MCSymbol *SectionSymbol = nullptr;
     const MCSection &SecA = SymA->getSection();
-    if (SecA.getKind().isText())
-      SectionSymbol = SectionFunctions.find(&SecA)->second;
-    else
+    if (SecA.getKind().isText()) {
+      auto SecSymIt = SectionFunctions.find(&SecA);
+      if (SecSymIt == SectionFunctions.end())
+        report_fatal_error("section doesn\'t have defining symbol");
+      SectionSymbol = SecSymIt->second;
+    } else {
       SectionSymbol = SecA.getBeginSymbol();
+    }
     if (!SectionSymbol)
       report_fatal_error("section symbol is required for relocation");
 
     C += Layout.getSymbolOffset(*SymA);
     SymA = cast<MCSymbolWasm>(SectionSymbol);
   }
 
   if (Type == wasm::R_WASM_TABLE_INDEX_REL_SLEB ||
for (const MCSymbol &S : Asm.symbols()) {
     }
     if (WS.hasImportName())
       Flags |= wasm::WASM_SYMBOL_EXPLICIT_NAME;
     if (WS.hasExportName())
       Flags |= wasm::WASM_SYMBOL_EXPORTED;
 
     wasm::WasmSymbolInfo Info;
     Info.Name = WS.getName();
     Info.Kind = WS.getType();
     Info.Flags = Flags;
     if (!WS.isData()) {
       assert(WasmIndices.count(&WS) > 0);
       Info.ElementIndex = WasmIndices.find(&WS)->second;
     } else if (WS.isDefined()) {
       assert(DataLocations.count(&WS) > 0);
       Info.DataRef = DataLocations.find(&WS)->second;
     }

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

case Match_InvalidOperand: {
       }
       return Parser.Error(ErrorLoc, "invalid operand for instruction");
     }
     }
     llvm_unreachable("Implement any new match types added!");
   }
 
   void doBeforeLabelEmit(MCSymbol *Symbol) override {
+    // Code below only applies to labels in text sections.
+    auto CWS = cast<MCSectionWasm>(getStreamer().getCurrentSection().first);
+    if (!CWS || !CWS->getKind().isText())
+      return;
+
+    auto WasmSym = cast<MCSymbolWasm>(Symbol);
+    // Unlike other targets, we don't allow data in text sections (labels
+    // declared with .type @object).
+    if (WasmSym->getDefinedType() == wasm::WASM_SYMBOL_TYPE_DATA) {
+      Parser.Error(Parser.getTok().getLoc(),
+                   "WASM doesn\'t support data symbols in text sections");
+      return;
+    }
+
     // Start a new section for the next function automatically, since our
     // object writer expects each function to have its own section. This way
     // The user can't forget this "convention".
     auto SymName = Symbol->getName();
     if (SymName.startswith(".L"))
       return; // Local Symbol.
 
-    // Only create a new text section if we're already in one.
     // TODO: If the user explicitly creates a new function section, we ignore
     // its name when we create this one. It would be nice to honor their
     // choice, while still ensuring that we create one if they forget.
     // (that requires coordination with WasmAsmParser::parseSectionDirective)
-    auto CWS = cast<MCSectionWasm>(getStreamer().getCurrentSection().first);
-    if (!CWS || !CWS->getKind().isText())
-      return;
     auto SecName = ".text." + SymName;
 
     auto *Group = CWS->getGroup();
     // If the current section is a COMDAT, also set the flag on the symbol.
     // TODO: Currently the only place that the symbols' comdat flag matters is
     // for importing comdat functions. But there's no way to specify that in
     // assembly currently.
     if (Group)
-      cast<MCSymbolWasm>(Symbol)->setComdat(true);
+      WasmSym->setComdat(true);
     auto *WS =
         getContext().getWasmSection(SecName, SectionKind::getText(), Group,
                                     MCContext::GenericSectionID, nullptr);
     getStreamer().SwitchSection(WS);
     // Also generate DWARF for this section if requested.
     if (getContext().getGenDwarfForAssembly())
       getContext().addGenDwarfSection(WS);
   }

llvm/test/MC/WebAssembly/basic-assembly-errors.s

 # RUN: not llvm-mc -triple=wasm32-unknown-unknown -mattr=+simd128,+nontrapping-fptoint,+exception-handling < %s 2>&1 | FileCheck %s
 
 # CHECK: invalid operand for instruction
 # (must be 0.0 or similar)
     f32.const 0
 
+# CHECK: WASM doesn't support data symbols in text sections
+  .type objerr,@object
+objerr:
+
 # CHECK: End of block construct with no start: end_try
     end_try
 test0:
     .functype   test0 () -> ()
 # CHECK: Block construct type mismatch, expected: end_function, instead got: end_loop
     end_loop
     block
 # CHECK: Block construct type mismatch, expected: end_block, instead got: end_if