[WebAssembly] Report error for inline assembly with unsupported opcodes

[mojingran]

Summary

For inline WebAssembly, passing a numeric operand to global.get is unsupported. This causes encodeInstruction to reach an llvm_unreachable block, leading to undefined behaviors. This patch fixes the issue for this invalid instruction encoding, making it report an error by adding an MCContext field in class WebAssemblyMCCodeEmitter.

[sbc100]

Nice! Can we get a test for this?

[mojingran]

In D154734#4481515, @sbc100 wrote:

Nice! Can we get a test for this?

I just added you as a reviewer in the test patch :)

[sbc100]

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp

129

Is this error specific to inline assembly?

Looking at global_set_expected_expression_operand in llvm/test/MC/WebAssembly/type-checker-errors.s I see a different error.. so I guess maybe it fails before it gets here in the case to non-inline assembly?

I wonder if we shouldn't just allow this.. even though its kind odd to want to do it..

[sbc100]

In D154734#4481538, @mojingran wrote:

In D154734#4481515, @sbc100 wrote:

Nice! Can we get a test for this?

I just added you as a reviewer in the test patch :)

Thanks. Wouldn't it be better to add the test in the same patch as the change its testing?

[mojingran]

In D154734#4481570, @sbc100 wrote:

Thanks. Wouldn't it be better to add the test in the same patch as the change its testing?

I followed this guide. I thought it might be better style.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp

129

Noted, I tried llvm-mc on the assembly generated from my testcase, and it is also giving a different error. I am not sure why it is behaving differently if we split up the 2 stages.

[dschuff]

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp

129

I wonder if we shouldn't just allow this.. even though its kind odd to want to do it..

In other architectures there are use cases for hardcoding register names as well as using symbols and letting the regalloc choose. Some of them are for constraints in particular instructions (e.g. instruction that always leaves 2 outputs in rax/rcx), or for ABIs that specify register use. Wasm doesn't really have either one of those. So I think I'd be fine having this limitation until someone proposes a use case for it.

[mojingran]

Added the testcase to check for an appropriate error message when an unsupported inline WebAssembly is provided.

[bryanpkc]

llvm/test/CodeGen/WebAssembly/inline-asm-failure.ll

3

NIT: Can you add a space between the semi-colon and RUN:? Same thing for the ;CHECK: line below.

4

It might be useful to rename this to "rust-issue-111471.c".

30

Can you remove !0 and !1, as well as !llvm.module.flags and !llvm.ident? These metadata are irrelevant to the test case. Rename !2 to !0.

[mojingran]

Abandoned irrelevant metadata for the testcase, and reformatted MCTargetDesc.h.

[mojingran]

Fixing testcase

[bryanpkc]

@mojingran Thanks. I think we can land this, unless @sbc100 has more comments.

[mojingran]

Missing WebAssemblyMCTargetDesc.cpp file added.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp

 ///
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/WebAssemblyFixupKinds.h"
 #include "MCTargetDesc/WebAssemblyMCTargetDesc.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/MC/MCCodeEmitter.h"
+#include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCFixup.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCSymbol.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/EndianStream.h"
 #include "llvm/Support/LEB128.h"
+#include "llvm/Support/SMLoc.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "mccodeemitter"
 
 STATISTIC(MCNumEmitted, "Number of MC instructions emitted.");
 STATISTIC(MCNumFixups, "Number of MC fixups created.");
 
 namespace {
 class WebAssemblyMCCodeEmitter final : public MCCodeEmitter {
   const MCInstrInfo &MCII;
-
+  MCContext &Ctx;
   // Implementation generated by tablegen.
   uint64_t getBinaryCodeForInstr(const MCInst &MI,
                                  SmallVectorImpl<MCFixup> &Fixups,
                                  const MCSubtargetInfo &STI) const;
 
   void encodeInstruction(const MCInst &MI, SmallVectorImpl<char> &CB,
                          SmallVectorImpl<MCFixup> &Fixups,
                          const MCSubtargetInfo &STI) const override;
 
 public:
-  WebAssemblyMCCodeEmitter(const MCInstrInfo &MCII) : MCII(MCII) {}
+  WebAssemblyMCCodeEmitter(const MCInstrInfo &MCII, MCContext &Ctx)
+      : MCII(MCII), Ctx{Ctx} {}
 };
 } // end anonymous namespace
 
-MCCodeEmitter *llvm::createWebAssemblyMCCodeEmitter(const MCInstrInfo &MCII) {
-  return new WebAssemblyMCCodeEmitter(MCII);
+MCCodeEmitter *llvm::createWebAssemblyMCCodeEmitter(const MCInstrInfo &MCII,
+                                                    MCContext &Ctx) {
+  return new WebAssemblyMCCodeEmitter(MCII, Ctx);
 }
 
 void WebAssemblyMCCodeEmitter::encodeInstruction(
     const MCInst &MI, SmallVectorImpl<char> &CB,
     SmallVectorImpl<MCFixup> &Fixups, const MCSubtargetInfo &STI) const {
   raw_svector_ostream OS(CB);
   uint64_t Start = OS.tell();
 
if (MO.isReg()) {
           break;
         case WebAssembly::OPERAND_VEC_I32IMM:
           support::endian::write<uint32_t>(OS, MO.getImm(), support::little);
           break;
         case WebAssembly::OPERAND_VEC_I64IMM:
           support::endian::write<uint64_t>(OS, MO.getImm(), support::little);
           break;
         case WebAssembly::OPERAND_GLOBAL:
-          llvm_unreachable("wasm globals should only be accessed symbolicly");
+          Ctx.reportError(
+              SMLoc(),
+              Twine("Wasm globals should only be accessed symbolically!"));
         default:
           encodeULEB128(uint64_t(MO.getImm()), OS);
         }
       } else {
         encodeULEB128(uint64_t(MO.getImm()), OS);
       }
 
     } else if (MO.isSFPImm()) {

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h

 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_WEBASSEMBLY_MCTARGETDESC_WEBASSEMBLYMCTARGETDESC_H
 #define LLVM_LIB_TARGET_WEBASSEMBLY_MCTARGETDESC_WEBASSEMBLYMCTARGETDESC_H
 
 #include "../WebAssemblySubtarget.h"
 #include "llvm/BinaryFormat/Wasm.h"
+#include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCInstrDesc.h"
 #include "llvm/Support/DataTypes.h"
 #include <memory>
 
 namespace llvm {
 
 class MCAsmBackend;
 class MCCodeEmitter;
 class MCInstrInfo;
 class MCObjectTargetWriter;
 class Triple;
 
-MCCodeEmitter *createWebAssemblyMCCodeEmitter(const MCInstrInfo &MCII);
+MCCodeEmitter *createWebAssemblyMCCodeEmitter(const MCInstrInfo &MCII,
+                                              MCContext &Ctx);
 
 MCAsmBackend *createWebAssemblyAsmBackend(const Triple &TT);
 
 std::unique_ptr<MCObjectTargetWriter>
 createWebAssemblyWasmObjectWriter(bool Is64Bit, bool IsEmscripten);
 
 namespace WebAssembly {
 

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.cpp

static MCInstPrinter *createMCInstPrinter(const Triple & /*T*/,
                                           const MCInstrInfo &MII,
                                           const MCRegisterInfo &MRI) {
   assert(SyntaxVariant == 0 && "WebAssembly only has one syntax variant");
   return new WebAssemblyInstPrinter(MAI, MII, MRI);
 }
 
 static MCCodeEmitter *createCodeEmitter(const MCInstrInfo &MCII,
                                         MCContext &Ctx) {
-  return createWebAssemblyMCCodeEmitter(MCII);
+  return createWebAssemblyMCCodeEmitter(MCII, Ctx);
 }
 
 static MCAsmBackend *createAsmBackend(const Target & /*T*/,
                                       const MCSubtargetInfo &STI,
                                       const MCRegisterInfo & /*MRI*/,
                                       const MCTargetOptions & /*Options*/) {
   return createWebAssemblyAsmBackend(STI.getTargetTriple());
 }

llvm/test/CodeGen/WebAssembly/inline-asm-failure.ll

+; RUN: not llc -O0 --mtriple=wasm32 -filetype=obj \
+; RUN:     -o /dev/null 2>&1 <%s | FileCheck %s
+source_filename = "rust-issue-111471.c"
+target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128-ni:1:10:20"
+target triple = "wasm32-unknown-unknown"
+
+@__main_void = hidden alias i32 (), ptr @main
+
+; Function Attrs: noinline nounwind optnone
+define hidden void @get_global() #0 {
+entry:
+  call void asm sideeffect "global.get 0", ""() #1, !srcloc !0
+  ret void
+}
+
+; Function Attrs: noinline nounwind optnone
+define hidden i32 @main() #0 {
+entry:
+  ret i32 0
+}
+
+attributes #0 = { noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+mutable-globals,+sign-ext" }
+attributes #1 = { nounwind }
+
+!0 = !{i64 32}
+; CHECK: <unknown>:0: error: Wasm globals should only be accessed symbolically!