[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

59

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

59

Was able to get rid of the deprecated method.

[sbc100]

llvm/lib/MC/WasmObjectWriter.cpp

1708

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; }
+  // Data is the default value if not set.
+  bool isData() const { return !Type || 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; }
+
+  Optional<wasm::WasmSymbolType> getType() 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);
   }
 
   bool isNoStrip() const {
     return getFlags() & wasm::WASM_SYMBOL_NO_STRIP;
   }
   void setNoStrip() const {
     modifyFlags(wasm::WASM_SYMBOL_NO_STRIP, wasm::WASM_SYMBOL_NO_STRIP);
   }

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 ||
if (Mode != DwoMode::DwoOnly) {
     // Populate WasmIndices and DataLocations for defined symbols.
     for (const MCSymbol &S : Asm.symbols()) {
       // Ignore unnamed temporary symbols, which aren't ever exported, imported,
       // or used in relocations.
       if (S.isTemporary() && S.getName().empty())
         continue;
 
       const auto &WS = static_cast<const MCSymbolWasm &>(S);
-      LLVM_DEBUG(
-          dbgs() << "MCSymbol: " << toString(WS.getType()) << " '" << S << "'"
+      LLVM_DEBUG(dbgs()
+                 << "MCSymbol: "
+                 << toString(WS.getType().getValueOr(wasm::WASM_SYMBOL_TYPE_DATA))
+                 << " '" << S << "'"
                  << " isDefined=" << S.isDefined() << " isExternal="
                  << S.isExternal() << " isTemporary=" << S.isTemporary()
                  << " isWeak=" << WS.isWeak() << " isHidden=" << WS.isHidden()
                  << " isVariable=" << WS.isVariable() << "\n");
 
       if (WS.isVariable())
         continue;
       if (WS.isComdat() && !WS.isDefined())
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.Kind = WS.getType().getValueOr(wasm::WASM_SYMBOL_TYPE_DATA);
     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->getType() == 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