Improvements on Diagnostic in Macro Expansions

[zhengkai]

Summary

Add two helper functions to check if the current macro expansion contains enough information for the error/warning message that we don't need further expansion.

Modify two test files which generates too verbose information in macros.

Add two test files. reduced-diags-macros is to check the different solutions when different error messages are generated. reduced-diags-macros-backtrace is to check the correctness of macro back trace limits in our new methods.

[rtrieu]

Some questions about your tests.

test/Misc/reduced-diags-macros-backtrace.cpp

27 ↗

(On Diff #31391)

Why are macro notes printed after this one? Clang found 'p' so it does not need to give any more information.

36 ↗

(On Diff #31391)

Same question, why are notes still printed after this one?

53–78 ↗

(On Diff #31391)

Did you mean "SKIP-NEXT"?

test/Misc/reduced-diags-macros.cpp

13–14 ↗

(On Diff #31391)

If this had one more macro expansion, would it be printed or not?

27–29 ↗

(On Diff #31391)

How is this different than the test for 'b' above? Specifically, why does the error for 'b' print no macros notes while the error for 'x' prints two notes?

[zhengkai]

Bug fixed

[rtrieu]

Which bug did you fix?

Also, are you planning to add more test cases? Something that impacts all macro printing should be well-tested.

[zhengkai]

The helper function checkRangesForMacroArgExpansion is to check if the current Loc and Ranges are expansions of a macro's arguments.
If so, the IgnoredEnd will record this position and thus omitting all later expanded macros (Later expanded macros are showed on top of the stack though).

Two test cases are added.
The reduced-diags-macros-backtrace.cpp is to show that my method works correctly with the option -fmacro-backtrace-limit
The reduced-diags-macros.cpp is to show that my method reduced verbose information printed indeed.

There are still a lot of other cases in which my method still works as the old version did.
And I think there are several bugs in the existing functions like "emitDiagnostic" and "mapDiagnosticRanges".
This is issued as a bug, the link is https://llvm.org/bugs/show_bug.cgi?id=24423.
Future work will be done to improve the current result.

test/Misc/reduced-diags-macros.cpp

14–15 ↗

(On Diff #31425)

Yes.
This works like the old version, should be improved later

[rtrieu]

There are still a lot of other cases in which my method still works as the old version did.
And I think there are several bugs in the existing functions like "emitDiagnostic" and "mapDiagnosticRanges".
This is issued as a bug, the link is https://llvm.org/bugs/show_bug.cgi?id=24423.
Future work will be done to improve the current result.

The bug you linked to is a tracking bug for all macro improvements. File bugs on specific examples you encounter and link it to that bug.

test/Misc/reduced-diags-macros.cpp

14–15 ↗

(On Diff #31425)

File this issue as another bug.

[zhengkai]

The example our method doesn't work: https://llvm.org/bugs/show_bug.cgi?id=24424

[rtrieu]

This is a first step towards making macro diagnostics better.

cfe/trunk/lib/Frontend/DiagnosticRenderer.cpp

if (MacroName.empty())
     Message << "expanded from here";
   else
     Message << "expanded from macro '" << MacroName << "'";
 
   emitDiagnostic(SpellingLoc, DiagnosticsEngine::Note, Message.str(),
                  SpellingRanges, None, &SM);
 }
 
+static bool checkRangeForMacroArgExpansion(CharSourceRange Range,
+                                           const SourceManager &SM) {
+  SourceLocation BegLoc = Range.getBegin(), EndLoc = Range.getEnd();
+  while (BegLoc != EndLoc) {
+    if (!SM.isMacroArgExpansion(BegLoc))
+      return false;
+    BegLoc.getLocWithOffset(1);
+  }
+
+  return SM.isMacroArgExpansion(BegLoc);
+}
+
+/// A helper function to check if the current ranges are all inside
+/// the macro expansions.
+static bool checkRangesForMacroArgExpansion(SourceLocation Loc,
+                                            ArrayRef<CharSourceRange> Ranges,
+                                            const SourceManager &SM) {
+  assert(Loc.isMacroID() && "Must be a macro expansion!");
+
+  SmallVector<CharSourceRange, 4> SpellingRanges;
+  mapDiagnosticRanges(Loc, Ranges, SpellingRanges, &SM);
+
+  if (!SM.isMacroArgExpansion(Loc))
+    return false;
+
+  for (auto I = SpellingRanges.begin(), E = SpellingRanges.end(); I != E; ++I)
+    if (!checkRangeForMacroArgExpansion(*I, SM))
+      return false;
+
+  return true;
+}
+
 /// \brief Recursively emit notes for each macro expansion and caret
 /// diagnostics where appropriate.
 ///
 /// Walks up the macro expansion stack printing expansion notes, the code
 /// snippet, caret, underlines and FixItHint display as appropriate at each
 /// level.
 ///
 /// \param Loc The location for this caret.
 /// \param Level The diagnostic level currently being emitted.
 /// \param Ranges The underlined ranges for this code snippet.
 /// \param Hints The FixIt hints active for this diagnostic.
 void DiagnosticRenderer::emitMacroExpansions(SourceLocation Loc,
                                              DiagnosticsEngine::Level Level,
                                              ArrayRef<CharSourceRange> Ranges,
                                              ArrayRef<FixItHint> Hints,
                                              const SourceManager &SM) {
   assert(!Loc.isInvalid() && "must have a valid source location here");
 
   // Produce a stack of macro backtraces.
   SmallVector<SourceLocation, 8> LocationStack;
+  unsigned IgnoredEnd = 0;
   while (Loc.isMacroID()) {
     LocationStack.push_back(Loc);
+    if (checkRangesForMacroArgExpansion(Loc, Ranges, SM))
+      IgnoredEnd = LocationStack.size();
+
     Loc = SM.getImmediateMacroCallerLoc(Loc);
     assert(!Loc.isInvalid() && "must have a valid source location here");
   }
 
+  LocationStack.erase(LocationStack.begin(),
+                      LocationStack.begin() + IgnoredEnd);
+
   unsigned MacroDepth = LocationStack.size();
   unsigned MacroLimit = DiagOpts->MacroBacktraceLimit;
   if (MacroDepth <= MacroLimit || MacroLimit == 0) {
     for (auto I = LocationStack.rbegin(), E = LocationStack.rend();
          I != E; ++I)
       emitSingleMacroExpansion(*I, Level, Ranges, SM);
     return;
   }

cfe/trunk/test/Misc/diag-presumed.c

 // RUN: not %clang_cc1 -fsyntax-only -pedantic-errors %s 2>&1 | FileCheck %s --check-prefix=PRESUMED
 // RUN: not %clang_cc1 -fsyntax-only -pedantic-errors -fno-diagnostics-use-presumed-location %s 2>&1 | FileCheck %s --check-prefix=SPELLING
 
 #line 100
 #define X(y) y
 X(int n = error);
 
 // PRESUMED: diag-presumed.c:101:11: error: use of undeclared identifier 'error'
-// PRESUMED: diag-presumed.c:100:14: note: expanded from
 // SPELLING: diag-presumed.c:6:11: error: use of undeclared identifier 'error'
-// SPELLING: diag-presumed.c:5:14: note: expanded from
 
 ;
-// PRESUMED: diag-presumed.c:108:1: error: extra ';' outside of a functio
-// SPELLING: diag-presumed.c:13:1: error: extra ';' outside of a functio
+// PRESUMED: diag-presumed.c:106:1: error: extra ';' outside of a functio
+// SPELLING: diag-presumed.c:11:1: error: extra ';' outside of a functio
 
 # 1 "thing1.cc" 1
 # 1 "thing1.h" 1
 # 1 "systemheader.h" 1 3
 ;
 // No diagnostic here: we're in a system header, even if we're using spelling
 // locations for the diagnostics..
 // PRESUMED-NOT: extra ';'
 // SPELLING-NOT: extra ';'
 
 another error;
-// PRESUMED: included from {{.*}}diag-presumed.c:112:
+// PRESUMED: included from {{.*}}diag-presumed.c:110:
 // PRESUMED: from thing1.cc:1:
 // PRESUMED: from thing1.h:1:
 // PRESUMED: systemheader.h:7:1: error: unknown type name 'another'
 
 // SPELLING-NOT: included from
-// SPELLING: diag-presumed.c:26:1: error: unknown type name 'another'
+// SPELLING: diag-presumed.c:24:1: error: unknown type name 'another'
 
 # 1 "thing1.h" 2
 # 1 "thing1.cc" 2

cfe/trunk/test/Misc/reduced-diags-macros-backtrace.cpp

+// RUN: not %clang_cc1 -fsyntax-only -fmacro-backtrace-limit 0 %s 2>&1 | FileCheck %s --check-prefix=ALL
+// RUN: not %clang_cc1 -fsyntax-only -fmacro-backtrace-limit 2 %s 2>&1 | FileCheck %s --check-prefix=SKIP
+
+#define F(x) x + 1
+#define G(x) F(x) + 2
+#define ADD(x,y) G(x) + y
+#define LEVEL4(x) ADD(p,x)
+#define LEVEL3(x) LEVEL4(x)
+#define LEVEL2(x) LEVEL3(x)
+#define LEVEL1(x) LEVEL2(x)
+
+int a = LEVEL1(b);
+
+// ALL: {{.*}}:12:9: error: use of undeclared identifier 'p'
+// ALL-NEXT: int a = LEVEL1(b);
+// ALL-NEXT:         ^
+// ALL-NEXT: {{.*}}:10:19: note: expanded from macro 'LEVEL1'
+// ALL-NEXT: #define LEVEL1(x) LEVEL2(x)
+// ALL-NEXT:                   ^
+// ALL-NEXT: {{.*}}:9:19: note: expanded from macro 'LEVEL2'
+// ALL-NEXT: #define LEVEL2(x) LEVEL3(x)
+// ALL-NEXT:                   ^
+// ALL-NEXT: {{.*}}:8:19: note: expanded from macro 'LEVEL3'
+// ALL-NEXT: #define LEVEL3(x) LEVEL4(x)
+// ALL-NEXT:                   ^
+// ALL-NEXT: {{.*}}:7:23: note: expanded from macro 'LEVEL4'
+// ALL-NEXT: #define LEVEL4(x) ADD(p,x)
+// ALL-NEXT:                       ^
+// ALL-NEXT: {{.*}}:12:16: error: use of undeclared identifier 'b'
+// ALL-NEXT: int a = LEVEL1(b);
+// ALL-NEXT:                ^
+// ALL-NEXT: 2 errors generated.
+
+// SKIP: {{.*}}:12:9: error: use of undeclared identifier 'p'
+// SKIP-NEXT: int a = LEVEL1(b);
+// SKIP-NEXT:         ^
+// SKIP-NEXT: {{.*}}:10:19: note: expanded from macro 'LEVEL1'
+// SKIP-NEXT: #define LEVEL1(x) LEVEL2(x)
+// SKIP-NEXT:                   ^
+// SKIP-NEXT: note: (skipping 2 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
+// SKIP-NEXT: {{.*}}:7:23: note: expanded from macro 'LEVEL4'
+// SKIP-NEXT: #define LEVEL4(x) ADD(p,x)
+// SKIP-NEXT:                       ^
+// SKIP-NEXT: {{.*}}:12:16: error: use of undeclared identifier 'b'
+// SKIP-NEXT: int a = LEVEL1(b);
+// SKIP-NEXT:                ^
+// SKIP-NEXT: 2 errors generated.

cfe/trunk/test/Misc/reduced-diags-macros.cpp

+// RUN: not %clang_cc1 -fsyntax-only %s 2>&1 | FileCheck %s -strict-whitespace
+
+#define NO_INITIATION(x) int a = x * 2
+#define NO_DEFINITION(x) int c = x * 2
+
+NO_INITIATION(a);
+NO_DEFINITION(b);
+
+// CHECK: {{.*}}:6:15: warning: variable 'a' is uninitialized when used within its own initialization
+// CHECK-NEXT: NO_INITIATION(a);
+// CHECK-NEXT: ~~~~~~~~~~~~~~^~
+// CHECK-NEXT: {{.*}}:3:34: note: expanded from macro 'NO_INITIATION'
+// CHECK-NEXT: #define NO_INITIATION(x) int a = x * 2
+// CHECK-NEXT:                                  ^
+
+// CHECK: {{.*}}:7:15: error: use of undeclared identifier 'b'
+// CHECK-NEXT: NO_DEFINITION(b);
+// CHECK-NEXT:               ^
+
+
+#define F(x) x + 1
+#define ADD(x,y) y + F(x)
+#define SWAP_ARGU(x,y) ADD(y,x)
+
+int  p = SWAP_ARGU(3, x);
+
+// CHECK: {{.*}}:25:23: error: use of undeclared identifier 'x'
+// CHECK-NEXT: int  p = SWAP_ARGU(3, x);
+// CHECK-NEXT:                       ^

cfe/trunk/test/Preprocessor/macro_arg_slocentry_merge.c

 // RUN: not %clang_cc1 -fsyntax-only %s 2>&1 | FileCheck %s
 
 #include "macro_arg_slocentry_merge.h"
 
 // CHECK: macro_arg_slocentry_merge.h:7:19: error: unknown type name 'win'
-// CHECK: macro_arg_slocentry_merge.h:5:16: note: expanded from macro 'WINDOW'
-// CHECK: macro_arg_slocentry_merge.h:6:18: note: expanded from macro 'P_'