martong updated this revision to Diff 342342. martong marked an inline comment as done. martong added a comment.
- Add the description to the note tag only if the SVal is interesting - Use 'Assuming the nth arg is in ...' form for the descriptions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101526/new/ https://reviews.llvm.org/D101526 Files: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp clang/test/Analysis/std-c-library-functions-arg-constraints.c
Index: clang/test/Analysis/std-c-library-functions-arg-constraints.c =================================================================== --- clang/test/Analysis/std-c-library-functions-arg-constraints.c +++ clang/test/Analysis/std-c-library-functions-arg-constraints.c @@ -238,6 +238,7 @@ // 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}} \ @@ -251,7 +252,6 @@ 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}} \ Index: clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp @@ -0,0 +1,19 @@ +// 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 + +int __single_val_0(int); // [0, 0] + +int test_note(int x, int y) { + __single_val_0(x); // expected-note{{Assuming the 1st arg is within the range [0, 0]}} + return y / x; // expected-warning{{Division by zero}} \ + // expected-note{{Division by zero}} +} Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp @@ -142,9 +142,18 @@ virtual StringRef getName() const = 0; + // Represents that in which context do we require a description of the + // constraint. + enum class DescritptionKind { + // 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(DescritptionKind 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 @@ -182,7 +191,7 @@ RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector &Ranges) : ValueConstraint(ArgN), Kind(Kind), Ranges(Ranges) {} - std::string describe(ProgramStateRef State, + std::string describe(DescritptionKind DK, ProgramStateRef State, const Summary &Summary) const override; const IntRangeVector &getRanges() const { return Ranges; } @@ -252,7 +261,7 @@ bool CannotBeNull = true; public: - std::string describe(ProgramStateRef State, + std::string describe(DescritptionKind DK, ProgramStateRef State, const Summary &Summary) const override; StringRef getName() const override { return "NonNull"; } ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, @@ -324,7 +333,7 @@ return Result; } - std::string describe(ProgramStateRef State, + std::string describe(DescritptionKind DK, ProgramStateRef State, const Summary &Summary) const override; ProgramStateRef apply(ProgramStateRef State, const CallEvent &Call, @@ -598,9 +607,12 @@ // 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::DescritptionKind::Violation, C.getState(), Summary); + // Capitalize the firs 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)); } @@ -618,24 +630,26 @@ } std::string StdLibraryFunctionsChecker::NotNullConstraint::describe( - ProgramStateRef State, const Summary &Summary) const { + DescritptionKind DK, ProgramStateRef State, const Summary &Summary) const { SmallString<48> Result; - Result += "The "; + const auto Violation = ValueConstraint::DescritptionKind::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 { + DescritptionKind 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::DescritptionKind::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"; @@ -672,11 +686,13 @@ } std::string StdLibraryFunctionsChecker::BufferSizeConstraint::describe( - ProgramStateRef State, const Summary &Summary) const { + DescritptionKind DK, ProgramStateRef State, const Summary &Summary) const { SmallString<96> Result; - Result += "The size of the "; + const auto Violation = ValueConstraint::DescritptionKind::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 less than the value of "; if (ConcreteSize) { ConcreteSize->toString(Result); } else if (SizeArgN) { @@ -810,6 +826,7 @@ 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 = @@ -819,17 +836,28 @@ 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::DescritptionKind::Assumption, + NewState, Summary); + const auto ArgSVal = Call.getArgSVal(Constraint->getArgNo()); + NewNode = C.addTransition( + NewState, NewNode, + C.getNoteTag([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, @@ -2570,6 +2598,10 @@ 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)
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits