[clang] Use i64 for the !srcloc metadata on asm IR nodes.

[simon_tatham]

Summary

This is part of a patch series working towards the ability to make
SourceLocation into a 64-bit type to handle larger translation units.

!srcloc is generated in clang codegen, and pulled back out by llvm
functions like AsmPrinter::emitInlineAsm that need to report errors in
the inline asm. From there it goes to LLVMContext::emitError, is
stored in DiagnosticInfoInlineAsm, and ends up back in clang, at
BackendConsumer::InlineAsmDiagHandler(), which reconstitutes a true
clang::SourceLocation from the integer cookie.

Throughout this code path, it's now 64-bit rather than 32, which means
that if SourceLocation is expanded to a 64-bit type, this error report
won't lose half of the data.

The compiler will tolerate both of i32 and i64 !srcloc metadata in
input IR without faulting. Test added in llvm/MC. (The semantic
accuracy of the metadata is another matter, but I don't know of any
situation where that matters: if you're reading an IR file written by
a previous run of clang, you don't have the SourceManager that can
relate those source locations back to the original source files.)

Original version of the patch by Mikhail Maltsev.

[tmatheson]

Looks sensible to me, I don't think slightly expanding the size of the metadata and the diagnostic will be an issue.

[dexonsmith]

Does this need an upgrade for old bitcode that has serialized 32-bit types? If not, can you explain why not?

[tmatheson]

@dexonsmith I don't think they need upgraded. Most of the places I can see referencing !srcloc are copying it around and will preserve the i32 type. Cases which actually read the value are already reading it as 64 bit via getZExtValue and have been updated here, e.g. MachineInstr::emitError

[dexonsmith]

In D105491#2882549, @tmatheson wrote:

@dexonsmith I don't think they need upgraded. Most of the places I can see referencing !srcloc are copying it around and will preserve the i32 type. Cases which actually read the value are already reading it as 64 bit via getZExtValue and have been updated here, e.g. MachineInstr::emitError

Seems reasonable to me. I'd suggest documenting in the textual IR testcases for !srcloc (I assume there are existing ones for consumers, such as AsmPrinter::emitInlineAsm or something?) that i32 should continue to work, even though emission will be i64 now, and that if that changes a bitcode upgrade should be added. And maybe you can/should add examples to the test that use i64.

[simon_tatham]

I looked into this yesterday, and realised that I don't actually know what the use case is for emitting !srcloc metadata in an IR file.

It's more or less ignored by llc, as far as I can see: if there's a late-breaking error in the inline asm string, you just get a "note: !srcloc = <nnn>" alongside the main error.

And if the IR file is read back in by a second invocation of clang, then the frontend callback does get the !srcloc back from the IR, but it can't relate that to the original source code, because the new clang's SourceManager doesn't know anything about the C source files that the IR was made from. In the example test I just did by hand, the error message cites a location in the IR file that corresponds to the byte position of the error in the original C, which is more or less useless.

So I think this mechanism is only actually useful when the whole compilation is happening within a single invocation of clang, so that the !srcloc finds its way back to a SourceManager that still has all the actual source code in mind. As soon as IR files are exported and re-imported, the best we can hope for is "no crash".

But I can add a couple of tests along those lines for i32 and i64 versions of !srcloc, if that's helpful.

[simon_tatham]

Added an i64 !srcloc to the only existing test of them I could find.

[dexonsmith]

LGTM (one comment in the test), although it'd be good to get someone more involved in lib/CodeGen to take a quick look / sign off (ideally someone that knows the use case for !srcloc...).

llvm/test/MC/ARM/inline-asm-srcloc.ll

19–25

Please add a comment somewhere saying something along the lines of "This tests that both i32 and i64 work, as a proxy for testing that there won't be a crash if old bitcode contains the wrong type" (or anything that will prevent someone from updating the i32s to i64s as a future cleanup, losing us the test coverage)

[simon_tatham]

In D105491#2891860, @dexonsmith wrote:

although it'd be good to get someone more involved in lib/CodeGen to take a quick look / sign off (ideally someone that knows the use case for !srcloc...).

Any idea who that might be? Looking through the logs, @lattner seems to have made all the commits that set up !srcloc in the first place, and nobody seems to have modified the mechanism much since then.

[dexonsmith]

In D105491#2893632, @simon_tatham wrote:

In D105491#2891860, @dexonsmith wrote:

although it'd be good to get someone more involved in lib/CodeGen to take a quick look / sign off (ideally someone that knows the use case for !srcloc...).

Any idea who that might be? Looking through the logs, @lattner seems to have made all the commits that set up !srcloc in the first place, and nobody seems to have modified the mechanism much since then.

I wonder if this is even reachable anymore, given that the one testcase is inline asm (I believe the frontend catches inline asm errors now) -- I mean, CGStmt still attaches these, but maybe the backend never fires a diagnostic in practice because the frontend error checking is good enough to avoid emitting IR that's broken. (Could be useful for other frontends, maybe, though...)

Anyway, I'm comfortable signing off; feel free to commit without finding someone else (remember to add a comment to the testcase).

[lattner]

The usecase for this is to get good diagnostics out of the integrated assembler. Try doing something like: asm("bogus");, GCC would produce an error pointing to the generated .s file. Clang should point to the C file (including macros expanded etc).

[lattner]

> cat f.c

void x() {
  asm("bogus");
}
> clang f.c
f.c:3:7: error: invalid instruction mnemonic 'bogus'
  asm("bogus");
      ^
<inline asm>:1:2: note: instantiated into assembly here
        bogus
        ^~~~~
1 error generated.

[simon_tatham]

@lattner, thanks for the help. In this case, the real question is whether there's any use case for !srcloc that involves writing it out into a bitcode or IR file and then having a separate instance of clang load it back in again.

I think that no such case can possibly give a useful mapping back to the original source, because IR doesn't serialise the SourceManager that knows how to turn source locations back into a useful error message.

[simon_tatham]

(But I've pushed this patch anyway, because I'm reasonably confident of the answer; if it turns out that there's some case along those lines that I should have taken care of, I'll fix or revert.)

[lattner]

@lattner, thanks for the help. In this case, the real question is whether there's any use case for !srcloc that involves writing it out into a bitcode or IR file and then having a separate instance of clang load it back in again.

I think that no such case can possibly give a useful mapping back to the original source, because IR doesn't serialise the SourceManager that knows how to turn source locations back into a useful error message.

Right, the only use case is with the entire compiler integrated together, for exactly the reason you point out.

clang/lib/CodeGen/CGStmt.cpp

 /// asm call instruction.  The !srcloc MDNode contains a list of constant
 /// integers which are the source locations of the start of each line in the
 /// asm.
 static llvm::MDNode *getAsmSrcLocInfo(const StringLiteral *Str,
                                       CodeGenFunction &CGF) {
   SmallVector<llvm::Metadata *, 8> Locs;
   // Add the location of the first line to the MDNode.
   Locs.push_back(llvm::ConstantAsMetadata::get(llvm::ConstantInt::get(
-      CGF.Int32Ty, Str->getBeginLoc().getRawEncoding())));
+      CGF.Int64Ty, Str->getBeginLoc().getRawEncoding())));
   StringRef StrVal = Str->getString();
   if (!StrVal.empty()) {
     const SourceManager &SM = CGF.CGM.getContext().getSourceManager();
     const LangOptions &LangOpts = CGF.CGM.getLangOpts();
     unsigned StartToken = 0;
     unsigned ByteOffset = 0;
 
     // Add the location of the start of each subsequent line of the asm to the
     // MDNode.
     for (unsigned i = 0, e = StrVal.size() - 1; i != e; ++i) {
       if (StrVal[i] != '\n') continue;
       SourceLocation LineLoc = Str->getLocationOfByte(
           i + 1, SM, LangOpts, CGF.getTarget(), &StartToken, &ByteOffset);
       Locs.push_back(llvm::ConstantAsMetadata::get(
-          llvm::ConstantInt::get(CGF.Int32Ty, LineLoc.getRawEncoding())));
+          llvm::ConstantInt::get(CGF.Int64Ty, LineLoc.getRawEncoding())));
     }
   }
 
   return llvm::MDNode::get(CGF.getLLVMContext(), Locs);
 }
 
 static void UpdateAsmCallInst(llvm::CallBase &Result, bool HasSideEffect,
                               bool HasUnwindClobber, bool ReadOnly,
static void UpdateAsmCallInst(llvm::CallBase &Result, bool HasSideEffect,
 
   // Slap the source location of the inline asm into a !srcloc metadata on the
   // call.
   if (const auto *gccAsmStmt = dyn_cast<GCCAsmStmt>(&S))
     Result.setMetadata("srcloc",
                        getAsmSrcLocInfo(gccAsmStmt->getAsmString(), CGF));
   else {
     // At least put the line number on MS inline asm blobs.
-    llvm::Constant *Loc = llvm::ConstantInt::get(CGF.Int32Ty,
-                                        S.getAsmLoc().getRawEncoding());
+    llvm::Constant *Loc =
+        llvm::ConstantInt::get(CGF.Int64Ty, S.getAsmLoc().getRawEncoding());
     Result.setMetadata("srcloc",
                        llvm::MDNode::get(CGF.getLLVMContext(),
                                          llvm::ConstantAsMetadata::get(Loc)));
   }
 
   if (CGF.getLangOpts().assumeFunctionsAreConvergent())
     // Conservatively, mark all inline asm blocks in CUDA or OpenCL as
     // convergent (meaning, they may call an intrinsically convergent op, such

llvm/include/llvm/IR/DiagnosticInfo.h

 
 using DiagnosticHandlerFunction = std::function<void(const DiagnosticInfo &)>;
 
 /// Diagnostic information for inline asm reporting.
 /// This is basically a message and an optional location.
 class DiagnosticInfoInlineAsm : public DiagnosticInfo {
 private:
   /// Optional line information. 0 if not set.
-  unsigned LocCookie = 0;
+  uint64_t LocCookie = 0;
   /// Message to be reported.
   const Twine &MsgStr;
   /// Optional origin of the problem.
   const Instruction *Instr = nullptr;
 
 public:
   /// \p MsgStr is the message to be reported to the frontend.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
   DiagnosticInfoInlineAsm(const Twine &MsgStr,
                           DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_InlineAsm, Severity), MsgStr(MsgStr) {}
 
   /// \p LocCookie if non-zero gives the line number for this report.
   /// \p MsgStr gives the message.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
-  DiagnosticInfoInlineAsm(unsigned LocCookie, const Twine &MsgStr,
+  DiagnosticInfoInlineAsm(uint64_t LocCookie, const Twine &MsgStr,
                           DiagnosticSeverity Severity = DS_Error)
       : DiagnosticInfo(DK_InlineAsm, Severity), LocCookie(LocCookie),
         MsgStr(MsgStr) {}
 
   /// \p Instr gives the original instruction that triggered the diagnostic.
   /// \p MsgStr gives the message.
   /// This class does not copy \p MsgStr, therefore the reference must be valid
   /// for the whole life time of the Diagnostic.
   /// Same for \p I.
   DiagnosticInfoInlineAsm(const Instruction &I, const Twine &MsgStr,
                           DiagnosticSeverity Severity = DS_Error);
 
-  unsigned getLocCookie() const { return LocCookie; }
+  uint64_t getLocCookie() const { return LocCookie; }
   const Twine &getMsgStr() const { return MsgStr; }
   const Instruction *getInstruction() const { return Instr; }
 
   /// \see DiagnosticInfo::print.
   void print(DiagnosticPrinter &DP) const override;
 
   static bool classof(const DiagnosticInfo *DI) {
     return DI->getKind() == DK_InlineAsm;

llvm/include/llvm/IR/LLVMContext.h

#undef LLVM_FIXED_MD_KIND
   /// any global mutex or cannot block the execution in another LLVM context.
   void yield();
 
   /// emitError - Emit an error message to the currently installed error handler
   /// with optional location information.  This function returns, so code should
   /// be prepared to drop the erroneous construct on the floor and "not crash".
   /// The generated code need not be correct.  The error message will be
   /// implicitly prefixed with "error: " and should not end with a ".".
-  void emitError(unsigned LocCookie, const Twine &ErrorStr);
+  void emitError(uint64_t LocCookie, const Twine &ErrorStr);
   void emitError(const Instruction *I, const Twine &ErrorStr);
   void emitError(const Twine &ErrorStr);
 
   /// Access the object which can disable optional passes and individual
   /// optimizations at compile time.
   OptPassGate &getOptPassGate() const;
 
   /// Set the object which can disable optional passes and individual

llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp

void AsmPrinter::emitInlineAsm(StringRef Str, const MCSubtargetInfo &STI,
   // Don't implicitly switch to the text section before the asm.
   (void)Parser->Run(/*NoInitialTextSection*/ true,
                     /*NoFinalize*/ true);
   emitInlineAsmEnd(STI, &TAP->getSTI());
 }
 
 static void EmitMSInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
                                MachineModuleInfo *MMI, AsmPrinter *AP,
-                               unsigned LocCookie, raw_ostream &OS) {
+                               uint64_t LocCookie, raw_ostream &OS) {
   // Switch to the inline assembly variant.
   OS << "\t.intel_syntax\n\t";
 
   const char *LastEmitted = AsmStr; // One past the last character emitted.
   unsigned NumOperands = MI->getNumOperands();
 
   while (*LastEmitted) {
     switch (*LastEmitted) {
while (*LastEmitted) {
     }
     }
   }
   OS << "\n\t.att_syntax\n" << (char)0;  // null terminate string.
 }
 
 static void EmitGCCInlineAsmStr(const char *AsmStr, const MachineInstr *MI,
                                 MachineModuleInfo *MMI, const MCAsmInfo *MAI,
-                                AsmPrinter *AP, unsigned LocCookie,
+                                AsmPrinter *AP, uint64_t LocCookie,
                                 raw_ostream &OS) {
   int CurVariant = -1;            // The number of the {.|.|.} region we are in.
   const char *LastEmitted = AsmStr; // One past the last character emitted.
   unsigned NumOperands = MI->getNumOperands();
   int AsmPrinterVariant = MAI->getAssemblerDialect();
 
   if (MAI->getEmitGNUAsmStartIndentationMarker())
     OS << '\t';
void AsmPrinter::emitInlineAsm(const MachineInstr *MI) const {
   }
 
   // Emit the #APP start marker.  This has to happen even if verbose-asm isn't
   // enabled, so we use emitRawComment.
   OutStreamer->emitRawComment(MAI->getInlineAsmStart());
 
   // Get the !srcloc metadata node if we have it, and decode the loc cookie from
   // it.
-  unsigned LocCookie = 0;
+  uint64_t LocCookie = 0;
   const MDNode *LocMD = nullptr;
   for (unsigned i = MI->getNumOperands(); i != 0; --i) {
     if (MI->getOperand(i-1).isMetadata() &&
         (LocMD = MI->getOperand(i-1).getMetadata()) &&
         LocMD->getNumOperands() != 0) {
       if (const ConstantInt *CI =
               mdconst::dyn_extract<ConstantInt>(LocMD->getOperand(0))) {
         LocCookie = CI->getZExtValue();

llvm/lib/CodeGen/MachineInstr.cpp

for (const MachineOperand &MO : MI->operands()) {
 
     HashComponents.push_back(hash_value(MO));
   }
   return hash_combine_range(HashComponents.begin(), HashComponents.end());
 }
 
 void MachineInstr::emitError(StringRef Msg) const {
   // Find the source location cookie.
-  unsigned LocCookie = 0;
+  uint64_t LocCookie = 0;
   const MDNode *LocMD = nullptr;
   for (unsigned i = getNumOperands(); i != 0; --i) {
     if (getOperand(i-1).isMetadata() &&
         (LocMD = getOperand(i-1).getMetadata()) &&
         LocMD->getNumOperands() != 0) {
       if (const ConstantInt *CI =
               mdconst::dyn_extract<ConstantInt>(LocMD->getOperand(0))) {
         LocCookie = CI->getZExtValue();

llvm/lib/IR/LLVMContext.cpp

void LLVMContext::diagnose(const DiagnosticInfo &DI) {
   DiagnosticPrinterRawOStream DP(errs());
   errs() << getDiagnosticMessagePrefix(DI.getSeverity()) << ": ";
   DI.print(DP);
   errs() << "\n";
   if (DI.getSeverity() == DS_Error)
     exit(1);
 }
 
-void LLVMContext::emitError(unsigned LocCookie, const Twine &ErrorStr) {
+void LLVMContext::emitError(uint64_t LocCookie, const Twine &ErrorStr) {
   diagnose(DiagnosticInfoInlineAsm(LocCookie, ErrorStr));
 }
 
 //===----------------------------------------------------------------------===//
 // Metadata Kind Uniquing
 //===----------------------------------------------------------------------===//
 
 /// Return a unique non-zero ID for the specified metadata kind.

llvm/test/MC/ARM/inline-asm-srcloc.ll

entry:
   call void asm sideeffect " wibble", ""() #1, !srcloc !3
 ; CHECK: note: !srcloc = 107
   ret void
 }
 
 ; Function Attrs: noinline nounwind
 define void @foo() #0 {
 entry:
   call void asm sideeffect " .word -bar", ""() #1, !srcloc !4
 ; CHECK: note: !srcloc = 181
   call void asm sideeffect " .word -foo", ""() #1, !srcloc !5
 ; CHECK: note: !srcloc = 257
+  call void asm sideeffect " .word -stoat", ""() #1, !srcloc !6
+; CHECK: note: !srcloc = 534
   ret void
 }
 
 attributes #0 = { noinline nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "frame-pointer"="all" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="cortex-a8" "target-features"="+dsp,+neon,+strict-align,+vfp3" "unsafe-fp-math"="false" "use-soft-float"="false" }
 attributes #1 = { nounwind }
 
 !llvm.module.flags = !{!0, !1}
 !llvm.ident = !{!2}
 
 !0 = !{i32 1, !"wchar_size", i32 4}
 !1 = !{i32 1, !"min_enum_size", i32 4}
 !2 = !{!"clang version 5.0.0 "}
 !3 = !{i32 107}
+
+; These !srcloc metadata nodes are intentionally not all the same type: D105491
+; changed the creation of !srcloc to generate i64 instead of the previous i32.
+; So one thing we're testing here is that both types are acceptable on input,
+; i.e. IR generated both before and after the change can be consumed.
 !4 = !{i32 181}
 !5 = !{i32 257}
+!6 = !{i64 534}