[cfe-commits] [PATCH] Fix crash printing diagnostic range spanning macros
Eli Friedman
eli.friedman at gmail.com
Thu Oct 25 18:09:45 PDT 2012
On Thu, Oct 25, 2012 at 3:32 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Thu, Oct 25, 2012 at 2:57 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
>> On Thu, Oct 25, 2012 at 1:27 PM, Chandler Carruth <chandlerc at google.com> wrote:
>>> On Thu, Oct 25, 2012 at 9:38 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>> I think that's the best we can do. Even if the range had the beginning
>>>> before the end (say, by trying to highlight the entirety of both macros), it
>>>> wouldn't be "correct". We should not show ranges that don't correspond to
>>>> something meaningful in the text.
>>>
>>> I actually think we can do a bit better.
>>
>> Yes, we could completely change what we display, but I'm not really
>> interested in embarking on a large architectural project at the
>> moment.
>>
>>>> ...though arguably we could show a line with all macros expanded, and put
>>>> the range there. But that's a big change in what you expect from diagnostic
>>>> printing, and it wouldn't work in IDEs anyway.
>>>
>>> We get pretty close with the macro backtrace. I have sometimes
>>> wondered if we should start the error with a synthetic preprocessed
>>> snippet, and then give the code the user wrote in the first note, and
>>> descend through the macro expansions in subsequent notes.
>>> Alternatively, we could add a final note to the macro backtrace that
>>> shows the fully preprocessed source, but that seems more likely to be
>>> ignored.
>>
>> Hmm, interesting; please file a bug. :)
>>
>>>>
>>>> Jordan
>>>>
>>>>
>>>> On Oct 24, 2012, at 19:36 , Eli Friedman <eli.friedman at gmail.com> wrote:
>>>>
>>>> Patch attached. Fixes a crash on a testcase like the following:
>>>>
>>>> +#define BAD_CONDITIONAL_OPERATOR (2<3)?4:5
>>>> +int x = BAD_CONDITIONAL_OPERATOR+BAD_CONDITIONAL_OPERATOR;
>>>>
>>>> We try to print a source range which starts at the 5 in the first
>>>> expansion, and ends just after the 3 in the second expansion.
>>>
>>> My suggestion would be this:
>>>
>>> When you have a source range to highlight, and it's start or stop
>>> location occurs within a macro, grow it to the start (or stop, resp.)
>>> of the macro info's expansion location. This should be the start of
>>> where the macro got expanded into the code.
>>>
>>> Then, if there the diagnostic location itself is inside a macro, as
>>> you do the macro backtrace walk you'll need to address the fixme in
>>> the diagnostic code to actually walk the source ranges back through
>>> the macro backtrace as well, and at each level to the analogous
>>> transform to grow the range at that level.
>>
>> We already do this; we just don't do it correctly for the case where
>> the start and/or end locations come from a different expansion than
>> the caret.
>
> Yes, but do we do the first paragraph correctly? I think we can do the
> first paragraph and fix the crash/misbehavior you're talking about.
I see what you mean. New patch attached.
-Eli
-------------- next part --------------
Index: test/Misc/caret-diags-macros.c
===================================================================
--- test/Misc/caret-diags-macros.c (revision 166711)
+++ test/Misc/caret-diags-macros.c (working copy)
@@ -1,13 +1,13 @@
-// RUN: %clang_cc1 -fsyntax-only %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -fsyntax-only %s 2>&1 | FileCheck %s -strict-whitespace
#define M1(x) x
#define M2 1;
void foo() {
M1(
M2);
- // CHECK: :7:{{[0-9]+}}: warning: expression result unused
- // CHECK: :4:{{[0-9]+}}: note: expanded from macro 'M2'
- // CHECK: :3:{{[0-9]+}}: note: expanded from macro 'M1'
+ // CHECK: {{.*}}:7:{{[0-9]+}}: warning: expression result unused
+ // CHECK: {{.*}}:4:{{[0-9]+}}: note: expanded from macro 'M2'
+ // CHECK: {{.*}}:3:{{[0-9]+}}: note: expanded from macro 'M1'
}
#define A 1
@@ -15,10 +15,10 @@
#define C B
void bar() {
C;
- // CHECK: :17:3: warning: expression result unused
- // CHECK: :15:11: note: expanded from macro 'C'
- // CHECK: :14:11: note: expanded from macro 'B'
- // CHECK: :13:11: note: expanded from macro 'A'
+ // CHECK: {{.*}}:17:3: warning: expression result unused
+ // CHECK: {{.*}}:15:11: note: expanded from macro 'C'
+ // CHECK: {{.*}}:14:11: note: expanded from macro 'B'
+ // CHECK: {{.*}}:13:11: note: expanded from macro 'A'
}
// rdar://7597492
@@ -40,12 +40,12 @@
#define macro_many_args3(x, y, z) macro_many_args2(x, y, z)
void test() {
- macro_args3(1);
+ macro_args3(11);
// CHECK: {{.*}}:43:15: warning: expression result unused
// Also check that the 'caret' printing agrees with the location here where
// its easy to FileCheck.
- // CHECK-NEXT: macro_args3(1);
- // CHECK-NEXT: ~~~~~~~~~~~~^~
+ // CHECK-NEXT: macro_args3(11);
+ // CHECK-NEXT: {{^ \^~}}
// CHECK: {{.*}}:36:36: note: expanded from macro 'macro_args3'
// CHECK: {{.*}}:35:36: note: expanded from macro 'macro_args2'
// CHECK: {{.*}}:34:24: note: expanded from macro 'macro_args1'
@@ -71,13 +71,13 @@
macro_many_args3(
1,
- macro_args2(2),
+ macro_args2(22),
3);
// CHECK: {{.*}}:74:17: warning: expression result unused
// This caret location needs to be printed *inside* a different macro's
// arguments.
- // CHECK-NEXT: macro_args2(2),
- // CHECK-NEXT: ~~~~~~~~~~~~^~~
+ // CHECK-NEXT: macro_args2(22),
+ // CHECK-NEXT: {{^ \^~}}
// CHECK: {{.*}}:35:36: note: expanded from macro 'macro_args2'
// CHECK: {{.*}}:34:24: note: expanded from macro 'macro_args1'
// CHECK: {{.*}}:40:55: note: expanded from macro 'macro_many_args3'
@@ -90,10 +90,10 @@
#define variadic_args3(x, y, ...) variadic_args2(x, y, __VA_ARGS__)
void test2() {
- variadic_args3(1, 2, 3, 4);
+ variadic_args3(1, 22, 3, 4);
// CHECK: {{.*}}:93:21: warning: expression result unused
- // CHECK-NEXT: variadic_args3(1, 2, 3, 4);
- // CHECK-NEXT: ~~~~~~~~~~~~~~~~~~^~~~~~~~
+ // CHECK-NEXT: variadic_args3(1, 22, 3, 4);
+ // CHECK-NEXT: {{^ \^~}}
// CHECK: {{.*}}:90:53: note: expanded from macro 'variadic_args3'
// CHECK: {{.*}}:89:50: note: expanded from macro 'variadic_args2'
// CHECK: {{.*}}:88:35: note: expanded from macro 'variadic_args1'
@@ -118,3 +118,27 @@
// CHECK: {{.*}}:104:70: note: expanded from macro 'variadic_pasting_args2a'
// CHECK: {{.*}}:102:41: note: expanded from macro 'variadic_pasting_args1'
}
+
+#define BAD_CONDITIONAL_OPERATOR (2<3)?2:3
+int test4 = BAD_CONDITIONAL_OPERATOR+BAD_CONDITIONAL_OPERATOR;
+// CHECK: {{.*}}:122:39: note: expanded from macro 'BAD_CONDITIONAL_OPERATOR'
+// CHECK: #define BAD_CONDITIONAL_OPERATOR (2<3)?2:3
+// CHECK: {{^ \^}}
+// CHECK: {{.*}}:122:39: note: expanded from macro 'BAD_CONDITIONAL_OPERATOR'
+// CHECK: #define BAD_CONDITIONAL_OPERATOR (2<3)?2:3
+// CHECK: {{^ \^}}
+// CHECK: {{.*}}:122:39: note: expanded from macro 'BAD_CONDITIONAL_OPERATOR'
+// CHECK: #define BAD_CONDITIONAL_OPERATOR (2<3)?2:3
+// CHECK: {{^ ~~~~~\^~~~}}
+
+#define QMARK ?
+#define TWOL (2<
+#define X 1+TWOL 3) QMARK 4:5
+int x = X;
+// CHECK: {{.*}}:137:9: note: place parentheses around the '?:' expression to evaluate it first
+// CHECK: int x = X;
+// CHECK: {{^ \^}}
+// CHECK: {{.*}}:136:21: note: expanded from macro 'X'
+// CHECK: #define X 1+TWOL 3) QMARK 4:5
+// CHECK: {{^ ~~~~~~~~\^~~~~~~~~}}
+
Index: lib/Frontend/DiagnosticRenderer.cpp
===================================================================
--- lib/Frontend/DiagnosticRenderer.cpp (revision 166711)
+++ lib/Frontend/DiagnosticRenderer.cpp (working copy)
@@ -18,6 +18,7 @@
#include "llvm/Support/MemoryBuffer.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/ErrorHandling.h"
+#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallString.h"
#include <algorithm>
using namespace clang;
@@ -218,6 +219,42 @@
emitIncludeLocation(Loc, PLoc, SM);
}
+// Helper function to fix up the source ranges so the start and end
+// have the same FileID, and that FileID is somewhere in the
+// caller chain for Loc.
+static void MapDiagnosticRanges(SourceLocation Loc,
+ SmallVectorImpl<CharSourceRange>& Ranges,
+ SmallVectorImpl<CharSourceRange>& SpellingRanges,
+ const SourceManager *SM) {
+ FileID LocFileID = SM->getFileID(Loc);
+
+ for (SmallVectorImpl<CharSourceRange>::iterator I = Ranges.begin(),
+ E = Ranges.end();
+ I != E; ++I) {
+ SourceLocation Start = I->getBegin(), End = I->getEnd();
+ bool IsTokenRange = I->isTokenRange();
+
+ while (Start.isMacroID() && SM->getFileID(Start) != LocFileID)
+ Start = SM->getImmediateMacroCallerLoc(Start);
+
+ while (End.isMacroID() && SM->getFileID(End) != LocFileID) {
+ // The computation of the next End is an inlined version of
+ // getImmediateMacroCallerLoc, except it chooses the end of an
+ // expansion range.
+ if (SM->isMacroArgExpansion(End)) {
+ End = SM->getImmediateSpellingLoc(End);
+ } else {
+ End = SM->getImmediateExpansionRange(End).second;
+ }
+ }
+
+ Start = SM->getSpellingLoc(Start);
+ End = SM->getSpellingLoc(End);
+ SpellingRanges.push_back(CharSourceRange(SourceRange(Start, End),
+ IsTokenRange));
+ }
+}
+
/// \brief Recursively emit notes for each macro expansion and caret
/// diagnostics where appropriate.
///
@@ -241,24 +278,27 @@
unsigned OnMacroInst)
{
assert(!Loc.isInvalid() && "must have a valid source location here");
-
+
+ // Skip over the expansions leading up to a macro argument, and trace
+ // the argument's expansion stack instead.
+ Loc = SM.skipToMacroArgExpansion(Loc);
+
+ // Map the ranges.
+ SmallVector<CharSourceRange, 4> SpellingRanges;
+ MapDiagnosticRanges(Loc, Ranges, SpellingRanges, &SM);
+
// If this is a file source location, directly emit the source snippet and
// caret line. Also record the macro depth reached.
if (Loc.isFileID()) {
assert(MacroDepth == 0 && "We shouldn't hit a leaf node twice!");
MacroDepth = OnMacroInst;
- emitCodeContext(Loc, Level, Ranges, Hints, SM);
+ emitCodeContext(Loc, Level, SpellingRanges, Hints, SM);
return;
}
// Otherwise recurse through each macro expansion layer.
- // When processing macros, skip over the expansions leading up to
- // a macro argument, and trace the argument's expansion stack instead.
- Loc = SM.skipToMacroArgExpansion(Loc);
-
SourceLocation OneLevelUp = SM.getImmediateMacroCallerLoc(Loc);
-
- // FIXME: Map ranges?
+
emitMacroExpansionsAndCarets(OneLevelUp, Level, Ranges, Hints, SM, MacroDepth,
OnMacroInst + 1);
@@ -280,17 +320,6 @@
bool Suppressed = (OnMacroInst >= MacroSkipStart &&
OnMacroInst < MacroSkipEnd);
- // Map the ranges.
- for (SmallVectorImpl<CharSourceRange>::iterator I = Ranges.begin(),
- E = Ranges.end();
- I != E; ++I) {
- SourceLocation Start = I->getBegin(), End = I->getEnd();
- if (Start.isMacroID())
- I->setBegin(SM.getImmediateMacroCalleeLoc(Start));
- if (End.isMacroID())
- I->setEnd(SM.getImmediateMacroCalleeLoc(End));
- }
-
if (Suppressed) {
// Tell the user that we've skipped contexts.
if (OnMacroInst == MacroSkipStart) {
@@ -310,7 +339,7 @@
<< getImmediateMacroName(MacroLoc, SM, LangOpts) << "'";
emitDiagnostic(SM.getSpellingLoc(Loc), DiagnosticsEngine::Note,
Message.str(),
- Ranges, ArrayRef<FixItHint>(), &SM);
+ SpellingRanges, ArrayRef<FixItHint>(), &SM);
}
DiagnosticNoteRenderer::~DiagnosticNoteRenderer() {}
Index: lib/Frontend/TextDiagnostic.cpp
===================================================================
--- lib/Frontend/TextDiagnostic.cpp (revision 166711)
+++ lib/Frontend/TextDiagnostic.cpp (working copy)
@@ -1062,17 +1062,9 @@
const SourceManager &SM) {
if (!R.isValid()) return;
- SourceLocation Begin = SM.getExpansionLoc(R.getBegin());
- SourceLocation End = SM.getExpansionLoc(R.getEnd());
+ SourceLocation Begin = R.getBegin();
+ SourceLocation End = R.getEnd();
- // If the End location and the start location are the same and are a macro
- // location, then the range was something that came from a macro expansion
- // or _Pragma. If this is an object-like macro, the best we can do is to
- // highlight the range. If this is a function-like macro, we'd also like to
- // highlight the arguments.
- if (Begin == End && R.getEnd().isMacroID())
- End = SM.getExpansionRange(R.getEnd()).second;
-
unsigned StartLineNo = SM.getExpansionLineNumber(Begin);
if (StartLineNo > LineNo || SM.getFileID(Begin) != FID)
return; // No intersection.
More information about the cfe-commits
mailing list