Error if inline assembly ends in unexpected mode

[niravd]

Summary

[x86] Inline assembly may issue directives that change the
bit-mode (i.e. .code16/.code32/.code64) but these changes do not do not
persist past the inline assembly call as is reasonable. In the case that
inline assembly terminates with a different mode, incorrect assembly may
be generated.

As this situation is likely a mistake and should be dealt with by the
user throw an error message.

[niravd]

ARM has a similar situation arise in their code but uses a different fix. There, this situation is fixed if needed if we use the integrated assembler and otherwise always emits a directive in no integrated assembler case. This guarantees any code llvm generates is always correct, but leaves no notification that there is a potential latent bug should you compile with a separate compiler.

We should be consistent here and either change ARM to also emit an error or change both to emit warnings.

[rovka]

In D20067#424935, @niravd wrote:

ARM has a similar situation arise in their code but uses a different fix. There, this situation is fixed if needed if we use the integrated assembler and otherwise always emits a directive in no integrated assembler case. This guarantees any code llvm generates is always correct, but leaves no notification that there is a potential latent bug should you compile with a separate compiler.

I believe this [1] is part of the discussion on the ARM side.

We should be consistent here and either change ARM to also emit an error or change both to emit warnings.

Changing ARM to emit an error or an on-by-default warning may break builds that didn't break before. Maybe a -Wall warning would be better? How does gcc handle this?

[1] http://reviews.llvm.org/D2255

[niravd]

Changing ARM to emit an error or an on-by-default warning may break builds that didn't break before. Maybe a -Wall warning would be better? How does gcc handle this?

I warning would be reasonable, but given GCC just inlines the assembly text which strikes me as them implicitly claiming this as ill-formed input as neither GCC nor clang with -fno-integrated-as have no way to ensure meaning propagation of mode changes past inline assembly, I'm leaning towards asserting a full on error over issuing a warning.

[1] http://reviews.llvm.org/D2255

I'll add the folks on D2255 and see what they think.

[rengolin]

Some more context:

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

[niravd]

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

Ahha. So gcc will insert .code 16 after inline assembly calls when called form thumb mode, but nothing for inline asm in arm mode (hence my confusion). Presumably this is because the textual thumb assembly language is a subset of the arm assembly language correct code is always generated. It should probably be left as is.

Regardless this does not happen in gcc's X86 compilation for any mode, so throwing an error here seems reasonable.

[jyknight]

I agree this looks like a good idea, and I do also think it probably ought to be an error, since the effects are likely fairly tragic if you get it wrong.

So, ARM is expected to always restore the proper thumb/arm mode after any inline asm block. But, what should (on arm) .cpu, .arch, .arch_extension, and .fpu do in inline-asm? Or .cpu, and .arch on AArch64? Or the similar things on MIPS?

IMO, those should all emit hard errors if we detect that the mode ends up being different than expected after the inline-asm block too. Perhaps the code to detect mode-switches should be done generically for all arches, after calling emitInlineAsmEnd (to give it the chance to restore the mode if appropriate)?

[dsanders]

MIPS avoids this problem in LLVM as a side-effect of something we did to match the assembler state that GCC uses for inline assembly. We emit a '.set push' in emitInlineAsmStart() to push the current assembler state onto a stack, followed by a few '.set ...' directives to change the state to what GCC uses. Then we emit a '.set pop' in emitInlineAsmEnd() to restore the state back to what we normally use for codegen. As a result, LLVM silently discards any state changes the user forgets to undo. This differs from GCC which will assume user did the right thing and will probably end up causing assembler errors or silent miscompilations.

That said, I think this kind of check is also useful for MIPS to catch portability bugs.

lib/Target/X86/X86AsmPrinter.h

public:
   }
 
   const X86Subtarget &getSubtarget() const { return *Subtarget; }
 
   void EmitStartOfAsmFile(Module &M) override;
 
   void EmitEndOfAsmFile(Module &M) override;
 
+  void emitInlineAsmEnd(const MCSubtargetInfo &StartInfo,
+                        const MCSubtargetInfo *EndInfo) const override;
+
   void EmitInstruction(const MachineInstr *MI) override;
 
   void EmitBasicBlockEnd(const MachineBasicBlock &MBB) override {
     SMShadowTracker.emitShadowPadding(*OutStreamer, getSubtargetInfo());
   }
 
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        unsigned AsmVariant, const char *ExtraCode,

lib/Target/X86/X86AsmPrinter.cpp

void X86AsmPrinter::EmitStartOfAsmFile(Module &M) {
 
   // If this is not inline asm and we're in 16-bit
   // mode prefix assembly with .code16.
   bool is16 = TT.getEnvironment() == Triple::CODE16;
   if (M.getModuleInlineAsm().empty() && is16)
     OutStreamer->EmitAssemblerFlag(MCAF_Code16);
 }
 
+void X86AsmPrinter::emitInlineAsmEnd(const MCSubtargetInfo &StartInfo,
+                                     const MCSubtargetInfo *EndInfo) const {
+  // If inline assembly left code directive in different state, this
+  // would change the compilation of the subsequent code. As we cannot
+  // fix this problem in the case of -fno-integrated-as explicitly fail
+  // with helpful error.
+  if (EndInfo && StartInfo.getFeatureBits() != EndInfo->getFeatureBits()) {
+    std::string msg;
+    raw_string_ostream Msg(msg);
+    Msg << "Inline Assembly terminated in "
+        << ((EndInfo->getFeatureBits()[X86::Mode64Bit])
+                ? "64"
+                : (EndInfo->getFeatureBits()[X86::Mode32Bit]) ? "32" : "16")
+        << "-bit mode, but should be returned to original mode. Consider "
+           "adding "
+        << ((StartInfo.getFeatureBits()[X86::Mode64Bit])
+                ? ".code64"
+                : (StartInfo.getFeatureBits()[X86::Mode32Bit]) ? ".code32"
+                                                               : ".code16")
+        << " directive";
+    MMI->getModule()->getContext().emitError(Msg.str());
+  }
+}
+
 static void
 emitNonLazySymbolPointer(MCStreamer &OutStreamer, MCSymbol *StubLabel,
                          MachineModuleInfoImpl::StubValueTy &MCSym) {
   // L_foo$stub:
   OutStreamer.EmitLabel(StubLabel);
   //   .indirect_symbol _foo
   OutStreamer.EmitSymbolAttribute(MCSym.getPointer(), MCSA_IndirectSymbol);
 

test/CodeGen/X86/inline-asm-mode-directives.ll

+; RUN: not llc < %s 2> %t
+; RUN: FileCheck %s < %t
+target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
+target triple = "i386-unknown-linux-code16"
+
+; Function Attrs: nounwind
+define i32 @main() #0 {
+  %retval = alloca i32, align 4
+  store i32 0, i32* %retval, align 4
+  call void asm sideeffect ".code32\0Apush 1", "~{dirflag},~{fpsr},~{flags}"() #0
+  ret i32 0
+}
+
+; CHECK: error: Inline Assembly terminated in 32-bit mode, but should be returned to original mode. Consider adding .code16 directive
+attributes #0 = { nounwind }
+
+!llvm.ident = !{!0}
+
+!0 = !{!"clang version 3.9.0 (trunk 265439) (llvm/trunk 265567)"}
 No newline at end of file