[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa97648b93846: [analyzer][StdLibraryFunctionsChecker] Add 
better diagnostics (authored by martong).

Changed prior to commit:
  https://reviews.llvm.org/D79431?vs=280376=290930#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -126,6 +126,8 @@
 }
 ArgNo getArgNo() const { return ArgN; }
 
+virtual StringRef getName() const = 0;
+
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
 
@@ -152,6 +154,7 @@
 IntRangeVector Ranges;
 
   public:
+StringRef getName() const override { return "Range"; }
 RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector )
 : ValueConstraint(ArgN), Kind(Kind), Ranges(Ranges) {}
 
@@ -205,6 +208,7 @@
 ArgNo OtherArgN;
 
   public:
+virtual StringRef getName() const override { return "Comparison"; };
 ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
  ArgNo OtherArgN)
 : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
@@ -221,6 +225,7 @@
 bool CannotBeNull = true;
 
   public:
+StringRef getName() const override { return "NonNull"; }
 ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
   const Summary ,
   CheckerContext ) const override {
@@ -272,6 +277,7 @@
 BinaryOperator::Opcode Op = BO_LE;
 
   public:
+StringRef getName() const override { return "BufferSize"; }
 BufferSizeConstraint(ArgNo Buffer, llvm::APSInt BufMinSize)
 : ValueConstraint(Buffer), ConcreteSize(BufMinSize) {}
 BufferSizeConstraint(ArgNo Buffer, ArgNo BufSize)
@@ -466,6 +472,8 @@
   return *this;
 }
 Summary (ValueConstraintPtr VC) {
+  assert(VC->getArgNo() != Ret &&
+ "Arg constraint should not refer to the return value");
   ArgConstraints.push_back(VC);
   return *this;
 }
@@ -549,17 +557,24 @@
   void initFunctionSummaries(CheckerContext ) const;
 
   void reportBug(const CallEvent , ExplodedNode *N,
- CheckerContext ) const {
+ const ValueConstraint *VC, CheckerContext ) const {
 if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
   return;
-// TODO Add detailed diagnostic.
-StringRef Msg = "Function argument constraint is not satisfied";
+// TODO Add more detailed diagnostic.
+std::string Msg =
+(Twine("Function argument constraint is not satisfied, constraint: ") +
+ VC->getName().data() + ", ArgN: " + Twine(VC->getArgNo()))
+.str();
 if (!BT_InvalidArg)
   BT_InvalidArg = std::make_unique(
   CheckNames[CK_StdCLibraryFunctionArgsChecker],
   "Unsatisfied argument constraints", categories::LogicError);
 auto R = std::make_unique(*BT_InvalidArg, Msg, N);
-bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
+bugreporter::trackExpressionValue(N, Call.getArgExpr(VC->getArgNo()), *R);
+
+// Highlight the range of the argument that was violated.
+R->addRange(Call.getArgSourceRange(VC->getArgNo()));
+
 C.emitReport(std::move(R));
   }
 };
@@ -696,7 +711,7 @@
 // The argument constraint is not satisfied.
 if (FailureSt && !SuccessSt) {
   if (ExplodedNode *N = C.generateErrorNode(NewState))
-reportBug(Call, N, C);
+reportBug(Call, N, Constraint.get(), C);
   break;
 } else {
   // We will apply the constraint even if we cannot reason about the


Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -126,6 +126,8 @@
 }
 ArgNo getArgNo() const { return ArgN; }
 
+virtual StringRef getName() const = 0;
+
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
 
@@ -152,6 +154,7 @@
 IntRangeVector Ranges;
 
   public:
+StringRef getName() const override { return "Range"; }
 RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector )
 : ValueConstraint(ArgN), Kind(Kind), Ranges(Ranges) {}
 
@@ -205,6 +208,7 @@
 ArgNo OtherArgN;
 
   public:
+virtual StringRef getName() const override { return "Comparison"; };
 ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
  

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D79431#2265155 , @Szelethus wrote:

> In D79431#2263693 , @martong wrote:
>
>> In D79431#2263690 , @martong wrote:
>>
>>> What if we'd add a `toString` method to the constraints and we'd add this 
>>> to `Msg`? This way we'd know the contents of the constraint, thus we we'd 
>>> know //how// the constraint is violated.
>>
>> I mean we'd know what is not satisfied. But, to know why exactly that is not 
>> satisfied we should dump the whole `State` but that's obviously not an 
>> option. Perhaps we could track which symbols and expressions are 
>> participating in the assumption related to the constraint and we could dump 
>> only those, but this seems to be a very complex approach.
>
> I realize that the //how// and //why// phrases in this context a bit too 
> vague :) What do you mean under having to dump the whole `State`? I didn't 
> mean to compress a bug path into a warning message, only what I mentioned in 
> D79431#2020951 . In any case, I 
> think its okay to just move on with this patch. LGTM!

First, thanks for the review!

Second, "dumping the whole State" is an overstatement. What I meant is this: to 
properly explain to the user how the constraint is failed is a hard task. 
Because we should dig up all symbols and expressions that are related to the 
constraint and we should dump their values. E.g. consider the violation of a 
range constraint here,

  ssize_t send(int socket, const void *buffer, size_t length, int flags);
  void foo() {
if (socket < 0)
  send(socket, buf, buflen, flags); // constraint violation for 'socket' it 
should be in [0, IntMax]
  }

We should print something like this: `'socket' it should be in [0, IntMax] but 
it is in [IntMin, -1]`
In this example, the related symbol is only `socket` but this could be more 
complex, e.g. if the constraint is related to a buffer size ...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

In D79431#2263693 , @martong wrote:

> In D79431#2263690 , @martong wrote:
>
>> What if we'd add a `toString` method to the constraints and we'd add this to 
>> `Msg`? This way we'd know the contents of the constraint, thus we we'd know 
>> //how// the constraint is violated.
>
> I mean we'd know what is not satisfied. But, to know why exactly that is not 
> satisfied we should dump the whole `State` but that's obviously not an 
> option. Perhaps we could track which symbols and expressions are 
> participating in the assumption related to the constraint and we could dump 
> only those, but this seems to be a very complex approach.

I realize that the //how// and //why// phrases in this context a bit too vague 
:) What do you mean under having to dump the whole `State`? I didn't mean to 
compress a bug path into a warning message, only what I mentioned in 
D79431#2020951 . In any case, I think 
its okay to just move on with this patch. LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D79431#2263690 , @martong wrote:

> In D79431#2215610 , @Szelethus wrote:
>
>> Ah, okay, silly me. Didn't catch that. Then the message is OK for now.
>>
>> edit: Describing //how// the violation happened might be a valuable for 
>> development purposes as well.
>
> What if we'd add a `toString` method to the constraints and we'd add this to 
> `Msg`? This way we'd know the contents of the constraint, thus we we'd know 
> //how// the constraint is violated.

I mean we'd know what is not satisfied. But, to know why exactly that is not 
satisfied we should dump the whole `State` but that's obviously not an option. 
Perhaps we could track which symbols and expressions are participating in the 
assumption related to the constraint and we could dump only those, but this 
seems to be a very complex approach.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D79431#2215610 , @Szelethus wrote:

> Ah, okay, silly me. Didn't catch that. Then the message is OK for now.
>
> edit: Describing //how// the violation happened might be a valuable for 
> development purposes as well.

What if we'd add a `toString` method to the constraints and we'd add this to 
`Msg`? This way we'd know the contents of the constraint, thus we we'd know 
//how// the constraint is violated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Ah, okay, silly me. Didn't catch that. Then the message is OK for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

There is a TODO comment at the error message, it needs improvement. The current 
form is still something for developers, not for end users. For final version I 
would accept textual descriptions (as mentioned above), not names like 
"BufferSize" and words like "ArgN" inside it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

Tests c:

I'm still not a huge fan of the warning message. Now it describes //what kind// 
of constraint was violated, but not //how// (too large? too small?). Also, 
while I respect that you didn't prefer my suggestion, I think the warning 
message there is still a lot more welcoming, even if it doesn't convey more 
information. But, I don't necessarily insist on it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 280376.
martong marked 3 inline comments as done.
martong added a comment.

- Use Twine


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -126,6 +126,8 @@
 }
 ArgNo getArgNo() const { return ArgN; }
 
+virtual StringRef getName() const = 0;
+
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
 
@@ -144,6 +146,7 @@
 IntRangeVector Args; // Polymorphic arguments.
 
   public:
+StringRef getName() const override { return "Range"; }
 RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector )
 : ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
 
@@ -198,6 +201,7 @@
 ArgNo OtherArgN;
 
   public:
+virtual StringRef getName() const override { return "Comparison"; };
 ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
  ArgNo OtherArgN)
 : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
@@ -214,6 +218,7 @@
 bool CannotBeNull = true;
 
   public:
+StringRef getName() const override { return "NonNull"; }
 ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
   const Summary ,
   CheckerContext ) const override {
@@ -259,6 +264,7 @@
 BinaryOperator::Opcode Op = BO_LE;
 
   public:
+StringRef getName() const override { return "BufferSize"; }
 BufferSizeConstraint(ArgNo Buffer, ArgNo BufSize)
 : ValueConstraint(Buffer), SizeArgN(BufSize) {}
 
@@ -413,6 +419,8 @@
   return *this;
 }
 Summary (ValueConstraintPtr VC) {
+  assert(VC->getArgNo() != Ret &&
+ "Arg constraint should not refer to the return value");
   ArgConstraints.push_back(VC);
   return *this;
 }
@@ -489,17 +497,24 @@
   void initFunctionSummaries(CheckerContext ) const;
 
   void reportBug(const CallEvent , ExplodedNode *N,
- CheckerContext ) const {
+ const ValueConstraint *VC, CheckerContext ) const {
 if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
   return;
-// TODO Add detailed diagnostic.
-StringRef Msg = "Function argument constraint is not satisfied";
+// TODO Add more detailed diagnostic.
+std::string Msg =
+(Twine("Function argument constraint is not satisfied, constraint: ") +
+ VC->getName().data() + ", ArgN: " + Twine(VC->getArgNo()))
+.str();
 if (!BT_InvalidArg)
   BT_InvalidArg = std::make_unique(
   CheckNames[CK_StdCLibraryFunctionArgsChecker],
   "Unsatisfied argument constraints", categories::LogicError);
 auto R = std::make_unique(*BT_InvalidArg, Msg, N);
-bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
+bugreporter::trackExpressionValue(N, Call.getArgExpr(VC->getArgNo()), *R);
+
+// Highlight the range of the argument that was violated.
+R->addRange(Call.getArgSourceRange(VC->getArgNo()));
+
 C.emitReport(std::move(R));
   }
 };
@@ -632,7 +647,7 @@
 // The argument constraint is not satisfied.
 if (FailureSt && !SuccessSt) {
   if (ExplodedNode *N = C.generateErrorNode(NewState))
-reportBug(Call, N, C);
+reportBug(Call, N, Constraint.get(), C);
   break;
 } else {
   // We will apply the constraint even if we cannot reason about the


Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -126,6 +126,8 @@
 }
 ArgNo getArgNo() const { return ArgN; }
 
+virtual StringRef getName() const = 0;
+
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
 
@@ -144,6 +146,7 @@
 IntRangeVector Args; // Polymorphic arguments.
 
   public:
+StringRef getName() const override { return "Range"; }
 RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector )
 : ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
 
@@ -198,6 +201,7 @@
 ArgNo OtherArgN;
 
   public:
+virtual StringRef getName() const override { return "Comparison"; };
 ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
  ArgNo OtherArgN)
 : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
@@ -214,6 +218,7 @@
 bool CannotBeNull = true;
 
   public:
+StringRef getName() const override { return 

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:323
+std::string("Function argument constraint is not satisfied, ") +
+VC->getName().data() + ", ArgN: " + std::to_string(VC->getArgNo());
 if (!BT_InvalidArg)

martong wrote:
> steakhal wrote:
> > balazske wrote:
> > > baloghadamsoftware wrote:
> > > > Instead of `std::string` we usually use `llvm::SmallString` and an 
> > > > `llvm::raw_svector_ostream` to print into it.
> > > This would look better?
> > > "Function argument constraint is not satisfied, constraint: , ArgN: 
> > > "
> > Maybe `llvm::Twine` would be a better choice to `llvm::raw_svector_ostream` 
> > for string concatenations.
> > docs:
> > > Twine - A lightweight data structure for efficiently representing the 
> > > concatenation of temporary values as strings.
> We can use a Twine to create the owning std::string without creating interim 
> objects. But then we must pass an owning object to the ctor of 
> PathSensitiveBugReport, otherwise we might end up with dangling pointers. 
> https://llvm.org/docs/ProgrammersManual.html#dss-twine
Yes, I like this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

martong marked 4 inline comments as done.
martong added a comment.

In D79431#2020951 , @Szelethus wrote:

> Sure, this is an improvement because we display more information, but I'd 
> argue that it isn't really a more readable warning message :) How about
>
> th argument to the call to 
>
> - cannot be represented with a character
> - is a null pointer
> - ... , which violates the function's preconditions.
>
>   WDYT?


I'd rather prefer @balazske's approach (see below) as that is similarly 
expressive but sparser.




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:88
   /// obviously uint32_t should be enough for all practical purposes.
   typedef uint32_t ArgNo;
   static const ArgNo Ret;

steakhal wrote:
> Shouldn't we use `using` instead?
Yes, we should. But it's legacy code from the times long before.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:323
+std::string("Function argument constraint is not satisfied, ") +
+VC->getName().data() + ", ArgN: " + std::to_string(VC->getArgNo());
 if (!BT_InvalidArg)

steakhal wrote:
> balazske wrote:
> > baloghadamsoftware wrote:
> > > Instead of `std::string` we usually use `llvm::SmallString` and an 
> > > `llvm::raw_svector_ostream` to print into it.
> > This would look better?
> > "Function argument constraint is not satisfied, constraint: , ArgN: 
> > "
> Maybe `llvm::Twine` would be a better choice to `llvm::raw_svector_ostream` 
> for string concatenations.
> docs:
> > Twine - A lightweight data structure for efficiently representing the 
> > concatenation of temporary values as strings.
We can use a Twine to create the owning std::string without creating interim 
objects. But then we must pass an owning object to the ctor of 
PathSensitiveBugReport, otherwise we might end up with dangling pointers. 
https://llvm.org/docs/ProgrammersManual.html#dss-twine


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 280368.
martong added a comment.

- Rebase to master


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -126,6 +126,8 @@
 }
 ArgNo getArgNo() const { return ArgN; }
 
+virtual StringRef getName() const = 0;
+
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
 
@@ -144,6 +146,7 @@
 IntRangeVector Args; // Polymorphic arguments.
 
   public:
+StringRef getName() const override { return "Range"; }
 RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector )
 : ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
 
@@ -198,6 +201,7 @@
 ArgNo OtherArgN;
 
   public:
+virtual StringRef getName() const override { return "Comparison"; };
 ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
  ArgNo OtherArgN)
 : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
@@ -214,6 +218,7 @@
 bool CannotBeNull = true;
 
   public:
+StringRef getName() const override { return "NonNull"; }
 ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
   const Summary ,
   CheckerContext ) const override {
@@ -259,6 +264,7 @@
 BinaryOperator::Opcode Op = BO_LE;
 
   public:
+StringRef getName() const override { return "BufferSize"; }
 BufferSizeConstraint(ArgNo Buffer, ArgNo BufSize)
 : ValueConstraint(Buffer), SizeArgN(BufSize) {}
 
@@ -413,6 +419,8 @@
   return *this;
 }
 Summary (ValueConstraintPtr VC) {
+  assert(VC->getArgNo() != Ret &&
+ "Arg constraint should not refer to the return value");
   ArgConstraints.push_back(VC);
   return *this;
 }
@@ -489,17 +497,23 @@
   void initFunctionSummaries(CheckerContext ) const;
 
   void reportBug(const CallEvent , ExplodedNode *N,
- CheckerContext ) const {
+ const ValueConstraint *VC, CheckerContext ) const {
 if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
   return;
-// TODO Add detailed diagnostic.
-StringRef Msg = "Function argument constraint is not satisfied";
+// TODO Add more detailed diagnostic.
+std::string Msg =
+std::string("Function argument constraint is not satisfied, ") +
+VC->getName().data() + ", ArgN: " + std::to_string(VC->getArgNo());
 if (!BT_InvalidArg)
   BT_InvalidArg = std::make_unique(
   CheckNames[CK_StdCLibraryFunctionArgsChecker],
   "Unsatisfied argument constraints", categories::LogicError);
 auto R = std::make_unique(*BT_InvalidArg, Msg, N);
-bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
+bugreporter::trackExpressionValue(N, Call.getArgExpr(VC->getArgNo()), *R);
+
+// Highlight the range of the argument that was violated.
+R->addRange(Call.getArgSourceRange(VC->getArgNo()));
+
 C.emitReport(std::move(R));
   }
 };
@@ -632,7 +646,7 @@
 // The argument constraint is not satisfied.
 if (FailureSt && !SuccessSt) {
   if (ExplodedNode *N = C.generateErrorNode(NewState))
-reportBug(Call, N, C);
+reportBug(Call, N, Constraint.get(), C);
   break;
 } else {
   // We will apply the constraint even if we cannot reason about the


Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -126,6 +126,8 @@
 }
 ArgNo getArgNo() const { return ArgN; }
 
+virtual StringRef getName() const = 0;
+
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
 
@@ -144,6 +146,7 @@
 IntRangeVector Args; // Polymorphic arguments.
 
   public:
+StringRef getName() const override { return "Range"; }
 RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector )
 : ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
 
@@ -198,6 +201,7 @@
 ArgNo OtherArgN;
 
   public:
+virtual StringRef getName() const override { return "Comparison"; };
 ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
  ArgNo OtherArgN)
 : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
@@ -214,6 +218,7 @@
 bool CannotBeNull = true;
 
   public:
+StringRef getName() const override { return "NonNull"; }
 ProgramStateRef apply(ProgramStateRef State, 

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

Could you add some tests demonstrating the refined diagnostics of the checker?
It would help to validate the quality of the emitted diagnostics.




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:88
   /// obviously uint32_t should be enough for all practical purposes.
   typedef uint32_t ArgNo;
   static const ArgNo Ret;

Shouldn't we use `using` instead?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:323
+std::string("Function argument constraint is not satisfied, ") +
+VC->getName().data() + ", ArgN: " + std::to_string(VC->getArgNo());
 if (!BT_InvalidArg)

balazske wrote:
> baloghadamsoftware wrote:
> > Instead of `std::string` we usually use `llvm::SmallString` and an 
> > `llvm::raw_svector_ostream` to print into it.
> This would look better?
> "Function argument constraint is not satisfied, constraint: , ArgN: 
> "
Maybe `llvm::Twine` would be a better choice to `llvm::raw_svector_ostream` for 
string concatenations.
docs:
> Twine - A lightweight data structure for efficiently representing the 
> concatenation of temporary values as strings.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:323
+std::string("Function argument constraint is not satisfied, ") +
+VC->getName().data() + ", ArgN: " + std::to_string(VC->getArgNo());
 if (!BT_InvalidArg)

baloghadamsoftware wrote:
> Instead of `std::string` we usually use `llvm::SmallString` and an 
> `llvm::raw_svector_ostream` to print into it.
This would look better?
"Function argument constraint is not satisfied, constraint: , ArgN: 
"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added a comment.

In D79431#2020951 , @Szelethus wrote:

> Sure, this is an improvement because we display more information, but I'd 
> argue that it isn't really a more readable warning message :) How about
>
> th argument to the call to 
>
> - cannot be represented with a character
> - is a null pointer
> - ... , which violates the function's preconditions.
>
>   WDYT?


+1 Warning messages should be as clear as possible. Although our users are 
programmers, they do not necessarily understand a message "Range constraint not 
satisfied.". Maybe the error messages could be stored together with the 
function summaries.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Balogh , Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:323
+std::string("Function argument constraint is not satisfied, ") +
+VC->getName().data() + ", ArgN: " + std::to_string(VC->getArgNo());
 if (!BT_InvalidArg)

Instead of `std::string` we usually use `llvm::SmallString` and an 
`llvm::raw_svector_ostream` to print into it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Also, I see no tests :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Sure, this is an improvement because we display more information, but I'd argue 
that it isn't really a more readable warning message :) How about

th argument to the call to 

- cannot be represented with a character
- is a null pointer
- ...

, which violates the function's preconditions.

WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79431/new/

https://reviews.llvm.org/D79431



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79431: [analyzer] StdLibraryFunctionsChecker: Add better diagnostics

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: Szelethus, NoQ, baloghadamsoftware, balazske, steakhal.
Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, gamesh411, 
dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, 
whisperity.
Herald added a project: clang.

Title says it, but there is still place for further improvements.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79431

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -114,6 +114,8 @@
 };
 ArgNo getArgNo() const { return ArgN; }
 
+virtual StringRef getName() const = 0;
+
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
   };
@@ -127,6 +129,7 @@
 IntRangeVector Args; // Polymorphic arguments.
 
   public:
+StringRef getName() const override { return "Range"; }
 RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector )
 : ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
 
@@ -172,6 +175,7 @@
 ArgNo OtherArgN;
 
   public:
+virtual StringRef getName() const override { return "Comparison"; };
 ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
  ArgNo OtherArgN)
 : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
@@ -187,6 +191,7 @@
 bool CannotBeNull = true;
 
   public:
+StringRef getName() const override { return "NonNull"; }
 ProgramStateRef apply(ProgramStateRef State, const CallEvent ,
   const Summary ) const override {
   SVal V = getArgSVal(Call, getArgNo());
@@ -309,17 +314,23 @@
   void initFunctionSummaries(CheckerContext ) const;
 
   void reportBug(const CallEvent , ExplodedNode *N,
- CheckerContext ) const {
+ const ValueConstraint *VC, CheckerContext ) const {
 if (!ChecksEnabled[CK_StdCLibraryFunctionArgsChecker])
   return;
-// TODO Add detailed diagnostic.
-StringRef Msg = "Function argument constraint is not satisfied";
+// TODO Add more detailed diagnostic.
+std::string Msg =
+std::string("Function argument constraint is not satisfied, ") +
+VC->getName().data() + ", ArgN: " + std::to_string(VC->getArgNo());
 if (!BT_InvalidArg)
   BT_InvalidArg = std::make_unique(
   CheckNames[CK_StdCLibraryFunctionArgsChecker],
   "Unsatisfied argument constraints", categories::LogicError);
 auto R = std::make_unique(*BT_InvalidArg, Msg, N);
-bugreporter::trackExpressionValue(N, Call.getArgExpr(0), *R);
+bugreporter::trackExpressionValue(N, Call.getArgExpr(VC->getArgNo()), *R);
+
+// Highlight the range of the argument that was violated.
+R->addRange(Call.getArgSourceRange(VC->getArgNo()));
+
 C.emitReport(std::move(R));
   }
 };
@@ -446,12 +457,14 @@
 
   ProgramStateRef NewState = State;
   for (const ValueConstraintPtr& VC : Summary.ArgConstraints) {
+assert(VC->getArgNo() != Ret &&
+   "Arg constraint should not refer to the return value");
 ProgramStateRef SuccessSt = VC->apply(NewState, Call, Summary);
 ProgramStateRef FailureSt = VC->negate()->apply(NewState, Call, Summary);
 // The argument constraint is not satisfied.
 if (FailureSt && !SuccessSt) {
   if (ExplodedNode *N = C.generateErrorNode(NewState))
-reportBug(Call, N, C);
+reportBug(Call, N, VC.get(), C);
   break;
 } else {
   // We will apply the constraint even if we cannot reason about the


Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -114,6 +114,8 @@
 };
 ArgNo getArgNo() const { return ArgN; }
 
+virtual StringRef getName() const = 0;
+
   protected:
 ArgNo ArgN; // Argument to which we apply the constraint.
   };
@@ -127,6 +129,7 @@
 IntRangeVector Args; // Polymorphic arguments.
 
   public:
+StringRef getName() const override { return "Range"; }
 RangeConstraint(ArgNo ArgN, RangeKind Kind, const IntRangeVector )
 : ValueConstraint(ArgN), Kind(Kind), Args(Args) {}
 
@@ -172,6 +175,7 @@
 ArgNo OtherArgN;
 
   public:
+virtual StringRef getName() const override { return "Comparison"; };
 ComparisonConstraint(ArgNo ArgN, BinaryOperator::Opcode Opcode,
  ArgNo OtherArgN)
 : ValueConstraint(ArgN), Opcode(Opcode), OtherArgN(OtherArgN) {}
@@ -187,6 +191,7 @@
 bool CannotBeNull = true;
 
   public:
+StringRef getName() const override {