inline asm: Don't copy llvm::MemoryBuffers when showing diagnostics

[thakis]

Summary

This fixes one TODO and one FIXME in ConvertBackendLocation() about not copying llvm::MemoryBuffers when converting from from llvm::SMDiagnostics to clang::Diagnostics.

The basic idea is to use SourceManager::createFileID(clang::SourceManager::Unowned, … to hand the llvm::SMDiagnostic's llvm::MemoryBuffer buffer directly to clang.

This has the implication that the SourceManager now has a reference to a MemoryBuffer that's only alive while LLVM's asm pass runs; in particular the buffer is gone by the time TextDiagnosticBuffer computes the expected line number for the diagnostics it records in -verify mode. To fix this, make TextDiagnosticBuffer do this conversion eagerly when the diagnostic is recorded. This in turn has two implications:

1. It matches how clang emits diagnostics in normal operation, which is a good thing. This e.g. identified a problem where clang's normal diagnostic emission code path works fine but the -verify code path is broken (PR41431).

2. Since HandleComment() for end-of-line comments in preprocessor directives is called _before_ the directive is completely processed, comments at the end of #line directives don't see the effect of the #line directive yet. Just update tests affected by this by moving the comments down a line and using the @-1 syntax to refer to the previous line. (This is less weird than it appears at first: @+1 lines for #line directives already don't work which is confusing some people – PR16952. But as you can see, only very few tests use this.) (See the XXX comment in the patch for details; I'll remove that comment before landing this.)

This is blocked on fixing PR41431.

[thakis]

clang/lib/Lex/Lexer.cpp

2355

It doesn't show up well in the diff, but there's a call to HandleComment() 13 lines further up that this refers to.

[rnk]

This is blocked on fixing PR41431.

I reviewed the code, but timed out while trying to understand PR41431. Maybe ping @rsmith about it again?

clang/include/clang/Frontend/TextDiagnosticBuffer.h

 #include <string>
 #include <utility>
 #include <vector>
 
 namespace clang {
 
 class TextDiagnosticBuffer : public DiagnosticConsumer {
 public:
-  using DiagList = std::vector<std::pair<SourceLocation, std::string>>;
+  // TextDiagnosticBuffer holds on to diagnostics longer than the buffer
+  // referred to by Loc might be alive for. This struct holds data that
+  // needs to be copied out of the buffer at the time a diagnostic is emitted,
+  // so that it's still available when TextDiagnosticBuffer's iterators are
+  // used.
+  struct StoredDiag {
+    StoredDiag(SourceLocation Loc, int PresumedLineNumber, std::string DiagText)
+        : Loc(Loc), PresumedLineNumber(PresumedLineNumber),
+          DiagText(std::move(DiagText)) {}
+    SourceLocation Loc;
+    int PresumedLineNumber;
+    std::string DiagText;
+  };
+  using DiagList = std::vector<StoredDiag>;
   using iterator = DiagList::iterator;
   using const_iterator = DiagList::const_iterator;
 
 private:
   DiagList Errors, Warnings, Remarks, Notes;
 
   /// All - All diagnostics in the order in which they were generated.  That
   /// order likely doesn't correspond to user input order, but it at least

clang/lib/CodeGen/CodeGenAction.cpp

 /// buffer to be a valid FullSourceLoc.
 static FullSourceLoc ConvertBackendLocation(const llvm::SMDiagnostic &D,
                                             SourceManager &CSM) {
   // Get both the clang and llvm source managers.  The location is relative to
   // a memory buffer that the LLVM Source Manager is handling, we need to add
   // a copy to the Clang source manager.
   const llvm::SourceMgr &LSM = *D.getSourceMgr();
 
-  // We need to copy the underlying LLVM memory buffer because llvm::SourceMgr
-  // already owns its one and clang::SourceManager wants to own its one.
+  // We let CSM have an unowned reference to the original buffer from LSM.
+  // The buffer in the LSM is only alive while LLVM's passes run while
+  // the CSM is alive longer, so the CSM will have a dangling pointer to a
+  // dead buffer for a while. Code must be careful to not access
+  // the SourceLocs returned by this function in a way that touches this buffer
+  // after LLVM's passes have run.
+  // In particular, in -verify mode, VerifyDiagnosticConsumer's
+  // TextDiagnosticBuffer must copy all buffer-related data when diagnostics
+  // are emitted, so that it doesn't need to access the buffer at diagnostics
+  // verification time (which happens after LLVM's passes have completed).
   const MemoryBuffer *LBuf =
       LSM.getMemoryBuffer(LSM.FindBufferContainingLoc(D.getLoc()));
-
-  // Create the copy and transfer ownership to clang::SourceManager.
-  // TODO: Avoid copying files into memory.
-  std::unique_ptr<llvm::MemoryBuffer> CBuf =
-      llvm::MemoryBuffer::getMemBufferCopy(LBuf->getBuffer(),
-                                           LBuf->getBufferIdentifier());
-  // FIXME: Keep a file ID map instead of creating new IDs for each location.
-  FileID FID = CSM.createFileID(std::move(CBuf));
+  FileID FID = CSM.createFileID(clang::SourceManager::Unowned, LBuf);
 
   // Translate the offset into the file.
   unsigned Offset = D.getLoc().getPointer() - LBuf->getBufferStart();
   SourceLocation NewLoc =
   CSM.getLocForStartOfFile(FID).getLocWithOffset(Offset);
   return FullSourceLoc(NewLoc, CSM);
 }
 

clang/lib/Frontend/TextDiagnosticBuffer.cpp

 //===- TextDiagnosticBuffer.cpp - Buffer Text Diagnostics -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This is a concrete diagnostic client, which buffers the diagnostic messages.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Frontend/TextDiagnosticBuffer.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LLVM.h"
+#include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/ErrorHandling.h"
 
 using namespace clang;
 
 /// HandleDiagnostic - Store the errors, warnings, and notes that are
 /// reported.
 void TextDiagnosticBuffer::HandleDiagnostic(DiagnosticsEngine::Level Level,
                                             const Diagnostic &Info) {
   // Default implementation (Warnings/errors count).
   DiagnosticConsumer::HandleDiagnostic(Level, Info);
 
+  int PresumedLineNumber = -1;
+  if (Info.hasSourceManager()) {
+    SourceManager &SM = Info.getSourceManager();
+    PresumedLineNumber = SM.getPresumedLineNumber(Info.getLocation());
+  }
+
   SmallString<100> Buf;
   Info.FormatDiagnostic(Buf);
   switch (Level) {
   default: llvm_unreachable(
                          "Diagnostic not handled during diagnostic buffering!");
   case DiagnosticsEngine::Note:
     All.emplace_back(Level, Notes.size());
-    Notes.emplace_back(Info.getLocation(), Buf.str());
+    Notes.emplace_back(Info.getLocation(), PresumedLineNumber, Buf.str());
     break;
   case DiagnosticsEngine::Warning:
     All.emplace_back(Level, Warnings.size());
-    Warnings.emplace_back(Info.getLocation(), Buf.str());
+    Warnings.emplace_back(Info.getLocation(), PresumedLineNumber, Buf.str());
     break;
   case DiagnosticsEngine::Remark:
     All.emplace_back(Level, Remarks.size());
-    Remarks.emplace_back(Info.getLocation(), Buf.str());
+    Remarks.emplace_back(Info.getLocation(), PresumedLineNumber, Buf.str());
     break;
   case DiagnosticsEngine::Error:
   case DiagnosticsEngine::Fatal:
     All.emplace_back(Level, Errors.size());
-    Errors.emplace_back(Info.getLocation(), Buf.str());
+    Errors.emplace_back(Info.getLocation(), PresumedLineNumber, Buf.str());
     break;
   }
 }
 
 void TextDiagnosticBuffer::FlushDiagnostics(DiagnosticsEngine &Diags) const {
   for (const auto &I : All) {
     auto Diag = Diags.Report(Diags.getCustomDiagID(I.first, "%0"));
     switch (I.first) {
     default: llvm_unreachable(
                            "Diagnostic not handled during diagnostic flushing!");
     case DiagnosticsEngine::Note:
-      Diag << Notes[I.second].second;
+      Diag << Notes[I.second].DiagText;
       break;
     case DiagnosticsEngine::Warning:
-      Diag << Warnings[I.second].second;
+      Diag << Warnings[I.second].DiagText;
       break;
     case DiagnosticsEngine::Remark:
-      Diag << Remarks[I.second].second;
+      Diag << Remarks[I.second].DiagText;
       break;
     case DiagnosticsEngine::Error:
     case DiagnosticsEngine::Fatal:
-      Diag << Errors[I.second].second;
+      Diag << Errors[I.second].DiagText;
       break;
     }
   }
 }

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

static unsigned PrintUnexpected(DiagnosticsEngine &Diags, SourceManager *SourceMgr,
                                 const_diag_iterator diag_begin,
                                 const_diag_iterator diag_end,
                                 const char *Kind) {
   if (diag_begin == diag_end) return 0;
 
   SmallString<256> Fmt;
   llvm::raw_svector_ostream OS(Fmt);
   for (const_diag_iterator I = diag_begin, E = diag_end; I != E; ++I) {
-    if (I->first.isInvalid() || !SourceMgr)
+    if (I->Loc.isInvalid() || !SourceMgr)
       OS << "\n  (frontend)";
     else {
       OS << "\n ";
       if (const FileEntry *File = SourceMgr->getFileEntryForID(
-                                                SourceMgr->getFileID(I->first)))
+                                                SourceMgr->getFileID(I->Loc)))
         OS << " File " << File->getName();
-      OS << " Line " << SourceMgr->getPresumedLineNumber(I->first);
+      OS << " Line " << I->PresumedLineNumber;
     }
-    OS << ": " << I->second;
+    OS << ": " << I->DiagText;
   }
 
   Diags.Report(diag::err_verify_inconsistent_diags).setForceEmit()
     << Kind << /*Unexpected=*/true << OS.str();
   return std::distance(diag_begin, diag_end);
 }
 
 /// Takes a list of diagnostics that were expected to have been generated
static unsigned CheckLists(DiagnosticsEngine &Diags, SourceManager &SourceMgr,
   for (auto &Owner : Left) {
     Directive &D = *Owner;
     unsigned LineNo1 = SourceMgr.getPresumedLineNumber(D.DiagnosticLoc);
 
     for (unsigned i = 0; i < D.Max; ++i) {
       DiagList::iterator II, IE;
       for (II = Right.begin(), IE = Right.end(); II != IE; ++II) {
         if (!D.MatchAnyLine) {
-          unsigned LineNo2 = SourceMgr.getPresumedLineNumber(II->first);
+          unsigned LineNo2 = II->PresumedLineNumber;
           if (LineNo1 != LineNo2)
             continue;
         }
 
         if (!D.DiagnosticLoc.isInvalid() &&
-            !IsFromSameFile(SourceMgr, D.DiagnosticLoc, II->first))
+            !IsFromSameFile(SourceMgr, D.DiagnosticLoc, II->Loc))
           continue;
 
-        const std::string &RightText = II->second;
+        const std::string &RightText = II->DiagText;
         if (D.match(RightText))
           break;
       }
       if (II == IE) {
         // Not found.
         if (i >= D.Min) break;
         LeftOnly.push_back(&D);
       } else {

clang/lib/Lex/Lexer.cpp

bool Lexer::SkipLineComment(Token &Result, const char *CurPtr,
   }
 
   // If we are returning comments as tokens, return this comment as a token.
   if (inKeepCommentMode())
     return SaveLineComment(Result, CurPtr);
 
   // If we are inside a preprocessor directive and we see the end of line,
   // return immediately, so that the lexer can return this as an EOD token.
+  // XXX so comment is handled before pp directive is done, so no linetable
+  // entry yet when the handler runs.
   if (ParsingPreprocessorDirective || CurPtr == BufferEnd) {
     BufferPtr = CurPtr;
     return false;
   }
 
   // Otherwise, eat the \n character.  We don't care if this is a \n\r or
   // \r\n sequence.  This is an efficiency hack (because we know the \n can't
   // contribute to another token), it isn't needed for correctness.  Note that

clang/test/Lexer/gnu-flags.c

 // expected-warning@+3 {{binary integer literals are a GNU extension}}
 #endif
 
 int b = 0b0101;
 
 
 // This case is handled differently because lit has a bug whereby #line 0 is reported to be on line 4294967295
 // http://llvm.org/bugs/show_bug.cgi?id=16952
+// FIXME: update comment
 #if ALL || LINE0
-#line 0 // expected-warning {{#line directive with zero argument is a GNU extension}}
+#line 0
+// expected-warning@-1{{#line directive with zero argument is a GNU extension}}
 #else
 #line 0
 #endif
 
 // WARNING: Do not add more tests after the #line 0 line!  Add them before the LINE0 test

clang/test/Modules/explicit-build-missing-files.cpp

 // RUN: %clang_cc1 -fmodules -I %t -fmodule-file=%t/a.pcm -fmodule-map-file=%t/modulemap.moved -std=c++1z -Wno-module-file-config-mismatch %s -Db=a
 // RUN: rm %t/a.h
 // RUN: %clang_cc1 -fmodules -I %t -fmodule-file=%t/a.pcm %s -verify
 // RUN: %clang_cc1 -fmodules -I %t -fmodule-file=%t/b.pcm %s -verify
 
 // Oftentimes on Windows there are open handles, and deletion will fail.
 // REQUIRES: can-remove-opened-file
 
-#include "a.h" // expected-error {{file not found}}
+// FIXME: This fails now, PR41431
+#include "a.h" // expected-error{{file not found}}
 int x = b;
 
 #ifdef ERRORS
 int y = a2<int>;
 // CHECK: In module 'a':
 // CHECK-NEXT: a.h:1:45: error:
 
 // MISSING-B: could not find file '{{.*}}b.h'
 // MISSING-B-NOT: please delete the module cache
 #endif
 
 // RUN: not %clang_cc1 -fmodules -I %t -emit-module -fmodule-name=a -x c++ /dev/null -o %t/c.pcm \
 // RUN:                -fmodules-embed-file=%t/does-not-exist 2>&1 | FileCheck %s --check-prefix=MISSING-EMBED
 // MISSING-EMBED: fatal error: file '{{.*}}does-not-exist' specified by '-fmodules-embed-file=' not found

clang/test/Preprocessor/line-directive.c

 // RUN: %clang_cc1 -std=c99 -fsyntax-only -verify -pedantic %s
 // RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:92:2: error: ABC'
 // RUN: not %clang_cc1 -E %s 2>&1 | grep 'blonk.c:93:2: error: DEF'
 
 #line 'a'            // expected-error {{#line directive requires a positive integer argument}}
-#line 0              // expected-warning {{#line directive with zero argument is a GNU extension}}
-#line 00             // expected-warning {{#line directive with zero argument is a GNU extension}}
-#line 2147483648     // expected-warning {{C requires #line number to be less than 2147483648, allowed as extension}}
+#line 0
+// expected-warning@-1{{#line directive with zero argument is a GNU extension}}
+#line 00
+// expected-warning@-1{{#line directive with zero argument is a GNU extension}}
+#line 2147483648
+// expected-warning@-1{{C requires #line number to be less than 2147483648, allowed as extension}}
 #line 42             // ok
 #line 42 'a'         // expected-error {{invalid filename for #line directive}}
 #line 42 "foo/bar/baz.h"  // ok
 
 
 // #line directives expand macros.
 #define A 42 "foo"
 #line A

 typedef int w;  // expected-warning {{redefinition of typedef 'w' is a C11 feature}}
 
 typedef int q;  // original definition in system header, should not diagnose.
 
 // This should not produce an "extra tokens at end of #line directive" warning,
 // because #line is allowed to contain expanded tokens.
 #define EMPTY()
 #line 2 "foo.c" EMPTY( )
-#line 2 "foo.c" NONEMPTY( )  // expected-warning{{extra tokens at end of #line directive}}
+#line 2 "foo.c" NONEMPTY( )
+// expected-warning@-1{{extra tokens at end of #line directive}}
 
 // PR3940
 #line 0xf  // expected-error {{#line directive requires a simple digit sequence}}
 #line 42U  // expected-error {{#line directive requires a simple digit sequence}}
 
 
 // Line markers are digit strings interpreted as decimal numbers, this is
 // 10, not 8.
-#line 010  // expected-warning {{#line directive interprets number as decimal, not octal}}
-extern int array[__LINE__ == 10 ? 1:-1];
-
-# 020      // expected-warning {{GNU line marker directive interprets number as decimal, not octal}}
-extern int array_gnuline[__LINE__ == 20 ? 1:-1];
+#line 010
+// expected-warning@-1{{#line directive interprets number as decimal, not octal}}
+extern int array[__LINE__ == 11 ? 1:-1];
+
+# 020
+// expected-warning@-1{{GNU line marker directive interprets number as decimal, not octal}}
+extern int array_gnuline[__LINE__ == 21 ? 1:-1];
 
 /* PR3917 */
 #line 41
 extern char array2[\
 _\
 _LINE__ == 42 ? 1: -1];  /* line marker is location of first _ */
 
 # 51
 extern char array2_gnuline[\
 _\
 _LINE__ == 52 ? 1: -1];  /* line marker is location of first _ */
 
 // rdar://11550996
-#line 0 "line-directive.c" // expected-warning {{#line directive with zero argument is a GNU extension}}
+#line 0 "line-directive.c"
+// expected-warning@-1{{#line directive with zero argument is a GNU extension}}
 undefined t; // expected-error {{unknown type name 'undefined'}}