NoQ updated this revision to Diff 192917.
NoQ added a comment.
Also, this is kinda weird. According to my logic, we should have written
`Assuming 'i' is equal to 4294967295` because that's what the user will see in
the macro popup. However, that's incorrect for the same reason: `i` is an int,
while `4294967295` doesn't fit into an int, so they can never be equal. Writing
`Assuming 'i' is equal to UINT32_MAX` is incorrect for the same reason, and the
current message is the only one that's technically the truth.
Writing what exactly we're assuming is, in my opinion, more important than
repeating what's literally written in the code.
Added a FIXME test to document this problem.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59121/new/
https://reviews.llvm.org/D59121
Files:
clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
clang/test/Analysis/diagnostics/macros.cpp
Index: clang/test/Analysis/diagnostics/macros.cpp
===================================================================
--- clang/test/Analysis/diagnostics/macros.cpp
+++ clang/test/Analysis/diagnostics/macros.cpp
@@ -3,7 +3,7 @@
#include "../Inputs/system-header-simulator.h"
#include "../Inputs/system-header-simulator-cxx.h"
-void testIntMacro(unsigned int i) {
+void testUnsignedIntMacro(unsigned int i) {
if (i == UINT32_MAX) { // expected-note {{Assuming 'i' is equal to UINT32_MAX}}
// expected-note@-1 {{Taking true branch}}
char *p = NULL; // expected-note {{'p' initialized to a null pointer value}}
@@ -12,6 +12,20 @@
}
}
+
+// FIXME: 'i' can never be equal to UINT32_MAX - it doesn't even fit into its
+// type ('int'). This should say "Assuming 'i' is equal to -1".
+void testIntMacro(int i) {
+ if (i == UINT32_MAX) { // expected-note {{Assuming 'i' is equal to UINT32_MAX}}
+ // expected-note@-1 {{Taking true branch}}
+ char *p = NULL; // expected-note {{'p' initialized to a null pointer value}}
+ *p = 7; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+ // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+ }
+}
+
+
+
void testNULLMacro(int *p) {
if (p == NULL) { // expected-note {{Assuming 'p' is equal to NULL}}
// expected-note@-1 {{Taking true branch}}
@@ -47,3 +61,14 @@
// expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
}
}
+
+#define nested_null_split(x) if ((x) != UINT32_MAX) {}
+
+void testNestedNullSplitMacro(int i, int *p) {
+ nested_null_split(i); // expected-note {{Assuming 'i' is equal to -1}}
+ // expected-note@-1 {{Taking false branch}}
+ if (!p) // expected-note {{Assuming 'p' is null}}
+ // expected-note@-1 {{Taking true branch}}
+ *p = 1; // expected-warning {{Dereference of null pointer (loaded from variable 'p')}}
+ // expected-note@-1 {{Dereference of null pointer (loaded from variable 'p')}}
+}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1969,43 +1969,22 @@
const Expr *OriginalExpr = Ex;
Ex = Ex->IgnoreParenCasts();
- // Use heuristics to determine if Ex is a macro expending to a literal and
- // if so, use the macro's name.
- SourceLocation LocStart = Ex->getBeginLoc();
- SourceLocation LocEnd = Ex->getEndLoc();
- if (LocStart.isMacroID() && LocEnd.isMacroID() &&
- (isa<GNUNullExpr>(Ex) ||
- isa<ObjCBoolLiteralExpr>(Ex) ||
- isa<CXXBoolLiteralExpr>(Ex) ||
- isa<IntegerLiteral>(Ex) ||
- isa<FloatingLiteral>(Ex))) {
- StringRef StartName = Lexer::getImmediateMacroNameForDiagnostics(LocStart,
- BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
- StringRef EndName = Lexer::getImmediateMacroNameForDiagnostics(LocEnd,
- BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
- bool beginAndEndAreTheSameMacro = StartName.equals(EndName);
-
- bool partOfParentMacro = false;
- if (ParentEx->getBeginLoc().isMacroID()) {
- StringRef PName = Lexer::getImmediateMacroNameForDiagnostics(
- ParentEx->getBeginLoc(), BRC.getSourceManager(),
- BRC.getASTContext().getLangOpts());
- partOfParentMacro = PName.equals(StartName);
- }
-
- if (beginAndEndAreTheSameMacro && !partOfParentMacro ) {
- // Get the location of the macro name as written by the caller.
- SourceLocation Loc = LocStart;
- while (LocStart.isMacroID()) {
- Loc = LocStart;
- LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);
+ if (isa<GNUNullExpr>(Ex) || isa<ObjCBoolLiteralExpr>(Ex) ||
+ isa<CXXBoolLiteralExpr>(Ex) || isa<IntegerLiteral>(Ex) ||
+ isa<FloatingLiteral>(Ex)) {
+ // Use heuristics to determine if the expression is a macro
+ // expanding to a literal and if so, use the macro's name.
+ SourceLocation BeginLoc = OriginalExpr->getBeginLoc();
+ SourceLocation EndLoc = OriginalExpr->getEndLoc();
+ if (BeginLoc.isMacroID() && EndLoc.isMacroID()) {
+ SourceManager &SM = BRC.getSourceManager();
+ const LangOptions &LO = BRC.getASTContext().getLangOpts();
+ if (Lexer::isAtStartOfMacroExpansion(BeginLoc, SM, LO) &&
+ Lexer::isAtEndOfMacroExpansion(EndLoc, SM, LO)) {
+ CharSourceRange R = Lexer::getAsCharRange({BeginLoc, EndLoc}, SM, LO);
+ Out << Lexer::getSourceText(R, SM, LO);
+ return false;
}
- StringRef MacroName = Lexer::getImmediateMacroNameForDiagnostics(
- Loc, BRC.getSourceManager(), BRC.getASTContext().getLangOpts());
-
- // Return the macro name.
- Out << MacroName;
- return false;
}
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
