[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