[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
whisperity added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:195-196 + llvm::raw_svector_ostream OS(SBuf); + OS << "Null pointer passed as an argument to a 'nonnull' "; + OS << Idx << llvm::getOrdinalSuffix(Idx) << " parameter"; + It seems to be as if now you're able to present which parameter is the `[[nonnull]]` one. Because of this, the output to the user sounds really bad and unfriendly, at least to me. How about this: "null pointer passed to nth parameter, but it's marked 'nonnull'"? "null pointer passed to nth parameter expecting 'nonnull'"? Something along these lines. To a parameter, we're always passing arguments, so saying "as an argument" seems redundant. And because we have the index always in our hands, we don't need to use the indefinite article. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
Szelethus added a comment. I really like the change, thanks! Let's settle on the diagnostic message then. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:202 + SVal l, + unsigned idx = -1u) const; ProgramStateRef CheckLocation(CheckerContext &C, I think `Optional` would be nicer. Also, `checkNonNull` as a function name doesn't scream about why we need an index parameter -- could you rename it to `IndexOfArg` or something similar? Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:195-196 + llvm::raw_svector_ostream OS(SBuf); + OS << "Null pointer passed as an argument to a 'nonnull' "; + OS << Idx << llvm::getOrdinalSuffix(Idx) << " parameter"; + whisperity wrote: > It seems to be as if now you're able to present which parameter is the > `[[nonnull]]` one. Because of this, the output to the user sounds really bad > and unfriendly, at least to me. > > How about this: > > "null pointer passed to nth parameter, but it's marked 'nonnull'"? > "null pointer passed to nth parameter expecting 'nonnull'"? > > Something along these lines. > > To a parameter, we're always passing arguments, so saying "as an argument" > seems redundant. > > And because we have the index always in our hands, we don't need to use the > indefinite article. Agreed. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. Thanks! I'm happy to accept this as soon as other reviewers are happy. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1548 - state = checkNonNull(C, state, Dst, DstVal); + state = checkNonNull(C, state, Dst, DstVal, 1); if (!state) You could also pass a description of the parameter (eg., "source" or "destination"). Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
bruntib updated this revision to Diff 215754.
bruntib added a comment.
The checker message has been changed according to the suggestion.
The last parameter of checkNonNull() doesn't require a default or optional
value, so it has been fixed.
The parameter has been renamed from Idx to IdxOfArg.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66333/new/
https://reviews.llvm.org/D66333
Files:
clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
clang/test/Analysis/misc-ps-region-store.m
clang/test/Analysis/null-deref-ps.c
Index: clang/test/Analysis/null-deref-ps.c
===
--- clang/test/Analysis/null-deref-ps.c
+++ clang/test/Analysis/null-deref-ps.c
@@ -88,21 +88,21 @@
int bar(int* p, int q) __attribute__((nonnull));
int f6(int *p) {
- return !p ? bar(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+ return !p ? bar(p, 1) // expected-warning {{Null pointer passed to 1st parameter expecting 'nonnull'}}
: bar(p, 0); // no-warning
}
int bar2(int* p, int q) __attribute__((nonnull(1)));
int f6b(int *p) {
- return !p ? bar2(p, 1) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+ return !p ? bar2(p, 1) // expected-warning {{Null pointer passed to 1st parameter expecting 'nonnull'}}
: bar2(p, 0); // no-warning
}
int bar3(int*p, int q, int *r) __attribute__((nonnull(1,3)));
int f6c(int *p, int *q) {
- return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed as an argument to a 'nonnull' parameter}}
+ return !p ? bar3(q, 2, p) // expected-warning {{Null pointer passed to 3rd parameter expecting 'nonnull'}}
: bar3(p, 2, q); // no-warning
}
Index: clang/test/Analysis/misc-ps-region-store.m
===
--- clang/test/Analysis/misc-ps-region-store.m
+++ clang/test/Analysis/misc-ps-region-store.m
@@ -1205,7 +1205,7 @@
void rdar_8642434_funcB(struct rdar_8642434_typeA *x, struct rdar_8642434_typeA *y) {
rdar_8642434_funcA(x);
if (!y)
-rdar_8642434_funcA(y); // expected-warning{{Null pointer passed as an argument to a 'nonnull' parameter}}
+rdar_8642434_funcA(y); // expected-warning{{Null pointer passed to 1st parameter expecting 'nonnull'}}
}
// - Handle loads and stores from a symbolic index
Index: clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
===
--- clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-output.m.plist
@@ -6141,12 +6141,12 @@
depth0
extended_message
- Null pointer passed as an argument to a 'nonnull' parameter
+ Null pointer passed to 1st parameter expecting 'nonnull'
message
- Null pointer passed as an argument to a 'nonnull' parameter
+ Null pointer passed to 1st parameter expecting 'nonnull'
- descriptionNull pointer passed as an argument to a 'nonnull' parameter
+ descriptionNull pointer passed to 1st parameter expecting 'nonnull'
categoryAPI
typeArgument with 'nonnull' attribute passed null
check_namecore.NonNullParamChecker
Index: clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
===
--- clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
+++ clang/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
@@ -10853,12 +10853,12 @@
depth0
extended_message
- Null pointer passed as an argument to a 'nonnull' parameter
+ Null pointer passed to 1st parameter expecting 'nonnull'
message
- Null pointer passed as an argument to a 'nonnull' parameter
+ Null pointer passed to 1st parameter expecting 'nonnull'
- descriptionNull pointer passed as an argument to a 'nonnull' parameter
+ descriptionNull pointer passed to 1st parameter expecting 'nonnull'
categoryAPI
typeArgument with 'nonnull' attribute passed null
check_namecore.NonNullParamChecker
Index: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -36,7 +36,9 @@
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
std::unique_ptr
- genReportNullAttrNonNull(const ExplodedNode *ErrorN, const Expr *ArgE) const;
+ genReportNullAttrNonNull(const ExplodedNode *ErrorN,
+ const Expr *ArgE,
+ unsigned IdxOfArg) const;
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
bruntib marked 3 inline comments as done. bruntib added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:202 + SVal l, + unsigned idx = -1u) const; ProgramStateRef CheckLocation(CheckerContext &C, Szelethus wrote: > I think `Optional` would be nicer. Also, `checkNonNull` as a > function name doesn't scream about why we need an index parameter -- could > you rename it to `IndexOfArg` or something similar? The default value of IdxOfArg comes from my laziness. There were two places where I was not sure what index number should be given, but I added those too. This way no default value or Optional is needed. Omitting this information wouldn't even be reasonable, since there is always an ordinal number of the argument. Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1548 - state = checkNonNull(C, state, Dst, DstVal); + state = checkNonNull(C, state, Dst, DstVal, 1); if (!state) NoQ wrote: > You could also pass a description of the parameter (eg., "source" or > "destination"). Could you please give some hint, how to include this in the message? I don't know how to do it concisely. Comment at: clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp:195-196 + llvm::raw_svector_ostream OS(SBuf); + OS << "Null pointer passed as an argument to a 'nonnull' "; + OS << Idx << llvm::getOrdinalSuffix(Idx) << " parameter"; + Szelethus wrote: > whisperity wrote: > > It seems to be as if now you're able to present which parameter is the > > `[[nonnull]]` one. Because of this, the output to the user sounds really > > bad and unfriendly, at least to me. > > > > How about this: > > > > "null pointer passed to nth parameter, but it's marked 'nonnull'"? > > "null pointer passed to nth parameter expecting 'nonnull'"? > > > > Something along these lines. > > > > To a parameter, we're always passing arguments, so saying "as an argument" > > seems redundant. > > > > And because we have the index always in our hands, we don't need to use the > > indefinite article. > Agreed. I used the original message and just extended it with a number. Are you sure that it is a good practice to change a checker message? I'm not against the modification, just a little afraid that somebody may rely on it. It's quite unlikely though, so I changed it :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
Szelethus accepted this revision. Szelethus added a comment. @whisperity, any objections to this? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
whisperity accepted this revision. whisperity added a comment. @Szelethus Nope. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
NoQ added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:295-297 + OS << "Null pointer argument in call to " << CurrentFunctionDescription + << ' ' << IdxOfArg << llvm::getOrdinalSuffix(IdxOfArg) + << " parameter"; It sounds like this code remained untested because tests don't match end-of-line. I noticed it because it produces warnings that don't look like valid English, such as: > "Null pointer argument in call to memory set function 1st parameter" Patches are welcome :) Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
Charusso added inline comments.
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1548
- state = checkNonNull(C, state, Dst, DstVal);
+ state = checkNonNull(C, state, Dst, DstVal, 1);
if (!state)
bruntib wrote:
> NoQ wrote:
> > You could also pass a description of the parameter (eg., "source" or
> > "destination").
> Could you please give some hint, how to include this in the message? I don't
> know how to do it concisely.
I have made this for that purpose:
```
struct CallContext {
CallContext(Optional DestinationPos,
Optional SourcePos = None,
Optional LengthPos = None)
: DestinationPos(DestinationPos), SourcePos(SourcePos),
LengthPos(LengthPos) {};
Optional DestinationPos;
Optional SourcePos;
Optional LengthPos;
};
```
```
// char *strcpy(char *dest, const char *src);
{{CDF_MaybeBuiltin, "stpcpy", 2}, {&CStringChecker::evalStpcpy, {0, 1}}},
```
May it helps.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66333/new/
https://reviews.llvm.org/D66333
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
This revision was automatically updated to reflect the committed changes.
Closed by commit rL370798: [analyzer] NonNullParamChecker and CStringChecker
parameter number in checker… (authored by Szelethus, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D66333?vs=215754&id=218492#toc
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66333/new/
https://reviews.llvm.org/D66333
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
cfe/trunk/test/Analysis/Inputs/expected-plists/edges-new.mm.plist
cfe/trunk/test/Analysis/Inputs/expected-plists/plist-output.m.plist
cfe/trunk/test/Analysis/misc-ps-region-store.m
cfe/trunk/test/Analysis/null-deref-ps.c
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
@@ -36,7 +36,9 @@
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
std::unique_ptr
- genReportNullAttrNonNull(const ExplodedNode *ErrorN, const Expr *ArgE) const;
+ genReportNullAttrNonNull(const ExplodedNode *ErrorN,
+ const Expr *ArgE,
+ unsigned IdxOfArg) const;
std::unique_ptr
genReportReferenceToNullPointer(const ExplodedNode *ErrorN,
const Expr *ArgE) const;
@@ -143,7 +145,7 @@
std::unique_ptr R;
if (haveAttrNonNull)
- R = genReportNullAttrNonNull(errorNode, ArgE);
+ R = genReportNullAttrNonNull(errorNode, ArgE, idx + 1);
else if (haveRefTypeParam)
R = genReportReferenceToNullPointer(errorNode, ArgE);
@@ -179,7 +181,8 @@
std::unique_ptr
NonNullParamChecker::genReportNullAttrNonNull(const ExplodedNode *ErrorNode,
- const Expr *ArgE) const {
+ const Expr *ArgE,
+ unsigned IdxOfArg) const {
// Lazily allocate the BugType object if it hasn't already been
// created. Ownership is transferred to the BugReporter object once
// the BugReport is passed to 'EmitWarning'.
@@ -187,9 +190,13 @@
BTAttrNonNull.reset(new BugType(
this, "Argument with 'nonnull' attribute passed null", "API"));
- auto R = std::make_unique(
- *BTAttrNonNull,
- "Null pointer passed as an argument to a 'nonnull' parameter", ErrorNode);
+ llvm::SmallString<256> SBuf;
+ llvm::raw_svector_ostream OS(SBuf);
+ OS << "Null pointer passed to "
+ << IdxOfArg << llvm::getOrdinalSuffix(IdxOfArg)
+ << " parameter expecting 'nonnull'";
+
+ auto R = llvm::make_unique(*BTAttrNonNull, SBuf, ErrorNode);
if (ArgE)
bugreporter::trackExpressionValue(ErrorNode, ArgE, *R);
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -198,7 +198,8 @@
ProgramStateRef checkNonNull(CheckerContext &C,
ProgramStateRef state,
const Expr *S,
- SVal l) const;
+ SVal l,
+ unsigned IdxOfArg) const;
ProgramStateRef CheckLocation(CheckerContext &C,
ProgramStateRef state,
const Expr *S,
@@ -277,7 +278,8 @@
ProgramStateRef CStringChecker::checkNonNull(CheckerContext &C,
ProgramStateRef state,
-const Expr *S, SVal l) const {
+const Expr *S, SVal l,
+unsigned IdxOfArg) const {
// If a previous check has failed, propagate the failure.
if (!state)
return nullptr;
@@ -288,11 +290,13 @@
if (stateNull && !stateNonNull) {
if (Filter.CheckCStringNullArg) {
SmallString<80> buf;
- llvm::raw_svector_ostream os(buf);
+ llvm::raw_svector_ostream OS(buf);
assert(CurrentFunctionDescription);
- os << "Null pointer argument in call to " << CurrentFunctionDescription;
+ OS << "Null pointer argument in call to " << CurrentFunctionDescription
+ << ' ' << IdxOfArg << llvm::getOrdinalSuffix(IdxOfArg)
+ << " parameter";
- emitNullArgBug(C, stateNull, S, os.str());
+ emitNullArgBug(C, stateNull, S, OS.str());
}
return nullptr;
}
@@ -384,7 +388,7 @@
// Check that the first buffer is non-null.
SVal BufVal
[PATCH] D66333: [analyzer] NonNullParamChecker and CStringChecker parameter number in checker message
NoQ added inline comments. Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:295-297 + OS << "Null pointer argument in call to " << CurrentFunctionDescription + << ' ' << IdxOfArg << llvm::getOrdinalSuffix(IdxOfArg) + << " parameter"; NoQ wrote: > It sounds like this code remained untested because tests don't match > end-of-line. > > I noticed it because it produces warnings that don't look like valid English, > such as: > > > "Null pointer argument in call to memory set function 1st parameter" > > Patches are welcome :) D71321. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66333/new/ https://reviews.llvm.org/D66333 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
