[CodeGen] emit inline asm clobber list warnings for reserved (cont)

[stuij]

Summary

This is a continuation of https://reviews.llvm.org/D49727
Below the original text, current changes in the comments:

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"
      ^

[stuij]

Unfortunately the original patch was a bit too general, in that on some architectures certain reserved registers on an asm clobber list will make sure that register isn't overridden. There's no one-to-one mapping between clobber behaviour and reserved registers.

An example of that is the PowerPC ctr register:

volatile int a[50];
int main(int i) {

  while(i--) {
    asm volatile(
        "li    29, 0"
        :
        :
        : "ctr"
                 );

    a[i] = 1;
 }

  return 1; 
}

In this example, the use of the ppc ctr instruction is avoided altogether, but not when it's not on the clobber list.

To avoid these kinds of false-positives, I've added a new isAsmClobberable method to TargetRegisterInfo that architectures can implement if they want to emit these asm clobber warning messages.

[efriedma]

LGTM

[efriedma]

Hmm... actually, looking at this again, I guess we actually have some support for "clobbering" sp, so maybe we shouldn't warn on that. Granted, I don't think it's documented anywhere.

[stuij]

Could you tell me how this sp clobbering support works?

[efriedma]

FunctionLoweringInfo::set detects inline asm which clobbers "sp", and calls setHasOpaqueSPAdjustment. And targets can check it to do something special with frame lowering... I guess the intent is to allow inline asm to allocate memory on the stack. It looks like only the x86 and PPC backends actually do anything with it, though.

[stuij]

Had a look at the uses for hasOpaqueSPAdjustment. It looks like the the PPC and X86 backends only use it to check for 'how trustworthy is this SP' kind of checks in things like base pointer logic. So we should be fine on this front.

[efriedma]

K, then let's not worry about it (particularly since it doesn't do anything on ARM or AArch64 anyway).

[craig.topper]

We're seeing a failure from this on this test case. Looks like somehow the operand after an InlineAsm::Kind_Clobber has become an immediate rather than a register. So the call to getReg fails.

source_filename = "test.c"
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@test_mem = dso_local global [16 x i8] c"UUUUUUUUUUUUUUUU", align 16

; Function Attrs: nounwind uwtable
define dso_local void @foo() local_unnamed_addr {
entry:
  tail call void asm sideeffect inteldialect "clc\0A\09cmpxchg8b $0\0A\09cmpxchg16b $1\0A\09clc", "=*m,=*m,~{eax},~{edx},~{flags},~{rax},~{rdx},~{dirflag},~{fpsr},~{flags}"([16 x i8]* nonnull @test_mem, [16 x i8]* nonnull @test_mem) #1
  ret void
}

[craig.topper]

For the failure I posted. It looks like the Live Variable Analysis Pass is removing a register operand for a clobber without removing the immediate operand preceeding that contains the inline assembly flags that indicate it was a clobber. So later we see the clobber flag, but the register operand after it is missing.

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/include/llvm/CodeGen/TargetRegisterInfo.h

public:
   /// - is not allocatable
   /// - is considered always live
   /// - is ignored by liveness tracking
   /// It is often necessary to reserve the super registers of a reserved
   /// register as well, to avoid them getting allocated indirectly. You may use
   /// markSuperRegs() and checkAllSuperRegsMarked() in this case.
   virtual BitVector getReservedRegs(const MachineFunction &MF) const = 0;
 
+  /// Returns false if we can't guarantee that Physreg, specified as an IR asm
+  /// clobber constraint, will be preserved across the statement.
+  virtual bool isAsmClobberable(const MachineFunction &MF,
+                               unsigned PhysReg) const {
+    return true;
+  }
+
   /// Returns true if PhysReg is unallocatable and constant throughout the
   /// function.  Used by MachineRegisterInfo::isConstantPhysReg().
   virtual bool isConstantPhysReg(unsigned PhysReg) const { return false; }
 
   /// Physical registers that may be modified within a function but are
   /// guaranteed to be restored before any uses. This is useful for targets that
   /// have call sequences where a GOT register may be updated by the caller
   /// prior to a call and is guaranteed to be restored (also by the caller)

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();
+      const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
+      if (InlineAsm::getKind(Flags) == InlineAsm::Kind_Clobber &&
+          !TRI->isAsmClobberable(*MF, MI->getOperand(I + 1).getReg())) {
+        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/lib/Target/AArch64/AArch64RegisterInfo.h

public:
   /// this property
   const uint32_t *getThisReturnPreservedMask(const MachineFunction &MF,
                                              CallingConv::ID) const;
 
   /// Stack probing calls preserve different CSRs to the normal CC.
   const uint32_t *getWindowsStackProbePreservedMask() const;
 
   BitVector getReservedRegs(const MachineFunction &MF) const override;
+  bool isAsmClobberable(const MachineFunction &MF,
+                       unsigned PhysReg) const override;
   bool isConstantPhysReg(unsigned PhysReg) const override;
   const TargetRegisterClass *
   getPointerRegClass(const MachineFunction &MF,
                      unsigned Kind = 0) const override;
   const TargetRegisterClass *
   getCrossCopyRegClass(const TargetRegisterClass *RC) const override;
 
   bool enableMultipleCopyHints() const override { return true; }

llvm/trunk/lib/Target/AArch64/AArch64RegisterInfo.cpp

bool AArch64RegisterInfo::isReservedReg(const MachineFunction &MF,
   case AArch64::FP:
   case AArch64::W29:
     return TFI->hasFP(MF) || TT.isOSDarwin();
   }
 
   return false;
 }
 
+bool AArch64RegisterInfo::isAsmClobberable(const MachineFunction &MF,
+                                          unsigned PhysReg) const {
+  return !isReservedReg(MF, PhysReg);
+}
+
 bool AArch64RegisterInfo::isConstantPhysReg(unsigned PhysReg) const {
   return PhysReg == AArch64::WZR || PhysReg == AArch64::XZR;
 }
 
 const TargetRegisterClass *
 AArch64RegisterInfo::getPointerRegClass(const MachineFunction &MF,
                                       unsigned Kind) const {
   return &AArch64::GPR64spRegClass;

llvm/trunk/lib/Target/ARM/ARMBaseRegisterInfo.h

public:
   /// i32 argument and an i32 return value)
   ///
   /// Should return NULL in the case that the calling convention does not have
   /// this property
   const uint32_t *getThisReturnPreservedMask(const MachineFunction &MF,
                                              CallingConv::ID) const;
 
   BitVector getReservedRegs(const MachineFunction &MF) const override;
+  bool isAsmClobberable(const MachineFunction &MF,
+                       unsigned PhysReg) const override;
 
   const TargetRegisterClass *
   getPointerRegClass(const MachineFunction &MF,
                      unsigned Kind = 0) const override;
   const TargetRegisterClass *
   getCrossCopyRegClass(const TargetRegisterClass *RC) const override;
 
   const TargetRegisterClass *

llvm/trunk/lib/Target/ARM/ARMBaseRegisterInfo.cpp

for (unsigned Reg : RC)
     for (MCSubRegIterator SI(Reg, this); SI.isValid(); ++SI)
       if (Reserved.test(*SI))
         markSuperRegs(Reserved, Reg);
 
   assert(checkAllSuperRegsMarked(Reserved));
   return Reserved;
 }
 
+bool ARMBaseRegisterInfo::
+isAsmClobberable(const MachineFunction &MF, unsigned PhysReg) const {
+  return !getReservedRegs(MF).test(PhysReg);
+}
+
 const TargetRegisterClass *
 ARMBaseRegisterInfo::getLargestLegalSuperClass(const TargetRegisterClass *RC,
                                                const MachineFunction &) const {
   const TargetRegisterClass *Super = RC;
   TargetRegisterClass::sc_iterator I = RC->getSuperClasses();
   do {
     switch (Super->getID()) {
     case ARM::GPRRegClassID:

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
+}