Author: Gabor Marton Date: 2022-10-26T16:33:25+02:00 New Revision: 82a50812f7e539b69fb8031bee9c823e5290d1c9
URL: https://github.com/llvm/llvm-project/commit/82a50812f7e539b69fb8031bee9c823e5290d1c9 DIFF: https://github.com/llvm/llvm-project/commit/82a50812f7e539b69fb8031bee9c823e5290d1c9.diff LOG: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints In this patch I add a new NoteTag for each applied argument constraint. This way, any other checker that reports a bug - where the applied constraint is relevant - will display the corresponding note. With this change we provide more information for the users to understand some bug reports easier. Differential Revision: https://reviews.llvm.org/D101526 Reviewed By: NoQ Added: clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp Modified: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp clang/test/Analysis/std-c-library-functions-arg-constraints.c Removed: ################################################################################ diff --git a/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp index 195a5fa977438..f9efcc5ccccd9 100644 --- a/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp @@ -115,7 +115,8 @@ bool ExprInspectionChecker::evalCall(const CallEvent &Call, .Case("clang_analyzer_hashDump", &ExprInspectionChecker::analyzerHashDump) .Case("clang_analyzer_denote", &ExprInspectionChecker::analyzerDenote) - .Case("clang_analyzer_express", + .Case("clang_analyzer_express", // This also marks the argument as + // interesting. &ExprInspectionChecker::analyzerExpress) .StartsWith("clang_analyzer_isTainted", &ExprInspectionChecker::analyzerIsTainted) @@ -530,14 +531,14 @@ void ExprInspectionChecker::analyzerExpress(const CallExpr *CE, SVal ArgVal = C.getSVal(CE->getArg(0)); SymbolRef Sym = ArgVal.getAsSymbol(); if (!Sym) { - reportBug("Not a symbol", C); + reportBug("Not a symbol", C, ArgVal); return; } SymbolExpressor V(C.getState()); auto Str = V.Visit(Sym); if (!Str) { - reportBug("Unable to express", C); + reportBug("Unable to express", C, ArgVal); return; } diff --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp index 25e80545154ee..c5fa43ac23d1e 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp @@ -134,9 +134,18 @@ class StdLibraryFunctionsChecker virtual StringRef getName() const = 0; + // Represents that in which context do we require a description of the + // constraint. + enum class DescriptionKind { + // The constraint is violated. + Violation, + // We assume that the constraint is satisfied. + Assumption + }; + // Give a description that explains the constraint to the user. Used when // the bug is reported. - virtual std::string describe(ProgramStateRef State, + virtual std::string describe(DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const { // There are some descendant classes that are not used as argument // constraints, e.g. ComparisonConstraint. In that case we can safely @@ -174,7 +183,7 @@ class StdLibraryFunctionsChecker RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector &Ranges) : ValueConstraint(ArgN), Kind(Kind), Ranges(Ranges) {} - std::string describe(ProgramStateRef State, + std::string describe(DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const override; const IntRangeVector &getRanges() const { return Ranges; } @@ -244,7 +253,7 @@ class StdLibraryFunctionsChecker bool CannotBeNull = true; public: - std::string describe(ProgramStateRef State, + std::string describe(DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const override; StringRef getName() const override { return "NonNull"; } ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, @@ -316,7 +325,7 @@ class StdLibraryFunctionsChecker return Result; } - std::string describe(ProgramStateRef State, + std::string describe(DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const override; ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, @@ -704,9 +713,12 @@ class StdLibraryFunctionsChecker // Highlight the range of the argument that was violated. R->addRange(Call.getArgSourceRange(VC->getArgNo())); - // Describe the argument constraint in a note. - R->addNote(VC->describe(C.getState(), Summary), R->getLocation(), - Call.getArgSourceRange(VC->getArgNo())); + // Describe the argument constraint violation in a note. + std::string Descr = VC->describe( + ValueConstraint::DescriptionKind::Violation, C.getState(), Summary); + // Capitalize the first letter b/c we want a full sentence. + Descr[0] = toupper(Descr[0]); + R->addNote(Descr, R->getLocation(), Call.getArgSourceRange(VC->getArgNo())); C.emitReport(std::move(R)); } @@ -735,24 +747,26 @@ static BasicValueFactory &getBVF(ProgramStateRef State) { } std::string StdLibraryFunctionsChecker::NotNullConstraint::describe( - ProgramStateRef State, const Summary &Summary) const { + DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const { SmallString<48> Result; - Result += "The "; + const auto Violation = ValueConstraint::DescriptionKind::Violation; + Result += "the "; Result += getArgDesc(ArgN); - Result += " should not be NULL"; + Result += DK == Violation ? " should not be NULL" : " is not NULL"; return Result.c_str(); } std::string StdLibraryFunctionsChecker::RangeConstraint::describe( - ProgramStateRef State, const Summary &Summary) const { + DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const { BasicValueFactory &BVF = getBVF(State); QualType T = Summary.getArgType(getArgNo()); SmallString<48> Result; - Result += "The "; + const auto Violation = ValueConstraint::DescriptionKind::Violation; + Result += "the "; Result += getArgDesc(ArgN); - Result += " should be "; + Result += DK == Violation ? " should be " : " is "; // Range kind as a string. Kind == OutOfRange ? Result += "out of" : Result += "within"; @@ -784,16 +798,18 @@ StdLibraryFunctionsChecker::getArgDesc(StdLibraryFunctionsChecker::ArgNo ArgN) { SmallString<8> Result; Result += std::to_string(ArgN + 1); Result += llvm::getOrdinalSuffix(ArgN + 1); - Result += " arg"; + Result += " argument"; return Result; } std::string StdLibraryFunctionsChecker::BufferSizeConstraint::describe( - ProgramStateRef State, const Summary &Summary) const { + DescriptionKind DK, ProgramStateRef State, const Summary &Summary) const { SmallString<96> Result; - Result += "The size of the "; + const auto Violation = ValueConstraint::DescriptionKind::Violation; + Result += "the size of the "; Result += getArgDesc(ArgN); - Result += " should be equal to or less than the value of "; + Result += DK == Violation ? " should be " : " is "; + Result += "equal to or greater than the value of "; if (ConcreteSize) { ConcreteSize->toString(Result); } else if (SizeArgN) { @@ -927,6 +943,7 @@ void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call, ProgramStateRef State = C.getState(); ProgramStateRef NewState = State; + ExplodedNode *NewNode = C.getPredecessor(); for (const ValueConstraintPtr &Constraint : Summary.getArgConstraints()) { ProgramStateRef SuccessSt = Constraint->apply(NewState, Call, Summary, C); ProgramStateRef FailureSt = @@ -936,17 +953,28 @@ void StdLibraryFunctionsChecker::checkPreCall(const CallEvent &Call, if (ExplodedNode *N = C.generateErrorNode(NewState)) reportBug(Call, N, Constraint.get(), Summary, C); break; - } else { - // We will apply the constraint even if we cannot reason about the - // argument. This means both SuccessSt and FailureSt can be true. If we - // weren't applying the constraint that would mean that symbolic - // execution continues on a code whose behaviour is undefined. - assert(SuccessSt); - NewState = SuccessSt; + } + // We will apply the constraint even if we cannot reason about the + // argument. This means both SuccessSt and FailureSt can be true. If we + // weren't applying the constraint that would mean that symbolic + // execution continues on a code whose behaviour is undefined. + assert(SuccessSt); + NewState = SuccessSt; + if (NewState != State) { + SmallString<64> Msg; + Msg += "Assuming "; + Msg += Constraint->describe(ValueConstraint::DescriptionKind::Assumption, + NewState, Summary); + const auto ArgSVal = Call.getArgSVal(Constraint->getArgNo()); + NewNode = C.addTransition( + NewState, NewNode, + C.getNoteTag([Msg = std::move(Msg), ArgSVal]( + PathSensitiveBugReport &BR, llvm::raw_ostream &OS) { + if (BR.isInteresting(ArgSVal)) + OS << Msg; + })); } } - if (NewState && NewState != State) - C.addTransition(NewState); } void StdLibraryFunctionsChecker::checkPostCall(const CallEvent &Call, @@ -2882,6 +2910,10 @@ void StdLibraryFunctionsChecker::initFunctionSummaries( Summary(EvalCallAsPure).ArgConstraint(NotNull(ArgNo(0)))); // Test range values. + addToFunctionSummaryMap( + "__single_val_0", Signature(ArgTypes{IntTy}, RetType{IntTy}), + Summary(EvalCallAsPure) + .ArgConstraint(ArgumentCondition(0U, WithinRange, SingleValue(0)))); addToFunctionSummaryMap( "__single_val_1", Signature(ArgTypes{IntTy}, RetType{IntTy}), Summary(EvalCallAsPure) diff --git a/clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp b/clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp new file mode 100644 index 0000000000000..1f36b2b03fac9 --- /dev/null +++ b/clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp @@ -0,0 +1,51 @@ +// RUN: %clang_analyze_cc1 %s \ +// RUN: -analyzer-checker=core \ +// RUN: -analyzer-checker=apiModeling.StdCLibraryFunctions \ +// RUN: -analyzer-checker=alpha.unix.StdCLibraryFunctionArgs \ +// RUN: -analyzer-checker=debug.StdCLibraryFunctionsTester \ +// RUN: -analyzer-config apiModeling.StdCLibraryFunctions:DisplayLoadedSummaries=true \ +// RUN: -analyzer-checker=debug.ExprInspection \ +// RUN: -analyzer-config eagerly-assume=false \ +// RUN: -triple i686-unknown-linux \ +// RUN: -analyzer-output=text \ +// RUN: -verify + +template <typename T> +void clang_analyzer_express(T x); +void clang_analyzer_eval(bool); +int clang_analyzer_getExtent(void *); + + +// Check NotNullConstraint assumption notes. +int __not_null(int *); +int test_not_null_note(int *x, int y) { + __not_null(x); // expected-note{{Assuming the 1st argument is not NULL}} + if (x) // expected-note{{'x' is non-null}} \ + // expected-note{{Taking true branch}} + if (!y) // expected-note{{Assuming 'y' is 0}} \ + // expected-note{{Taking true branch}} + return 1 / y; // expected-warning{{Division by zero}} \ + // expected-note{{Division by zero}} + + return 0; +} + +// Check the RangeConstraint assumption notes. +int __single_val_0(int); // [0, 0] +int test_range_constraint_note(int x, int y) { + __single_val_0(x); // expected-note{{Assuming the 1st argument is within the range [0, 0]}} + return y / x; // expected-warning{{Division by zero}} \ + // expected-note{{Division by zero}} +} + +// Check the BufferSizeConstraint assumption notes. +int __buf_size_arg_constraint_concrete(const void *buf); // size of buf must be >= 10 +void test_buffer_size_note(char *buf, int y) { + __buf_size_arg_constraint_concrete(buf); // expected-note {{Assuming the size of the 1st argument is equal to or greater than the value of 10}} + clang_analyzer_eval(clang_analyzer_getExtent(buf) >= 10); // expected-warning{{TRUE}} \ + // expected-note{{TRUE}} + + // clang_analyzer_express marks the argument as interesting. + clang_analyzer_express(buf); // expected-warning {{}} // the message does not really matter \ + // expected-note {{}} +} diff --git a/clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp b/clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp index baa7ad5dc0fe7..7481a015e5580 100644 --- a/clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp +++ b/clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp @@ -15,7 +15,7 @@ int __not_null(int *); void test_not_null(int *x) { __not_null(nullptr); // \ - // expected-note{{The 1st arg should not be NULL}} \ + // expected-note{{The 1st argument should not be NULL}} \ // expected-warning{{}} } @@ -29,21 +29,21 @@ void test_buffer_size(int x) { case 1: { char buf[9]; __buf_size_arg_constraint_concrete(buf); // \ - // expected-note{{The size of the 1st arg should be equal to or less than the value of 10}} \ + // expected-note{{The size of the 1st argument should be equal to or greater than the value of 10}} \ // expected-warning{{}} break; } case 2: { char buf[3]; __buf_size_arg_constraint(buf, 4); // \ - // expected-note{{The size of the 1st arg should be equal to or less than the value of the 2nd arg}} \ + // expected-note{{The size of the 1st argument should be equal to or greater than the value of the 2nd arg}} \ // expected-warning{{}} break; } case 3: { char buf[3]; __buf_size_arg_constraint_mul(buf, 4, 2); // \ - // expected-note{{The size of the 1st arg should be equal to or less than the value of the 2nd arg times the 3rd arg}} \ + // expected-note{{The size of the 1st argument should be equal to or greater than the value of the 2nd argument times the 3rd argument}} \ // expected-warning{{}} break; } @@ -56,7 +56,7 @@ int __range_1_2(int); // [1, 2] int __range_1_2__4_5(int); // [1, 2], [4, 5] void test_range(int x) { __single_val_1(2); // \ - // expected-note{{The 1st arg should be within the range [1, 1]}} \ + // expected-note{{The 1st argument should be within the range [1, 1]}} \ // expected-warning{{}} } // Do more specific check against the range strings. diff --git a/clang/test/Analysis/std-c-library-functions-arg-constraints.c b/clang/test/Analysis/std-c-library-functions-arg-constraints.c index 32511c619addc..b5f1df8877943 100644 --- a/clang/test/Analysis/std-c-library-functions-arg-constraints.c +++ b/clang/test/Analysis/std-c-library-functions-arg-constraints.c @@ -239,6 +239,7 @@ void test_constraints_on_multiple_args(int x, int y) { // State split should not happen here. I.e. x == 1 should not be evaluated // FALSE. __two_constrained_args(x, y); + //NOTE! Because of the second `clang_analyzer_eval` call we have two bug clang_analyzer_eval(x == 1); // \ // report-warning{{TRUE}} \ // bugpath-warning{{TRUE}} \ @@ -252,7 +253,6 @@ void test_constraints_on_multiple_args(int x, int y) { int __arg_constrained_twice(int); void test_multiple_constraints_on_same_arg(int x) { __arg_constrained_twice(x); - // Check that both constraints are applied and only one branch is there. clang_analyzer_eval(x < 1 || x > 2); // \ // report-warning{{TRUE}} \ // bugpath-warning{{TRUE}} \ _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits