[CodeGen] emit inline asm clobber list warnings for reserved

[stuij]

Summary

Currently, in line with GCC, when specifying reserved registers like sp or pc on an inline asm() clobber list, we don't always preserve the original value across the statement. And in general, overwriting reserved registers can have surprising results.

For example:

extern int bar(int[]);

int foo(int i) {
  int a[i]; // VLA
  asm volatile(
      "mov r7, #1"
    :
    :
    : "r7"
  );

  return 1 + bar(a);
}

Compiled for thumb, this gives:

$ clang --target=arm-arm-none-eabi -march=armv7a -c test.c -o - -S -O1 -mthumb
...
foo:
        .fnstart
@ %bb.0:                                @ %entry
        .save   {r4, r5, r6, r7, lr}
        push    {r4, r5, r6, r7, lr}
        .setfp  r7, sp, #12
        add     r7, sp, #12
        .pad    #4
        sub     sp, #4
        movs    r1, #7
        add.w   r0, r1, r0, lsl #2
        bic     r0, r0, #7
        sub.w   r0, sp, r0
        mov     sp, r0
        @APP
        mov.w   r7, #1
        @NO_APP
        bl      bar
        adds    r0, #1
        sub.w   r4, r7, #12
        mov     sp, r4
        pop     {r4, r5, r6, r7, pc}
...

r7 is used as the frame pointer for thumb targets, and this function needs to restore the SP from the FP because of the variable-length stack allocation a. r7 is clobbered by the inline assembly (and r7 is included in the clobber list), but LLVM does not preserve the value of the frame pointer across the assembly block.

This type of behavior is similar to GCC's and has been discussed on the bugtracker: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=11807 . No consensus seemed to have been reached on the way forward. Clang behavior has briefly been discussed on the CFE mailing (starting here: http://lists.llvm.org/pipermail/cfe-dev/2018-July/058392.html). I've opted for following Eli Friedman's advice to print warnings when there are reserved registers on the clobber list so as not to diverge from GCC behavior for now.

The patch uses MachineRegisterInfo's target-specific knowledge of reserved registers, just before we convert the inline asm string in the AsmPrinter.

If we find a reserved register, we print a warning:

repro.c:6:7: warning: inline asm clobber list contains reserved registers: R7 [-Winline-asm]
      "mov r7, #1"
      ^

[efriedma]

This is a good idea.

Can you add a note to the diagnostic explaining why this is a problem?

Please add a testcase for clobbering the base pointer in a function with a VLA.

[stuij]

Added an explanatory note to the warning, and added a VLA testcase.

[efriedma]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

564 ↗

(On Diff #157486)

I'm not sure what "may not be preserved across function calls" has to do with this warning. And I'd prefer "undefined behavior" rather than "unexpected results".

[stuij]

updated warning message

[stuij]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

564 ↗

(On Diff #157486)

Ah thanks, "across function calls" was a logical typo. That should read "across the asm statement".

[stuij]

ping

[efriedma]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

556 ↗

(On Diff #157932)

Standard style is std::string Msg = "...";.

566 ↗

(On Diff #157932)

Doesn't the note need the same location to print correctly?

[stuij]

lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

566 ↗

(On Diff #157932)

With a clean SMLoc, the note doesn't contain line info. I thought that was cleaner than adding line info. This way it's more obvious on sight that the note is explaining something about the warning.

But thinking about it a bit more, it is a bit idiosyncratic. Changed to regular behavior in this patch.

[stuij]

Follow LLVM style.

[efriedma]

LGTM

llvm/trunk/include/llvm/CodeGen/AsmPrinter.h

EmitInlineAsm(StringRef Str, const MCSubtargetInfo &STI,
                 const MCTargetOptions &MCOptions,
                 const MDNode *LocMDNode = nullptr,
                 InlineAsm::AsmDialect AsmDialect = InlineAsm::AD_ATT) const;
 
   /// This method formats and emits the specified machine instruction that is an
   /// inline asm.
   void EmitInlineAsm(const MachineInstr *MI) const;
 
+  /// Add inline assembly info to the diagnostics machinery, so we can
+  /// emit file and position info. Returns SrcMgr memory buffer position.
+  unsigned addInlineAsmDiagBuffer(StringRef AsmStr,
+                                  const MDNode *LocMDNode) const;
+
   //===------------------------------------------------------------------===//
   // Internal Implementation Details
   //===------------------------------------------------------------------===//
 
   /// This emits visibility information about symbol, if this is supported by
   /// the target.
   void EmitVisibility(MCSymbol *Sym, unsigned Visibility,
                       bool IsDefinition = true) const;

llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

if (LocInfo->getNumOperands() != 0)
       if (const ConstantInt *CI =
               mdconst::dyn_extract<ConstantInt>(LocInfo->getOperand(ErrorLine)))
         LocCookie = CI->getZExtValue();
   }
 
   DiagInfo->DiagHandler(Diag, DiagInfo->DiagContext, LocCookie);
 }
 
-/// EmitInlineAsm - Emit a blob of inline asm to the output streamer.
-void AsmPrinter::EmitInlineAsm(StringRef Str, const MCSubtargetInfo &STI,
-                               const MCTargetOptions &MCOptions,
-                               const MDNode *LocMDNode,
-                               InlineAsm::AsmDialect Dialect) const {
-  assert(!Str.empty() && "Can't emit empty inline asm block");
-
-  // Remember if the buffer is nul terminated or not so we can avoid a copy.
-  bool isNullTerminated = Str.back() == 0;
-  if (isNullTerminated)
-    Str = Str.substr(0, Str.size()-1);
-
-  // If the output streamer does not have mature MC support or the integrated
-  // assembler has been disabled, just emit the blob textually.
-  // Otherwise parse the asm and emit it via MC support.
-  // This is useful in case the asm parser doesn't handle something but the
-  // system assembler does.
-  const MCAsmInfo *MCAI = TM.getMCAsmInfo();
-  assert(MCAI && "No MCAsmInfo");
-  if (!MCAI->useIntegratedAssembler() &&
-      !OutStreamer->isIntegratedAssemblerRequired()) {
-    emitInlineAsmStart();
-    OutStreamer->EmitRawText(Str);
-    emitInlineAsmEnd(STI, nullptr);
-    return;
-  }
-
+unsigned AsmPrinter::addInlineAsmDiagBuffer(StringRef AsmStr,
+                                            const MDNode *LocMDNode) const {
   if (!DiagInfo) {
     DiagInfo = make_unique<SrcMgrDiagInfo>();
 
     MCContext &Context = MMI->getContext();
     Context.setInlineSourceManager(&DiagInfo->SrcMgr);
 
     LLVMContext &LLVMCtx = MMI->getModule()->getContext();
     if (LLVMCtx.getInlineAsmDiagnosticHandler()) {
       DiagInfo->DiagHandler = LLVMCtx.getInlineAsmDiagnosticHandler();
       DiagInfo->DiagContext = LLVMCtx.getInlineAsmDiagnosticContext();
       DiagInfo->SrcMgr.setDiagHandler(srcMgrDiagHandler, DiagInfo.get());
     }
   }
 
   SourceMgr &SrcMgr = DiagInfo->SrcMgr;
-  SrcMgr.setIncludeDirs(MCOptions.IASSearchPaths);
 
   std::unique_ptr<MemoryBuffer> Buffer;
-  // The inline asm source manager will outlive Str, so make a copy of the
+  // The inline asm source manager will outlive AsmStr, so make a copy of the
   // string for SourceMgr to own.
-  Buffer = MemoryBuffer::getMemBufferCopy(Str, "<inline asm>");
+  Buffer = MemoryBuffer::getMemBufferCopy(AsmStr, "<inline asm>");
 
   // Tell SrcMgr about this buffer, it takes ownership of the buffer.
   unsigned BufNum = SrcMgr.AddNewSourceBuffer(std::move(Buffer), SMLoc());
 
   // Store LocMDNode in DiagInfo, using BufNum as an identifier.
   if (LocMDNode) {
     DiagInfo->LocInfos.resize(BufNum);
-    DiagInfo->LocInfos[BufNum-1] = LocMDNode;
+    DiagInfo->LocInfos[BufNum - 1] = LocMDNode;
   }
 
-  std::unique_ptr<MCAsmParser> Parser(
-      createMCAsmParser(SrcMgr, OutContext, *OutStreamer, *MAI, BufNum));
+  return BufNum;
+}
+
+
+/// EmitInlineAsm - Emit a blob of inline asm to the output streamer.
+void AsmPrinter::EmitInlineAsm(StringRef Str, const MCSubtargetInfo &STI,
+                               const MCTargetOptions &MCOptions,
+                               const MDNode *LocMDNode,
+                               InlineAsm::AsmDialect Dialect) const {
+  assert(!Str.empty() && "Can't emit empty inline asm block");
+
+  // Remember if the buffer is nul terminated or not so we can avoid a copy.
+  bool isNullTerminated = Str.back() == 0;
+  if (isNullTerminated)
+    Str = Str.substr(0, Str.size()-1);
+
+  // If the output streamer does not have mature MC support or the integrated
+  // assembler has been disabled, just emit the blob textually.
+  // Otherwise parse the asm and emit it via MC support.
+  // This is useful in case the asm parser doesn't handle something but the
+  // system assembler does.
+  const MCAsmInfo *MCAI = TM.getMCAsmInfo();
+  assert(MCAI && "No MCAsmInfo");
+  if (!MCAI->useIntegratedAssembler() &&
+      !OutStreamer->isIntegratedAssemblerRequired()) {
+    emitInlineAsmStart();
+    OutStreamer->EmitRawText(Str);
+    emitInlineAsmEnd(STI, nullptr);
+    return;
+  }
+
+  unsigned BufNum = addInlineAsmDiagBuffer(Str, LocMDNode);
+  DiagInfo->SrcMgr.setIncludeDirs(MCOptions.IASSearchPaths);
+
+  std::unique_ptr<MCAsmParser> Parser(createMCAsmParser(
+          DiagInfo->SrcMgr, OutContext, *OutStreamer, *MAI, BufNum));
 
   // Do not use assembler-level information for parsing inline assembly.
   OutStreamer->setUseAssemblerInfoForParsing(false);
 
   // We create a new MCInstrInfo here since we might be at the module level
   // and not have a MachineFunction to initialize the TargetInstrInfo from and
   // we only need MCInstrInfo for asm parsing. We create one unconditionally
   // because it's not subtarget dependent.
void AsmPrinter::EmitInlineAsm(const MachineInstr *MI) const {
   else
     EmitMSInlineAsmStr(AsmStr, MI, MMI, InlineAsmVariant, AP, LocCookie, OS);
 
   // Reset SanitizeAddress based on the function's attribute.
   MCTargetOptions MCOptions = TM.Options.MCOptions;
   MCOptions.SanitizeAddress =
       MF->getFunction().hasFnAttribute(Attribute::SanitizeAddress);
 
+  // Emit warnings if we use reserved registers on the clobber list, as
+  // that might give surprising results.
+  std::vector<std::string> RestrRegs;
+  // Start with the first operand descriptor, and iterate over them.
+  for (unsigned I = InlineAsm::MIOp_FirstOperand, NumOps = MI->getNumOperands();
+       I < NumOps; ++I) {
+    const MachineOperand &MO = MI->getOperand(I);
+    if (MO.isImm()) {
+      unsigned Flags = MO.getImm();
+      if (InlineAsm::getKind(Flags) == InlineAsm::Kind_Clobber &&
+          MF->getRegInfo().isReserved(MI->getOperand(I + 1).getReg())) {
+        const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
+        RestrRegs.push_back(TRI->getName(MI->getOperand(I + 1).getReg()));
+      }
+      // Skip to one before the next operand descriptor, if it exists.
+      I += InlineAsm::getNumOperandRegisters(Flags);
+    }
+  }
+
+  if (!RestrRegs.empty()) {
+    unsigned BufNum = addInlineAsmDiagBuffer(OS.str(), LocMD);
+    auto &SrcMgr = DiagInfo->SrcMgr;
+    SMLoc Loc = SMLoc::getFromPointer(
+        SrcMgr.getMemoryBuffer(BufNum)->getBuffer().begin());
+
+    std::string Msg = "inline asm clobber list contains reserved registers: ";
+    for (auto I = RestrRegs.begin(), E = RestrRegs.end(); I != E; I++) {
+      if(I != RestrRegs.begin())
+        Msg += ", ";
+      Msg += *I;
+    }
+    std::string Note = "Reserved registers on the clobber list may not be "
+                "preserved across the asm statement, and clobbering them may "
+                "lead to undefined behaviour.";
+    SrcMgr.PrintMessage(Loc, SourceMgr::DK_Warning, Msg);
+    SrcMgr.PrintMessage(Loc, SourceMgr::DK_Note, Note);
+  }
+
   EmitInlineAsm(OS.str(), getSubtargetInfo(), MCOptions, LocMD,
                 MI->getInlineAsmDialect());
 
   // Emit the #NOAPP end marker.  This has to happen even if verbose-asm isn't
   // enabled, so we use emitRawComment.
   OutStreamer->emitRawComment(MAI->getInlineAsmEnd());
 }
 

llvm/trunk/test/CodeGen/AArch64/inline-asm-clobber.ll

+; RUN: llc <%s -mtriple=aarch64-none-eabi 2>&1  | FileCheck %s
+
+; CHECK: warning: inline asm clobber list contains reserved registers: SP
+
+define void @foo() nounwind {
+  call void asm sideeffect "mov x7, #1", "~{x7},~{sp}"()
+  ret void
+}

llvm/trunk/test/CodeGen/ARM/inline-asm-clobber.ll

+; RUN: llc <%s -mtriple=arm-none-eabi 2>&1 | FileCheck %s -check-prefix=CHECK
+
+; RUN: llc <%s -mtriple=arm-none-eabi -relocation-model=rwpi 2>&1 \
+; RUN:   | FileCheck %s -check-prefix=RWPI
+
+; RUN: llc <%s -mtriple=arm-none-eabi --disable-fp-elim 2>&1 \
+; RUN:   | FileCheck %s -check-prefix=NO_FP_ELIM
+
+; CHECK: warning: inline asm clobber list contains reserved registers: SP, PC
+; CHECK: warning: inline asm clobber list contains reserved registers: R11
+; RWPI: warning: inline asm clobber list contains reserved registers: R9, SP, PC
+; RWPI: warning: inline asm clobber list contains reserved registers: R11
+; NO_FP_ELIM: warning: inline asm clobber list contains reserved registers: R11, SP, PC
+; NO_FP_ELIM: warning: inline asm clobber list contains reserved registers: R11
+
+define void @foo() nounwind {
+  call void asm sideeffect "mov r7, #1",
+    "~{r9},~{r11},~{r12},~{lr},~{sp},~{pc},~{r10}"()
+  ret void
+}
+
+define i32 @bar(i32 %i) {
+  %vla = alloca i32, i32 %i, align 4
+  tail call void asm sideeffect "mov r7, #1", "~{r11}"()
+  %1 = load volatile i32, i32* %vla, align 4
+  ret i32 %1
+}

llvm/trunk/test/CodeGen/X86/inline-asm-clobber.ll

+; RUN: llc <%s -mtriple=x86_64-unknown-unknown -- 2>&1  | FileCheck %s
+
+; CHECK: warning: inline asm clobber list contains reserved registers: RSP, EBP
+
+define void @foo() nounwind {
+  call void asm sideeffect "mov $$0x12, %eax", "~{eax},~{rsp},~{esi},~{ebp}"()
+  ret void
+}