thakis created this revision.
thakis added reviewers: rnk, rsmith.
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.
https://reviews.llvm.org/D60418
Files:
clang/include/clang/Frontend/TextDiagnosticBuffer.h
clang/lib/CodeGen/CodeGenAction.cpp
clang/lib/Frontend/TextDiagnosticBuffer.cpp
clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
clang/lib/Lex/Lexer.cpp
clang/test/Lexer/gnu-flags.c
clang/test/Modules/explicit-build-missing-files.cpp
clang/test/Preprocessor/line-directive.c
Index: clang/test/Preprocessor/line-directive.c
===================================================================
--- clang/test/Preprocessor/line-directive.c
+++ clang/test/Preprocessor/line-directive.c
@@ -3,9 +3,12 @@
// 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
@@ -73,7 +76,8 @@
// 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}}
@@ -82,11 +86,13 @@
// 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];
+#line 010
+// expected-warning@-1{{#line directive interprets number as decimal, not octal}}
+extern int array[__LINE__ == 11 ? 1:-1];
-# 020 // expected-warning {{GNU line marker directive interprets number as decimal, not octal}}
-extern int array_gnuline[__LINE__ == 20 ? 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
@@ -100,7 +106,8 @@
_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'}}
Index: clang/test/Modules/explicit-build-missing-files.cpp
===================================================================
--- clang/test/Modules/explicit-build-missing-files.cpp
+++ clang/test/Modules/explicit-build-missing-files.cpp
@@ -42,7 +42,8 @@
// 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
Index: clang/test/Lexer/gnu-flags.c
===================================================================
--- clang/test/Lexer/gnu-flags.c
+++ clang/test/Lexer/gnu-flags.c
@@ -47,8 +47,10 @@
// 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
Index: clang/lib/Lex/Lexer.cpp
===================================================================
--- clang/lib/Lex/Lexer.cpp
+++ clang/lib/Lex/Lexer.cpp
@@ -2351,6 +2351,8 @@
// 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;
Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===================================================================
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -704,16 +704,16 @@
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()
@@ -787,16 +787,16 @@
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;
}
Index: clang/lib/Frontend/TextDiagnosticBuffer.cpp
===================================================================
--- clang/lib/Frontend/TextDiagnosticBuffer.cpp
+++ clang/lib/Frontend/TextDiagnosticBuffer.cpp
@@ -13,6 +13,7 @@
#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"
@@ -25,6 +26,12 @@
// 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) {
@@ -32,20 +39,20 @@
"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;
}
}
@@ -57,17 +64,17 @@
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;
}
}
Index: clang/lib/CodeGen/CodeGenAction.cpp
===================================================================
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -391,18 +391,19 @@
// 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));
+ LSM.getMemoryBuffer(LSM.FindBufferContainingLoc(D.getLoc()));
+ FileID FID = CSM.createFileID(clang::SourceManager::Unowned, LBuf);
// Translate the offset into the file.
unsigned Offset = D.getLoc().getPointer() - LBuf->getBufferStart();
Index: clang/include/clang/Frontend/TextDiagnosticBuffer.h
===================================================================
--- clang/include/clang/Frontend/TextDiagnosticBuffer.h
+++ clang/include/clang/Frontend/TextDiagnosticBuffer.h
@@ -24,7 +24,20 @@
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;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
