[llvm][AsmPrinter] Restore source location to register clobber warning

[DavidSpickett]

Summary

Since 5de2d189e6ad466a1f0616195e8c524a4eb3cbc0 this particular warning
hasn't had the location of the source file containing the inline
assembly.

Fix this by reporting via LLVMContext. Which means that we no longer
have the "instantiated into assembly here" lines but they were going to
point to the start of the inline asm string anyway.

This message is already tested via IR in llvm. However we won't have
the required location info there so I've added a C file test in clang
to cover it.
(though strictly, this is testing llvm code)

[DavidSpickett]

This works as expected, let me know if this is the intended use relative to the changes you made.

The existing test in llvm llvm/test/CodeGen/AArch64/inline-asm-clobber.ll never reported the location in the .ll file so I can't test it there. (which might be its own issue) I looked for any similar tests but didn't find anything, testing from clang with a C file seems like the only way.

clang/test/Misc/inline-asm-clobber-warning.c

26

I think this ^ is wrong because we don't/can't account for the asm printer tabbing in the instruction.

I still wanted to check that there are ^ just because it's one aspect of the location information.

[ychen]

clang/test/Misc/inline-asm-clobber-warning.c

26

This probably is due to Loc is not right.

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

545

For this diagnose, it is better to use LLVMContext since the location inside inlineasm does not matter. There is an example above.

Msg << "invalid operand in inline asm: '" << AsmStr << "'";
MMI->getModule()->getContext().emitError(LocCookie, Msg.str());

[DavidSpickett]

• Report via LLVMContext

[ychen]

Looks good except for one suggestion and the patch need rebasing on an upstream commit.

llvm/include/llvm/IR/LLVMContext.h

300 ↗

(On Diff #344821)

It would be best to avoid adding these two methods for single-use since LLVMContext.h is included everywhere. We could add them once it turns out to be needed in other places. Could we use LLVMContext::diagnose instead?

[DavidSpickett]

• Use diagnose instead of adding methods to LLVMContext
• clang-format the test file

[ychen]

LGTM. Thanks.

clang/test/Misc/inline-asm-clobber-warning.c

+/// This test checks that the warning includes the location in the C source
+/// file that contains the inline asm. Instead of saying <inline asm> for both.
+/// Although this warning is emitted in llvm it cannot be tested from IR as
+/// it does not have that location information at that stage.
+
+// RUN: %clang -target arm-arm-none-eabi -march=armv7-m -c %s -o /dev/null \
+// RUN:   2>&1 | FileCheck %s
+
+// REQUIRES: arm-registered-target
+
+void bar(void) {
+    __asm__ __volatile__ ( "nop" : : : "sp");
+}
+
+// CHECK:      inline-asm-clobber-warning.c:12:28: warning: inline asm clobber list contains reserved registers: SP [-Winline-asm]
+// CHECK-NEXT:     __asm__ __volatile__ ( "nop" : : : "sp");
+// CHECK-NEXT:                            ^
+// CHECK-NEXT: <inline asm>:1:1: note: instantiated into assembly here
+// CHECK-NEXT:         nop
+// CHECK-NEXT: ^
+// CHECK-NEXT: inline-asm-clobber-warning.c:12:28: note: Reserved registers on the clobber list may not be preserved across the asm statement, and clobbering them may lead to undefined behaviour.
+// CHECK-NEXT:     __asm__ __volatile__ ( "nop" : : : "sp");
+// CHECK-NEXT:                            ^
+// CHECK-NEXT: <inline asm>:1:1: note: instantiated into assembly here
+// CHECK-NEXT:         nop
+// CHECK-NEXT: ^

llvm/include/llvm/MC/MCContext.h

public:
     }
 
     void deallocate(void *Ptr) {}
 
     bool hadError() { return HadError; }
     void diagnose(const SMDiagnostic &SMD);
     void reportError(SMLoc L, const Twine &Msg);
     void reportWarning(SMLoc L, const Twine &Msg);
+    void reportNote(SMLoc L, const Twine &Msg);
     // Unrecoverable error has occurred. Display the best diagnostic we can
     // and bail via exit(1). For now, most MC backend errors are unrecoverable.
     // FIXME: We should really do something about that.
     LLVM_ATTRIBUTE_NORETURN void reportFatalError(SMLoc L, const Twine &Msg);
 
     const MCAsmMacro *lookupMacro(StringRef Name) {
       StringMap<MCAsmMacro>::iterator I = MacroMap.find(Name);
       return (I == MacroMap.end()) ? nullptr : &I->getValue();

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

if (!RestrRegs.empty()) {
     for (const Register &RR : RestrRegs) {
       Msg += LS;
       Msg += TRI->getName(RR);
     }
     const char *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);
+    MMI->getContext().reportWarning(Loc, Msg);
+    MMI->getContext().reportNote(Loc, Note);
   }
 
   emitInlineAsm(OS.str(), getSubtargetInfo(), TM.Options.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/lib/MC/MCContext.cpp

if (TargetOptions && TargetOptions->MCFatalWarnings) {
     reportError(Loc, Msg);
   } else {
     reportCommon(Loc, [&](SMDiagnostic &D, const SourceMgr *SMP) {
       D = SMP->GetMessage(Loc, SourceMgr::DK_Warning, Msg);
     });
   }
 }
 
+void MCContext::reportNote(SMLoc Loc, const Twine &Msg) {
+  reportCommon(Loc, [&](SMDiagnostic &D, const SourceMgr *SMP) {
+    D = SMP->GetMessage(Loc, SourceMgr::DK_Note, Msg);
+  });
+}
+
 void MCContext::reportFatalError(SMLoc Loc, const Twine &Msg) {
   reportError(Loc, Msg);
 
   // If we reached here, we are failing ungracefully. Run the interrupt handlers
   // to make sure any special cleanups get done, in particular that we remove
   // files registered with RemoveFileOnSignal.
   sys::RunInterruptHandlers();
   exit(1);
 }