[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

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

steakhal added a comment.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.

@hgabii Are you planning on finishing this? If not, I'd happily commandeer if 
not.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D48866



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

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

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

steakhal commandeered this revision.
steakhal added a reviewer: hgabii.
steakhal added a comment.

If you don't mind I will finish the leftover work. This will still be committed 
under your name.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D48866



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

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

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

steakhal updated this revision to Diff 202717.
steakhal added a comment.

Unfortunately the changes that I've made are not available in a diff because 
I've moved to the monorepo version.
Although, you can see the changes in detail on my llvm-project github fork 
.

There were only minor changes.

- Updated the license, as requested
- Moved the checker to the 'bugprone' category from 'misc'
- Fixed bug: now using `getAsRecordDecl` instead of `getAsCXXRecordDecl`

**There is still an open question** whether we should relay on the warnings of 
the `-Wcast-align` option, but I'm not convinced.


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

https://reviews.llvm.org/D48866

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-incorrect-pointer-cast.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-incorrect-pointer-cast.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-incorrect-pointer-cast.cpp
@@ -0,0 +1,206 @@
+// RUN: %check_clang_tidy %s bugprone-incorrect-pointer-cast %t -- \
+// RUN:   -config="{CheckOptions: [{key: bugprone-incorrect-pointer-cast.WarnForDifferentSignedness, value: 1}]}" --
+
+bool b;
+char __attribute__((aligned(4))) a[16];
+
+struct S0 {
+  char a[16];
+};
+
+struct S01 {
+  char __attribute__((aligned(4))) a[16];
+  struct S0 __attribute__((aligned(4))) s0;
+};
+
+struct S1 {
+  int a;
+  int b;
+};
+
+struct S2 {
+  int a;
+};
+
+struct S3 {
+  int a;
+  double b;
+};
+
+struct S4 {
+  int x;
+  double y;
+};
+
+struct S5 {
+  double y;
+  int x;
+};
+
+struct __attribute__((aligned(16))) SAligned {
+  char buffer[16];
+};
+
+void initDouble(double *d) {
+  *d = 0.5;
+}
+
+void castCharToInt(void) {
+  char c = 'x';
+  b = (int *)&c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'char' to 'int' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+  b = reinterpret_cast(&c);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'char' to 'int' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+}
+
+void castCharToSort(char c) {
+  b = (short *)&c;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'char' to 'short' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+  b = reinterpret_cast(&c);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'char' to 'short' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+}
+
+void castShortToInt(short s) {
+  b = (int *)&s;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'short' to 'int' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+  b = reinterpret_cast(&s);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'short' to 'int' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+}
+
+void castWideCharToLong(wchar_t wc) {
+  b = (long *)&wc;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'wchar_t' to 'long' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+  b = reinterpret_cast(&wc);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'wchar_t' to 'long' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+}
+
+void castFloatToDouble(float f) {
+  initDouble((double *)&f);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: cast from 'float' to 'double' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+  initDouble(reinterpret_cast(&f));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: cast from 'float' to 'double' may lead to access memory based on invalid memory layout; pointed to type is wider than the allocated type [bugprone-incorrect-pointer-cast]
+}
+
+void castToS2(char *data, unsigned offset) {
+  b = (struct S2 *)(data + offset);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: wa

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

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

steakhal added a comment.

The problem with the `-Wcast-align` is that it will only fire for C-style 
bitcast expressions, not for `reinterpret_cast` ones. example 

Does anyone know why is that behavior?


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

https://reviews.llvm.org/D48866



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

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

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

steakhal marked 8 inline comments as done.
steakhal added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:69-70
+ "cast from %0 to %1 may lead to access memory based on invalid "
+ "memory layout; pointed to type is strictly aligned than the "
+ "allocated type")
+<< SrcPointedType << DestPointedType;

lebedev.ri wrote:
> "*more* strictly aligned" perhaps?
> Similarly, specify the actual values.
Which metrics should I use? Bits or char units? For now I will stick to bits.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:98-107
+  } else if (WarnForDifferentSignedness &&
+ ((SrcPointedType->isSignedIntegerType() &&
+   DestPointedType->isUnsignedIntegerType()) ||
+  (SrcPointedType->isUnsignedIntegerType() &&
+   DestPointedType->isSignedIntegerType( {
+diag(castExpr->getBeginLoc(),
+ "cast from %0 to %1 may lead to access memory based on invalid "

lebedev.ri wrote:
> What does this mean?
> How can signedness affect memory layout?
You are right, signess difference is **explicitly** allowed in bitcasts. It has 
nothing to do with the actual memory layout. I thought it might be useful for 
detecting those cases too. It should be in a different checker in the future.
For now I've removed the related code from this checker.



Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.h:18
+
+/// Warns for cases when pointer is cast and the pointed to type is 
incompatible
+/// with allocated memory area type. This may lead to access memory based on

lebedev.ri wrote:
> is cast*ed*
> s/pointed to/new/
Casted fixed now.

What do you think about **destination type** and **original type** instead of 
**new type**?



Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.h:19
+/// Warns for cases when pointer is cast and the pointed to type is 
incompatible
+/// with allocated memory area type. This may lead to access memory based on
+/// invalid memory layout.

lebedev.ri wrote:
> How do you know anything about the actual `allocated memory area type`.
> You probably want to talk about the original type before cast.
You are right, fixed.


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

https://reviews.llvm.org/D48866



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

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

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

steakhal updated this revision to Diff 202913.
steakhal marked 4 inline comments as done.
steakhal added a comment.

- Removed different signess related parts.
- Don't warn for the casts which are already covered by '-Wcast-align' check.
- Improved the diagnostic messages:
  - Now adds notes for the first incompatible members of structs.
  - Added alignment, size and type information to most of the warning messages.


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

https://reviews.llvm.org/D48866

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-incorrect-pointer-cast.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-incorrect-pointer-cast.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-incorrect-pointer-cast.cpp
@@ -0,0 +1,205 @@
+// RUN: %check_clang_tidy %s bugprone-incorrect-pointer-cast %t -- \
+// RUN:   -config="{CheckOptions: [{key: bugprone-incorrect-pointer-cast.WarnForDifferentSignedness, value: 1}]}" --
+
+bool b;
+char __attribute__((aligned(4))) a[16];
+
+struct S0 {
+  char a[16];
+};
+
+struct S01 {
+  char __attribute__((aligned(4))) a[16];
+  struct S0 __attribute__((aligned(4))) s0;
+};
+
+struct S1 {
+  int a;
+  int b;
+};
+
+struct S2 {
+  int a;
+};
+
+struct S3 {
+  int a;
+  double b;
+};
+
+struct S4 {
+  int x;
+  double y;
+};
+
+struct S5 {
+  double y;
+  int x;
+};
+
+struct __attribute__((aligned(16))) SAligned {
+  char buffer[16];
+};
+
+void initDouble(double *d) {
+  *d = 0.5;
+}
+
+void castCharToInt(void) {
+  char c = 'x';
+  b = (int *)&c; // -Wcast-align already warns for this
+  b = reinterpret_cast(&c); // -Wcast-align does NOT warn for this
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'char' to 'int' may lead to access memory based on invalid memory layout; new type is wider (1) than the original type (4) [bugprone-incorrect-pointer-cast]
+}
+
+void castCharToSort(char c) {
+  b = (short *)&c; // -Wcast-align already warns for this
+  b = reinterpret_cast(&c); // -Wcast-align does NOT warn for this
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'char' to 'short' may lead to access memory based on invalid memory layout; new type is wider (1) than the original type (2) [bugprone-incorrect-pointer-cast]
+}
+
+void castShortToInt(short s) {
+  b = (int *)&s; // -Wcast-align already warns for this
+  b = reinterpret_cast(&s); // -Wcast-align does NOT warn for this
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'short' to 'int' may lead to access memory based on invalid memory layout; new type is wider (2) than the original type (4) [bugprone-incorrect-pointer-cast]
+}
+
+void castWideCharToLong(wchar_t wc) {
+  b = (long *)&wc; // -Wcast-align already warns for this
+  b = reinterpret_cast(&wc); // -Wcast-align does NOT warn for this
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'wchar_t' to 'long' may lead to access memory based on invalid memory layout; new type is wider (4) than the original type (8) [bugprone-incorrect-pointer-cast]
+}
+
+void castFloatToDouble(float f) {
+  initDouble((double *)&f); // -Wcast-align already warns for this
+  initDouble(reinterpret_cast(&f)); // -Wcast-align does NOT warn for this
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: cast from 'float' to 'double' may lead to access memory based on invalid memory layout; new type is wider (4) than the original type (8) [bugprone-incorrect-pointer-cast]
+}
+
+void castToS2(char *data, unsigned offset) {
+  b = (struct S2 *)(data + offset); // -Wcast-align already warns for this
+  b = reinterpret_cast(data + offset); // -Wcast-align does NOT warn for this
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'char' to 'struct S2' may lead to access memory based on invalid memory layout; new type is wider (1) than the original type (4) [bugprone-incorrect-pointer-cast]
+}
+
+void castS3ToS1(struct S3 s3) {
+  b = (struct S1 *)&s3;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'struct S3' to 'struct S1' may lead to access memory based on invalid memory layout; struct members are incompatible [bugprone-incorrect-pointer-cast]
+  // CHECK-MESSAGES: :27:3: note: first incompatible member of S3 is declared here; at 64 bit offset with 16 bit width
+  // CHECK-MESSAGES: :18:3: note: first incompatible member of S1 is declared here; at 32 bit offset with 8 bit width
+
+  b = reinterpret_cast(&s3);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: cast from 'struct S3' to 'struct S1' may lead to access memory based o

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

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

steakhal added a comment.

In D48866#1527540 , @lebedev.ri wrote:

> In D48866#1527506 , @steakhal wrote:
>
> > The problem with the `-Wcast-align` is that it will only fire for C-style 
> > bitcast expressions, not for `reinterpret_cast` ones. example 
> > 
> >  Does anyone know why is that behavior?
>
>
> Because `reinterpret_cast` is by definition allowed to perform these casts, 
> so it is assumed that no warning should be issued.
>  This part of the check i look forward to.


I still don't understand why should `-Wcast-align` issue warning only for 
c-style casts. What is the difference in the given example? What are the 
related paragraphs in the standard?
I assume in the example we violated the basic.lval/11 
 paragraph, so neither of those pointers 
are safely dereferencable.

In case we are casting from **char* ** we don't know what is the **dynamic 
type** of the object (same with std::byte, void * etc.). From my point of view 
the style of the cast is not important in this case, only the fact that both 
are performing bitcasts.
Have I missed something?


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

https://reviews.llvm.org/D48866



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

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

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

steakhal added a comment.
Herald added a subscriber: whisperity.

Do you have some time @Szelethus to check this change?
Your experience and comments would help a lot to finish this.


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

https://reviews.llvm.org/D48866



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

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

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

steakhal added a comment.

What do you think, what should I improve in this checker?
Your remarks, @lebedev.ri, were really valuable.


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

https://reviews.llvm.org/D48866



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

[PATCH] D59516: [analyzer] Add custom filter functions for GenericTaintChecker

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

steakhal added a comment.

I think this patch is ok.

Although there are remarks:

- I think the current implementation of the taint filtering functions does not 
follow the expected semantics. Now the modelling would remove taint before 
calling the function (//pre statement//). One might expect that the modelling 
would remove taint only on function return (//post statement//). This way we 
would not catch this:

  int myFilter(int* p) { // this function is stated as a filter function in the 
config file
int lookupTable[32] = {};
int value = lookupTable[*p]; // using a tainted value for indexing
escape(p);
return value;
  }

- I agree with @NoQ about the `testConfigurationFilter`, which now does not 
test the implementation but the behavior of the static analyzer if it 
conservatively invalidates in case of an unknown function call.
- I also think that we should have testcases for testing the filtering 
functionality of the config file. Eg. using the `myFilter1` could do the job 
here in the tests.

All in all, I still say that it seems to be a good patch.




Comment at: clang/test/Analysis/taint-generic.c:59
 
+#define bool int
+

Have you considered using `_Bool` instead?


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

https://reviews.llvm.org/D59516



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

[PATCH] D66733: [analyzer] Add a checker option to detect nested dead stores

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

steakhal added a comment.

Thank you guys the responses. I cannot agree more. The sole reason why this 
option exists is that if you scroll back in the git blame of that line, you 
would find a commit, which removed this warning for this exact scenario.
Possibly due to some seemingly false positives.
I've added the author of that patch to the reviewers of this change, but did 
not respond.

If you interested to dig deeper, you could start here:
rC123394  same on github: 
https://github.com/steakhal/llvm-project/commit/f224820b45c6847b91071da8d7ade59f373b96f3

And again, thank you that mentioned it in the release notes and for the kind 
responses as well.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66733



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

[PATCH] D69181: [clang-tidy] Adding misc-signal-terminated-thread check

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

steakhal added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp:34-48
+  Preprocessor::macro_iterator It = PP->macro_begin();
+  while (It != PP->macro_end() && !SigtermValue.hasValue()) {
+if (It->first->getName() == "SIGTERM") {
+  const auto *MI = PP->getMacroInfo(It->first);
+  const auto &T = MI->tokens().back();
+  StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());
+  llvm::APInt IntValue;

I think using `while` loop here makes this less readable compared to using an 
algorithm like `llvm::find_if`.

I think a more declarative approach would help here.
- Handles properly if the definition of `SIGTERM` uses other than decimal base.
- Checks if `ValueStr.getAsInteger()` succeeded.
Like.:
```
  const auto IsSigterm = [](const auto &KeyValue) -> bool {
return KeyValue.first->getName() == "SIGTERM";
  };
  const auto TryExpandAsInteger =
  [PP = PP](Preprocessor::macro_iterator It) -> Optional {
if (It == PP->macro_end())
  return llvm::None;
const MacroInfo *MI = PP->getMacroInfo(It->first);
const Token &T = MI->tokens().back();
StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength());

llvm::APInt IntValue;
constexpr unsigned AutoSenseRadix = 0;
if (ValueStr.getAsInteger(AutoSenseRadix, IntValue))
  return llvm::None;
return IntValue.getZExtValue();
  };

  const auto SigtermMacro = llvm::find_if(PP->macros(), IsSigterm);

  if (!SigtermValue && !(SigtermValue = TryExpandAsInteger(SigtermMacro)))
return;
```



Comment at: clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp:55
+diag(MatchedExpr->getBeginLoc(),
+ "Thread should not be terminated by signal.");
+  }

I think it would be valuable to mention the name of the signal.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:76-77
 
+- New :doc:`misc-signal-terminated-thread
+  ` check.
+

Eugene.Zelenko wrote:
> Please keep alphabetical order in new checks list.
Your checker's name is different from these. Could you check these two lines 
please?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-bad-signal-to-kill-thread.rst:7
+Warn on uses of the ``pthread_kill`` function when thread is 
+terminated by ``SIGTERM`` signal.
+.. code-block: c++

It would be kind to offer a compliant solution for fixing the warning.



Comment at: 
clang-tools-extra/test/clang-tidy/misc-bad-signal-to-kill-thread.cpp:26
+
+  if ((result = pthread_cancel(thread)) != 0) {
+  }

Maybe worth to note here that this would be the compliant solution.

Also check if it doesn't warn for different signals, but warns for the 
`pthread_kill(thread, 0xF))`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D69181



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

[PATCH] D69181: [clang-tidy] Adding misc-signal-terminated-thread check

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

steakhal added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp:28
+
+static Preprocessor *PP;
+

Can't we place this `PP` variable to the checker class?
Like the [[ 
https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.h#L39
 | modernize/UseTrailingReturnTypeCheck ]] does?



Comment at: clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp:35
+  const auto TryExpandAsInteger =
+  [PP = PP](Preprocessor::macro_iterator It) -> Optional {
+if (It == PP->macro_end())

aaron.ballman wrote:
> This lambda capture looks suspicious -- why do you need to initialize the 
> capture?
You are right, I forgot that `PP` is a global variable when I offered this 
solution.
Global variables should not be captured, so empty capture is good enough.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D69181



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

[PATCH] D83660: [analyzer] Fix a crash for dereferencing an empty llvm::Optional variable in SMTConstraintManager.h.

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

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

Although I'm not the most experienced one here, I think it's good to go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83660



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

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

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

steakhal added a comment.

In D84316#2168462 , @NoQ wrote:

> Shared accessors look amazing.
>
> [...] you're splitting up the part which performs boring bookkeeping for the 
> state trait from the part which models `strlen()` and other various functions.


Exactly.

> Such separation also looks amazing because ultimately the first part can be 
> made checker-inspecific (i.e., a reusable half-baked trait that can be 
> instantiated multiple times to track various things regardless of their 
> meaning).

Could you elaborate on the latter part? (instantiated multiple times...)

> I don't think i understand having `unix.cstring.CStringChecker` as one more 
> entry in `Checkers.td`. Do you expect there to be a situation when enabling 
> `CStringModeling` without `CStringChecker` actually makes sense?

You seem to be right.
Enabling only the cstring modeling part does not make much sense without 
enabling //at least// the CStringChecker to model the cstring manipulation 
functions - even if the reporting is disabled of such functions.

> If not, why not keep them agglutinated?

I wanted to have a separate class for bookkeeping while minimalizing the 
necessary changes.
What do you think would be the best way to organize this separation?

Few notes:

- Checkers are implemented in the anonymous namespace, so only the given file 
has access to them.
- I wanted to separate the bookkeeping logic from the reporting/function 
modeling logic in different files.
- I like the fact that after the change the CStringChecker implements only the 
evalCall checker callback.

Let me know if I misunderstood something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84316



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

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

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

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

In D84316#2169195 , @Szelethus wrote:

> [...] you really cant make CStringChecker work without CStringModeling


How should I fuse the `CStringModeling` and the `CStringChecker` together while 
splitting them up?

I mean, that would be the best if the `CStringChecker` would focus on modeling 
the related cstring functions while letting the `CStringModeling` do the 
bookkeeping.
I see some contradiction here.




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:429
 
 def CStringModeling : Checker<"CStringModeling">,
+  HelpText<"Responsible for essential modeling of cstring lengths. "

Szelethus wrote:
> What other aspects of c strings needs to be modeled? Is it only length? If 
> so, how about we rename the checker to `CStringLengthModeling`?
For now I think the cstring length is enough.
I'm not sure if we will want to have other properties as well.
You are probably right.



Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:495
   ]>,
-  Dependencies<[CStringModeling]>,
+  Dependencies<[CStringChecker]>,
   Documentation,

Szelethus wrote:
> I dug around a bit, and found this commit as to why this was needed: 
> rGe56167e8f87acf87a9de3d383752e18a738cf056. So this dependency is appropriate.
Interesting.
I was just doing a search & replace though :)



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:74
 enum class ConcatFnKind { none = 0, strcat = 1, strlcat = 2 };
-class CStringChecker : public Checker< eval::Call,
- check::PreStmt,
- check::LiveSymbols,
- check::DeadSymbols,
- check::RegionChanges
- > {
+class CStringChecker : public Checker {
   mutable std::unique_ptr BT_Null, BT_Bounds, BT_Overlap,

Szelethus wrote:
> This is somewhat orthogonal to the patch, but shouldn't precondition 
> violations be reported at `preCall`?
That is the current behavior.
We should consider in the future using `preCall` if we refactor so relentlessly.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringLength.cpp:175-181
+void ento::registerCStringModeling(CheckerManager &Mgr) {
+  Mgr.registerChecker();
+}
+
+bool ento::shouldRegisterCStringModeling(const CheckerManager &) {
+  return true;
+}

Szelethus wrote:
> We traditionally put these on the bottom of the file -- I don't think this 
> would upset the structure too much :)
I wanted to place the class definition as close as possible to the registration 
function.
I can move this though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84316



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

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

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

steakhal updated this revision to Diff 280398.
steakhal added a comment.

- Created the `CStringChecker` subdirectory holding the related files.
- `CStringChecker` split up to 4 files:
  - `CStringChecker.h`: Declaration of the checker class.
  - `CStringChecker.cpp`: Definitions ONLY of the cstirng modeling functions.
  - `CStringLength.h`: The interface declaration of the cstring 
query/modification API (aka. //API header//).
  - `CStringLengthModeling.cpp`: Definitions of the bookkeeping part of the 
`CStringChecker` class AND the definitions of the //API header//.
- Added the `CStringChecker` folder as an include directory for the analyzer 
library, to make it easier to include the //API header//.


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

https://reviews.llvm.org/D84316

Files:
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.h
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp
@@ -0,0 +1,313 @@
+//=== CStringLengthModeling.cpp  Implementation of CStringLength API C++ -*--=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Implements the CStringLength API and the CStringChecker bookkeeping parts.
+// Updates the associated cstring lengths of memory regions:
+//  - Infers the cstring length of string literals.
+//  - Removes cstring length associations of dead symbols.
+//  - Handles region invalidation.
+//
+//===--===//
+
+#include "CStringChecker.h"
+#include "CStringLength.h"
+
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang;
+using namespace ento;
+using namespace cstring;
+
+/// Associates an strlen to a memory region.
+REGISTER_MAP_WITH_PROGRAMSTATE(CStringLengthMap, const MemRegion *, SVal)
+
+//===--===//
+// Implementation of the public CStringLength API.
+//===--===//
+
+ProgramStateRef cstring::setCStringLength(ProgramStateRef State,
+  const MemRegion *MR, SVal StrLength) {
+  assert(!StrLength.isUndef() && "Attempt to set an undefined string length");
+
+  MR = MR->StripCasts();
+
+  switch (MR->getKind()) {
+  case MemRegion::StringRegionKind:
+// FIXME: This can happen if we strcpy() into a string region. This is
+// undefined [C99 6.4.5p6], but we should still warn about it.
+return State;
+
+  case MemRegion::SymbolicRegionKind:
+  case MemRegion::AllocaRegionKind:
+  case MemRegion::NonParamVarRegionKind:
+  case MemRegion::ParamVarRegionKind:
+  case MemRegion::FieldRegionKind:
+  case MemRegion::ObjCIvarRegionKind:
+// These are the types we can currently track string lengths for.
+break;
+
+  case MemRegion::ElementRegionKind:
+// FIXME: Handle element regions by upper-bounding the parent region's
+// string length.
+return State;
+
+  default:
+// Other regions (mostly non-data) can't have a reliable C string length.
+// For now, just ignore the change.
+// FIXME: These are rare but not impossible. We should output some kind of
+// warning for things like strcpy((char[]){'a', 0}, "b");
+return State;
+  }
+
+  if (StrLength.isUnknown())
+return removeCStringLength(State, MR);
+  return State->set(MR, StrLength);
+}
+
+ProgramStateRef cstring::removeCStringLength(ProgramStateRef State,
+ const MemRegion *MR) {
+  return State->remove(MR);
+}
+
+static SVal getCStringLengthForRegion(CheckerContext &Ctx,
+  ProgramStateRef &State, const Expr *Ex,
+  const MemRegion *MR, bool Hypothetical) {
+  if (!Hypothetical) {
+// If there's a recorded length, go ahead and return it.
+if (const SVal *Recorded = State->get(MR))
+  return *Recorded;
+  }
+
+  // Otherwise, get a new symbol and update the state.
+ 

[PATCH] D84736: [analyzer][RFC] Handle pointer difference of ElementRegion and SymbolicRegion

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

steakhal created this revision.
steakhal added reviewers: NoQ, vsavchenko, martong, baloghadamsoftware, 
ASDenysPetrov, xazax.hun, Szelethus.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity.
Herald added a project: clang.
Harbormaster failed remote builds in B65978: Diff 281152!
Harbormaster returned this revision to the author for changes because remote 
builds failed.
steakhal requested review of this revision.
steakhal added a comment.

Manually force **request review**, to start a discussion about this.

Although I managed to implement this in a way to pass the tests, but the logic 
became somewhat messy, that's why I stick to this //dummy// patch.
I'm gonna clean that up and update the diff, if you think that such handing of 
memregions are valid.
Sorry for the `cfe-dev` spam, I did not know that I can manually //request 
review//.


Currently, if the analyzer evaluates an expression like `to - from`, it only 
computes reasonable answer if both are representing ElementRegions.

Formally, **for all** memory region `X` and **for all** SVal offset `Y`:
`&Element{SymRegion{X},Y,char} - &SymRegion{X}` => `Y`
The same for the symmetric version, but returning `-Y` instead.

I'm curious why don't we handle the case, when only one of the operands is an 
`ElementRegion` and the other is a plain `SymbolicRegion`.
IMO if the SuperMemoryRegion is the same, we can still return a reasonable 
answer.

In this patch, I suppose an extension to resolve this.
If this seems to be a good idea, I will:

- implement the symmetric version of the check
- add tests


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84736

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp


Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1017,6 +1017,21 @@
   }
 }
 
+// Try to get the Index as Nonloc, then cast to ArrayIndexTy.
+// Otherwise return None.
+const auto MaybeNonLocIndexOfElementRegion =
+[this](const ElementRegion *ElemReg) -> Optional {
+  SVal IndexVal = ElemReg->getIndex();
+  Optional Index = IndexVal.getAs();
+  if (!Index)
+return llvm::None;
+  IndexVal = evalCastFromNonLoc(*Index, ArrayIndexTy);
+  Index = IndexVal.getAs();
+  if (!Index)
+return llvm::None;
+  return Index.getValue();
+};
+
 // Handle special cases for when both regions are element regions.
 const ElementRegion *RightER = dyn_cast(RightMR);
 const ElementRegion *LeftER = dyn_cast(LeftMR);
@@ -1027,31 +1042,32 @@
   // give the correct answer.
   if (LeftER->getSuperRegion() == RightER->getSuperRegion() &&
   LeftER->getElementType() == RightER->getElementType()) {
-// Get the left index and cast it to the correct type.
-// If the index is unknown or undefined, bail out here.
-SVal LeftIndexVal = LeftER->getIndex();
-Optional LeftIndex = LeftIndexVal.getAs();
-if (!LeftIndex)
-  return UnknownVal();
-LeftIndexVal = evalCastFromNonLoc(*LeftIndex, ArrayIndexTy);
-LeftIndex = LeftIndexVal.getAs();
-if (!LeftIndex)
-  return UnknownVal();
 
-// Do the same for the right index.
-SVal RightIndexVal = RightER->getIndex();
-Optional RightIndex = RightIndexVal.getAs();
-if (!RightIndex)
-  return UnknownVal();
-RightIndexVal = evalCastFromNonLoc(*RightIndex, ArrayIndexTy);
-RightIndex = RightIndexVal.getAs();
-if (!RightIndex)
+Optional LeftIndex = MaybeNonLocIndexOfElementRegion(LeftER);
+Optional RightIndex = MaybeNonLocIndexOfElementRegion(RightER);
+
+if (!LeftIndex || !RightIndex)
   return UnknownVal();
 
 // Actually perform the operation.
 // evalBinOpNN expects the two indexes to already be the right type.
 return evalBinOpNN(state, op, *LeftIndex, *RightIndex, resultTy);
   }
+} else if (LeftER && isa(RightMR)) {
+  if (LeftER->getSuperRegion() != RightMR)
+return UnknownVal();
+  if (Optional LeftIndex = MaybeNonLocIndexOfElementRegion(LeftER))
+return LeftIndex.getValue();
+  return UnknownVal();
+} else if (RightER && isa(LeftMR)) {
+  if (RightER->getSuperRegion() != LeftMR)
+return UnknownVal();
+  if (Optional RightIndex =
+  MaybeNonLocIndexOfElementRegion(RightER))
+llvm_unreachable(
+"TODO: return the negated value of RightIndex - figure out how 
:D\n"
+"Maybe a unary minus operator would do the job.");
+  return UnknownVal();
 }
 
 // Special handling of the FieldRegions, even with symbolic offsets.


Index: clang/lib/StaticAnalyzer/Core/Simple

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

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

steakhal added a comment.

Thanks for checking this @balazske.




Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43
+ ProgramStateRef &State, const Expr *Ex,
+ SVal Buf, bool Hypothetical = false);
+

balazske wrote:
> I do not like that the //get// and //set// (CStringLength) functions are not 
> symmetrical. I (and other developers) would think that the get function 
> returns a stored value and the set function sets it. The `getCStringLength` 
> is more a `computeCStringLength` and additionally may manipulate the `State` 
> too. In this form it is usable mostly only for CStringChecker. (A separate 
> function to get the value stored in the length map should exist instead of 
> this `Hypothetical` thing.)
> [...] get function returns a stored value and the set function sets it.
Certainly a burden to understand. It would be more appealing, but more useful?
The user would have to check and create if necessary regardless. So fusing 
these two functions is more like a feature.
What use case do you think of using only the query function? In other words, 
how can you guarantee that you will find a length for a symbol?

> In this form it is usable mostly only for CStringChecker. (A separate 
> function to get the value stored in the length map should exist instead of 
> this Hypothetical thing.)
You are right. However, I want to focus on splitting parts without modifying 
the already existing API reducing the risk of breaking things.
You should expect such a change in an upcoming patch.


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

https://reviews.llvm.org/D84316

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

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

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

steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43
+ ProgramStateRef &State, const Expr *Ex,
+ SVal Buf, bool Hypothetical = false);
+

steakhal wrote:
> balazske wrote:
> > I do not like that the //get// and //set// (CStringLength) functions are 
> > not symmetrical. I (and other developers) would think that the get function 
> > returns a stored value and the set function sets it. The `getCStringLength` 
> > is more a `computeCStringLength` and additionally may manipulate the 
> > `State` too. In this form it is usable mostly only for CStringChecker. (A 
> > separate function to get the value stored in the length map should exist 
> > instead of this `Hypothetical` thing.)
> > [...] get function returns a stored value and the set function sets it.
> Certainly a burden to understand. It would be more appealing, but more useful?
> The user would have to check and create if necessary regardless. So fusing 
> these two functions is more like a feature.
> What use case do you think of using only the query function? In other words, 
> how can you guarantee that you will find a length for a symbol?
> 
> > In this form it is usable mostly only for CStringChecker. (A separate 
> > function to get the value stored in the length map should exist instead of 
> > this Hypothetical thing.)
> You are right. However, I want to focus on splitting parts without modifying 
> the already existing API reducing the risk of breaking things.
> You should expect such a change in an upcoming patch.
On second thought, It probably worth having a cleaner API to a slight 
inconvenience. If he feels like, still can wrap them.
I will investigate it tomorrow.


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

https://reviews.llvm.org/D84316

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

[PATCH] D84736: [analyzer][RFC] Handle pointer difference of ElementRegion and SymbolicRegion

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

steakhal planned changes to this revision.
steakhal added a comment.

Thank you all for the comments.

In D84736#2179828 , @NoQ wrote:

> I think the overall idea is correct.

So I will keep going in this direction, you can expect the more complex version 
of this.
I plan to reuse this revision.

So we can formalize this logic like this: 
//for all// `MemReg`, and `Y`: `&Element{MemReg,Y,char} - &MemReg` => `Y`




Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1059-1060
+return UnknownVal();
+  if (Optional LeftIndex = MaybeNonLocIndexOfElementRegion(LeftER))
+return LeftIndex.getValue();
+  return UnknownVal();

NoQ wrote:
> You'll have to multiply by the size of a single element - it's an index, not 
> an offset.
Nice catch.



Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1067-1069
+llvm_unreachable(
+"TODO: return the negated value of RightIndex - figure out how 
:D\n"
+"Maybe a unary minus operator would do the job.");

NoQ wrote:
> Yeah, sounds like we'll have to stick to `UnknownVal` for now when 
> `RightIndex` is symbolic. You can easily compute unary minus for a concrete 
> index though, and that'll probably be your most important case anyway.
I managed to get around this by turning the unary minus into a binary 
substitution from zero (using `ArrayIndexTy` type).
Although I should have improved the `SimpleSValBuilder::minus` function to 
handle other svals besides concrete ints instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84736

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

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

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

steakhal added inline comments.



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:19
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Config/config.h"
+#include "gtest/gtest.h"

mgorny wrote:
> `config.h` is a private LLVM header and must not be used from clang.
I'm not a CMake profession, but shouldn't it be declared private to the LLVM 
target in the corresponding CMakeLists file then?

How do you query whether clang has built with Z3 or not @mgorny?
I'm including that header only for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78704

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

[PATCH] D84736: [analyzer][RFC] Handle pointer difference of ElementRegion and SymbolicRegion

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

steakhal updated this revision to Diff 281556.
steakhal marked 3 inline comments as done.
steakhal added a comment.

Reworked `ElementRegion` handling.
Previously it handled only element regions in the form of: `Element{X,n} OP 
Element{X,m}`
Now supports `Element{X,n} OP X` and `X OP Element{X,n}` as well.
Still misses nested ElementRegions though like: `Element{X,n} OP 
Element{Element{X,k},m}`.

No tests yet.
Regression test vs unit test - which should I prefer?


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

https://reviews.llvm.org/D84736

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -1017,40 +1017,77 @@
   }
 }
 
-// Handle special cases for when both regions are element regions.
-const ElementRegion *RightER = dyn_cast(RightMR);
-const ElementRegion *LeftER = dyn_cast(LeftMR);
-if (RightER && LeftER) {
-  // Next, see if the two ERs have the same super-region and matching types.
-  // FIXME: This should do something useful even if the types don't match,
-  // though if both indexes are constant the RegionRawOffset path will
-  // give the correct answer.
-  if (LeftER->getSuperRegion() == RightER->getSuperRegion() &&
-  LeftER->getElementType() == RightER->getElementType()) {
-// Get the left index and cast it to the correct type.
-// If the index is unknown or undefined, bail out here.
-SVal LeftIndexVal = LeftER->getIndex();
-Optional LeftIndex = LeftIndexVal.getAs();
-if (!LeftIndex)
-  return UnknownVal();
-LeftIndexVal = evalCastFromNonLoc(*LeftIndex, ArrayIndexTy);
-LeftIndex = LeftIndexVal.getAs();
-if (!LeftIndex)
+// Handle: \forall MemRegion X, \forall NonLoc n, m:
+//  - Element{X,n} OP Element{X,m}
+//  - Element{X,n} OP X
+//  -X OP Element{X,n}
+// We don't handle here nested ElementRegions like in the this expression:
+// Element{Element{x,3,int [10]},5,int}  ==  Element{x,35,int}
+{
+  // Calculates the byte offset within the memory region to the referred
+  // element.
+  const auto ByteOffsetOfElement =
+  [this, state](const ElementRegion *ElemReg) -> NonLoc {
+NonLoc Index = evalCastFromNonLoc(ElemReg->getIndex(), ArrayIndexTy)
+   .castAs();
+CharUnits SingleElementSize =
+ElemReg->getContext().getTypeSizeInChars(ElemReg->getElementType());
+return evalBinOpNN(state, BO_Mul, Index,
+   makeArrayIndex(SingleElementSize.getQuantity()),
+   ArrayIndexTy)
+.castAs();
+  };
+
+  // If both has the same memory region, and the index has a concrete value,
+  // we can evaluate equality operators.
+  const auto EvaluateEqualityOperators =
+  [this, state, op, resultTy](NonLoc Index,
+  bool HasSameMemRegions) -> SVal {
+assert(BinaryOperator::isEqualityOp(op));
+const llvm::APSInt *Int = getKnownValue(state, Index);
+if (!Int)
   return UnknownVal();
 
-// Do the same for the right index.
-SVal RightIndexVal = RightER->getIndex();
-Optional RightIndex = RightIndexVal.getAs();
-if (!RightIndex)
-  return UnknownVal();
-RightIndexVal = evalCastFromNonLoc(*RightIndex, ArrayIndexTy);
-RightIndex = RightIndexVal.getAs();
-if (!RightIndex)
-  return UnknownVal();
+const bool EQResult = HasSameMemRegions && *Int == 0;
+return makeTruthVal(op == BO_EQ ? EQResult : !EQResult, resultTy);
+  };
+
+  const ElementRegion *RightER = dyn_cast(RightMR);
+  const ElementRegion *LeftER = dyn_cast(LeftMR);
+  if (RightER && LeftER) {
+// Next, see if the two ERs have the same super-region and matching
+// types.
+// FIXME: This should do something useful even if the types don't match,
+// though if both indexes are constant the RegionRawOffset path will
+// give the correct answer.
+if (LeftER->getSuperRegion() == RightER->getSuperRegion() &&
+LeftER->getElementType() == RightER->getElementType()) {
+  return evalBinOpNN(state, op, ByteOffsetOfElement(LeftER),
+ ByteOffsetOfElement(RightER), resultTy);
+}
+  } else if (LeftER && !RightER) {
+NonLoc LeftIndex = ByteOffsetOfElement(LeftER);
+const bool HasSameMemRegions = LeftER->getSuperRegion() == RightMR;
 
-// Actually perform the operation.
-// evalBinOpNN expects the two indexes to already be the right type.
-return evalBinOpNN(state, op, *Lef

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

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

steakhal added inline comments.



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:19
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Config/config.h"
+#include "gtest/gtest.h"

whisperity wrote:
> steakhal wrote:
> > mgorny wrote:
> > > `config.h` is a private LLVM header and must not be used from clang.
> > I'm not a CMake profession, but shouldn't it be declared private to the 
> > LLVM target in the corresponding CMakeLists file then?
> > 
> > How do you query whether clang has built with Z3 or not @mgorny?
> > I'm including that header only for that.
> I've did a quick skim of the code, and it seems there is no way in it 
> currently to query this.
> Your best bet would be either adding an extra flag to Clang's CMake generated 
> Config header that inherits this flag, or checking `llvm::CreateZ3Solver()` - 
> right now, this method reports a fatal error, but you could create a bool 
> function in the header which reports constant true or false, and turn the 
> test's skip into a runtime condition?
How could I use a //private// header in the first place?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78704

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

[PATCH] D84929: [analyzer] Fix out-of-tree only clang build by not relaying on private header

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

steakhal created this revision.
steakhal added reviewers: NoQ, mgorny, xazax.hun, Szelethus, martong, balazske.
Herald added subscribers: llvm-commits, cfe-commits, ASDenysPetrov, Charusso, 
dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, 
baloghadamsoftware, whisperity.
Herald added projects: clang, LLVM.
steakhal requested review of this revision.

It turned out that the D78704  included a 
private LLVM header, which is excluded from the LLVM install target.
I'm substituting that `#include` with the public one by moving the necessary 
`#define` into that.
There was a discussion about this at D78704  
and on the cfe-dev 
 mailing list.

I'm also placing a note to remind others of this pitfall.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84929

Files:
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Config/llvm-config.h.cmake


Index: llvm/include/llvm/Config/llvm-config.h.cmake
===
--- llvm/include/llvm/Config/llvm-config.h.cmake
+++ llvm/include/llvm/Config/llvm-config.h.cmake
@@ -79,6 +79,9 @@
  */
 #cmakedefine01 LLVM_FORCE_ENABLE_STATS
 
+/* Define if we have z3 and want to build it */
+#cmakedefine LLVM_WITH_Z3 ${LLVM_WITH_Z3}
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic 
library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -1,6 +1,9 @@
 #ifndef CONFIG_H
 #define CONFIG_H
 
+// Include this header only under the llvm source tree.
+// This is a private header.
+
 /* Exported configuration */
 #include "llvm/Config/llvm-config.h"
 
@@ -335,9 +338,6 @@
 /* Whether GlobalISel rule coverage is being collected */
 #cmakedefine01 LLVM_GISEL_COV_ENABLED
 
-/* Define if we have z3 and want to build it */
-#cmakedefine LLVM_WITH_Z3 ${LLVM_WITH_Z3}
-
 /* Define to the default GlobalISel coverage file prefix */
 #cmakedefine LLVM_GISEL_COV_PREFIX "${LLVM_GISEL_COV_PREFIX}"
 
Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -16,7 +16,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
-#include "llvm/Config/config.h"
+#include "llvm/Config/llvm-config.h"
 #include "gtest/gtest.h"
 
 // FIXME: Use GTEST_SKIP() instead if GTest is updated to version 1.10.0


Index: llvm/include/llvm/Config/llvm-config.h.cmake
===
--- llvm/include/llvm/Config/llvm-config.h.cmake
+++ llvm/include/llvm/Config/llvm-config.h.cmake
@@ -79,6 +79,9 @@
  */
 #cmakedefine01 LLVM_FORCE_ENABLE_STATS
 
+/* Define if we have z3 and want to build it */
+#cmakedefine LLVM_WITH_Z3 ${LLVM_WITH_Z3}
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -1,6 +1,9 @@
 #ifndef CONFIG_H
 #define CONFIG_H
 
+// Include this header only under the llvm source tree.
+// This is a private header.
+
 /* Exported configuration */
 #include "llvm/Config/llvm-config.h"
 
@@ -335,9 +338,6 @@
 /* Whether GlobalISel rule coverage is being collected */
 #cmakedefine01 LLVM_GISEL_COV_ENABLED
 
-/* Define if we have z3 and want to build it */
-#cmakedefine LLVM_WITH_Z3 ${LLVM_WITH_Z3}
-
 /* Define to the default GlobalISel coverage file prefix */
 #cmakedefine LLVM_GISEL_COV_PREFIX "${LLVM_GISEL_COV_PREFIX}"
 
Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -16,7 +16,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
-#include "llvm/Config/config.h"
+#include "llvm/Config/llvm-config.h"
 #include "gtest/gtest.h"
 
 // FIXME: Use GTEST_SKIP() instead if GTest is updated to version 1.10.0
_

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

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

steakhal added inline comments.



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:19
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Config/config.h"
+#include "gtest/gtest.h"

steakhal wrote:
> whisperity wrote:
> > steakhal wrote:
> > > mgorny wrote:
> > > > `config.h` is a private LLVM header and must not be used from clang.
> > > I'm not a CMake profession, but shouldn't it be declared private to the 
> > > LLVM target in the corresponding CMakeLists file then?
> > > 
> > > How do you query whether clang has built with Z3 or not @mgorny?
> > > I'm including that header only for that.
> > I've did a quick skim of the code, and it seems there is no way in it 
> > currently to query this.
> > Your best bet would be either adding an extra flag to Clang's CMake 
> > generated Config header that inherits this flag, or checking 
> > `llvm::CreateZ3Solver()` - right now, this method reports a fatal error, 
> > but you could create a bool function in the header which reports constant 
> > true or false, and turn the test's skip into a runtime condition?
> How could I use a //private// header in the first place?
The fix is on the way: D84929


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78704

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

[PATCH] D77792: [analyzer] Extend constraint manager to be able to compare simple SymSymExprs

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

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

This patch extends the constraint manager to be able to reason about trivial 
sym-sym comparisons.

We all know that `a < b` if the maximum value of `a` is still less than the 
minimum value of `b`.
This reasoning can be applied for the <,<=,>,>= relation operators.
This patch solely implements this functionality.

This patch does not address transitity like:
If `a < b` and `b < c` than `a < c`.

This patch is necessary to be able to express //hidden function preconditions//.
For example, with the `D69726` we could express the connection between the 
extent size of `src` and the size (`n`) parameter of the function.

  #define ANALYZER_ASSERT assert
  
  void my_memcpy(char *dst, char *src, int n) {
ANALYZER_ASSERT(clang_analyzer_getExtent(src) >= n)
ANALYZER_ASSERT(clang_analyzer_getExtent(dst) >= n)

for (int i = 0; i < n; ++i) {
  // The extent of dst would be compared to the index i.
  dst[i] = src[i]; // each memory access in-bound, no-warning
}
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D77792

Files:
  
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
  clang/test/Analysis/constraint-manager-sym-sym.c

Index: clang/test/Analysis/constraint-manager-sym-sym.c
===
--- /dev/null
+++ clang/test/Analysis/constraint-manager-sym-sym.c
@@ -0,0 +1,172 @@
+// RUN: %clang_analyze_cc1 -verify -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false %s
+
+
+void clang_analyzer_eval(int);
+
+extern void __assert_fail(__const char *__assertion, __const char *__file,
+  unsigned int __line, __const char *__function)
+__attribute__((__noreturn__));
+#define assert(expr) \
+  ((expr) ? (void)(0) : __assert_fail(#expr, __FILE__, __LINE__, __func__))
+
+
+// Given [a1,a2] and [b1,b2] intervals.
+// Pin the [b1,b2] interval to eg. [5,10]
+// Choose the a1,a2 points from 0,2,5,7,10,12 where a1 < a2.
+// There will be 5+4+3+2+1 cases.
+
+// [0,2] and [5,10]
+void test_range1(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 2);
+  clang_analyzer_eval(l <  r); // expected-warning{{TRUE}}
+  clang_analyzer_eval(l <= r); // expected-warning{{TRUE}}
+  clang_analyzer_eval(l >  r); // expected-warning{{FALSE}}
+  clang_analyzer_eval(l >= r); // expected-warning{{FALSE}}
+}
+
+// [0,5] and [5,10]
+void test_range2(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 5);
+  clang_analyzer_eval(l <  r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{TRUE}}
+  clang_analyzer_eval(l >  r); // expected-warning{{FALSE}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [0,7] and [5,10]
+void test_range3(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 7);
+  clang_analyzer_eval(l <  r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >  r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [0,10] and [5,10]
+void test_range4(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 10);
+  clang_analyzer_eval(l <  r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >  r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [0,12] and [5,10]
+void test_range5(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 12);
+  clang_analyzer_eval(l <  r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >  r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+
+// [2,5] and [5,10]
+void test_range6(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(2 <= l && l <= 5);
+  clang_analyzer_eval(l <  r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{TRUE}}
+  clang_analyzer_eval(l >  r); // expected-warning{{FALSE}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [2,7] and [5,10]
+void test_range7(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(2 <= l && l <= 7);
+  clang_analyzer_eval(l <  r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >  r); // expected-warning{{UNKNOWN}}
+  clang

[PATCH] D74806: [analyzer] NFCi: Refactor CStringChecker: use strongly typed internal API

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG30e5c7e82fa1: [analyzer] NFCi: Refactor CStringChecker: use 
strongly typed internal API (authored by steakhal).
Herald added a subscriber: ASDenysPetrov.

Changed prior to commit:
  https://reviews.llvm.org/D74806?vs=245300&id=256301#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74806

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/bsd-string.c
  clang/test/Analysis/bstring.c
  clang/test/Analysis/null-deref-ps-region.c
  clang/test/Analysis/string.c

Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -353,7 +353,7 @@
 void strcpy_overflow(char *y) {
   char x[4];
   if (strlen(y) == 4)
-strcpy(x, y); // expected-warning{{String copy function overflows destination buffer}}
+strcpy(x, y); // expected-warning{{String copy function overflows the destination buffer}}
 }
 #endif
 
@@ -394,7 +394,7 @@
 void stpcpy_overflow(char *y) {
   char x[4];
   if (strlen(y) == 4)
-stpcpy(x, y); // expected-warning{{String copy function overflows destination buffer}}
+stpcpy(x, y); // expected-warning{{String copy function overflows the destination buffer}}
 }
 #endif
 
@@ -451,19 +451,19 @@
 void strcat_overflow_0(char *y) {
   char x[4] = "12";
   if (strlen(y) == 4)
-strcat(x, y); // expected-warning{{String copy function overflows destination buffer}}
+strcat(x, y); // expected-warning{{String concatenation function overflows the destination buffer}}
 }
 
 void strcat_overflow_1(char *y) {
   char x[4] = "12";
   if (strlen(y) == 3)
-strcat(x, y); // expected-warning{{String copy function overflows destination buffer}}
+strcat(x, y); // expected-warning{{String concatenation function overflows the destination buffer}}
 }
 
 void strcat_overflow_2(char *y) {
   char x[4] = "12";
   if (strlen(y) == 2)
-strcat(x, y); // expected-warning{{String copy function overflows destination buffer}}
+strcat(x, y); // expected-warning{{String concatenation function overflows the destination buffer}}
 }
 #endif
 
@@ -547,25 +547,28 @@
 // of the C-string checker.
 void cstringchecker_bounds_nocrash() {
   char *p = malloc(2);
-  strncpy(p, "AAA", sizeof("AAA")); // expected-warning {{Size argument is greater than the length of the destination buffer}}
+  strncpy(p, "AAA", sizeof("AAA"));
+  // expected-warning@-1 {{String copy function overflows the destination buffer}}
   free(p);
 }
 
 void strncpy_overflow(char *y) {
   char x[4];
   if (strlen(y) == 4)
-strncpy(x, y, 5); // expected-warning{{Size argument is greater than the length of the destination buffer}}
+strncpy(x, y, 5);
+// expected-warning@-1 {{String copy function overflows the destination buffer}}
 #ifndef VARIANT
-  // expected-warning@-2{{size argument is too large; destination buffer has size 4, but size argument is 5}}
+// expected-warning@-3 {{size argument is too large; destination buffer has size 4, but size argument is 5}}
 #endif
 }
 
 void strncpy_no_overflow(char *y) {
   char x[4];
   if (strlen(y) == 3)
-strncpy(x, y, 5); // expected-warning{{Size argument is greater than the length of the destination buffer}}
+strncpy(x, y, 5);
+// expected-warning@-1 {{String copy function overflows the destination buffer}}
 #ifndef VARIANT
-  // expected-warning@-2{{size argument is too large; destination buffer has size 4, but size argument is 5}}
+// expected-warning@-3 {{size argument is too large; destination buffer has size 4, but size argument is 5}}
 #endif
 }
 
@@ -575,7 +578,8 @@
 
   char x[4];
   if (strlen(y) == 3)
-strncpy(x, y, n); // expected-warning{{Size argument is greater than the length of the destination buffer}}
+strncpy(x, y, n);
+  // expected-warning@-1 {{String copy function overflows the destination buffer}}
 }
 #endif
 
@@ -658,25 +662,29 @@
 void strncat_overflow_0(char *y) {
   char x[4] = "12";
   if (strlen(y) == 4)
-strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
+strncat(x, y, strlen(y));
+  // expected-warning@-1 {{String concatenation function overflows the destination buffer}}
 }
 
 void strncat_overflow_1(char *y) {
   char x[4] = "12";
   if (strlen(y) == 3)
-strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
+strncat(x, y, strlen(y));
+  // expected-warning@-1 {{String concatenation function overflows the destination buffer}}
 }
 
 void strncat_overflow_2(char *y) {
   char x[4] = "12";
   if (strlen(y) == 2)
-strncat(x, y, strlen(y)); // expected-warning{{Size argument is greater than the free space in the destination buffer}}
+strncat(x, y, strlen(y));

[PATCH] D77792: [analyzer] Extend constraint manager to be able to compare simple SymSymExprs

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

steakhal updated this revision to Diff 256420.
steakhal marked 4 inline comments as done.
steakhal added a comment.

- add full diff context
- NFC refactored `RangeSet` comparison function
- add tests for compund `RangeSet`s like: `{[0,1],[5,6]} < 
{[9,9],[11,42],[44,44]}`
- NFC clang-format test file


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

https://reviews.llvm.org/D77792

Files:
  
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
  clang/test/Analysis/constraint-manager-sym-sym.c

Index: clang/test/Analysis/constraint-manager-sym-sym.c
===
--- /dev/null
+++ clang/test/Analysis/constraint-manager-sym-sym.c
@@ -0,0 +1,197 @@
+// RUN: %clang_analyze_cc1 -verify -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+extern void __assert_fail(__const char *__assertion, __const char *__file,
+  unsigned int __line, __const char *__function)
+__attribute__((__noreturn__));
+#define assert(expr) \
+  ((expr) ? (void)(0) : __assert_fail(#expr, __FILE__, __LINE__, __func__))
+
+// Given [a1,a2] and [b1,b2] intervals.
+// Pin the [b1,b2] interval to eg. [5,10]
+// Choose the a1,a2 points from 0,2,5,7,10,12 where a1 < a2.
+// There will be 5+4+3+2+1 cases.
+
+// [0,2] and [5,10]
+void test_range1(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 2);
+  clang_analyzer_eval(l < r);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(l <= r); // expected-warning{{TRUE}}
+  clang_analyzer_eval(l > r);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(l >= r); // expected-warning{{FALSE}}
+}
+
+// [0,5] and [5,10]
+void test_range2(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 5);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{TRUE}}
+  clang_analyzer_eval(l > r);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [0,7] and [5,10]
+void test_range3(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 7);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [0,10] and [5,10]
+void test_range4(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 10);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [0,12] and [5,10]
+void test_range5(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 12);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [2,5] and [5,10]
+void test_range6(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(2 <= l && l <= 5);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{TRUE}}
+  clang_analyzer_eval(l > r);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [2,7] and [5,10]
+void test_range7(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(2 <= l && l <= 7);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [2,10] and [5,10]
+void test_range8(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(2 <= l && l <= 10);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [2,12] and [5,10]
+void test_range9(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(2 <= l && l <= 12);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [5,7] and [5,10]
+void test_range10(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(5 <= l && l <= 7);
+  clang_analyzer_eval(l < r);  // expect

[PATCH] D77792: [analyzer] Extend constraint manager to be able to compare simple SymSymExprs

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

steakhal added a comment.

In D77792#1971921 , @Szelethus wrote:

> You seem to have forgotten `-U` :^)


Nice catch!




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:774
 
+Optional RangeConstraintManager::tryAssumeSymSymOp(
+ProgramStateRef State, BinaryOperator::Opcode Op, SymbolRef LHSSym,

martong wrote:
> Is it possible to ever return with `None`? Other `assume` functions usually 
> just return with `nullptr` on infeasible state as you do. What would be the 
> meaning if `None` is returned, is that another kind of infeasibility?
The rest of the functions there is no `maybe` answer. There is always `yes` or 
`no`, returning the `State` or the `nullptr`.
In our case, we don't know in advance that we know a definitive answer.

Imagine the following:
When the analyzer sees the `a < b` comparison.
It queries whether it `canReasonAbout()`. In our case that would return `true` 
due to my change.
When tries to reason about the given `SymSymExpr` (`a < b`), we don't know if 
the corresponding value ranges of `a` and `b` are disjunct or not, we need to 
check that.
If we can prove that the ranges are disjunct (or just connected: `a: [a1,x]` 
and `b: [x,b2]`) then we know an answer. That can be `true` (`State`) or 
`false` (`nullptr`). Otherwise the result should be `UNKNOWN` (`llvm::None`).



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:797
+
+  switch (Op) {
+  case BO_LT:

martong wrote:
> Perhaps this hunk could be greatly simplified if `CompareResult` was actually 
> `BinaryOperator::Opcode`.
> 
> Maybe (?):
> ```
> if (Res == Op)
>   return State;
> return InfeasibleState;
> ```
Honestly, I had exactly the same thoughts.

I think an `Opcode` is a different thing than a result of a comparison.
Here, we have a partial ordering among `RangeSet`s.
But you can convince me :)

But I agree, it looks clunky, looking forward to a better solution. Any idea?



Comment at: clang/test/Analysis/constraint-manager-sym-sym.c:70
+
+// [2,5] and [5,10]
+void test_range6(int l, int r) {

martong wrote:
> How is it different than `// [0,5] and [5,10]`?
It covers the same. I just wanted a full and clear showcase of the possible 
intervals.
I can be convinced to remove this testcase.



Comment at: clang/test/Analysis/constraint-manager-sym-sym.c:172
+}
+

martong wrote:
> I think we could benefit from tests for cases like this: 
> ```
> {[0,1],[5,6]} < {[9,9],[11,42],[44,44]}
> ```
Really good idea.

I added tests for these. But I'm not really sure what's going on there :D
I'm sure that these test cases are not testing what I meant to test.
//(The exploded graphs are showing that each subrange is assumed for a given 
value (`l` and `r`) on a given path)//

Any idea of how to express the proper value ranges?


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

https://reviews.llvm.org/D77792



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

[PATCH] D77792: [analyzer] Extend constraint manager to be able to compare simple SymSymExprs

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

steakhal updated this revision to Diff 256551.
steakhal added a comment.

- rewritten the `RangeSet::compare` function and checked the assumptions on 
WolframAlpha
- moved the `RangeSet::compare` function to a cpp file
- added comments to the `RangeSet::compare` function
- fixed the comment in `RangeConstraintManager::canReasonAbout` function
- introduced the `RangeSet::CompareResult::identical` enum value to be complete
- updated the `RangeConstraintManager::tryAssumeSymSymOp` accoding the 
`identical` CompareResult.
- omited testing the `[2,5] < [5,10]` testcase, since that is covered by `[0,5] 
< [5,10]`


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

https://reviews.llvm.org/D77792

Files:
  
clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h
  clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
  clang/test/Analysis/constraint-manager-sym-sym.c

Index: clang/test/Analysis/constraint-manager-sym-sym.c
===
--- /dev/null
+++ clang/test/Analysis/constraint-manager-sym-sym.c
@@ -0,0 +1,189 @@
+// RUN: %clang_analyze_cc1 -verify -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false %s
+
+void clang_analyzer_eval(int);
+
+extern void __assert_fail(__const char *__assertion, __const char *__file,
+  unsigned int __line, __const char *__function)
+__attribute__((__noreturn__));
+#define assert(expr) \
+  ((expr) ? (void)(0) : __assert_fail(#expr, __FILE__, __LINE__, __func__))
+
+// Given [a1,a2] and [b1,b2] intervals.
+// Pin the [b1,b2] interval to eg. [5,10]
+// Choose the a1,a2 points from 0,2,5,7,10,12 where a1 < a2.
+// There will be 5+4+3+2+1 cases.
+
+// [0,2] and [5,10]
+void test_range1(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 2);
+  clang_analyzer_eval(l < r);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(l <= r); // expected-warning{{TRUE}}
+  clang_analyzer_eval(l > r);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(l >= r); // expected-warning{{FALSE}}
+}
+
+// [0,5] and [5,10]
+void test_range2(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 5);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{TRUE}}
+  clang_analyzer_eval(l > r);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [0,7] and [5,10]
+void test_range3(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 7);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [0,10] and [5,10]
+void test_range4(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 10);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [0,12] and [5,10]
+void test_range5(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(0 <= l && l <= 12);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [2,5] and [5,10] omitted because it is the same as the '[0,5] and [5,10]'
+
+// [2,7] and [5,10]
+void test_range7(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(2 <= l && l <= 7);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [2,10] and [5,10]
+void test_range8(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(2 <= l && l <= 10);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [2,12] and [5,10]
+void test_range9(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(2 <= l && l <= 12);
+  clang_analyzer_eval(l < r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l <= r); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l > r);  // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(l >= r); // expected-warning{{UNKNOWN}}
+}
+
+// [5,7] and [5,10]
+void test_range10(int l, int r) {
+  assert(5 <= r && r <= 10);
+  assert(5 <= l && l <= 7);
+  clang_analyz

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

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

steakhal added a comment.

It took me some time to understand what you were doing.
Probably a bit ASCII-art would have had help (eg. about the wrapping behavior).

By the same token, I wouldn't mind more tests, especially exercising the 
implemented behavior.
Probably `unit-tests` would fit better than `lit-tests` for this purpose.
Eg. showing that if the `RangeSet`  includes the `MIN`, then the negated 
`RangeSet` also includes that.
And the negated RangeSet really unites consecutive ranges.

Please note that **I'm not a math expert** and I haven't spent much time 
working with `RangeSets`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77802



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

[PATCH] D83190: [analyzer] Model iterator random incrementation symmetrically

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

steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:267
   BinaryOperatorKind OK = BO->getOpcode();
-  SVal RVal = State->getSVal(BO->getRHS(), C.getLocationContext());
+  Expr *LHS = BO->getLHS();
+  Expr *RHS = BO->getRHS();

You should probably use const where applicable.
Especially where the refs value depends on a condition operator (eg. few lines 
below) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83190



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

[PATCH] D83555: [analyzer] Fix ctu-on-demand-parsing: REQUIRES: linux -> REQUIRES: system-linux

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd96a47c61625: [analyzer] ctu-on-demand-parsing tests: 
replace linux -> system-linux (authored by steakhal).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83555

Files:
  clang/test/Analysis/ctu-on-demand-parsing.c
  clang/test/Analysis/ctu-on-demand-parsing.cpp


Index: clang/test/Analysis/ctu-on-demand-parsing.cpp
===
--- clang/test/Analysis/ctu-on-demand-parsing.cpp
+++ clang/test/Analysis/ctu-on-demand-parsing.cpp
@@ -30,7 +30,7 @@
 // CHECK: CTU loaded AST file: {{.*}}ctu-chain.cpp
 //
 // FIXME: Path handling should work on all platforms.
-// REQUIRES: linux
+// REQUIRES: system-linux
 
 #include "ctu-hdr.h"
 
Index: clang/test/Analysis/ctu-on-demand-parsing.c
===
--- clang/test/Analysis/ctu-on-demand-parsing.c
+++ clang/test/Analysis/ctu-on-demand-parsing.c
@@ -19,7 +19,7 @@
 // RUN:   -verify ctu-on-demand-parsing.c
 //
 // FIXME: Path handling should work on all platforms.
-// REQUIRES: linux
+// REQUIRES: system-linux
 
 void clang_analyzer_eval(int);
 


Index: clang/test/Analysis/ctu-on-demand-parsing.cpp
===
--- clang/test/Analysis/ctu-on-demand-parsing.cpp
+++ clang/test/Analysis/ctu-on-demand-parsing.cpp
@@ -30,7 +30,7 @@
 // CHECK: CTU loaded AST file: {{.*}}ctu-chain.cpp
 //
 // FIXME: Path handling should work on all platforms.
-// REQUIRES: linux
+// REQUIRES: system-linux
 
 #include "ctu-hdr.h"
 
Index: clang/test/Analysis/ctu-on-demand-parsing.c
===
--- clang/test/Analysis/ctu-on-demand-parsing.c
+++ clang/test/Analysis/ctu-on-demand-parsing.c
@@ -19,7 +19,7 @@
 // RUN:   -verify ctu-on-demand-parsing.c
 //
 // FIXME: Path handling should work on all platforms.
-// REQUIRES: linux
+// REQUIRES: system-linux
 
 void clang_analyzer_eval(int);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83660: [analyzer] Fix a crash for dereferencing an empty llvm::Optional variable in SMTConstraintManager.h.

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

steakhal added a comment.

In D83660#2148834 , @NoQ wrote:

> Looks like a copy-paste error indeed.
>
> @OikawaKirie do you accidentally have a test case to reproduce the crash, so 
> that we could add it to our regression test suite? 'Cause traditionally we 
> add a test to every patch.
>
> Note that SMT-backed constraint managers aren't an actual supported mode; 
> it's an experimental project that isn't yet supposed to be used anywhere and 
> probably won't be ever officially supported, unless reworked dramatically.


I cannot agree more. Although I would appreciate some progress improving Z3 
constraint manager. Might in the future I would spend some time on it - we will 
see.

BTW nice catch & fix @OikawaKirie!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83660



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

[PATCH] D83660: [analyzer] Fix a crash for dereferencing an empty llvm::Optional variable in SMTConstraintManager.h.

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

steakhal added a comment.

In D83660#2149509 , @OikawaKirie wrote:

> > Might in the future I would spend some time on it - we will see.
>
> [...] If there is anything that our research group can do, you are free to 
> contact us.


I would use the official LLVM/clang-static-analyzer 
 channel for lightweight discussions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83660



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

[PATCH] D78189: [analyzer] StdLibraryFunctionsChecker: Add statistics

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

steakhal marked an inline comment as done.
steakhal added a comment.

Besides @balazske's comments, looks good to me.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:504-505
 
-if (NewState && NewState != State)
+if (NewState && NewState != State) {
+  ++NumCaseApplied;
   C.addTransition(NewState);

At first, it was strange to check `NewState != State` since the `addTransition` 
will do this check regardless.
Then I recognized the statistic increment.
It makes sense to count only the new state transitions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78189



___
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 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] D83717: [clang-tidy] Add check fo SEI CERT item ENV32-C

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

steakhal added a comment.

Unfortunately, I'm not qualified enough to have much to say.




Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:1
+//===--- Env32CCheck.cpp - clang-tidy 
-===//
+//

Env32CCheck.cpp -> ExitHandlerCheck.cpp



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:38
+bool isExitFunction(StringRef FnName) {
+  return FnName == EF__Exit || FnName == EF_exit || FnName == EF_quick_exit;
+}

If `EF__Exit`, `EF_exit` and `EF_quick_exit` referred only once, we could 
simply inline those,

Same for the `isJumpFunction`.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:60
+  /// Iteration over the collector is iteration over the found FunctionDecls.
+  auto begin() const -> decltype(CalledFunctions.begin()) {
+return CalledFunctions.begin();

IMO `-> decltype(CalledFunctions.begin())` is unnecessary.
Return type deduction does the right thing for this case.

Same for the `auto end() ...`



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:84
+  hasArgument(
+0, // the first argument is the handler function
+declRefExpr(

If you disabled clang-format for having inline comments, Then you could create 
smaller matchers and give names for them.

Something similar to this:
```lang=cpp
const auto DesciptiveName = hasArgument(0, 
declRefExpr(hasDeclaration(functionDecl().bind("handler_decl";
```



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:132
+   "terminate by returning");
+  break;
+}

Why don't we `return` here?
Same for the next `break`.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.cpp:148-154
+// Collect all the referenced FunctionDecls.
+Collector.TraverseStmt(CurrentDefWithBody->getBody());
+// Add called functions to the worklist.
+std::copy(Collector.begin(), Collector.end(),
+  std::back_inserter(CalledFunctions));
+// Reset the ASTVisitor instance results.
+Collector.clear();

nit: I would clear the `Collector` before the `TraverseStmt`.



Comment at: clang-tools-extra/clang-tidy/cert/ExitHandlerCheck.h:20
+/// Checker for SEI CERT rule ENV32-C
+/// All exit handlers must return normal.
+/// Exit handlers must terminate by returning. It is important and potentially

In the documentation you use the:
clang-tools-extra/docs/clang-tidy/checks/cert-env32-c.rst
> All exit handlers must return normally.

You should be consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83717



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

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe22cae32c5c4: [analyzer][NFC] Add unittest for 
FalsePositiveRefutationBRVisitor (authored by steakhal).

Changed prior to commit:
  https://reviews.llvm.org/D78704?vs=260104&id=274133#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78704

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/CheckerRegistration.h
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -0,0 +1,175 @@
+//===- unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CheckerRegistration.h"
+#include "Reusables.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Config/config.h"
+#include "gtest/gtest.h"
+
+// FIXME: Use GTEST_SKIP() instead if GTest is updated to version 1.10.0
+#define SKIP_WITHOUT_Z3\
+  do   \
+if (!LLVM_WITH_Z3) \
+  return;  \
+  while (0)
+
+namespace clang {
+namespace ento {
+namespace {
+
+class FalsePositiveGenerator : public Checker {
+  using Self = FalsePositiveGenerator;
+  const BuiltinBug FalsePositiveGeneratorBug{this, "FalsePositiveGenerator"};
+  using HandlerFn = bool (Self::*)(const CallEvent &Call,
+   CheckerContext &) const;
+  CallDescriptionMap Callbacks = {
+  {{"reachedWithContradiction", 0}, &Self::reachedWithContradiction},
+  {{"reachedWithNoContradiction", 0}, &Self::reachedWithNoContradiction},
+  {{"reportIfCanBeTrue", 1}, &Self::reportIfCanBeTrue},
+  };
+
+  bool report(CheckerContext &C, ProgramStateRef State,
+  StringRef Description) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode(State);
+if (!Node)
+  return false;
+
+auto Report = std::make_unique(
+FalsePositiveGeneratorBug, Description, Node);
+C.emitReport(std::move(Report));
+return true;
+  }
+
+  bool reachedWithNoContradiction(const CallEvent &, CheckerContext &C) const {
+return report(C, C.getState(), "REACHED_WITH_NO_CONTRADICTION");
+  }
+
+  bool reachedWithContradiction(const CallEvent &, CheckerContext &C) const {
+return report(C, C.getState(), "REACHED_WITH_CONTRADICTION");
+  }
+
+  // Similar to ExprInspectionChecker::analyzerEval except it emits warning only
+  // if the argument can be true. The report emits the report in the state where
+  // the assertion true.
+  bool reportIfCanBeTrue(const CallEvent &Call, CheckerContext &C) const {
+// A specific instantiation of an inlined function may have more constrained
+// values than can generally be assumed. Skip the check.
+if (C.getPredecessor()->getLocationContext()->getStackFrame()->getParent())
+  return false;
+
+SVal AssertionVal = Call.getArgSVal(0);
+if (AssertionVal.isUndef())
+  return false;
+
+ProgramStateRef State = C.getPredecessor()->getState();
+ProgramStateRef StTrue;
+std::tie(StTrue, std::ignore) =
+State->assume(AssertionVal.castAs());
+if (StTrue)
+  return report(C, StTrue, "CAN_BE_TRUE");
+return false;
+  }
+
+public:
+  bool evalCall(const CallEvent &Call, CheckerContext &C) const {
+if (const HandlerFn *Callback = Callbacks.lookup(Call))
+  return (this->*(*Callback))(Call, C);
+return false;
+  }
+};
+
+void addFalsePositiveGenerator(AnalysisASTConsumer &AnalysisConsumer,
+   AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.FalsePositiveGenerator", true},
+{"debug.ViewExplodedGraph", false}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistr

[PATCH] D78457: [analyzer][Z3-refutation] Fix a refutation BugReporterVisitor bug

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rGde361df3f6d0: [analyzer][Z3-refutation] Fix a refutation 
BugReporterVisitor bug (authored by steakhal).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78457

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -170,6 +170,54 @@
 "test.FalsePositiveGenerator:CAN_BE_TRUE\n");
 }
 
+TEST(FalsePositiveRefutationBRVisitor,
+ UnSatAtErrorNodeDueToRefinedConstraintNoReport) {
+  SKIP_WITHOUT_Z3;
+  constexpr auto Code = R"(
+void reportIfCanBeTrue(bool);
+void reachedWithNoContradiction();
+void test(unsigned x, unsigned n) {
+  if (n >= 1 && n <= 2) {
+if (x >= 3)
+  return;
+// x: [0,2] and n: [1,2]
+int y = x + n; // y: '(x+n)' Which is in approximately between 1 and 4.
+
+// Registers the symbol 'y' with the constraint [1, MAX] in the true
+// branch.
+if (y > 0) {
+  // Since the x: [0,2] and n: [1,2], the 'y' is indeed greater than
+  // zero. If we emit a warning here, the constraints on the BugPath is
+  // SAT. Therefore that report is NOT invalidated.
+  reachedWithNoContradiction(); // 'y' can be greater than zero. OK
+
+  // If we ask the analyzer whether the 'y' can be 5. It won't know,
+  // therefore, the state will be created where the 'y' expression is 5.
+  // Although, this assumption is false!
+  // 'y' can not be 5 if the maximal value of both x and n is 2.
+  // The BugPath which become UnSAT in the ErrorNode with a refined
+  // constraint, should be invalidated.
+  reportIfCanBeTrue(y == 5);
+}
+  }
+})";
+
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCodeWithArgs(
+  Code, LazyAssumeAndCrossCheckArgs, Diags));
+  EXPECT_EQ(Diags,
+"test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+  // Single warning. The second report was invalidated by the visitor.
+
+  // Without enabling the crosscheck-with-z3 both reports are displayed.
+  std::string Diags2;
+  EXPECT_TRUE(runCheckerOnCodeWithArgs(
+  Code, LazyAssumeArgs, Diags2));
+  EXPECT_EQ(Diags2,
+"test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
+"test.FalsePositiveGenerator:CAN_BE_TRUE\n");
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2819,7 +2819,7 @@
 BugReporterContext &BRC, const ExplodedNode *EndPathNode,
 PathSensitiveBugReport &BR) {
   // Collect new constraints
-  VisitNode(EndPathNode, BRC, BR);
+  addConstraints(EndPathNode, /*OverwriteConstraintsOnExistingSyms=*/true);
 
   // Create a refutation manager
   llvm::SMTSolverRef RefutationSolver = llvm::CreateZ3Solver();
@@ -2830,30 +2830,30 @@
 const SymbolRef Sym = I.first;
 auto RangeIt = I.second.begin();
 
-llvm::SMTExprRef Constraints = SMTConv::getRangeExpr(
+llvm::SMTExprRef SMTConstraints = SMTConv::getRangeExpr(
 RefutationSolver, Ctx, Sym, RangeIt->From(), RangeIt->To(),
 /*InRange=*/true);
 while ((++RangeIt) != I.second.end()) {
-  Constraints = RefutationSolver->mkOr(
-  Constraints, SMTConv::getRangeExpr(RefutationSolver, Ctx, Sym,
- RangeIt->From(), RangeIt->To(),
- /*InRange=*/true));
+  SMTConstraints = RefutationSolver->mkOr(
+  SMTConstraints, SMTConv::getRangeExpr(RefutationSolver, Ctx, Sym,
+RangeIt->From(), RangeIt->To(),
+/*InRange=*/true));
 }
 
-RefutationSolver->addConstraint(Constraints);
+RefutationSolver->addConstraint(SMTConstraints);
   }
 
   // And check for satisfiability
-  Optional isSat = RefutationSolver->check();
-  if (!isSat.hasValue())
+  Optional IsSAT = RefutationSolver->check();
+  if (!IsSAT.hasValue())
 return;
 
-  if (!isSat.getValue())
+  if (!IsSAT.getValue())
 BR.markInvalid("Infeasible constraints", EndPathNode->getLocationContext());
 }
 
-PathDiagnosticPieceR

[PATCH] D82856: [analyzer][Z3-refutation] Add statistics for refutation visitor

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

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

This patch adds two statics.
The diff speaks for itself.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82856

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp


Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -52,6 +52,7 @@
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
@@ -2812,12 +2813,19 @@
 // Implementation of FalsePositiveRefutationBRVisitor.
 
//===--===//
 
+#define DEBUG_TYPE "FalsePositiveRefutationBRVisitor"
+STATISTIC(CrosscheckedBugReports,
+  "The # of bug reports which were checked for infeasible 
constraints");
+STATISTIC(CrosscheckInvalidatedBugReports,
+  "The # of bug reports invalidated due to infeasible constraints");
+
 FalsePositiveRefutationBRVisitor::FalsePositiveRefutationBRVisitor()
 : Constraints(ConstraintRangeTy::Factory().getEmptyMap()) {}
 
 void FalsePositiveRefutationBRVisitor::finalizeVisitor(
 BugReporterContext &BRC, const ExplodedNode *EndPathNode,
 PathSensitiveBugReport &BR) {
+  ++CrosscheckedBugReports;
   // Collect new constraints
   addConstraints(EndPathNode, /*OverwriteConstraintsOnExistingSyms=*/true);
 
@@ -2848,8 +2856,10 @@
   if (!IsSAT.hasValue())
 return;
 
-  if (!IsSAT.getValue())
+  if (!IsSAT.getValue()) {
+++CrosscheckInvalidatedBugReports;
 BR.markInvalid("Infeasible constraints", 
EndPathNode->getLocationContext());
+  }
 }
 
 void FalsePositiveRefutationBRVisitor::addConstraints(


Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -52,6 +52,7 @@
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
@@ -2812,12 +2813,19 @@
 // Implementation of FalsePositiveRefutationBRVisitor.
 //===--===//
 
+#define DEBUG_TYPE "FalsePositiveRefutationBRVisitor"
+STATISTIC(CrosscheckedBugReports,
+  "The # of bug reports which were checked for infeasible constraints");
+STATISTIC(CrosscheckInvalidatedBugReports,
+  "The # of bug reports invalidated due to infeasible constraints");
+
 FalsePositiveRefutationBRVisitor::FalsePositiveRefutationBRVisitor()
 : Constraints(ConstraintRangeTy::Factory().getEmptyMap()) {}
 
 void FalsePositiveRefutationBRVisitor::finalizeVisitor(
 BugReporterContext &BRC, const ExplodedNode *EndPathNode,
 PathSensitiveBugReport &BR) {
+  ++CrosscheckedBugReports;
   // Collect new constraints
   addConstraints(EndPathNode, /*OverwriteConstraintsOnExistingSyms=*/true);
 
@@ -2848,8 +2856,10 @@
   if (!IsSAT.hasValue())
 return;
 
-  if (!IsSAT.getValue())
+  if (!IsSAT.getValue()) {
+++CrosscheckInvalidatedBugReports;
 BR.markInvalid("Infeasible constraints", EndPathNode->getLocationContext());
+  }
 }
 
 void FalsePositiveRefutationBRVisitor::addConstraints(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82856: [analyzer][Z3-refutation] Add statistics for refutation visitor

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

steakhal added a comment.

In D82856#2122330 , @xazax.hun wrote:

> Yay! This looks good to me and I love statistics, so a huge +1. 
>  I have one question though. Isn't this counting all the reports in an 
> equivalence class? I.e. if the analyzer finds something multiple times it 
> will only be displayed once but here it will be counted multiple times. I 
> think it might be worth to have two statistics, one for all bug reports and 
> one for equivalence classes. What do you think?


You might be right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82856



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

[PATCH] D82856: [analyzer][Z3-refutation] Add statistics for refutation visitor

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

steakhal updated this revision to Diff 274510.
steakhal added a comment.

- adds statistic to undecidable branch


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

https://reviews.llvm.org/D82856

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp


Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -52,6 +52,7 @@
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
@@ -2812,12 +2813,23 @@
 // Implementation of FalsePositiveRefutationBRVisitor.
 
//===--===//
 
+#define DEBUG_TYPE "FalsePositiveRefutationBRVisitor"
+STATISTIC(CrosscheckedBugReports,
+  "The # of bug reports which were checked for infeasible 
constraints");
+STATISTIC(
+CrosscheckUndecidable,
+"The # of bug reports not invalidated due to undecidable constraints");
+STATISTIC(CrosscheckInvalidatedBugReports,
+  "The # of bug reports invalidated due to infeasible constraints");
+#undef DEBUG_TYPE
+
 FalsePositiveRefutationBRVisitor::FalsePositiveRefutationBRVisitor()
 : Constraints(ConstraintRangeTy::Factory().getEmptyMap()) {}
 
 void FalsePositiveRefutationBRVisitor::finalizeVisitor(
 BugReporterContext &BRC, const ExplodedNode *EndPathNode,
 PathSensitiveBugReport &BR) {
+  ++CrosscheckedBugReports;
   // Collect new constraints
   addConstraints(EndPathNode, /*OverwriteConstraintsOnExistingSyms=*/true);
 
@@ -2845,11 +2857,15 @@
 
   // And check for satisfiability
   Optional IsSAT = RefutationSolver->check();
-  if (!IsSAT.hasValue())
+  if (!IsSAT.hasValue()) {
+++CrosscheckUndecidable;
 return;
+  }
 
-  if (!IsSAT.getValue())
+  if (!IsSAT.getValue()) {
+++CrosscheckInvalidatedBugReports;
 BR.markInvalid("Infeasible constraints", 
EndPathNode->getLocationContext());
+  }
 }
 
 void FalsePositiveRefutationBRVisitor::addConstraints(


Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -52,6 +52,7 @@
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
@@ -2812,12 +2813,23 @@
 // Implementation of FalsePositiveRefutationBRVisitor.
 //===--===//
 
+#define DEBUG_TYPE "FalsePositiveRefutationBRVisitor"
+STATISTIC(CrosscheckedBugReports,
+  "The # of bug reports which were checked for infeasible constraints");
+STATISTIC(
+CrosscheckUndecidable,
+"The # of bug reports not invalidated due to undecidable constraints");
+STATISTIC(CrosscheckInvalidatedBugReports,
+  "The # of bug reports invalidated due to infeasible constraints");
+#undef DEBUG_TYPE
+
 FalsePositiveRefutationBRVisitor::FalsePositiveRefutationBRVisitor()
 : Constraints(ConstraintRangeTy::Factory().getEmptyMap()) {}
 
 void FalsePositiveRefutationBRVisitor::finalizeVisitor(
 BugReporterContext &BRC, const ExplodedNode *EndPathNode,
 PathSensitiveBugReport &BR) {
+  ++CrosscheckedBugReports;
   // Collect new constraints
   addConstraints(EndPathNode, /*OverwriteConstraintsOnExistingSyms=*/true);
 
@@ -2845,11 +2857,15 @@
 
   // And check for satisfiability
   Optional IsSAT = RefutationSolver->check();
-  if (!IsSAT.hasValue())
+  if (!IsSAT.hasValue()) {
+++CrosscheckUndecidable;
 return;
+  }
 
-  if (!IsSAT.getValue())
+  if (!IsSAT.getValue()) {
+++CrosscheckInvalidatedBugReports;
 BR.markInvalid("Infeasible constraints", EndPathNode->getLocationContext());
+  }
 }
 
 void FalsePositiveRefutationBRVisitor::addConstraints(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82900: [analyzer][Z3-refutation] Add statistics tracking invalidated bug report classes

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

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

Counts how many bug report equivalence classes were created and
how many of them were completely invalidated.

In other words how many times avoided emitting diagnostic to the user due to
Z3 refutation.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82900

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp


Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2849,7 +2849,7 @@
 return;
 
   if (!IsSAT.getValue())
-BR.markInvalid("Infeasible constraints", 
EndPathNode->getLocationContext());
+BR.markInvalid(Tag, EndPathNode->getLocationContext());
 }
 
 void FalsePositiveRefutationBRVisitor::addConstraints(
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -82,6 +82,11 @@
 STATISTIC(MaxValidBugClassSize,
   "The maximum number of bug reports in the same equivalence class "
   "where at least one report is valid (not suppressed)");
+STATISTIC(TotalBugReportEquivClasses,
+  "The # of bug report equivalence classes");
+STATISTIC(InvalidatedBugReportEquivClasses,
+  "The # of bug report equivalence classes invalidated due to "
+  "infeasible constraints");
 
 BugReporterVisitor::~BugReporterVisitor() = default;
 
@@ -2403,6 +2408,9 @@
 }
 
 void BugReporter::FlushReports() {
+  // Increment the total bug report equivalence classes processed statistic.
+  TotalBugReportEquivClasses += EQClassesVector.size();
+
   // We need to flush reports in deterministic order to ensure the order
   // of the reports is consistent between runs.
   for (const auto EQ : EQClassesVector)
@@ -2998,6 +3006,16 @@
   std::unique_ptr Diagnostics =
   generateDiagnosticForConsumerMap(report, Consumers, bugReports);
 
+  // Increment the statistic if necessary.
+  const bool IsEQClassInvalidatedByCrosscheck = llvm::all_of(
+  EQ.getReports(), [](const std::unique_ptr &RawReport) {
+const auto *Report = dyn_cast(RawReport.get());
+return Report && Report->isInvalidatedByTag(
+ FalsePositiveRefutationBRVisitor::Tag);
+  });
+  if (IsEQClassInvalidatedByCrosscheck)
+++InvalidatedBugReportEquivClasses;
+
   for (auto &P : *Diagnostics) {
 PathDiagnosticConsumer *Consumer = P.first;
 std::unique_ptr &PD = P.second;
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -378,6 +378,8 @@
 public:
   FalsePositiveRefutationBRVisitor();
 
+  static constexpr char *Tag = "Infeasible constraints";
+
   void Profile(llvm::FoldingSetNodeID &ID) const override;
 
   PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===
--- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -480,6 +480,14 @@
 Invalidations.insert(std::make_pair(Tag, Data));
   }
 
+  /// Returns whether or not this report was marked invalid by a given Tag.
+  bool isInvalidatedByTag(const void *Tag) const {
+const auto EqByTag = [Tag](const auto &TagAndData) {
+  return TagAndData.first == Tag;
+};
+return llvm::any_of(Invalidations, EqByTag);
+  }
+
   /// Profile to identify equivalent bug reports for error report coalescing.
   /// Reports are uniqued to ensure that we do not emit multiple diagnostics
   /// for each bug.


Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2849,7 +2849,7 @@
 return;
 
   if (!IsSAT.getValue())
-BR.markInvalid("Infeasible constraints", EndPathNode->getLocationContext());
+BR.markInvalid(Tag, EndPathNode->getLocationContext());
 }
 
 void FalsePositiveRefutationB

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

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

steakhal added a comment.

I'm impressed.
Though, I had some nits, please don't take it to heart :)

And consider joining the to the pre-merge beta testing 
 project to benefit from buildbots 
and much more - for free.




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:217
+
+  const llvm::APSInt sampleValue = getMinValue();
+  const bool isUnsigned = sampleValue.isUnsigned();

Should we take it as `const ref` to prevent copying?



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:235
+// handle a special case for MIN value
+if (isFromMinValue) {
+  // if [from, to] are [MIN, MAX], then just return the same [MIN, MAX]

AFAIK in a `RangeSet` the ranges are sorted by the `From` point in ascending 
order.
We should not check it for each range, only for the first.

Also, note that small and flat loops are more readable.
Consider using //pure// lambdas to reduce the body of the loop and of course to 
bind algorithms to names.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:572-573
   if (const RangeSet *negV = State->get(negSym)) {
-// Unsigned range set cannot be negated, unless it is [0, 0].
-if ((negV->getConcreteValue() &&
- (*negV->getConcreteValue() == 0)) ||
+if (T->isUnsignedIntegerOrEnumerationType() ||
 T->isSignedIntegerOrEnumerationType())
   return negV;

Couldn't we simplify the disjunction to single `T->isEnumerationType()` or to 
something similar?



Comment at: clang/test/Analysis/constraint_manager_negate_difference.c:114-118
+void negated_unsigned_range(unsigned x, unsigned y) {
+  clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} 
expected-warning{{TRUE}}
+  clang_analyzer_eval(y - x != 0); // expected-warning{{FALSE}} 
expected-warning{{TRUE}}
+  clang_analyzer_eval(x - y != 0); // expected-warning{{FALSE}} 
expected-warning{{TRUE}}
+}

What does this test case demonstrate? Could you elaborate on that?
Why do you evaluate the `x - y != 0` here twice?



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:23
+template  struct TestCase {
+  RangeSet original;
+  RangeSet expected;

According to the [LLVM coding 
style](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)
 we should use UpperCamelCase for variable names for new code.

Note that the Analyzer Core was written in a different style, we should follow 
that there instead (at least IMO).



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:27-28
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+   const std::initializer_list &originalList,
+   const std::initializer_list &expectedList)
+  : original(createRangeSetFromList(BVF, F, originalList)),

AFAIK since `std::initializer_list` is just two pointers we should take it by 
value.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:51
+  // init block
+  DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions};
+  FileSystemOptions FileSystemOpts;

Generally, `new expressions` are a code smell. We should use something like an 
`std::make_unique` to prevent memory leaks on exceptions.
Though, I'm not sure if there is a similar function for 
`llvm::IntrusiveRefCntPtr`s.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:66
+  template  void checkNegate() {
+using type = T;
+

To be honest, I'm not sure if `type` is more descriptive than `T`.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:68
+
+// use next values of the range {MIN, A, B, MID, C, D, MAX}
+

AFAIK full sentences are required for comments.
https://llvm.org/docs/CodingStandards.html#commenting



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:113-120
+  // c.original.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // c.expected.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // negatedFromOriginal.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // negatedBackward.print(llvm::dbgs());

Should we keep this?


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

https://reviews.llvm.org/D77802



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

[PATCH] D78457: [analyzer][Z3-refutation] Fix refutation BugReporterVisitor bug and refactor

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

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

`FalsePositiveRefutationBRVisitor` had a bug(*) where the constraints were not 
properly collected thus crosschecked with Z3.
This patch fixes that bug by eliminating the `FalsePositiveRefutationBRVisitor` 
completely.

As turned out we don't even need a `BugReporterVisitor` for doing the 
crosscheck.
We should simply use the constraints of the `ErrorNode` since that has all the 
necessary information.

Bug(*):
The visitor collected all the constraints on a `BugPath`.
Since it is a visitor, it visited the node before the `ErrorNode`, and so on 
until the `RootNode`.
At the end visited the ErrorNode explicitly, but that visit had no effect.
Since the constraints were collected into a map, mapping each symbol to its 
`RangeSet`.
If the map already had a mapping with the symbol, then it was skipped.

Consider the following case:
We already had a constraint on a symbol, but az the end in the ErrorNode we 
have a stricter constraint on that.
This visitor would not utilize that stricter constraint in the crosschecking.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78457

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2809,74 +2809,6 @@
   return nullptr;
 }
 
-//===--===//
-// Implementation of FalsePositiveRefutationBRVisitor.
-//===--===//
-
-FalsePositiveRefutationBRVisitor::FalsePositiveRefutationBRVisitor()
-: Constraints(ConstraintRangeTy::Factory().getEmptyMap()) {}
-
-void FalsePositiveRefutationBRVisitor::finalizeVisitor(
-BugReporterContext &BRC, const ExplodedNode *EndPathNode,
-PathSensitiveBugReport &BR) {
-  // Collect new constraints
-  VisitNode(EndPathNode, BRC, BR);
-
-  // Create a refutation manager
-  llvm::SMTSolverRef RefutationSolver = llvm::CreateZ3Solver();
-  ASTContext &Ctx = BRC.getASTContext();
-
-  // Add constraints to the solver
-  for (const auto &I : Constraints) {
-const SymbolRef Sym = I.first;
-auto RangeIt = I.second.begin();
-
-llvm::SMTExprRef Constraints = SMTConv::getRangeExpr(
-RefutationSolver, Ctx, Sym, RangeIt->From(), RangeIt->To(),
-/*InRange=*/true);
-while ((++RangeIt) != I.second.end()) {
-  Constraints = RefutationSolver->mkOr(
-  Constraints, SMTConv::getRangeExpr(RefutationSolver, Ctx, Sym,
- RangeIt->From(), RangeIt->To(),
- /*InRange=*/true));
-}
-
-RefutationSolver->addConstraint(Constraints);
-  }
-
-  // And check for satisfiability
-  Optional isSat = RefutationSolver->check();
-  if (!isSat.hasValue())
-return;
-
-  if (!isSat.getValue())
-BR.markInvalid("Infeasible constraints", EndPathNode->getLocationContext());
-}
-
-PathDiagnosticPieceRef FalsePositiveRefutationBRVisitor::VisitNode(
-const ExplodedNode *N, BugReporterContext &, PathSensitiveBugReport &) {
-  // Collect new constraints
-  const ConstraintRangeTy &NewCs = N->getState()->get();
-  ConstraintRangeTy::Factory &CF =
-  N->getState()->get_context();
-
-  // Add constraints if we don't have them yet
-  for (auto const &C : NewCs) {
-const SymbolRef &Sym = C.first;
-if (!Constraints.contains(Sym)) {
-  Constraints = CF.add(Constraints, Sym, C.second);
-}
-  }
-
-  return nullptr;
-}
-
-void FalsePositiveRefutationBRVisitor::Profile(
-llvm::FoldingSetNodeID &ID) const {
-  static int Tag = 0;
-  ID.AddPointer(&Tag);
-}
-
 //===--===//
 // Implementation of TagVisitor.
 //===--===//
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporter.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporter.cpp
@@ -38,6 +38,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/Static

[PATCH] D78457: [analyzer][Z3-refutation] Fix refutation BugReporterVisitor bug and refactor

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

steakhal added a comment.

In D78457#1991288 , @xazax.hun wrote:

> > As turned out we don't even need a BugReporterVisitor for doing the 
> > crosscheck.
> >  We should simply use the constraints of the ErrorNode since that has all 
> > the necessary information.
>
> This should not be true. But we should definitely have a test case that 
> proves this. The main idea is that unused symbols can be garbage collected. 
> The problem is that the ErrorNode does not have any information about the 
> symbols that were garbage collected earlier. This is why we need the visitor.


You might be right. Could you give a short example to a garbage-collected 
symbol?
Either way, we need to fix the bug which I stated earlier.

I tried to create a unit-test for the bug, but I miserably failed.
To call the `visitor::finalizeVisitor` you would need almost the entire world.  
(A `ExprEngine`, a `CompilerInstance`, etc.)
I also considered a `lit-test`, though I really doubt that is the right tool 
for testing this.
Since we are constantly improving the checkers, we should not bake in a 
false-positive bugreport which is then invalidated by the Z3 visitor...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78457



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

[PATCH] D72035: [analyzer][NFC] Use CallEvent checker callback in GenericTaintChecker

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

steakhal marked 2 inline comments as done.
steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:113
  const CheckerContext &C) {
-  const FunctionDecl *FDecl = C.getCalleeDecl(CE);
+  assert(Call.getDecl());
+  const FunctionDecl *FDecl = Call.getDecl()->getAsFunction();

NoQ wrote:
> rG878194414107e94600de31a11be09a347fb2598b!
Nice catch! Thank you for the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72035



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

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

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

steakhal created this revision.
steakhal added reviewers: NoQ, xazax.hun, rnkovacs, Szelethus, 
baloghadamsoftware, mikhail.ramalho.
Herald added subscribers: cfe-commits, ASDenysPetrov, martong, Charusso, 
dkrupp, donat.nagy, a.sidorin, szepet, whisperity, mgorny.
Herald added a project: clang.
steakhal added a child revision: D78457: [analyzer][Z3-refutation] Fix 
refutation BugReporterVisitor bug and refactor.
steakhal updated this revision to Diff 259531.
steakhal added a comment.

Upload the right diff.


Adds the test infrastructure for testing the `FalsePositiveRefutationBRVisitor`.

It will be extended later, to demonstrate a bug in the visitor.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78704

Files:
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/CheckerRegistration.h
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -0,0 +1,171 @@
+//===- unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CheckerRegistration.h"
+#include "Reusables.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Config/config.h"
+#include "gtest/gtest.h"
+
+#define SKIP_WITHOUT_Z3\
+  do   \
+if (!LLVM_WITH_Z3) \
+  return;  \
+  while (0)
+
+namespace clang {
+namespace ento {
+namespace {
+
+class FalsePositiveGenerator : public Checker {
+  using Self = FalsePositiveGenerator;
+  const BuiltinBug FalsePositiveGeneratorBug{this, "FalsePositiveGenerator"};
+  using HandlerFn = bool (Self::*)(const CallEvent &Call, CheckerContext &,
+   ProgramStateRef State) const;
+  CallDescriptionMap Callbacks = {
+  {{"reachedWithContradiction", 0}, &Self::reachedWithContradiction},
+  {{"reachedWithNoContradiction", 0}, &Self::reachedWithNoContradiction},
+  {{"reportIfCanBeZero", 1}, &Self::reportIfCanBeZero},
+  };
+
+  bool report(CheckerContext &C, ProgramStateRef State,
+  StringRef Description) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode(State);
+if (!Node)
+  return false;
+
+auto Report = std::make_unique(
+FalsePositiveGeneratorBug, Description, Node);
+C.emitReport(std::move(Report));
+return true;
+  }
+
+  bool reachedWithNoContradiction(const CallEvent &, CheckerContext &C,
+  ProgramStateRef State) const {
+return report(C, State, "REACHED_WITH_NO_CONTRADICTION");
+  }
+
+  bool reachedWithContradiction(const CallEvent &, CheckerContext &C,
+ProgramStateRef State) const {
+return report(C, State, "REACHED_WITH_CONTRADICTION");
+  }
+
+  bool reportIfCanBeZero(const CallEvent &Call, CheckerContext &C,
+ ProgramStateRef State) const {
+ProgramStateRef ArgIsZero, ArgIsNotZero;
+std::tie(ArgIsZero, ArgIsNotZero) = assumeArgEqZero(Call, C, State);
+
+if (ArgIsZero)
+  return report(C, ArgIsZero, "ZERO_STATE_SHOULD_NOT_EXIST");
+return false;
+  }
+
+  std::pair
+  assumeArgEqZero(const CallEvent &Call, CheckerContext &C,
+  ProgramStateRef State) const {
+SValBuilder &SVB = C.getSValBuilder();
+const QualType ArgType = Call.getArgExpr(0)->getType();
+const DefinedOrUnknownSVal Zero = SVB.makeZeroVal(ArgType);
+const SVal ArgEqZero = SVB.evalEQ(State, Call.getArgSVal(0), Zero);
+return State->assume(ArgEqZero.castAs());
+  }
+
+public:
+  bool evalCall(const CallEvent &Call, CheckerContext &C) const {
+if (const HandlerFn *Callback = Callbacks.lookup(Call))
+  return (this->*(*Callback))(Call, C, C.getState());
+return false;
+  }
+};
+
+void addFalsePositiveGenerator(AnalysisASTConsumer &AnalysisConsumer,
+   Analyzer

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

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

steakhal updated this revision to Diff 259531.
steakhal added a comment.

Upload the right diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78704

Files:
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/CheckerRegistration.h
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -0,0 +1,171 @@
+//===- unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CheckerRegistration.h"
+#include "Reusables.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Config/config.h"
+#include "gtest/gtest.h"
+
+#define SKIP_WITHOUT_Z3\
+  do   \
+if (!LLVM_WITH_Z3) \
+  return;  \
+  while (0)
+
+namespace clang {
+namespace ento {
+namespace {
+
+class FalsePositiveGenerator : public Checker {
+  using Self = FalsePositiveGenerator;
+  const BuiltinBug FalsePositiveGeneratorBug{this, "FalsePositiveGenerator"};
+  using HandlerFn = bool (Self::*)(const CallEvent &Call, CheckerContext &,
+   ProgramStateRef State) const;
+  CallDescriptionMap Callbacks = {
+  {{"reachedWithContradiction", 0}, &Self::reachedWithContradiction},
+  {{"reachedWithNoContradiction", 0}, &Self::reachedWithNoContradiction},
+  {{"reportIfCanBeZero", 1}, &Self::reportIfCanBeZero},
+  };
+
+  bool report(CheckerContext &C, ProgramStateRef State,
+  StringRef Description) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode(State);
+if (!Node)
+  return false;
+
+auto Report = std::make_unique(
+FalsePositiveGeneratorBug, Description, Node);
+C.emitReport(std::move(Report));
+return true;
+  }
+
+  bool reachedWithNoContradiction(const CallEvent &, CheckerContext &C,
+  ProgramStateRef State) const {
+return report(C, State, "REACHED_WITH_NO_CONTRADICTION");
+  }
+
+  bool reachedWithContradiction(const CallEvent &, CheckerContext &C,
+ProgramStateRef State) const {
+return report(C, State, "REACHED_WITH_CONTRADICTION");
+  }
+
+  bool reportIfCanBeZero(const CallEvent &Call, CheckerContext &C,
+ ProgramStateRef State) const {
+ProgramStateRef ArgIsZero, ArgIsNotZero;
+std::tie(ArgIsZero, ArgIsNotZero) = assumeArgEqZero(Call, C, State);
+
+if (ArgIsZero)
+  return report(C, ArgIsZero, "ZERO_STATE_SHOULD_NOT_EXIST");
+return false;
+  }
+
+  std::pair
+  assumeArgEqZero(const CallEvent &Call, CheckerContext &C,
+  ProgramStateRef State) const {
+SValBuilder &SVB = C.getSValBuilder();
+const QualType ArgType = Call.getArgExpr(0)->getType();
+const DefinedOrUnknownSVal Zero = SVB.makeZeroVal(ArgType);
+const SVal ArgEqZero = SVB.evalEQ(State, Call.getArgSVal(0), Zero);
+return State->assume(ArgEqZero.castAs());
+  }
+
+public:
+  bool evalCall(const CallEvent &Call, CheckerContext &C) const {
+if (const HandlerFn *Callback = Callbacks.lookup(Call))
+  return (this->*(*Callback))(Call, C, C.getState());
+return false;
+  }
+};
+
+void addFalsePositiveGenerator(AnalysisASTConsumer &AnalysisConsumer,
+   AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.FalsePositiveGenerator", true},
+{"debug.ViewExplodedGraph", false},
+{"debug.ExprInspection", false}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+Registry.addChecker(
+"test.FalsePositiveGenerator", "EmptyDescription", "EmptyDocsUri");
+  });
+}
+
+const std::vector CrossCheckArgs{
+"-Xclang", "-analyzer-config", "-Xclang"

[PATCH] D78457: [analyzer][Z3-refutation] Fix refutation BugReporterVisitor bug and refactor

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

steakhal updated this revision to Diff 259532.
steakhal added a comment.

- keep the visitor
- fix the bug
- add test demonstrating the bug and the fix


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78457

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -166,6 +166,53 @@
 "test.FalsePositiveGenerator:ZERO_STATE_SHOULD_NOT_EXIST\n");
 }
 
+TEST(FalsePositiveRefutationBRVisitor,
+ UnSatAtErrorNodeDueToRefinedConstraintNoReport) {
+  SKIP_WITHOUT_Z3;
+  std::string Diags;
+  constexpr auto Code = R"(
+void reportIfCanBeZero(int);
+void reachedWithNoContradiction();
+void test(unsigned x, unsigned n) {
+  if (n >= 1 && n <= 2) {
+if (x >= 3)
+  return;
+// x: [0,2] and n: [1,2]
+int y = x + n; // y: ($x+$n) Which is in approximately between 1 and 4.
+
+// Registers the symbol '($x+$n)' with the constraint [1, MAX] in the
+// true branch.
+if (y > 0) {
+  // Since the x: [0,2] and n: [1,2] the ($x+$n) is indeed greater than
+  // zero. If we emit a warning here, the constraints on the bugpath is
+  // SAT. Therefore that bugreport is NOT invalidated.
+  reachedWithNoContradiction(); // ($x+$n) can be grater than zero. OK
+
+  // If we ask the analyzer whether the 'y-5' can be zero. It won't know,
+  // therefore the state will be created where the 'y-5' expression is 0.
+  // This assumption is false!
+  // 'y' can not be 5 if the maximal value of both x and y is 2.
+  // This bugreport must be caught by the visitor, since the constraints
+  // of the bugpath are UnSAT.
+  reportIfCanBeZero(y - 5);
+}
+  }
+})";
+
+  // Only the first diagnostic is visible.
+  EXPECT_TRUE(runFalsePositiveGeneratorOnCode(Code, Diags, CrossCheckArgs));
+  EXPECT_EQ(Diags,
+"test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+
+  // Without enabling the crosscheck-with-z3 the BugPath is not invalidated.
+  // Both diagnostics are visible.
+  std::string Diags2;
+  EXPECT_TRUE(runFalsePositiveGeneratorOnCode(Code, Diags2));
+  EXPECT_EQ(Diags2,
+"test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
+"test.FalsePositiveGenerator:ZERO_STATE_SHOULD_NOT_EXIST\n");
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2820,7 +2820,7 @@
 BugReporterContext &BRC, const ExplodedNode *EndPathNode,
 PathSensitiveBugReport &BR) {
   // Collect new constraints
-  VisitNode(EndPathNode, BRC, BR);
+  addConstraints(EndPathNode, /*OnlyForNewSymbols=*/false);
 
   // Create a refutation manager
   llvm::SMTSolverRef RefutationSolver = llvm::CreateZ3Solver();
@@ -2831,30 +2831,30 @@
 const SymbolRef Sym = I.first;
 auto RangeIt = I.second.begin();
 
-llvm::SMTExprRef Constraints = SMTConv::getRangeExpr(
+llvm::SMTExprRef SMTConstraints = SMTConv::getRangeExpr(
 RefutationSolver, Ctx, Sym, RangeIt->From(), RangeIt->To(),
 /*InRange=*/true);
 while ((++RangeIt) != I.second.end()) {
-  Constraints = RefutationSolver->mkOr(
-  Constraints, SMTConv::getRangeExpr(RefutationSolver, Ctx, Sym,
- RangeIt->From(), RangeIt->To(),
- /*InRange=*/true));
+  SMTConstraints = RefutationSolver->mkOr(
+  SMTConstraints, SMTConv::getRangeExpr(RefutationSolver, Ctx, Sym,
+RangeIt->From(), RangeIt->To(),
+/*InRange=*/true));
 }
 
-RefutationSolver->addConstraint(Constraints);
+RefutationSolver->addConstraint(SMTConstraints);
   }
 
   // And check for satisfiability
-  Optional isSat = RefutationSolver->check();
-  if (!isSat.hasValue())
+  Optional IsSAT = RefutationSolver->check();
+  if (!IsSAT.hasValue())
 return;
 
-  if (!isSat.getValue())
+  if (!IsSAT.getValue())
 BR.markInvalid("Infeasible constraints", EndPathNode->getLocationContext());
 }
 
-PathDiagnosticPieceRef FalsePositiveRefutationBRVisitor::VisitN

[PATCH] D78457: [analyzer][Z3-refutation] Fix refutation BugReporterVisitor bug and refactor

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

steakhal marked an inline comment as done.
steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2871
+  // Overwrite the associated constraint of the Symbol.
+  Constraints = CF.remove(Constraints, Sym);
   Constraints = CF.add(Constraints, Sym, C.second);

How can we simplify this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78457



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

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

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

steakhal updated this revision to Diff 259668.
steakhal marked 6 inline comments as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78704

Files:
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/CheckerRegistration.h
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -0,0 +1,174 @@
+//===- unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CheckerRegistration.h"
+#include "Reusables.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Config/config.h"
+#include "gtest/gtest.h"
+
+#define SKIP_WITHOUT_Z3\
+  do   \
+if (!LLVM_WITH_Z3) \
+  return;  \
+  while (0)
+
+namespace clang {
+namespace ento {
+namespace {
+
+class FalsePositiveGenerator : public Checker {
+  using Self = FalsePositiveGenerator;
+  const BuiltinBug FalsePositiveGeneratorBug{this, "FalsePositiveGenerator"};
+  using HandlerFn = bool (Self::*)(const CallEvent &Call,
+   CheckerContext &) const;
+  CallDescriptionMap Callbacks = {
+  {{"reachedWithContradiction", 0}, &Self::reachedWithContradiction},
+  {{"reachedWithNoContradiction", 0}, &Self::reachedWithNoContradiction},
+  {{"reportIfCanBeTrue", 1}, &Self::reportIfCanBeTrue},
+  };
+
+  bool report(CheckerContext &C, ProgramStateRef State,
+  StringRef Description) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode(State);
+if (!Node)
+  return false;
+
+auto Report = std::make_unique(
+FalsePositiveGeneratorBug, Description, Node);
+C.emitReport(std::move(Report));
+return true;
+  }
+
+  bool reachedWithNoContradiction(const CallEvent &, CheckerContext &C) const {
+return report(C, C.getState(), "REACHED_WITH_NO_CONTRADICTION");
+  }
+
+  bool reachedWithContradiction(const CallEvent &, CheckerContext &C) const {
+return report(C, C.getState(), "REACHED_WITH_CONTRADICTION");
+  }
+
+  // Similar to ExprInspectionChecker::analyzerEval except it emits warning only
+  // if the argument can be true. The report emits the report in the state where
+  // the assertion true.
+  bool reportIfCanBeTrue(const CallEvent &Call, CheckerContext &C) const {
+// A specific instantiation of an inlined function may have more constrained
+// values than can generally be assumed. Skip the check.
+if (C.getPredecessor()->getLocationContext()->getStackFrame()->getParent())
+  return false;
+
+SVal AssertionVal = Call.getArgSVal(0);
+if (AssertionVal.isUndef())
+  return false;
+
+ProgramStateRef State = C.getPredecessor()->getState();
+ProgramStateRef StTrue;
+std::tie(StTrue, std::ignore) =
+State->assume(AssertionVal.castAs());
+if (StTrue)
+  return report(C, StTrue, "CAN_BE_TRUE");
+return false;
+  }
+
+public:
+  bool evalCall(const CallEvent &Call, CheckerContext &C) const {
+if (const HandlerFn *Callback = Callbacks.lookup(Call))
+  return (this->*(*Callback))(Call, C);
+return false;
+  }
+};
+
+void addFalsePositiveGenerator(AnalysisASTConsumer &AnalysisConsumer,
+   AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.FalsePositiveGenerator", true},
+{"debug.ViewExplodedGraph", false}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+Registry.addChecker(
+"test.FalsePositiveGenerator", "EmptyDescription", "EmptyDocsUri");
+  });
+}
+
+// C++20 use constexpr below.
+const std::vector LazyAssumeArgs{
+"-Xclang", "-analyzer-config", "-Xclang", "eagerly-assume=false"};
+const std::vector LazyAssumeAndCrossCheckArgs{
+"

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

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

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

I addressed most of the comments.




Comment at: clang/unittests/StaticAnalyzer/CheckerRegistration.h:81
+template 
+bool runCheckerOnCodeWithArgs(const std::string &Code, std::string &Diags,
+  const std::vector &Args,

martong wrote:
> Do you use this function template anywhere?
No, I'm just following the pattern of the twin-functions above.
To be honest, I don't know what is the purpose of the variadic template here.



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:24
+  do   
\
+if (!LLVM_WITH_Z3) 
\
+  return;  
\

xazax.hun wrote:
> I think this might not be the idiomatic way to skip a test. Consider using ` 
> GTEST_SKIP();`.
I agree, though that is not yet supported in the `gtest` in the repository.
Should we update that to benefit from this macro?

There are several places where we could use that like:
- 
[llvm/unittests/ExecutionEngine/MCJIT/MCJITTestAPICommon.h:29](https://github.com/llvm/llvm-project/blob/master/llvm/unittests/ExecutionEngine/MCJIT/MCJITTestAPICommon.h#L29-L33)
 (expanded 37 times in the codebase)
- 
[llvm/unittests/Support/ThreadPool.cpp:81](https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Support/ThreadPool.cpp#L81-L85)
 (expanded 7 times in the file)
- 
[llvm/unittests/Support/MemoryTest.cpp:89](https://github.com/llvm/llvm-project/blob/master/llvm/unittests/Support/MemoryTest.cpp#L89-L94)
 (expanded 10 time in the file)



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:55
+
+  bool reachedWithNoContradiction(const CallEvent &, CheckerContext &C,
+  ProgramStateRef State) const {

martong wrote:
> It this not used in the test?
Now I'm using it :) Thx.



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:65
+
+  bool reportIfCanBeZero(const CallEvent &Call, CheckerContext &C,
+ ProgramStateRef State) const {

martong wrote:
> xazax.hun wrote:
> > Maybe `reportIfArgCanBeZero`?
> Perhaps a generic `reportIfTrue` would be more useful. For that you must use 
> another eval function instead of `evalEQ`
I like it! Fixed.



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:115
+  llvm::raw_string_ostream OS(Diags);
+  return tooling::runToolOnCodeWithArgs(
+  std::make_unique>(OS), Code, Args,

xazax.hun wrote:
> Wasn't `runCheckerOnCodeWithArgs` created for this purpose?
Somewhat yes.
For testing purposes, it's valuable to encode the testcase's name into the 
`FileName`, to be visible in the exploded graph enabled for debugging.

I have updated the `runCheckerOnCodeWithArgs` function to address your comment.
Please note that it introduced a dependency between the corresponding header 
and the `gtest` header.



Comment at: 
clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp:138
+  std::string Diags2;
+  EXPECT_TRUE(runFalsePositiveGeneratorOnCode(Code, Diags2));
+  EXPECT_EQ(Diags2, 
"test.FalsePositiveGenerator:REACHED_WITH_CONTRADICTION\n");

martong wrote:
> There is no need to have `Diags2`. You could reuse `Diags` if in 
> `runFalsePositiveGeneratorOnCode` you cleared the diag param.
I would doubt whether that is more readable.
Btw, I moved the declaration closer to the usage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78704



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

[PATCH] D78457: [analyzer][Z3-refutation] Fix refutation BugReporterVisitor bug and refactor

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

steakhal marked 7 inline comments as done.
steakhal added a comment.

I'm really embarrassed that I uploaded the wrong diff.
I appologize.

Most of the comments are done :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78457



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

[PATCH] D78457: [analyzer][Z3-refutation] Fix refutation BugReporterVisitor bug and refactor

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

steakhal updated this revision to Diff 259674.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78457

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -169,6 +169,54 @@
 "test.FalsePositiveGenerator:CAN_BE_TRUE\n");
 }
 
+TEST(FalsePositiveRefutationBRVisitor,
+ UnSatAtErrorNodeDueToRefinedConstraintNoReport) {
+  SKIP_WITHOUT_Z3;
+  constexpr auto Code = R"(
+void reportIfCanBeTrue(bool);
+void reachedWithNoContradiction();
+void test(unsigned x, unsigned n) {
+  if (n >= 1 && n <= 2) {
+if (x >= 3)
+  return;
+// x: [0,2] and n: [1,2]
+int y = x + n; // y: '(x+n)' Which is in approximately between 1 and 4.
+
+// Registers the symbol 'y' with the constraint [1, MAX] in the true
+// branch.
+if (y > 0) {
+  // Since the x: [0,2] and n: [1,2], the 'y' is indeed greater than
+  // zero. If we emit a warning here, the constraints on the BugPath is
+  // SAT. Therefore that report is NOT invalidated.
+  reachedWithNoContradiction(); // 'y' can be greater than zero. OK
+
+  // If we ask the analyzer whether the 'y' can be 5. It won't know,
+  // therefore, the state will be created where the 'y' expression is 5.
+  // Although, this assumption is false!
+  // 'y' can not be 5 if the maximal value of both x and y is 2.
+  // The BugPath which become UnSAT in the ErrorNode with a refined
+  // constraint, should be invalidated.
+  reportIfCanBeTrue(y == 5);
+}
+  }
+})";
+
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCodeWithArgs(
+  Code, LazyAssumeAndCrossCheckArgs, Diags));
+  EXPECT_EQ(Diags,
+"test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+  // Single warning. The second report was invalidated by the visitor.
+
+  // Without enabling the crosscheck-with-z3 both reports are displayed.
+  std::string Diags2;
+  EXPECT_TRUE(runCheckerOnCodeWithArgs(
+  Code, LazyAssumeArgs, Diags2));
+  EXPECT_EQ(Diags2,
+"test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
+"test.FalsePositiveGenerator:CAN_BE_TRUE\n");
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2820,7 +2820,7 @@
 BugReporterContext &BRC, const ExplodedNode *EndPathNode,
 PathSensitiveBugReport &BR) {
   // Collect new constraints
-  VisitNode(EndPathNode, BRC, BR);
+  addConstraints(EndPathNode, /*OnlyForNewSymbols=*/false);
 
   // Create a refutation manager
   llvm::SMTSolverRef RefutationSolver = llvm::CreateZ3Solver();
@@ -2831,30 +2831,30 @@
 const SymbolRef Sym = I.first;
 auto RangeIt = I.second.begin();
 
-llvm::SMTExprRef Constraints = SMTConv::getRangeExpr(
+llvm::SMTExprRef SMTConstraints = SMTConv::getRangeExpr(
 RefutationSolver, Ctx, Sym, RangeIt->From(), RangeIt->To(),
 /*InRange=*/true);
 while ((++RangeIt) != I.second.end()) {
-  Constraints = RefutationSolver->mkOr(
-  Constraints, SMTConv::getRangeExpr(RefutationSolver, Ctx, Sym,
- RangeIt->From(), RangeIt->To(),
- /*InRange=*/true));
+  SMTConstraints = RefutationSolver->mkOr(
+  SMTConstraints, SMTConv::getRangeExpr(RefutationSolver, Ctx, Sym,
+RangeIt->From(), RangeIt->To(),
+/*InRange=*/true));
 }
 
-RefutationSolver->addConstraint(Constraints);
+RefutationSolver->addConstraint(SMTConstraints);
   }
 
   // And check for satisfiability
-  Optional isSat = RefutationSolver->check();
-  if (!isSat.hasValue())
+  Optional IsSAT = RefutationSolver->check();
+  if (!IsSAT.hasValue())
 return;
 
-  if (!isSat.getValue())
+  if (!IsSAT.getValue())
 BR.markInvalid("Infeasible constraints", EndPathNode->getLocationContext());
 }
 
-PathDiagnosticPieceRef FalsePositiveRefutationBRVisitor::VisitNode(
-const ExplodedNode *N, BugReporterContext &, PathSensitiveBugReport &) {
+void FalsePositiveRefutationBRVisitor::a

[PATCH] D78704: [analyzer][NFC] Add unittest for FalsePositiveRefutationBRVisitor

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

steakhal updated this revision to Diff 260104.
steakhal edited the summary of this revision.
steakhal added a comment.

- added `GTEST_SKIP` FIXME comment
- reformatted the summary of the patch


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

https://reviews.llvm.org/D78704

Files:
  clang/unittests/StaticAnalyzer/CMakeLists.txt
  clang/unittests/StaticAnalyzer/CheckerRegistration.h
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -0,0 +1,175 @@
+//===- unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "CheckerRegistration.h"
+#include "Reusables.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
+#include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
+#include "llvm/Config/config.h"
+#include "gtest/gtest.h"
+
+// FIXME: Use GTEST_SKIP() instead if GTest is updated to version 1.10.0
+#define SKIP_WITHOUT_Z3\
+  do   \
+if (!LLVM_WITH_Z3) \
+  return;  \
+  while (0)
+
+namespace clang {
+namespace ento {
+namespace {
+
+class FalsePositiveGenerator : public Checker {
+  using Self = FalsePositiveGenerator;
+  const BuiltinBug FalsePositiveGeneratorBug{this, "FalsePositiveGenerator"};
+  using HandlerFn = bool (Self::*)(const CallEvent &Call,
+   CheckerContext &) const;
+  CallDescriptionMap Callbacks = {
+  {{"reachedWithContradiction", 0}, &Self::reachedWithContradiction},
+  {{"reachedWithNoContradiction", 0}, &Self::reachedWithNoContradiction},
+  {{"reportIfCanBeTrue", 1}, &Self::reportIfCanBeTrue},
+  };
+
+  bool report(CheckerContext &C, ProgramStateRef State,
+  StringRef Description) const {
+ExplodedNode *Node = C.generateNonFatalErrorNode(State);
+if (!Node)
+  return false;
+
+auto Report = std::make_unique(
+FalsePositiveGeneratorBug, Description, Node);
+C.emitReport(std::move(Report));
+return true;
+  }
+
+  bool reachedWithNoContradiction(const CallEvent &, CheckerContext &C) const {
+return report(C, C.getState(), "REACHED_WITH_NO_CONTRADICTION");
+  }
+
+  bool reachedWithContradiction(const CallEvent &, CheckerContext &C) const {
+return report(C, C.getState(), "REACHED_WITH_CONTRADICTION");
+  }
+
+  // Similar to ExprInspectionChecker::analyzerEval except it emits warning only
+  // if the argument can be true. The report emits the report in the state where
+  // the assertion true.
+  bool reportIfCanBeTrue(const CallEvent &Call, CheckerContext &C) const {
+// A specific instantiation of an inlined function may have more constrained
+// values than can generally be assumed. Skip the check.
+if (C.getPredecessor()->getLocationContext()->getStackFrame()->getParent())
+  return false;
+
+SVal AssertionVal = Call.getArgSVal(0);
+if (AssertionVal.isUndef())
+  return false;
+
+ProgramStateRef State = C.getPredecessor()->getState();
+ProgramStateRef StTrue;
+std::tie(StTrue, std::ignore) =
+State->assume(AssertionVal.castAs());
+if (StTrue)
+  return report(C, StTrue, "CAN_BE_TRUE");
+return false;
+  }
+
+public:
+  bool evalCall(const CallEvent &Call, CheckerContext &C) const {
+if (const HandlerFn *Callback = Callbacks.lookup(Call))
+  return (this->*(*Callback))(Call, C);
+return false;
+  }
+};
+
+void addFalsePositiveGenerator(AnalysisASTConsumer &AnalysisConsumer,
+   AnalyzerOptions &AnOpts) {
+  AnOpts.CheckersAndPackages = {{"test.FalsePositiveGenerator", true},
+{"debug.ViewExplodedGraph", false}};
+  AnalysisConsumer.AddCheckerRegistrationFn([](CheckerRegistry &Registry) {
+Registry.addChecker(
+"test.FalsePositiveGenerator", "EmptyDescription", "EmptyDocsUri");
+  });
+}
+
+// C++20 use constexpr below.
+const std::vector La

[PATCH] D78457: [analyzer][Z3-refutation] Fix a refutation BugReporterVisitor bug

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

steakhal updated this revision to Diff 260108.
steakhal retitled this revision from "[analyzer][Z3-refutation] Fix refutation 
BugReporterVisitor bug and refactor" to "[analyzer][Z3-refutation] Fix a 
refutation BugReporterVisitor bug".
steakhal edited the summary of this revision.
steakhal added a comment.

- fix patch summary
- fix patch title
- renamed function parameter to `OverwriteConstraintsOnExistingSyms`
- fixed `x+y` to `x+n` in the test comment


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

https://reviews.llvm.org/D78457

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp

Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -170,6 +170,54 @@
 "test.FalsePositiveGenerator:CAN_BE_TRUE\n");
 }
 
+TEST(FalsePositiveRefutationBRVisitor,
+ UnSatAtErrorNodeDueToRefinedConstraintNoReport) {
+  SKIP_WITHOUT_Z3;
+  constexpr auto Code = R"(
+void reportIfCanBeTrue(bool);
+void reachedWithNoContradiction();
+void test(unsigned x, unsigned n) {
+  if (n >= 1 && n <= 2) {
+if (x >= 3)
+  return;
+// x: [0,2] and n: [1,2]
+int y = x + n; // y: '(x+n)' Which is in approximately between 1 and 4.
+
+// Registers the symbol 'y' with the constraint [1, MAX] in the true
+// branch.
+if (y > 0) {
+  // Since the x: [0,2] and n: [1,2], the 'y' is indeed greater than
+  // zero. If we emit a warning here, the constraints on the BugPath is
+  // SAT. Therefore that report is NOT invalidated.
+  reachedWithNoContradiction(); // 'y' can be greater than zero. OK
+
+  // If we ask the analyzer whether the 'y' can be 5. It won't know,
+  // therefore, the state will be created where the 'y' expression is 5.
+  // Although, this assumption is false!
+  // 'y' can not be 5 if the maximal value of both x and n is 2.
+  // The BugPath which become UnSAT in the ErrorNode with a refined
+  // constraint, should be invalidated.
+  reportIfCanBeTrue(y == 5);
+}
+  }
+})";
+
+  std::string Diags;
+  EXPECT_TRUE(runCheckerOnCodeWithArgs(
+  Code, LazyAssumeAndCrossCheckArgs, Diags));
+  EXPECT_EQ(Diags,
+"test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n");
+  // Single warning. The second report was invalidated by the visitor.
+
+  // Without enabling the crosscheck-with-z3 both reports are displayed.
+  std::string Diags2;
+  EXPECT_TRUE(runCheckerOnCodeWithArgs(
+  Code, LazyAssumeArgs, Diags2));
+  EXPECT_EQ(Diags2,
+"test.FalsePositiveGenerator:REACHED_WITH_NO_CONTRADICTION\n"
+"test.FalsePositiveGenerator:CAN_BE_TRUE\n");
+}
+
 } // namespace
 } // namespace ento
 } // namespace clang
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2820,7 +2820,7 @@
 BugReporterContext &BRC, const ExplodedNode *EndPathNode,
 PathSensitiveBugReport &BR) {
   // Collect new constraints
-  VisitNode(EndPathNode, BRC, BR);
+  addConstraints(EndPathNode, /*OverwriteConstraintsOnExistingSyms=*/true);
 
   // Create a refutation manager
   llvm::SMTSolverRef RefutationSolver = llvm::CreateZ3Solver();
@@ -2831,30 +2831,30 @@
 const SymbolRef Sym = I.first;
 auto RangeIt = I.second.begin();
 
-llvm::SMTExprRef Constraints = SMTConv::getRangeExpr(
+llvm::SMTExprRef SMTConstraints = SMTConv::getRangeExpr(
 RefutationSolver, Ctx, Sym, RangeIt->From(), RangeIt->To(),
 /*InRange=*/true);
 while ((++RangeIt) != I.second.end()) {
-  Constraints = RefutationSolver->mkOr(
-  Constraints, SMTConv::getRangeExpr(RefutationSolver, Ctx, Sym,
- RangeIt->From(), RangeIt->To(),
- /*InRange=*/true));
+  SMTConstraints = RefutationSolver->mkOr(
+  SMTConstraints, SMTConv::getRangeExpr(RefutationSolver, Ctx, Sym,
+RangeIt->From(), RangeIt->To(),
+/*InRange=*/true));
 }
 
-RefutationSolver->addConstraint(Constraints);
+RefutationSolver->addConstraint(SMTConstraints);
   }
 
   // And check for satisfiability
-  Optional isSat = RefutationSolver->check();
-  if (!isSat.hasValue())
+  Optional IsSAT = Refu

[PATCH] D78457: [analyzer][Z3-refutation] Fix a refutation BugReporterVisitor bug

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

steakhal added a comment.

I'm planning to measure how impactful this patch is using the CodeChecker.
Stay tuned if you are interested!


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

https://reviews.llvm.org/D78457



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

[PATCH] D84979: [analyzer][NFC] Refine CStringLength modeling API

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

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

Refines the CStringLength modeling API to be pure.
Also removes the magic //hypothetical// parameter from the CStringChecker.

Now the getters and setters are behaving as expected.
This is enforced by taking the State as `const` pointer.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84979

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.h
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp
@@ -78,16 +78,11 @@
   return State->remove(MR);
 }
 
-static SVal getCStringLengthForRegion(CheckerContext &Ctx,
-  ProgramStateRef &State, const Expr *Ex,
-  const MemRegion *MR, bool Hypothetical) {
-  if (!Hypothetical) {
-// If there's a recorded length, go ahead and return it.
-if (const SVal *Recorded = State->get(MR))
-  return *Recorded;
-  }
+NonLoc cstring::createCStringLength(ProgramStateRef &State, CheckerContext &Ctx,
+const Expr *Ex, const MemRegion *MR) {
+  assert(Ex);
+  assert(MR);
 
-  // Otherwise, get a new symbol and update the state.
   SValBuilder &SVB = Ctx.getSValBuilder();
   QualType SizeTy = SVB.getContext().getSizeType();
   NonLoc CStrLen =
@@ -95,25 +90,22 @@
Ctx.getLocationContext(), Ctx.blockCount())
   .castAs();
 
-  if (!Hypothetical) {
-// In case of unbounded calls strlen etc bound the range to SIZE_MAX/4
-BasicValueFactory &BVF = SVB.getBasicValueFactory();
-const llvm::APSInt &MaxValue = BVF.getMaxValue(SizeTy);
-const llvm::APSInt Four = APSIntType(MaxValue).getValue(4);
-const llvm::APSInt *MaxLength = BVF.evalAPSInt(BO_Div, MaxValue, Four);
-const NonLoc MaxLengthSVal = SVB.makeIntVal(*MaxLength);
-SVal Constrained =
-SVB.evalBinOpNN(State, BO_LE, CStrLen, MaxLengthSVal, SizeTy);
-State = State->assume(Constrained.castAs(), true);
-State = State->set(MR, CStrLen);
-  }
-
+  // Implicitly constrain the range to SIZE_MAX/4
+  BasicValueFactory &BVF = SVB.getBasicValueFactory();
+  const llvm::APSInt &MaxValue = BVF.getMaxValue(SizeTy);
+  const llvm::APSInt Four = APSIntType(MaxValue).getValue(4);
+  const llvm::APSInt *MaxLength = BVF.evalAPSInt(BO_Div, MaxValue, Four);
+  const NonLoc MaxLengthSVal = SVB.makeIntVal(*MaxLength);
+  SVal Constrained =
+  SVB.evalBinOpNN(State, BO_LE, CStrLen, MaxLengthSVal, SizeTy);
+  State = State->assume(Constrained.castAs(), true);
+  State = State->set(MR, CStrLen);
   return CStrLen;
 }
 
-SVal cstring::getCStringLength(CheckerContext &Ctx, ProgramStateRef &State,
-   const Expr *Ex, SVal Buf,
-   bool Hypothetical /*=false*/) {
+Optional cstring::getCStringLength(CheckerContext &Ctx,
+ const ProgramStateRef &State,
+ SVal Buf) {
   if (Buf.isUnknownOrUndef())
 return Buf;
 
@@ -145,7 +137,9 @@
   case MemRegion::ParamVarRegionKind:
   case MemRegion::FieldRegionKind:
   case MemRegion::ObjCIvarRegionKind:
-return getCStringLengthForRegion(Ctx, State, Ex, MR, Hypothetical);
+if (const SVal *RecordedLength = State->get(MR))
+  return *RecordedLength;
+return llvm::None;
   case MemRegion::CompoundLiteralRegionKind:
 // FIXME: Can we track this? Is it necessary?
 return UnknownVal();
@@ -159,7 +153,7 @@
   }
 }
 
-void cstring::dumpCStringLengths(ProgramStateRef State, raw_ostream &Out,
+void cstring::dumpCStringLengths(const ProgramStateRef State, raw_ostream &Out,
  const char *NL, const char *Sep) {
   const CStringLengthMapTy Items = State->get();
   if (!Items.isEmpty())
@@ -212,7 +206,7 @@
 SVal StrVal = C.getSVal(Init);
 assert(StrVal.isValid() && "Initializer string is unknown or undefined");
 DefinedOrUnknownSVal strLength =
-getCStringLength(C, state, Init, StrVal).castAs();
+getCStringLength(C, state, StrVal)->castAs();
 
 state = state->set(MR, strLength);
   }
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h
=

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

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

steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43
+ ProgramStateRef &State, const Expr *Ex,
+ SVal Buf, bool Hypothetical = false);
+

steakhal wrote:
> steakhal wrote:
> > balazske wrote:
> > > I do not like that the //get// and //set// (CStringLength) functions are 
> > > not symmetrical. I (and other developers) would think that the get 
> > > function returns a stored value and the set function sets it. The 
> > > `getCStringLength` is more a `computeCStringLength` and additionally may 
> > > manipulate the `State` too. In this form it is usable mostly only for 
> > > CStringChecker. (A separate function to get the value stored in the 
> > > length map should exist instead of this `Hypothetical` thing.)
> > > [...] get function returns a stored value and the set function sets it.
> > Certainly a burden to understand. It would be more appealing, but more 
> > useful?
> > The user would have to check and create if necessary regardless. So fusing 
> > these two functions is more like a feature.
> > What use case do you think of using only the query function? In other 
> > words, how can you guarantee that you will find a length for a symbol?
> > 
> > > In this form it is usable mostly only for CStringChecker. (A separate 
> > > function to get the value stored in the length map should exist instead 
> > > of this Hypothetical thing.)
> > You are right. However, I want to focus on splitting parts without 
> > modifying the already existing API reducing the risk of breaking things.
> > You should expect such a change in an upcoming patch.
> On second thought, It probably worth having a cleaner API to a slight 
> inconvenience. If he feels like, still can wrap them.
> I will investigate it tomorrow.
I made a separate patch for cleansing this API.
In the D84979 now these API functions will behave as expected.


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

https://reviews.llvm.org/D84316

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

[PATCH] D84929: [analyzer] Fix out-of-tree only clang build by not relaying on private header

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG63d3aeb529a7: [analyzer] Fix out-of-tree only clang build by 
not relaying on private header (authored by steakhal).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84929

Files:
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
  llvm/include/llvm/Config/config.h.cmake
  llvm/include/llvm/Config/llvm-config.h.cmake


Index: llvm/include/llvm/Config/llvm-config.h.cmake
===
--- llvm/include/llvm/Config/llvm-config.h.cmake
+++ llvm/include/llvm/Config/llvm-config.h.cmake
@@ -79,6 +79,9 @@
  */
 #cmakedefine01 LLVM_FORCE_ENABLE_STATS
 
+/* Define if we have z3 and want to build it */
+#cmakedefine LLVM_WITH_Z3 ${LLVM_WITH_Z3}
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic 
library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -1,6 +1,9 @@
 #ifndef CONFIG_H
 #define CONFIG_H
 
+// Include this header only under the llvm source tree.
+// This is a private header.
+
 /* Exported configuration */
 #include "llvm/Config/llvm-config.h"
 
@@ -335,9 +338,6 @@
 /* Whether GlobalISel rule coverage is being collected */
 #cmakedefine01 LLVM_GISEL_COV_ENABLED
 
-/* Define if we have z3 and want to build it */
-#cmakedefine LLVM_WITH_Z3 ${LLVM_WITH_Z3}
-
 /* Define to the default GlobalISel coverage file prefix */
 #cmakedefine LLVM_GISEL_COV_PREFIX "${LLVM_GISEL_COV_PREFIX}"
 
Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -16,7 +16,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
-#include "llvm/Config/config.h"
+#include "llvm/Config/llvm-config.h"
 #include "gtest/gtest.h"
 
 // FIXME: Use GTEST_SKIP() instead if GTest is updated to version 1.10.0


Index: llvm/include/llvm/Config/llvm-config.h.cmake
===
--- llvm/include/llvm/Config/llvm-config.h.cmake
+++ llvm/include/llvm/Config/llvm-config.h.cmake
@@ -79,6 +79,9 @@
  */
 #cmakedefine01 LLVM_FORCE_ENABLE_STATS
 
+/* Define if we have z3 and want to build it */
+#cmakedefine LLVM_WITH_Z3 ${LLVM_WITH_Z3}
+
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
Index: llvm/include/llvm/Config/config.h.cmake
===
--- llvm/include/llvm/Config/config.h.cmake
+++ llvm/include/llvm/Config/config.h.cmake
@@ -1,6 +1,9 @@
 #ifndef CONFIG_H
 #define CONFIG_H
 
+// Include this header only under the llvm source tree.
+// This is a private header.
+
 /* Exported configuration */
 #include "llvm/Config/llvm-config.h"
 
@@ -335,9 +338,6 @@
 /* Whether GlobalISel rule coverage is being collected */
 #cmakedefine01 LLVM_GISEL_COV_ENABLED
 
-/* Define if we have z3 and want to build it */
-#cmakedefine LLVM_WITH_Z3 ${LLVM_WITH_Z3}
-
 /* Define to the default GlobalISel coverage file prefix */
 #cmakedefine LLVM_GISEL_COV_PREFIX "${LLVM_GISEL_COV_PREFIX}"
 
Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -16,7 +16,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Frontend/AnalysisConsumer.h"
 #include "clang/StaticAnalyzer/Frontend/CheckerRegistry.h"
-#include "llvm/Config/config.h"
+#include "llvm/Config/llvm-config.h"
 #include "gtest/gtest.h"
 
 // FIXME: Use GTEST_SKIP() instead if GTest is updated to version 1.10.0
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84979: [analyzer][NFC] Refine CStringLength modeling API

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

steakhal added a comment.

It seems the Pre-merge clang-format went crazy.
I think just ignore the lint warnings for the unchanged lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84979

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

[PATCH] D85026: [analyzer] Minor refactoring of SVal::getSubKind function

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

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

I like the change.
I also proved the equivalence, just for fun really :D

  from z3 import *
  
  def proof_equality(F, G):
s = Solver()
s.add(Not(F == G))
r = s.check()
if r == unsat:
  print("proved")
else:
  print("counterexample")
  print(s.model())
  
  
  Kind = BitVec('Kind', 32)
  BaseMask = BitVecVal(0b11, 32)
  BaseBits = BitVecVal(2, 32)
  
  Before = (Kind & ~BaseMask) >> BaseBits
  After = Kind >> BaseBits
  
  proof_equality(Before, After)
  # prints: proved 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85026

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

[PATCH] D85034: [analyzer] Simplified functions SVal::getAsSymbolicExpression and similar ones

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

steakhal resigned from this revision.
steakhal added a comment.
Herald added a subscriber: steakhal.

It would be great to simplify these. I have also wondered once why those 
different functions exist.
Does anyone know what was the original intention of these functions?
Since I'm not confident in this area, I resign.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85034

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

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

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

steakhal added a comment.

In D84316#2187372 , @19n07u5 wrote:

> The title is a little bit confusing because only the C-string size model is 
> going to be separated and be accessible.

Could you elaborate on why is the title not precise?
It seems that the modeling logic and the reporting logic will be separated:

- modeling will be implemented in `CStringLengthModeling.cpp`
- reporting will be implemented in `CStringChecker.cpp` (just as like it was 
before)

I just wanted a short (at most 80 char long) title, if you offer any better I 
would be pleased.

---

> Other than that as @NoQ pointed out we need lot more of these 
> common-API-separation patches. It is a great starting point for the 
> `CStringChecker`.

Thanks. I'm thinking about making the checker cleaner - we will see.




Comment at: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt:146
+  CStringChecker
+  )

19n07u5 wrote:
> Other common checker functionality folders and headers do not require extra 
> CMake support long ago. I think when we need such support, we could define it 
> later, so that you could revert this.
It would be easier to use the `CStringLength.h` header without specifying the 
complete path to it.
IMO `#include "CStringChecker/CStringLength.h"` is kindof verbose compared to 
simply using `#include "CStringLength.h"`.
As of now, I'm sticking to this.



Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h:43
+ ProgramStateRef &State, const Expr *Ex,
+ SVal Buf, bool Hypothetical = false);
+

19n07u5 wrote:
> steakhal wrote:
> > steakhal wrote:
> > > steakhal wrote:
> > > > balazske wrote:
> > > > > I do not like that the //get// and //set// (CStringLength) functions 
> > > > > are not symmetrical. I (and other developers) would think that the 
> > > > > get function returns a stored value and the set function sets it. The 
> > > > > `getCStringLength` is more a `computeCStringLength` and additionally 
> > > > > may manipulate the `State` too. In this form it is usable mostly only 
> > > > > for CStringChecker. (A separate function to get the value stored in 
> > > > > the length map should exist instead of this `Hypothetical` thing.)
> > > > > [...] get function returns a stored value and the set function sets 
> > > > > it.
> > > > Certainly a burden to understand. It would be more appealing, but more 
> > > > useful?
> > > > The user would have to check and create if necessary regardless. So 
> > > > fusing these two functions is more like a feature.
> > > > What use case do you think of using only the query function? In other 
> > > > words, how can you guarantee that you will find a length for a symbol?
> > > > 
> > > > > In this form it is usable mostly only for CStringChecker. (A separate 
> > > > > function to get the value stored in the length map should exist 
> > > > > instead of this Hypothetical thing.)
> > > > You are right. However, I want to focus on splitting parts without 
> > > > modifying the already existing API reducing the risk of breaking things.
> > > > You should expect such a change in an upcoming patch.
> > > On second thought, It probably worth having a cleaner API to a slight 
> > > inconvenience. If he feels like, still can wrap them.
> > > I will investigate it tomorrow.
> > I made a separate patch for cleansing this API.
> > In the D84979 now these API functions will behave as expected.
> > I (and other developers) would think that the get function returns a stored 
> > value and the set function sets it.
> Developers should not believe the getters are pure getters. As a 
> checker-writer point of view, you do not care whether the C-string already 
> exist or the checker creates it during symbolic execution, you only want to 
> get the C-string.
> 
> Think about all the Static Analyzer getters as factory functions, that is the 
> de facto standard now. For example, when you are trying to get a symbolic 
> value with `getSVal()`, for the first occurrence of an expression no `SVal` 
> exist, so it also creates it.
> 
> With that in mind, @steakhal, could you partially revert the renaming related 
> refactors of D84979, please?
> [...] As a checker-writer point of view, you do not care whether the C-string 
> already exist or the checker creates it during symbolic execution, you only 
> want to get the C-string.
I would have agreed with you - before I made the D84979 patch.
Now I believe if the interface can be implemented //purely// then it should be 
done so.

> Think about all the Static Analyzer getters as factory functions, that is the 
> de facto standard now.
We can always change them.

> For example, when you are trying to get a symbolic value with `getSVal()`, 
> for the first occurrence of an expression no `SVal` exist, so it also creates 
> it.
I'm not really familiar with the interna

[PATCH] D84736: [analyzer][RFC] Handle pointer difference of ElementRegion and SymbolicRegion

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

steakhal planned changes to this revision.
steakhal marked an inline comment as done.
steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1035-1038
+return evalBinOpNN(state, BO_Mul, Index,
+   makeArrayIndex(SingleElementSize.getQuantity()),
+   ArrayIndexTy)
+.castAs();

vsavchenko wrote:
> I would prefer having a comment for this line with a very simple formula of 
> the thing we are calculating.  I think it can increase the readability of the 
> code because when there is a call accepting a whole bunch of arguments it is 
> never obvious and it always takes time to parse through.
Sure, I will add something like this:
```
// T buf[n];   Element{buf,2} --> 2 * sizeof(T)
```



Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1041
+
+  // If both has the same memory region, and the index has a concrete 
value,
+  // we can evaluate equality operators.

vsavchenko wrote:
> This comment is a bit deceiving IMO.  It returns a concrete value even when 
> `HasSameMemRegions` is false, but from the comment it seems like we evaluate 
> the operator ONLY when `HasSameMemRegions` is true.
No, the comment is up to date.
This lambda handles both equality and inequality operators, and this is why its 
called EvaluateEqualityOperators.

EQResult will be true only if the two memory regions are the same and the index 
is zero.
The result of the **inequality**  is just the negated version of the result of 
equality.



Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1068
+}
+  } else if (LeftER && !RightER) {
+NonLoc LeftIndex = ByteOffsetOfElement(LeftER);

vsavchenko wrote:
> I think it's better to add comments for each case describing what is the 
> situation we handle here in a simplified form.
> And for each return within the case - a short comment with the reason.
I'm not sure if it's necessary.
I have already described these 3 cases at the beginning.
Quote:
```
// Handle: \forall MemRegion X, \forall NonLoc n, m:
//  - Element{X,n} OP Element{X,m}
//  - Element{X,n} OP X
//  -X OP Element{X,n}
// We don't handle here nested ElementRegions like in the this expression:
// Element{Element{x,3,int [10]},5,int}  ==  Element{x,35,int}
```
If you still insist, I will defenately add more comments though.


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

https://reviews.llvm.org/D84736

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

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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

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

We ignored the cast if the enum was **scoped**.
This is bad since there is **no implicit conversion** from the scoped enum 
**to** the corresponding **underlying type**.

This materialized in crashes on analyzing the LLVM itself using the Z3 
refutation.
Refutation synthesized the given Z3 expression with the wrong bitwidth in the 
end.

Now, we evaluate the cast according to the standard.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85528

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/test/Analysis/z3-refute-enum-crash.cpp


Index: clang/test/Analysis/z3-refute-enum-crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/z3-refute-enum-crash.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config crosscheck-with-z3=true -verify %s
+
+void clang_analyzer_warnIfReached();
+
+using sugar_t = unsigned char;
+enum class ScopedSugared : sugar_t {};
+enum class ScopedPrimitive : unsigned char {};
+enum UnscopedSugared : sugar_t {};
+enum UnscopedPrimitive : unsigned char {};
+
+ScopedSugared conjure_scoped_enum_with_sugar_type();
+ScopedPrimitive conjure_scoped_enum_with_primitive_type();
+UnscopedSugared conjure_unscoped_enum_with_sugar_type();
+UnscopedPrimitive conjure_unscoped_enum_with_primitive_type();
+
+void test() {
+  auto var1 = conjure_scoped_enum_with_sugar_type();
+  auto var2 = conjure_scoped_enum_with_primitive_type();
+  auto var3 = conjure_unscoped_enum_with_sugar_type();
+  auto var4 = conjure_unscoped_enum_with_primitive_type();
+
+  int sym1 = static_cast(var1) & 0x0F;
+  if (sym1)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+
+  int sym2 = static_cast(var2) & 0x0F;
+  if (sym2)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+
+  int sym3 = static_cast(var3) & 0x0F;
+  if (sym3)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+
+  int sym4 = static_cast(var4) & 0x0F;
+  if (sym4)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -95,11 +95,22 @@
   }
 
   bool haveSameType(QualType Ty1, QualType Ty2) {
+const auto IsIntegralOrUnscopedCompleteEnumerationType = [](QualType Ty) {
+  const Type *CanonicalType = Ty.getCanonicalType().getTypePtr();
+  if (const auto *ET = dyn_cast(CanonicalType))
+return ET->getDecl()->isComplete() && !ET->getDecl()->isScoped();
+
+  return Ty->isIntegralOrEnumerationType();
+};
+
 // FIXME: Remove the second disjunct when we support symbolic
 // truncation/extension.
-return (Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2) ||
-(Ty1->isIntegralOrEnumerationType() &&
- Ty2->isIntegralOrEnumerationType()));
+const bool BothHaveSameCanonicalTypes =
+Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2);
+const bool BothHaveIntegralLikeTypes =
+IsIntegralOrUnscopedCompleteEnumerationType(Ty1) &&
+IsIntegralOrUnscopedCompleteEnumerationType(Ty2);
+return BothHaveSameCanonicalTypes || BothHaveIntegralLikeTypes;
   }
 
   SVal evalCast(SVal val, QualType castTy, QualType originalType);


Index: clang/test/Analysis/z3-refute-enum-crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/z3-refute-enum-crash.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config crosscheck-with-z3=true -verify %s
+
+void clang_analyzer_warnIfReached();
+
+using sugar_t = unsigned char;
+enum class ScopedSugared : sugar_t {};
+enum class ScopedPrimitive : unsigned char {};
+enum UnscopedSugared : sugar_t {};
+enum UnscopedPrimitive : unsigned char {};
+
+ScopedSugared conjure_scoped_enum_with_sugar_type();
+ScopedPrimitive conjure_scoped_enum_with_primitive_type();
+UnscopedSugared conjure_unscoped_enum_with_sugar_type();
+UnscopedPrimitive conjure_unscoped_enum_with_primitive_type();
+
+void test() {
+  auto var1 = conjure_scoped_enum_with_sugar_type();
+  auto var2 = conjure_scoped_enum_with_primitive_type();
+  auto var3 = conjure_unscoped_enum_with_sugar_type();
+  auto var4 = conjure_unscoped_enum_with_pr

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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

steakhal planned changes to this revision.
steakhal added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:98-104
+const auto IsIntegralOrUnscopedCompleteEnumerationType = [](QualType Ty) {
+  const Type *CanonicalType = Ty.getCanonicalType().getTypePtr();
+  if (const auto *ET = dyn_cast(CanonicalType))
+return ET->getDecl()->isComplete() && !ET->getDecl()->isScoped();
+
+  return Ty->isIntegralOrEnumerationType();
+};

vsavchenko wrote:
> I don't really see any reasons why is this a lambda and not a free function
It's somewhat domain-specific that we require:
 - `bool-uint128` builtin types
 - complete, scoped `enum` types ((I think we need completeness for the 
analysis))

---
The one that comes quite close to these requirements was the 
`isIntegralOrEnumerationType`, but that does not check if the enum is 
//unscoped//.

We can extend the `Type` header with this.
Should we go on that route?



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:101
+  if (const auto *ET = dyn_cast(CanonicalType))
+return ET->getDecl()->isComplete() && !ET->getDecl()->isScoped();
+

vsavchenko wrote:
> I think it's better to add tests for this condition.
You are right, I will add such cases as well.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:106-107
+
 // FIXME: Remove the second disjunct when we support symbolic
 // truncation/extension.
+const bool BothHaveSameCanonicalTypes =

vsavchenko wrote:
> Maybe this comment should be moved closer to the `return` statement?
Yes, I will move it closer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85528

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

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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

steakhal updated this revision to Diff 283936.
steakhal added a comment.

- Moved the FIXME closer to the subject.
- Added tests for covering incomplete enums as well.
- Added `REQUIRES: z3`. This will mark the test case `unsupported` on every 
buildbots. See my notes about this behavior at D83677 
.
- Refined test file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85528

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/test/Analysis/z3-refute-enum-crash.cpp

Index: clang/test/Analysis/z3-refute-enum-crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/z3-refute-enum-crash.cpp
@@ -0,0 +1,70 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config crosscheck-with-z3=true -verify %s
+// REQUIRES: z3
+//
+// Requires z3 only for refutation. Works with both constraint managers.
+
+void clang_analyzer_warnIfReached();
+
+using sugar_t = unsigned char;
+
+// Complete enum types
+enum class ScopedSugaredComplete : sugar_t {};
+enum class ScopedPrimitiveComplete : unsigned char {};
+enum UnscopedSugaredComplete : sugar_t {};
+enum UnscopedPrimitiveComplete : unsigned char {};
+
+// Incomplete enum types
+enum class ScopedSugaredIncomplete : sugar_t;
+enum class ScopedPrimitiveIncomplete : unsigned char;
+enum UnscopedSugaredIncomplete : sugar_t;
+enum UnscopedPrimitiveIncomplete : unsigned char;
+
+template 
+T conjure();
+
+void test_complete_enum_types() {
+  auto var1 = conjure();
+  auto var2 = conjure();
+  auto var3 = conjure();
+  auto var4 = conjure();
+
+  int sym1 = static_cast(var1) & 0x0F;
+  if (sym1)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+
+  int sym2 = static_cast(var2) & 0x0F;
+  if (sym2)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+
+  int sym3 = static_cast(var3) & 0x0F;
+  if (sym3)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+
+  int sym4 = static_cast(var4) & 0x0F;
+  if (sym4)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+}
+
+void test_incomplete_enum_types() {
+  auto var1 = conjure();
+  auto var2 = conjure();
+  auto var3 = conjure();
+  auto var4 = conjure();
+
+  int sym1 = static_cast(var1) & 0x0F;
+  if (sym1)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+
+  int sym2 = static_cast(var2) & 0x0F;
+  if (sym2)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+
+  int sym3 = static_cast(var3) & 0x0F;
+  if (sym3)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+
+  int sym4 = static_cast(var4) & 0x0F;
+  if (sym4)
+clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}} no-crash
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -95,11 +95,22 @@
   }
 
   bool haveSameType(QualType Ty1, QualType Ty2) {
+const auto IsIntegralOrUnscopedCompleteEnumerationType = [](QualType Ty) {
+  const Type *CanonicalType = Ty.getCanonicalType().getTypePtr();
+  if (const auto *ET = dyn_cast(CanonicalType))
+return ET->getDecl()->isComplete() && !ET->getDecl()->isScoped();
+  return Ty->isIntegralOrEnumerationType();
+};
+
+const bool BothHaveSameCanonicalTypes =
+Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2);
+const bool BothHaveIntegralLikeTypes =
+IsIntegralOrUnscopedCompleteEnumerationType(Ty1) &&
+IsIntegralOrUnscopedCompleteEnumerationType(Ty2);
+
 // FIXME: Remove the second disjunct when we support symbolic
 // truncation/extension.
-return (Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2) ||
-(Ty1->isIntegralOrEnumerationType() &&
- Ty2->isIntegralOrEnumerationType()));
+return BothHaveSameCanonicalTypes || BothHaveIntegralLikeTypes;
   }
 
   SVal evalCast(SVal val, QualType castTy, QualType originalType);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85424: [Analyzer] Crash fix for alpha.cplusplus.IteratorRange

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

steakhal added a comment.

LGTM

In D85424#2199471 , 
@baloghadamsoftware wrote:

> Unfortunately, I could not create test for it. It is extremely rare that the 
> //Analyzer// creates an `UndefinedVal`.

You can work around the issue by creating a unit-test instead.
In that test, you could create a custom checker, which returns the required 
`Undefined` SVal of your desired type.

We need a test to approve this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85424

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

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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

steakhal updated this revision to Diff 284126.
steakhal marked 3 inline comments as done.
steakhal edited the summary of this revision.
steakhal added a comment.

- Using `dump`s instead of `reaching` in tests.
- Not requiring complete enums anymore //(unlike we did before the patch)//.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85528

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/test/Analysis/z3-refute-enum-crash.cpp


Index: clang/test/Analysis/z3-refute-enum-crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/z3-refute-enum-crash.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config crosscheck-with-z3=true -verify %s
+// REQUIRES: z3
+//
+// Requires z3 only for refutation. Works with both constraint managers.
+
+void clang_analyzer_dump(int);
+
+using sugar_t = unsigned char;
+
+// Enum types
+enum class ScopedSugared : sugar_t {};
+enum class ScopedPrimitive : unsigned char {};
+enum UnscopedSugared : sugar_t {};
+enum UnscopedPrimitive : unsigned char {};
+
+template 
+T conjure();
+
+void test_enum_types() {
+  int sym1 = static_cast(conjure()) & 0x0F;
+  int sym2 = static_cast(conjure()) & 0x0F;
+  int sym3 = static_cast(conjure()) & 0x0F;
+  int sym4 = static_cast(conjure()) & 0x0F;
+
+  if (sym1 && sym2 && sym3 && sym4) {
+// no-crash on these dumps
+clang_analyzer_dump(sym1); // expected-warning{{((unsigned char) (conj_}}
+clang_analyzer_dump(sym2); // expected-warning{{((unsigned char) (conj_}}
+clang_analyzer_dump(sym3); // expected-warning{{(conj_}}
+clang_analyzer_dump(sym4); // expected-warning{{(conj_}}
+  }
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -95,11 +95,15 @@
   }
 
   bool haveSameType(QualType Ty1, QualType Ty2) {
+const bool BothHaveSameCanonicalTypes =
+Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2);
+const bool BothHaveIntegralOrUnscopedEnumerationType =
+Ty1->isIntegralOrUnscopedEnumerationType() &&
+Ty2->isIntegralOrUnscopedEnumerationType();
 // FIXME: Remove the second disjunct when we support symbolic
 // truncation/extension.
-return (Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2) ||
-(Ty1->isIntegralOrEnumerationType() &&
- Ty2->isIntegralOrEnumerationType()));
+return BothHaveSameCanonicalTypes ||
+   BothHaveIntegralOrUnscopedEnumerationType;
   }
 
   SVal evalCast(SVal val, QualType castTy, QualType originalType);


Index: clang/test/Analysis/z3-refute-enum-crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/z3-refute-enum-crash.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config crosscheck-with-z3=true -verify %s
+// REQUIRES: z3
+//
+// Requires z3 only for refutation. Works with both constraint managers.
+
+void clang_analyzer_dump(int);
+
+using sugar_t = unsigned char;
+
+// Enum types
+enum class ScopedSugared : sugar_t {};
+enum class ScopedPrimitive : unsigned char {};
+enum UnscopedSugared : sugar_t {};
+enum UnscopedPrimitive : unsigned char {};
+
+template 
+T conjure();
+
+void test_enum_types() {
+  int sym1 = static_cast(conjure()) & 0x0F;
+  int sym2 = static_cast(conjure()) & 0x0F;
+  int sym3 = static_cast(conjure()) & 0x0F;
+  int sym4 = static_cast(conjure()) & 0x0F;
+
+  if (sym1 && sym2 && sym3 && sym4) {
+// no-crash on these dumps
+clang_analyzer_dump(sym1); // expected-warning{{((unsigned char) (conj_}}
+clang_analyzer_dump(sym2); // expected-warning{{((unsigned char) (conj_}}
+clang_analyzer_dump(sym3); // expected-warning{{(conj_}}
+clang_analyzer_dump(sym4); // expected-warning{{(conj_}}
+  }
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -95,11 +95,15 @@
   }
 
   bool haveSameType(QualType Ty1, QualType Ty2) {
+const bool BothHaveSameCanonicalTypes =
+Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2);
+const bool BothHaveIntegralOrUnscopedEnumerationType =
+Ty1->isIntegralOrUnscopedEnumerationType() &&
+Ty2->isIntegralOrUnscopedEnumerationType();
 // FIXME: Remove the second disjunct when we support symbolic
 // truncation/extension.
-return (Context.getCa

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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

steakhal added a comment.

In D85528#2203325 , @NoQ wrote:

> Aha, ok, sounds like the right thing to do. Like, for Z3 it's actually the 
> wrong thing to do (you'd prefer to evaluate the cast perfectly by adding 
> `SymbolCast`) but for pure RangeConstraintManager this is the lesser of two 
> evils.

My primary objective is to fix all the crashes related to //Range CM + Z3 
refutation//.

> Because this patch changes the behavior of regular analysis (without Z3), i 
> expect tests to reflect that.

What do you expect exactly?

`REQUIRES: z3` is necessary for the refutation.
However, adding this requirement would not mean that this test will run if you 
have Z3 installed though.
You should add the extra `llvm-lit` param to enable such tests.
I don't want to repeat myself too much but D83677 
 describes all the details of this test infra 
fiasco.
I would appreciate some feedback there.

> Please add `ExprInspection`-based tests to test values produced by casts.

Ok, I fixed that - thanks.

> I don't understand why should the behavior be different for incomplete types. 
> Can you explain?

You should be right. Fixed that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85528

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

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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

steakhal added a comment.

In D85528#2205094 , @xazax.hun wrote:

> Looks reasonable to me, but I am not very familiar with the impacts of the 
> additional casts. Do we lose some modeling power when we are using the 
> regular constraint solver?
>
> For example, when we have constraints both on the original and the cased 
> symbol can the analyzer "merge" them?
>
> Something like:
>
>   ScopedPrimitive sym = conjure();
>   if (sym == ScopedPrimitive::Max)
> return;
>   int sym2 = static_cast(sym);
>   if (sym2 == 0)
> return;
>   // Do we know here that both sym and sym2 has the same range?
>   // Is there a change in the behavior compared to before the patch?

Huh, it's a really interesting question.
Here is the result:

---

Here is the test code:

  enum class ScopedPrimitive : unsigned char { Min = 2, Max = 8 };
  void foo() {
auto sym = conjure();
if (sym == ScopedPrimitive::Max)
  return;
  
int sym2 = static_cast(sym);
if (sym2 == 0)
  return;
  
// Do we know here that both sym and sym2 has the same range?
// Is there a change in the behavior compared to before the patch?
clang_analyzer_printState();
(void)sym;
(void)sym2;
  }

Before the patch:

  "constraints": [
  { "symbol": "conj_$2{enum ScopedPrimitive, LC1, S1083, #1}", "range": "{ 
[1, 7], [9, 255] }" }
]

After the patch:

  "constraints": [
  { "symbol": "conj_$2{enum ScopedPrimitive, LC1, S1881, #1}", "range": "{ 
[0, 7], [9, 255] }" },
  { "symbol": "(unsigned char) (conj_$2{enum ScopedPrimitive, LC1, S1881, 
#1})", "range": "{ [1, 255] }" }
]



---

> For example, when we have constraints both on the original and the cased 
> symbol can the analyzer "merge" them?

Apparently, not xD.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85528

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

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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

steakhal added a comment.

I mean, this shouldn't be an issue. Since we already omitted the 'unnecessary' 
cast expressions... That decision is the root cause of this, we should have 
expected that.

IMO we do the right thing here. If we want to treat sym and sym2 to refer to 
the same symbol, we should patch the CM to canonize, and remove such casts 
before storing the constraint.
But the cast symbols should exist for casting scoped enums.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85528

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

[PATCH] D81254: [analyzer] Produce symbolic values for C-array elements

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

steakhal added a comment.

In D81254#2122449 , @ASDenysPetrov 
wrote:

> @NoQ, thanks for the examples.
>
> I didn't get the first one. How do you get to the "//In reality we don't 
> know//", if we don't touch `a[index1]`:

If `index1` and `index2` has the **same** value, we should not be confident 
that the `x == y` holds.

>   void foo(int *a); // Potentially modifies elements of 'a'.
>   void fooRef(int *&a); // Potentially modifies elements of 'a'.
>   
>   void test2(int *a) {
> // a - &SymRegion{reg_$6}
> foo(a); // after this, CSA doesn't change a.
> // a - &SymRegion{reg_$6}
> fooRef(a); // after this, CSA changes a.
> // a - &SymRegion{conj_$10{int *, LC1, S925, #1}}
>   }

I don't think this is the right way expressing this.
AFAIK this means that we //don't// know where the resulting pointer points to - 
which is not true.
We know where it points to (aka. the pointer's value is conjured), but we don't 
know what the value if there.

Unfortunately, I don't know the solution to your problem, but I'm pretty 
confident that this is something else.
However, I'm really interested in where this discussion goes.

---

I feel like this problem should be handled by some sort of invalidation 
mechanism.
Take my comments on these with a pinch of salt though - I'm really not 
experienced in this :|




Comment at: clang/test/Analysis/PR9289.cpp:13-21
+int index_int(const int *a, int index) {
+  int var;
+  int ret = 0;
+  if (a[42] < 2)
+var = 1;
+  if (a[42] < 2)
+ret = var; // no warning about garbage value

Why do we test this?
Here we can //reason// about the index.
This patch preserves the current behavior relating to this.



Comment at: clang/test/Analysis/PR9289.cpp:36-40
+  if (a[index] < 2)
+var = 1;
+  index = index2;
+  if (a[index] < 2)
+ret = var; // expected-warning{{Assigned value is garbage or undefined 
[core.uninitialized.Assign]}}

I'm not sure why do you overwrite the `index` if you could simply index with 
`index2` instead in the following expression. That would make no difference, 
only make the test more readable IMO.
Same comment for all the`*_change` test cases.


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

https://reviews.llvm.org/D81254

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

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

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

steakhal updated this revision to Diff 286209.
steakhal marked an inline comment as done.
steakhal added a comment.

Add an extra `RUN` line without //refutation//.
The expected result is the same as with refutation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85528

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  clang/test/Analysis/z3-refute-enum-crash.cpp


Index: clang/test/Analysis/z3-refute-enum-crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/z3-refute-enum-crash.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config crosscheck-with-z3=true -verify %s
+//
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -verify %s
+//
+// REQUIRES: z3
+//
+// Requires z3 only for refutation. Works with both constraint managers.
+
+void clang_analyzer_dump(int);
+
+using sugar_t = unsigned char;
+
+// Enum types
+enum class ScopedSugared : sugar_t {};
+enum class ScopedPrimitive : unsigned char {};
+enum UnscopedSugared : sugar_t {};
+enum UnscopedPrimitive : unsigned char {};
+
+template 
+T conjure();
+
+void test_enum_types() {
+  int sym1 = static_cast(conjure()) & 0x0F;
+  int sym2 = static_cast(conjure()) & 0x0F;
+  int sym3 = static_cast(conjure()) & 0x0F;
+  int sym4 = static_cast(conjure()) & 0x0F;
+
+  if (sym1 && sym2 && sym3 && sym4) {
+// no-crash on these dumps
+clang_analyzer_dump(sym1); // expected-warning{{((unsigned char) (conj_}}
+clang_analyzer_dump(sym2); // expected-warning{{((unsigned char) (conj_}}
+clang_analyzer_dump(sym3); // expected-warning{{(conj_}}
+clang_analyzer_dump(sym4); // expected-warning{{(conj_}}
+  }
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -95,11 +95,15 @@
   }
 
   bool haveSameType(QualType Ty1, QualType Ty2) {
+const bool BothHaveSameCanonicalTypes =
+Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2);
+const bool BothHaveIntegralOrUnscopedEnumerationType =
+Ty1->isIntegralOrUnscopedEnumerationType() &&
+Ty2->isIntegralOrUnscopedEnumerationType();
 // FIXME: Remove the second disjunct when we support symbolic
 // truncation/extension.
-return (Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2) ||
-(Ty1->isIntegralOrEnumerationType() &&
- Ty2->isIntegralOrEnumerationType()));
+return BothHaveSameCanonicalTypes ||
+   BothHaveIntegralOrUnscopedEnumerationType;
   }
 
   SVal evalCast(SVal val, QualType castTy, QualType originalType);


Index: clang/test/Analysis/z3-refute-enum-crash.cpp
===
--- /dev/null
+++ clang/test/Analysis/z3-refute-enum-crash.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config crosscheck-with-z3=true -verify %s
+//
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -verify %s
+//
+// REQUIRES: z3
+//
+// Requires z3 only for refutation. Works with both constraint managers.
+
+void clang_analyzer_dump(int);
+
+using sugar_t = unsigned char;
+
+// Enum types
+enum class ScopedSugared : sugar_t {};
+enum class ScopedPrimitive : unsigned char {};
+enum UnscopedSugared : sugar_t {};
+enum UnscopedPrimitive : unsigned char {};
+
+template 
+T conjure();
+
+void test_enum_types() {
+  int sym1 = static_cast(conjure()) & 0x0F;
+  int sym2 = static_cast(conjure()) & 0x0F;
+  int sym3 = static_cast(conjure()) & 0x0F;
+  int sym4 = static_cast(conjure()) & 0x0F;
+
+  if (sym1 && sym2 && sym3 && sym4) {
+// no-crash on these dumps
+clang_analyzer_dump(sym1); // expected-warning{{((unsigned char) (conj_}}
+clang_analyzer_dump(sym2); // expected-warning{{((unsigned char) (conj_}}
+clang_analyzer_dump(sym3); // expected-warning{{(conj_}}
+clang_analyzer_dump(sym4); // expected-warning{{(conj_}}
+  }
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -95,11 +95,15 @@
   }
 
   bool haveSameType(QualType Ty1, QualType Ty2) {
+const bool BothHaveSameCanonicalTypes =
+Context.getCanonicalType(Ty1) == Context.getCanonicalType(Ty2);
+const bool BothHaveIntegralOrUnscopedEnumerationType =
+Ty1->isIntegralOrUnscopedEnumerationType() &&
+Ty2->isIntegralOrUnscopedEnume

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

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

steakhal added a comment.

Ping


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

https://reviews.llvm.org/D84316

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

[PATCH] D77792: [analyzer] Extend constraint manager to be able to compare simple SymSymExprs

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

steakhal abandoned this revision.
steakhal added a comment.

I no longer think that we should support Symbol to Symbol comparisons.
It would introduce certain anomalies, like //Why could the CM reason about this 
and that comparisons wile could not in others//.

As of now, it's clear that we can not compare Symbols to Symbols - preventing 
such confusing questions to arise in the future.
If we were to implement Symbol to Symbol comparisons, we should cover a lot 
more cases than this patch could.

So I decided to abandon this patch.


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

https://reviews.llvm.org/D77792

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

[PATCH] D84979: [analyzer][NFC] Refine CStringLength modeling API

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

steakhal updated this revision to Diff 286212.
steakhal marked 2 inline comments as done.
steakhal edited the summary of this revision.
steakhal added a comment.
Herald added a subscriber: mgorny.

Fixed Artem's inline comments:

- `cstring::getCStringLength` now takes `StateRef` by value
- `cstring::dumpCStringLengths` now takes by `StateRef` by non const value


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84979

Files:
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringChecker.h
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLength.h
  clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp

Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp
===
--- /dev/null
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker/CStringLengthModeling.cpp
@@ -0,0 +1,306 @@
+//=== CStringLengthModeling.cpp  Implementation of CStringLength API C++ -*--=//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Implements the CStringLength API and the CStringChecker bookkeeping parts.
+// Updates the associated cstring lengths of memory regions:
+//  - Infers the cstring length of string literals.
+//  - Removes cstring length associations of dead symbols.
+//  - Handles region invalidation.
+//
+//===--===//
+
+#include "CStringChecker.h"
+#include "CStringLength.h"
+
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace clang;
+using namespace ento;
+using namespace cstring;
+
+/// Associates an strlen to a memory region.
+REGISTER_MAP_WITH_PROGRAMSTATE(CStringLengthMap, const MemRegion *, SVal)
+
+//===--===//
+// Implementation of the public CStringLength API.
+//===--===//
+
+ProgramStateRef cstring::setCStringLength(ProgramStateRef State,
+  const MemRegion *MR, SVal StrLength) {
+  assert(!StrLength.isUndef() && "Attempt to set an undefined string length");
+
+  MR = MR->StripCasts();
+
+  switch (MR->getKind()) {
+  case MemRegion::StringRegionKind:
+// FIXME: This can happen if we strcpy() into a string region. This is
+// undefined [C99 6.4.5p6], but we should still warn about it.
+return State;
+
+  case MemRegion::SymbolicRegionKind:
+  case MemRegion::AllocaRegionKind:
+  case MemRegion::NonParamVarRegionKind:
+  case MemRegion::ParamVarRegionKind:
+  case MemRegion::FieldRegionKind:
+  case MemRegion::ObjCIvarRegionKind:
+// These are the types we can currently track string lengths for.
+break;
+
+  case MemRegion::ElementRegionKind:
+// FIXME: Handle element regions by upper-bounding the parent region's
+// string length.
+return State;
+
+  default:
+// Other regions (mostly non-data) can't have a reliable C string length.
+// For now, just ignore the change.
+// FIXME: These are rare but not impossible. We should output some kind of
+// warning for things like strcpy((char[]){'a', 0}, "b");
+return State;
+  }
+
+  if (StrLength.isUnknown())
+return removeCStringLength(State, MR);
+  return State->set(MR, StrLength);
+}
+
+ProgramStateRef cstring::removeCStringLength(ProgramStateRef State,
+ const MemRegion *MR) {
+  return State->remove(MR);
+}
+
+NonLoc cstring::createCStringLength(ProgramStateRef &State, CheckerContext &Ctx,
+const Expr *Ex, const MemRegion *MR) {
+  assert(Ex);
+  assert(MR);
+
+  SValBuilder &SVB = Ctx.getSValBuilder();
+  QualType SizeTy = SVB.getContext().getSizeType();
+  NonLoc CStrLen =
+  SVB.getMetadataSymbolVal(CStringChecker::getTag(), MR, Ex, SizeTy,
+   Ctx.getLocationContext(), Ctx.blockCount())
+  .castAs();
+
+  // Implicitly constrain the range to SIZE_MAX/4
+  BasicValueFactory &BVF = SVB.getBasicValueFactory();
+  const llvm::APSInt &MaxValue = BVF.getMaxValue(SizeTy);
+  const llvm::APSInt Four = APSIntType(MaxValue).getValue(4);
+  const llvm::APSInt *MaxLength = BVF.ev

[PATCH] D84979: [analyzer][NFC] Refine CStringLength modeling API

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

steakhal added a comment.

In D84979#2190996 , @balazske wrote:

> Really I am still not totally familiar how the checkers work and if it is 
> good to have these function names.

Yea, naming things is pretty hard. I'm open to have a less verbose one - 
especially since these API functions will live under some namespace.

> [...]  It could get the data from CStringChecker using a "get" function or 
> test if there is data at all. In this case it may happen that this get 
> function modifies state or computes the length at that point?

This patch makes sure that the //get// function will not modify the state 
(takes a `const ProgramStateRef`).
We don't really want to store the length of a `StringLiteral`, since that can 
be computed on demand.

> And how do the //set// operation work, it only stores a value computed by the 
> calling code?

It tries to avoid storing the length information. E.g. if a region refers to a 
string literal - we don't have to store the length. (This was the previous 
behavior, I don't want to change that.)
However, we don't handle ElementRegions (for now, but I'm working on it).

> And the //create// operation then does the computation and //set// together?

CStringChecker needs a way to defer associating the length of the region.
I'm not sure why, but the magic //Hypothetical// parameter was introduced just 
for accomplishing that.
I'm not confident enough to refactor the function which exploits this. [1]

> The functions look not symmetrical, the set and create takes a `MemRegion` 
> but the get takes a `SVal`.

The current implementation checks if the SVal is a GotoLabel (which is not a 
MemRegion). I didn't want to change its behavior in this patch.
`MemRegion` parameter would fit much more nicely, just as you said.

> The create function sets the length for the passed memory region

No, it just creates a symbol, representing a cstring length. That symbol is 
(strictly speaking) not yet associated with the memory region.
Furthermore, it applies implicit constraints like assuming that the cstring 
length is always less than the corresponding extent (It is not yet done, but 
I'm working on this too).

> but makes no tests on it like the set function (can we call the set function 
> from the create?).

We could call, but we need a mechanism to defer associating the length of a 
region.
CStringChecker relies on this behavior. See my paragraph about the 
//Hypothetical// parameter.

---

@balazske Unfortunately, I don't have satisfying answers to you, but any 
further improvements could be done in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84979

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

[PATCH] D84736: [analyzer] Handle pointer difference of ElementRegion and SymbolicRegion

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

steakhal added inline comments.



Comment at: clang/test/Analysis/pointer-arithmetic.c:88
+  clang_analyzer_dump_int(p - pn);   // expected-warning-re {{0 - 
(reg_${{[0-9]+}})}}
+  clang_analyzer_dump_int((p + 1) - q);  // expected-warning {{Unknown}} // 
FIXME: Might point to the same region, we should hold the expression 'p+1+q' 
instead.
+  clang_analyzer_dump_int((p + 1) - p);  // expected-warning {{1 S32b}}

steakhal wrote:
> The suggested expression should be `p+1-q`.
xD more like `(p+1)-q`



Comment at: clang/test/Analysis/pointer-arithmetic.c:100
+  clang_analyzer_dump_int(pn - p);   // expected-warning-re 
{{reg_${{[0-9]+
+  clang_analyzer_dump_int(q - (p + 1));  // expected-warning {{Unknown}} // 
FIXME: Might point to the same region, we should hold the expression 'p+1+q' 
instead.
+  clang_analyzer_dump_int(p - (p + 1));  // expected-warning {{-1 S32b}}

steakhal wrote:
> The suggested expression should be `q-p+1`.
`q-(p+1)`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84736

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

[PATCH] D84736: [analyzer] Handle pointer difference of ElementRegion and SymbolicRegion

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

steakhal added inline comments.



Comment at: clang/test/Analysis/pointer-arithmetic.c:88
+  clang_analyzer_dump_int(p - pn);   // expected-warning-re {{0 - 
(reg_${{[0-9]+}})}}
+  clang_analyzer_dump_int((p + 1) - q);  // expected-warning {{Unknown}} // 
FIXME: Might point to the same region, we should hold the expression 'p+1+q' 
instead.
+  clang_analyzer_dump_int((p + 1) - p);  // expected-warning {{1 S32b}}

The suggested expression should be `p+1-q`.



Comment at: clang/test/Analysis/pointer-arithmetic.c:100
+  clang_analyzer_dump_int(pn - p);   // expected-warning-re 
{{reg_${{[0-9]+
+  clang_analyzer_dump_int(q - (p + 1));  // expected-warning {{Unknown}} // 
FIXME: Might point to the same region, we should hold the expression 'p+1+q' 
instead.
+  clang_analyzer_dump_int(p - (p + 1));  // expected-warning {{-1 S32b}}

The suggested expression should be `q-p+1`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84736

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

[PATCH] D84736: [analyzer] Handle pointer difference of ElementRegion and SymbolicRegion

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

steakhal updated this revision to Diff 286318.
steakhal marked 4 inline comments as done.
steakhal retitled this revision from "[analyzer][RFC] Handle pointer difference 
of ElementRegion and SymbolicRegion" to "[analyzer] Handle pointer difference 
of ElementRegion and SymbolicRegion".
steakhal edited the summary of this revision.
steakhal added a comment.

- Refined documentation comments as noted.
- Added tests.
- Removed the complicated `ByteOffsetOfElement` lambda.
- Rename revision.

---

Before this patch, only these reported `Unknown` instead of the currently 
expected value.
Besides that all tests passed as-is on master:

  clang_analyzer_dump_int(p - p0);   // expected-warning {{0 S32b}}
  clang_analyzer_dump_int(p - p1);   // expected-warning {{-1 S32b}}
  clang_analyzer_dump_int(p - pn);   // expected-warning-re {{0 - 
(reg_${{[0-9]+}})}}
  clang_analyzer_dump_int((p + 1) - p);  // expected-warning {{1 S32b}}
  
  // Swapped operands:
  clang_analyzer_dump_int(p0 - p);   // expected-warning {{0 S32b}}
  clang_analyzer_dump_int(p1 - p);   // expected-warning {{1 S32b}}
  clang_analyzer_dump_int(pn - p);   // expected-warning-re 
{{reg_${{[0-9]+
  clang_analyzer_dump_int(p - (p + 1));  // expected-warning {{-1 S32b}}



---

Further notes:
Element{X, n, Ty1} and Element{X, m, Ty2} should compare equal if and only if 
the `n * sizeof(Ty1)` equals to `n * sizeof(Ty2)`.
However, previously it did not take the size of the types into account (there 
is the corresponding FIXIT).
I'm not fixing this either for now.

The analyzer returns `Unknown` for this call:
clang_analyzer_dump_int((p + 1) - q);
However, IMO it should hold the expression 'p+1+q' instead - regardless of `p` 
alias (or not) the same memory region of `p`
There is a FIXME in the testcode for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84736

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/pointer-arithmetic.c

Index: clang/test/Analysis/pointer-arithmetic.c
===
--- clang/test/Analysis/pointer-arithmetic.c
+++ clang/test/Analysis/pointer-arithmetic.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection \
+// RUN:   -analyzer-config eagerly-assume=false -verify %s
+
+void clang_analyzer_eval(int);
+void clang_analyzer_dump_ptr(int *);
+void clang_analyzer_dump_int(int);
 
 int test1() {
   int *p = (int *)sizeof(int);
@@ -28,3 +33,88 @@
   p += 1;
   return *p; // expected-warning {{Dereference of null pointer}}
 }
+
+void simplify_symregion_and_elementregion_pointer_arithmetic_and_comparison(int *p, int n, int m, int *q) {
+  // 'q' is SymReg{q}
+  // 'p' is SymReg{p}
+  int *p1 = p + 1;  // Element{p,1}
+  int *p0 = p1 - 1; // Element{p,0}
+  int *pn = p + n;  // Element{p,n}
+  int *pm = p + m;  // Element{p,m}
+
+  clang_analyzer_dump_ptr(q);
+  clang_analyzer_dump_ptr(p);
+  clang_analyzer_dump_ptr(p0);
+  clang_analyzer_dump_ptr(p1);
+  clang_analyzer_dump_ptr(pn);
+  clang_analyzer_dump_ptr(pm);
+  // expected-warning-re@-6 {{&SymRegion{reg_${{[0-9]+}
+  // expected-warning-re@-6 {{&SymRegion{reg_${{[0-9]+}
+  // expected-warning-re@-6 {{&Element{SymRegion{reg_${{[0-9]+}}},0 S64b,int
+  // expected-warning-re@-6 {{&Element{SymRegion{reg_${{[0-9]+}}},1 S64b,int}}}
+  // expected-warning-re@-6 {{&Element{SymRegion{reg_${{[0-9]+}}},reg_${{[0-9]+}},int}}}
+  // expected-warning-re@-6 {{&Element{SymRegion{reg_${{[0-9]+}}},reg_${{[0-9]+}},int}}}
+
+  // Test the equality operator:
+  clang_analyzer_eval(p == p0);  // expected-warning {{TRUE}}
+  clang_analyzer_eval(p == p1);  // expected-warning {{FALSE}}
+  clang_analyzer_eval(p1 == pn); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(pn == pm); // expected-warning {{UNKNOWN}}
+
+  // Reverse operands:
+  clang_analyzer_eval(p0 == p);  // expected-warning {{TRUE}}
+  clang_analyzer_eval(p1 == p);  // expected-warning {{FALSE}}
+  clang_analyzer_eval(pn == p1); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(pm == pn); // expected-warning {{UNKNOWN}}
+
+  // Test the inequality operator:
+  clang_analyzer_eval(p != p0);  // expected-warning {{FALSE}}
+  clang_analyzer_eval(p != p1);  // expected-warning {{TRUE}}
+  clang_analyzer_eval(p1 != pn); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(pn != pm); // expected-warning {{UNKNOWN}}
+
+  // Reverse operands:
+  clang_analyzer_eval(p0 != p);  // expected-warning {{FALSE}}
+  clang_analyzer_eval(p1 != p);  // expected-warning {{TRUE}}
+  clang_analyzer_eval(pn != p1); // expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(pm != pn); // expected-warning {{UNKNOWN}}
+
+  // Test the subtraction operator:
+  clang_analyzer_dump_int(p - q);// expected-warning-re {{(reg_${{[0-9]+}}) - (reg_

[PATCH] D132142: [analyzer] Prefer wrapping SymbolicRegions by ElementRegions

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

steakhal added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:799
+  /// we actually know their types.
+  QualType getApproximatedType() const {
+return sym->getType()->getPointeeType();

martong wrote:
> I think we should express that this returns the
> 
> 
>   - type of the pointee
>   - the type that is the "static" type, id est, the type of the declaration; 
> `void`, `char` and `base` in your example above.
> 
> In this sense, what about `getPointeeStaticType`?
> 
> 
I think it's important to discourage people from using this; hence the 
`approximated` calls attention that this might always be accurate.
I'm okay to add the `pointee` somewhere in the name.

I don't have a strong opinion about this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132142

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

[PATCH] D133119: [clang-tidy] Add checker 'bugprone-suspicious-realloc-usage'.

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

steakhal added a comment.

Consider adding a test case demonstrating if the checker recognizes and 
suppresses the check if there are multiple variables referring to the memory 
being reallocated.
If the check does not handle that case, we should probably state this 
limitation explicitly. And marking the corresponding test case with a FIXME.

I was thinking about something like this:

  void escape(void *);
  void foo(void *p) {
void *q = p;
p = realloc(p, 10); // no-warning
escape(q);
  }
  void bar(void *p) {
p = realloc(p, 10); // warn: ...
void *q = p;
escape(q);
  }

The `MallocOverflowCheck` does something similar. It uses 
`isBeforeInTranslationUnit()`.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp:20-21
+namespace {
+class IsSamePtrExpr : public clang::StmtVisitor /*,
+ public clang::DeclVisitor*/
+{

Commented code?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousReallocUsageCheck.cpp:23
+{
+  /// The other expression to compare with.
+  /// This variable is used to pass the data from a \c check function to any of

against


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133119

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

[PATCH] D132142: [analyzer] Prefer wrapping SymbolicRegions by ElementRegions

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

This revision was automatically updated to reflect the committed changes.
steakhal marked 3 inline comments as done.
Closed by commit rGf8643a9b31c4: [analyzer] Prefer wrapping SymbolicRegions by 
ElementRegions (authored by steakhal).

Changed prior to commit:
  https://reviews.llvm.org/D132142?vs=454859&id=459656#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132142

Files:
  clang/include/clang/StaticAnalyzer/Checkers/SValExplainer.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
  clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/MemRegion.cpp
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/ctor.mm
  clang/test/Analysis/expr-inspection.c
  clang/test/Analysis/memory-model.cpp
  clang/test/Analysis/ptr-arith.c
  clang/test/Analysis/ptr-arith.cpp
  clang/test/Analysis/trivial-copy-struct.cpp

Index: clang/test/Analysis/trivial-copy-struct.cpp
===
--- /dev/null
+++ clang/test/Analysis/trivial-copy-struct.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+template  void clang_analyzer_dump(T);
+void clang_analyzer_warnIfReached();
+
+struct Node { int* ptr; };
+
+void copy_on_stack(Node* n1) {
+  Node tmp = *n1;
+  Node* n2 = &tmp;
+  clang_analyzer_dump(n1); // expected-warning-re {{&SymRegion{reg_${{[0-9]+}
+  clang_analyzer_dump(n2); // expected-warning {{&tmp}}
+
+  clang_analyzer_dump(n1->ptr); // expected-warning-re {{&SymRegion{reg_${{[0-9]+}}},0 S64b,struct Node}.ptr>}}}
+  clang_analyzer_dump(n2->ptr); // expected-warning-re {{&SymRegion{reg_${{[0-9]+}}},0 S64b,struct Node}.ptr>}}}
+
+  if (n1->ptr != n2->ptr)
+clang_analyzer_warnIfReached(); // unreachable
+  (void)(n1->ptr);
+  (void)(n2->ptr);
+}
+
+void copy_on_heap(Node* n1) {
+  Node* n2 = new Node(*n1);
+
+  clang_analyzer_dump(n1); // expected-warning-re {{&SymRegion{reg_${{[0-9]+}
+  clang_analyzer_dump(n2); // expected-warning-re {{&HeapSymRegion{conj_${{[0-9]+}}{Node *, LC{{[0-9]+}}, S{{[0-9]+}}, #{{[0-9]+}}
+
+  clang_analyzer_dump(n1->ptr); // expected-warning-re {{&SymRegion{reg_${{[0-9]+}}},0 S64b,struct Node}.ptr>}}}
+  clang_analyzer_dump(n2->ptr); // expected-warning {{Unknown}} FIXME: This should be the same as above.
+
+  if (n1->ptr != n2->ptr)
+clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}} FIXME: This should not be reachable.
+  (void)(n1->ptr);
+  (void)(n2->ptr);
+}
Index: clang/test/Analysis/ptr-arith.cpp
===
--- clang/test/Analysis/ptr-arith.cpp
+++ clang/test/Analysis/ptr-arith.cpp
@@ -134,10 +134,10 @@
 int parse(parse_t *p) {
   unsigned copy = p->bits2;
   clang_analyzer_dump(copy);
-  // expected-warning@-1 {{reg_$1}.bits2>}}
+  // expected-warning@-1 {{reg_$1},0 S64b,struct Bug_55934::parse_t}.bits2>}}
   header *bits = (header *)©
   clang_analyzer_dump(bits->b);
-  // expected-warning@-1 {{derived_$2{reg_$1}.bits2>,Element{copy,0 S64b,struct Bug_55934::header}.b}}}
+  // expected-warning@-1 {{derived_$2{reg_$1},0 S64b,struct Bug_55934::parse_t}.bits2>,Element{copy,0 S64b,struct Bug_55934::header}.b}}}
   return bits->b; // no-warning
 }
 } // namespace Bug_55934
Index: clang/test/Analysis/ptr-arith.c
===
--- clang/test/Analysis/ptr-arith.c
+++ clang/test/Analysis/ptr-arith.c
@@ -340,11 +340,11 @@
 void struct_pointer_canon(struct s *ps) {
   struct s ss = *ps;
   clang_analyzer_dump((*ps).v);
-  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}}.v>}}
+  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}},0 S64b,struct s}.v>}}
   clang_analyzer_dump(ps[0].v);
-  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}}.v>}}
+  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}},0 S64b,struct s}.v>}}
   clang_analyzer_dump(ps->v);
-  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}}.v>}}
+  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}},0 S64b,struct s}.v>}}
   clang_analyzer_eval((*ps).v == ps[0].v); // expected-warning{{TRUE}}
   clang_analyzer_eval((*ps).v == ps->v);   // expected-warning{{TRUE}}
   clang_analyzer_eval(ps[0].v == ps->v);   // expected-warning{{TRUE}}
@@ -360,11 +360,11 @@
 void struct_pointer_canon_typedef(T1 *ps) {
   T2 ss = *ps;
   clang_analyzer_dump((*ps).v);
-  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}}.v>}}
+  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}},0 S64b,struct s}.v>}}
   clang_analyzer_dump(ps[0].v);
-  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}}.v>}}
+  // expected-warning-re@-1{{reg_${{[[:digit:]]+}}},0 S64b,struct s}.v>}}
   clang_analyzer_dump(ps->v);
-  //

[PATCH] D132143: [analyzer] LazyCompoundVals should be always bound as default bindings

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rGafcd862b2e0a: [analyzer] LazyCompoundVals should be always 
bound as default bindings (authored by steakhal).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132143

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/trivial-copy-struct.cpp


Index: clang/test/Analysis/trivial-copy-struct.cpp
===
--- clang/test/Analysis/trivial-copy-struct.cpp
+++ clang/test/Analysis/trivial-copy-struct.cpp
@@ -27,10 +27,10 @@
   clang_analyzer_dump(n2); // expected-warning-re 
{{&HeapSymRegion{conj_${{[0-9]+}}{Node *, LC{{[0-9]+}}, S{{[0-9]+}}, 
#{{[0-9]+}}
 
   clang_analyzer_dump(n1->ptr); // expected-warning-re 
{{&SymRegion{reg_${{[0-9]+}}},0 S64b,struct Node}.ptr>}}}
-  clang_analyzer_dump(n2->ptr); // expected-warning {{Unknown}} FIXME: This 
should be the same as above.
+  clang_analyzer_dump(n2->ptr); // expected-warning-re 
{{&SymRegion{reg_${{[0-9]+}}},0 S64b,struct Node}.ptr>}}}
 
   if (n1->ptr != n2->ptr)
-clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}} FIXME: 
This should not be reachable.
+clang_analyzer_warnIfReached(); // unreachable
   (void)(n1->ptr);
   (void)(n2->ptr);
 }
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2400,7 +2400,11 @@
 
   // Clear out bindings that may overlap with this binding.
   RegionBindingsRef NewB = removeSubRegionBindings(B, cast(R));
-  return NewB.addBinding(BindingKey::Make(R, BindingKey::Direct), V);
+
+  // LazyCompoundVals should be always bound as 'default' bindings.
+  auto KeyKind = isa(V) ? BindingKey::Default
+ : BindingKey::Direct;
+  return NewB.addBinding(BindingKey::Make(R, KeyKind), V);
 }
 
 RegionBindingsRef


Index: clang/test/Analysis/trivial-copy-struct.cpp
===
--- clang/test/Analysis/trivial-copy-struct.cpp
+++ clang/test/Analysis/trivial-copy-struct.cpp
@@ -27,10 +27,10 @@
   clang_analyzer_dump(n2); // expected-warning-re {{&HeapSymRegion{conj_${{[0-9]+}}{Node *, LC{{[0-9]+}}, S{{[0-9]+}}, #{{[0-9]+}}
 
   clang_analyzer_dump(n1->ptr); // expected-warning-re {{&SymRegion{reg_${{[0-9]+}}},0 S64b,struct Node}.ptr>}}}
-  clang_analyzer_dump(n2->ptr); // expected-warning {{Unknown}} FIXME: This should be the same as above.
+  clang_analyzer_dump(n2->ptr); // expected-warning-re {{&SymRegion{reg_${{[0-9]+}}},0 S64b,struct Node}.ptr>}}}
 
   if (n1->ptr != n2->ptr)
-clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}} FIXME: This should not be reachable.
+clang_analyzer_warnIfReached(); // unreachable
   (void)(n1->ptr);
   (void)(n2->ptr);
 }
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2400,7 +2400,11 @@
 
   // Clear out bindings that may overlap with this binding.
   RegionBindingsRef NewB = removeSubRegionBindings(B, cast(R));
-  return NewB.addBinding(BindingKey::Make(R, BindingKey::Direct), V);
+
+  // LazyCompoundVals should be always bound as 'default' bindings.
+  auto KeyKind = isa(V) ? BindingKey::Default
+ : BindingKey::Direct;
+  return NewB.addBinding(BindingKey::Make(R, KeyKind), V);
 }
 
 RegionBindingsRef
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D132109: [analyzer] Dump the environment entry kind as well

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7cddf9cad18a: [analyzer] Dump the environment entry kind as 
well (authored by steakhal).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132109

Files:
  clang/lib/StaticAnalyzer/Core/Environment.cpp
  clang/test/Analysis/expr-inspection.c


Index: clang/test/Analysis/expr-inspection.c
===
--- clang/test/Analysis/expr-inspection.c
+++ clang/test/Analysis/expr-inspection.c
@@ -36,7 +36,7 @@
 // CHECK-NEXT:   ]},
 // CHECK-NEXT:   "environment": { "pointer": "{{0x[0-9a-f]+}}", "items": [
 // CHECK-NEXT: { "lctx_id": {{[0-9]+}}, "location_context": "#0 Call", 
"calling": "foo", "location": null, "items": [
-// CHECK-NEXT:   { "stmt_id": {{[0-9]+}}, "pretty": 
"clang_analyzer_printState", "value": "&code{clang_analyzer_printState}" }
+// CHECK-NEXT:   { "stmt_id": {{[0-9]+}}, "kind": "ImplicitCastExpr", 
"pretty": "clang_analyzer_printState", "value": 
"&code{clang_analyzer_printState}" }
 // CHECK-NEXT: ]}
 // CHECK-NEXT:   ]},
 // CHECK-NEXT:   "constraints": [
Index: clang/lib/StaticAnalyzer/Core/Environment.cpp
===
--- clang/lib/StaticAnalyzer/Core/Environment.cpp
+++ clang/lib/StaticAnalyzer/Core/Environment.cpp
@@ -274,7 +274,8 @@
 
   const Stmt *S = I->first.getStmt();
   Indent(Out, InnerSpace, IsDot)
-  << "{ \"stmt_id\": " << S->getID(Ctx) << ", \"pretty\": ";
+  << "{ \"stmt_id\": " << S->getID(Ctx) << ", \"kind\": \""
+  << S->getStmtClassName() << "\", \"pretty\": ";
   S->printJson(Out, nullptr, PP, /*AddQuotes=*/true);
 
   Out << ", \"value\": ";


Index: clang/test/Analysis/expr-inspection.c
===
--- clang/test/Analysis/expr-inspection.c
+++ clang/test/Analysis/expr-inspection.c
@@ -36,7 +36,7 @@
 // CHECK-NEXT:   ]},
 // CHECK-NEXT:   "environment": { "pointer": "{{0x[0-9a-f]+}}", "items": [
 // CHECK-NEXT: { "lctx_id": {{[0-9]+}}, "location_context": "#0 Call", "calling": "foo", "location": null, "items": [
-// CHECK-NEXT:   { "stmt_id": {{[0-9]+}}, "pretty": "clang_analyzer_printState", "value": "&code{clang_analyzer_printState}" }
+// CHECK-NEXT:   { "stmt_id": {{[0-9]+}}, "kind": "ImplicitCastExpr", "pretty": "clang_analyzer_printState", "value": "&code{clang_analyzer_printState}" }
 // CHECK-NEXT: ]}
 // CHECK-NEXT:   ]},
 // CHECK-NEXT:   "constraints": [
Index: clang/lib/StaticAnalyzer/Core/Environment.cpp
===
--- clang/lib/StaticAnalyzer/Core/Environment.cpp
+++ clang/lib/StaticAnalyzer/Core/Environment.cpp
@@ -274,7 +274,8 @@
 
   const Stmt *S = I->first.getStmt();
   Indent(Out, InnerSpace, IsDot)
-  << "{ \"stmt_id\": " << S->getID(Ctx) << ", \"pretty\": ";
+  << "{ \"stmt_id\": " << S->getID(Ctx) << ", \"kind\": \""
+  << S->getStmtClassName() << "\", \"pretty\": ";
   S->printJson(Out, nullptr, PP, /*AddQuotes=*/true);
 
   Out << ", \"value\": ";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133851: [analyzer] Initialize ShouldEmitErrorsOnInvalidConfigValue analyzer option

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

steakhal created this revision.
steakhal added reviewers: NoQ, martong, Szelethus.
Herald added subscribers: manas, ASDenysPetrov, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: All.
steakhal requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Downstream users who don't make use of the clang cc1 frontend for
commandline argument parsing, won't benefit from the Marshalling
provided default initialization of the AnalyzerOptions entries. More
about this later.
Those analyzer option fields, as they are bitfields, cannot be default
initialized at the declaration (prior c++20), hence they are initialized
at the constructor.
The only problem is that `ShouldEmitErrorsOnInvalidConfigValue` was
forgotten.

In this patch, I'm proposing to initialize that field with the rest.

Note that this value is read by
`CheckerRegistry.cpp:insertAndValidate()`.
The analyzer options are initialized by the marshalling at
`CompilerInvocation.cpp:GenerateAnalyzerArgs()` by the expansion of the
`ANALYZER_OPTION_WITH_MARSHALLING` xmacro to the appropriate default
value regardless of the constructor initialized list which I'm touching.
Due to that this only affects users using CSA as a library, without
serious effort, I believe we cannot test this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133851

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h


Index: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -289,7 +289,8 @@
 ShowCheckerHelpAlpha(false), ShowCheckerHelpDeveloper(false),
 ShowCheckerOptionList(false), ShowCheckerOptionAlphaList(false),
 ShowCheckerOptionDeveloperList(false), ShowEnabledCheckerList(false),
-ShowConfigOptionsList(false), AnalyzeAll(false),
+ShowConfigOptionsList(false),
+ShouldEmitErrorsOnInvalidConfigValue(false), AnalyzeAll(false),
 AnalyzerDisplayProgress(false), eagerlyAssumeBinOpBifurcation(false),
 TrimGraph(false), visualizeExplodedGraphWithGraphViz(false),
 UnoptimizedCFG(false), PrintStats(false), NoRetryExhausted(false),


Index: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -289,7 +289,8 @@
 ShowCheckerHelpAlpha(false), ShowCheckerHelpDeveloper(false),
 ShowCheckerOptionList(false), ShowCheckerOptionAlphaList(false),
 ShowCheckerOptionDeveloperList(false), ShowEnabledCheckerList(false),
-ShowConfigOptionsList(false), AnalyzeAll(false),
+ShowConfigOptionsList(false),
+ShouldEmitErrorsOnInvalidConfigValue(false), AnalyzeAll(false),
 AnalyzerDisplayProgress(false), eagerlyAssumeBinOpBifurcation(false),
 TrimGraph(false), visualizeExplodedGraphWithGraphViz(false),
 UnoptimizedCFG(false), PrintStats(false), NoRetryExhausted(false),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D133851: [analyzer] Initialize ShouldEmitErrorsOnInvalidConfigValue analyzer option

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb8e1da050673: [analyzer] Initialize 
ShouldEmitErrorsOnInvalidConfigValue analyzer option (authored by steakhal).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133851

Files:
  clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h


Index: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -289,7 +289,8 @@
 ShowCheckerHelpAlpha(false), ShowCheckerHelpDeveloper(false),
 ShowCheckerOptionList(false), ShowCheckerOptionAlphaList(false),
 ShowCheckerOptionDeveloperList(false), ShowEnabledCheckerList(false),
-ShowConfigOptionsList(false), AnalyzeAll(false),
+ShowConfigOptionsList(false),
+ShouldEmitErrorsOnInvalidConfigValue(false), AnalyzeAll(false),
 AnalyzerDisplayProgress(false), eagerlyAssumeBinOpBifurcation(false),
 TrimGraph(false), visualizeExplodedGraphWithGraphViz(false),
 UnoptimizedCFG(false), PrintStats(false), NoRetryExhausted(false),


Index: clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
===
--- clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
+++ clang/include/clang/StaticAnalyzer/Core/AnalyzerOptions.h
@@ -289,7 +289,8 @@
 ShowCheckerHelpAlpha(false), ShowCheckerHelpDeveloper(false),
 ShowCheckerOptionList(false), ShowCheckerOptionAlphaList(false),
 ShowCheckerOptionDeveloperList(false), ShowEnabledCheckerList(false),
-ShowConfigOptionsList(false), AnalyzeAll(false),
+ShowConfigOptionsList(false),
+ShouldEmitErrorsOnInvalidConfigValue(false), AnalyzeAll(false),
 AnalyzerDisplayProgress(false), eagerlyAssumeBinOpBifurcation(false),
 TrimGraph(false), visualizeExplodedGraphWithGraphViz(false),
 UnoptimizedCFG(false), PrintStats(false), NoRetryExhausted(false),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64087: [clang] Correct source locations for instantiations of out-of-line defaulted special member functions. (PR25683)

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

steakhal added a comment.

This patch appears to introduce a bug in some source ranges.
Reported the regression at https://github.com/llvm/llvm-project/issues/71161.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64087

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

[PATCH] D129269: [analyzer] Fix use of length in CStringChecker

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

steakhal added a comment.

Actually, there is one more bug there. If the string has an embedded 
null-terminator; a wrong length will be returned.
Please add tests to demonstrate the wrong behavior. Try different target 
triples for having irregular char sizes and pin that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129269

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

[PATCH] D128535: [analyzer] Improve loads from reinterpret-cast fields

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

steakhal added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1897
+  QualType Ty = SubReg->getValueType();
+  if (BaseTy->isScalarType() && Ty->isScalarType()) {
+if (Ctx.getTypeSizeInChars(BaseTy) >= Ctx.getTypeSizeInChars(Ty)) {

BTW I'm not sure why this check is here. I would expect this to work eve 
without this.
OOh, I think I know why. `getTypeSizeInChars` can only be called on //scalars//?



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2014-2017
+  // FIXME: This is a hack, and doesn't do anything really intelligent yet.
+  // FIXME: This hack is analogous with the one present in
+  // `getBindingForElement`, maybe we should handle both in
+  // `getBindingForFieldOrElementCommon`.

martong wrote:
> Could you please elaborate why this is a hack? What should be done and where 
> to solve it properly? (Disclaimer, I did not look into 
> `getBindingForFieldOrElementCommon`.)
I'm not sure what Jordan was actually referring to. IMO hardcoding to look for 
a highly specific scenario (a typed memregion, and a symbol to be exact) could 
be thought about implementing a 'hack'. IMO this fits in the current ecosystem, 
so for me it doesn't look like a hack; hence IMO this is the way of 
implementing this.

To make it clear, I simply hoisted the nested `dyn_cast`s and checks into a 
function, while preserved the 'hack' comment at the callsite; so it's not added 
by me.

One additional note. We relay on the type information embedded into the 
memregion - which we know might not always be present and even accurate. That 
could be another reason why this should be considered as a 'hack'. IDK


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128535

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

[PATCH] D127874: [analyzer] Reimplement UnreachableCodeChecker using worklists

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

steakhal added a comment.

In D127874#3628112 , @martong wrote:

>> I don't think any of the issues mentioned in this patch relates to strongly 
>> connected components, thus I don't think I can answer to this question.
>
> In your example above, repeated here:
>
>   #6(entry)  #2(goto a;)
>|  |^
>   #5(goto end;)   | \
>| #4(goto b;) |
>   #1(end:)|  |
>| #3(goto c;) |
>   #0(exit) \/
>
> [#2, #4, #3] is a strongly connected (and unreachable) component  of the CFG, 
> isn't it?

Right; those three blocks are unreachable in the CFG.

Let me clarify that this (previous) example has nothing to do with the 
visitation order. For that, yes either BFS and DFS order would work.
The `magic_clamp` example supposed to underpin the rationale behind choosing 
BFS instead of DFS.
In the summary, you will find a step-by-step playthrough how the DFS visitation 
worked previously, and resulted in falsely leaving `B3` and `B5` unreachable 
due to the order in which their predecessor nodes were visited. Let me know if 
it helped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127874

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

[PATCH] D128704: [clang-extdef-mapping] Directly process .ast files

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

steakhal added a comment.

Sorry about my delayed response. I was busy.
I've left a couple comments inline. Nothing serious.
Thanks for the patch!




Comment at: clang/docs/ReleaseNotes.rst:593
+
+- clang-extdef-mapping now accepts .ast files as input. This is faster than to
+  recompile the files from sources when extracting method definitons. This can

I think we need to use double back ticks for preformatted/code texts.



Comment at: clang/docs/ReleaseNotes.rst:595
+  recompile the files from sources when extracting method definitons. This can
+  be really beneficial when creating .ast files for input to the 
clang-static-analyzer.
+





Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:42
+  : Ctx(Context), SM(Context.getSourceManager()) {
+CurrentFileName = astFilePath.str();
+  }

Why is this not initialized in the //initialized-list// like the rest of the 
members?



Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:149
+  if (!CI)
+CI = new CompilerInstance();
+

What takes the ownership of `CI`? When is it deleted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128704

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

[PATCH] D128704: [clang-extdef-mapping] Directly process .ast files

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

steakhal added inline comments.



Comment at: clang/tools/clang-extdef-mapping/ClangExtDefMapGen.cpp:149
+  if (!CI)
+CI = new CompilerInstance();
+

thieta wrote:
> steakhal wrote:
> > What takes the ownership of `CI`? When is it deleted?
> I don't think anyone takes ownership and it's never properly deleted. I don't 
> think we really need to since this application just runs and exits and never 
> really keep any state. Do you see a problem with it never being deleted?
Not a big deal. I just think that in general, leak suggests bad software design.
On the other hand, one should not have non-primitive global variables, such as 
smart-pointers. I mean, it's still bad, but let's not touch it xD


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128704

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

[PATCH] D122244: [analyzer] Turn missing tablegen doc entry of a checker into fatal error

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

steakhal added a comment.

In D122244#3404727 , @Szelethus wrote:

> LGTM! You did check whether a missing doc field will actually trigger this 
> error, right?

Yup, it works as expected!

  ninja: Entering directory `build/release'
  [1/375] Building Checkers.inc...
  FAILED: tools/clang/include/clang/StaticAnalyzer/Checkers/Checkers.inc 
/git/llvm-project/build/release/tools/clang/include/clang/StaticAnalyzer/Checkers/Checkers.inc
  cd /git/llvm-project/build/release && 
/git/llvm-project/build/release/bin/clang-tblgen -gen-clang-sa-checkers -I 
/git/llvm-project/clang/include/clang/StaticAnalyzer/Checkers 
-I/git/llvm-project/clang/include 
-I/git/llvm-project/build/release/tools/clang/include 
-I/git/llvm-project/build/release/include -I/git/llvm-project/llvm/include 
/git/llvm-project/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td 
--write-if-changed -o 
tools/clang/include/clang/StaticAnalyzer/Checkers/Checkers.inc -d 
tools/clang/include/clang/StaticAnalyzer/Checkers/Checkers.inc.d
  
/git/llvm-project/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:585:1:
 error: missing Documentation for cplusplus.NewDelete
  def NewDeleteChecker : Checker<"NewDelete">,
  ^
  ninja: build stopped: subcommand failed.




Comment at: clang/utils/TableGen/ClangSACheckersEmitter.cpp:82
+PrintFatalError(R.getLoc(),
+"missing Documentation for " + getCheckerFullName(&R));
+

Szelethus wrote:
> We might as well give an extra hand, and state that a `Documentation<> field` 
> is missing.
Hmm, indeed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122244

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

[PATCH] D122244: [analyzer] Turn missing tablegen doc entry of a checker into fatal error

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

steakhal updated this revision to Diff 417862.
steakhal added a comment.

reword error message


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

https://reviews.llvm.org/D122244

Files:
  clang/utils/TableGen/ClangSACheckersEmitter.cpp


Index: clang/utils/TableGen/ClangSACheckersEmitter.cpp
===
--- clang/utils/TableGen/ClangSACheckersEmitter.cpp
+++ clang/utils/TableGen/ClangSACheckersEmitter.cpp
@@ -76,14 +76,17 @@
 
 static std::string getCheckerDocs(const Record &R) {
   StringRef LandingPage;
-  if (BitsInit *BI = R.getValueAsBitsInit("Documentation")) {
-uint64_t V = getValueFromBitsInit(BI, R);
-if (V == 1)
-  LandingPage = "available_checks.html";
-else if (V == 2)
-  LandingPage = "alpha_checks.html";
-  }
-  
+  const BitsInit *BI = R.getValueAsBitsInit("Documentation");
+  if (!BI)
+PrintFatalError(R.getLoc(), "missing Documentation<...> member for " +
+getCheckerFullName(&R));
+
+  uint64_t V = getValueFromBitsInit(BI, R);
+  if (V == 1)
+LandingPage = "available_checks.html";
+  else if (V == 2)
+LandingPage = "alpha_checks.html";
+
   if (LandingPage.empty())
 return "";
 


Index: clang/utils/TableGen/ClangSACheckersEmitter.cpp
===
--- clang/utils/TableGen/ClangSACheckersEmitter.cpp
+++ clang/utils/TableGen/ClangSACheckersEmitter.cpp
@@ -76,14 +76,17 @@
 
 static std::string getCheckerDocs(const Record &R) {
   StringRef LandingPage;
-  if (BitsInit *BI = R.getValueAsBitsInit("Documentation")) {
-uint64_t V = getValueFromBitsInit(BI, R);
-if (V == 1)
-  LandingPage = "available_checks.html";
-else if (V == 2)
-  LandingPage = "alpha_checks.html";
-  }
-  
+  const BitsInit *BI = R.getValueAsBitsInit("Documentation");
+  if (!BI)
+PrintFatalError(R.getLoc(), "missing Documentation<...> member for " +
+getCheckerFullName(&R));
+
+  uint64_t V = getValueFromBitsInit(BI, R);
+  if (V == 1)
+LandingPage = "available_checks.html";
+  else if (V == 2)
+LandingPage = "alpha_checks.html";
+
   if (LandingPage.empty())
 return "";
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits