[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 162942.
Charusso marked an inline comment as done.
Charusso retitled this revision from "[clang-tidy] New checker for not 
null-terminated result caused by strlen or wcslen" to "[clang-tidy] New checker 
for not null-terminated result caused by strlen(), size() or equal length".
Charusso edited the summary of this revision.
Charusso added a comment.

- The checker by default search for `__STDC_WANT_LIB_EXT1__` macro to determine 
if `_s` suffixed functions are available. This behaviour could be overridden by 
specifying `AreSafeFunctionsAvailable`, which is became a string (it was an 
integer, but it can be used in the same way).

- If the destination array length is based on a custom `malloc()` the current 
approach only want to rewrite it if it is 100% sure the chosen argument is 
specifying the length.

- If the checker works with `size()` function now it is only match for 
`std::string` (e.g. it matched to StringRefs).

- Now it handles macro defined lengths properly.


https://reviews.llvm.org/D45050

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tidy/bugprone/NotNullTerminatedResultCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-not-null-terminated-result-in-initialization-strlen.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-cxx.cpp
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-other.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen.cpp
  test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp

Index: test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
@@ -0,0 +1,135 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c++11
+
+#define __STDC_WANT_LIB_EXT1__ 1
+
+typedef unsigned int size_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+template 
+errno_t wcsncpy_s(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+errno_t wcsncpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+
+template 
+wchar_t *wcsncpy(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+wchar_t *wcsncpy(wchar_t *, const wchar_t *, size_t);
+
+template 
+errno_t wcscpy_s(wchar_t (&dest)[size], const wchar_t *);
+errno_t wcscpy_s(wchar_t *, size_t, const wchar_t *);
+
+template 
+wchar_t *wcscpy(wchar_t (&dest)[size], const wchar_t *);
+wchar_t *wcscpy(wchar_t *, const wchar_t *);
+
+errno_t wmemcpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+wchar_t *wmemcpy(wchar_t *, const wchar_t *, size_t);
+
+
+//===--===//
+// wmemcpy() - destination array tests
+//===--===//
+
+void bad_wmemcpy_known_dest(const wchar_t *src) {
+  wchar_t dest01[13];
+  wmemcpy(dest01, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest01[14];
+  // CHECK-FIXES-NEXT: wcscpy_s(dest01, src);
+}
+
+void good_wmemcpy_known_dest(const wchar_t *src) {
+  wchar_t dst01[14];
+  wcscpy_s(dst01, src);
+}
+
+//===--===//
+// wmemcpy() - length tests
+//===--===//
+
+void bad_wmemcpy_full_source_length(const wchar_t *src) {
+  wchar_t dest20[13];
+  wmemcpy(dest20, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest20[14];
+  // CHECK-FIXES-NEXT: wcscpy_s(dest20, src);
+}
+
+void good_wmemcpy_full_source_length(const wchar_t *src) {
+  wchar_t dst20[14];
+  wcscpy_s(dst20, src);
+}
+
+void bad_wmemcpy_partial_source_length(const wchar_t *src) {
+  wchar_t dest21[13];
+  wmemcpy(dest21, src, wcslen(src) - 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest21[14];
+  // CHECK-FIXES-NEXT: wcsncpy_s(dest21, src, wcslen(src) - 1);
+}
+
+void good_wmemcpy_partial_source_length(const wchar_t *src) {
+  wchar_t dst21[14];
+  wcsncpy_s(dst21, src, wcslen(src) - 1);
+}
+
+//===-

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 157916.
Charusso added a comment.

Excuse me for the huge patch, but what I can done wrongly, I did, so I have 
made tons of revision.

I would like to show the current direction of the checker. Currently it has too 
much overlapping function, so please don't go too deep into the core.

Major fixes in every function:

- Equal length of `strlen(src)` is now handled.
- It can read out the length of the destination buffer.
- If the length increases in the passed argument, it also increases the 
destination buffer (if overflow is looks like to possible).

Major fixes in `memcpy()` and `memmove()`:

- It can decide which function is the best in performance and safety. (The last 
implementation was rely on C++11 version, which was a huge mistake.)
- Handles cases where the destination is `unsigned char` or `signed char`, 
which cannot be passed to any string handler function. (It haven't checked the 
type so that made wrong fix-its.)

Minor fixes:

- Removed all memory allocation matchers in general, but the current functions 
behave the same to increase the buffer length.
- Now the test files' `RUN` command working well. (CheckOptions was wrong.)

Problematic:

1. It allows all custom memory allocation function. Sometimes the read buffer 
size is not the size parameter.
2. May it is not a good idea to heuristically read and increase buffer lengths.
3. If the new function transformed from `memcpy()` to `strncpy()`, the checker 
adds the null terminator expression in the next line, like `dest[length] = 
'\0'`, which is looks like a quite big injection.


https://reviews.llvm.org/D45050

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tidy/bugprone/NotNullTerminatedResultCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-not-null-terminated-result-in-initialization-strlen.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-cxx.cpp
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-other.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen.cpp
  test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp

Index: test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
@@ -0,0 +1,133 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c++11
+
+typedef unsigned int size_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+template 
+errno_t wcsncpy_s(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+errno_t wcsncpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+
+template 
+wchar_t *wcsncpy(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+wchar_t *wcsncpy(wchar_t *, const wchar_t *, size_t);
+
+template 
+errno_t wcscpy_s(wchar_t (&dest)[size], const wchar_t *);
+errno_t wcscpy_s(wchar_t *, size_t, const wchar_t *);
+
+template 
+wchar_t *wcscpy(wchar_t (&dest)[size], const wchar_t *);
+wchar_t *wcscpy(wchar_t *, const wchar_t *);
+
+errno_t wmemcpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+wchar_t *wmemcpy(wchar_t *, const wchar_t *, size_t);
+
+
+//===--===//
+// wmemcpy() - destination array tests
+//===--===//
+
+void bad_wmemcpy_known_dest(const wchar_t *src) {
+  wchar_t dest01[13];
+  wmemcpy(dest01, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest01[14];
+  // CHECK-FIXES-NEXT: wcscpy_s(dest01, src);
+}
+
+void good_wmemcpy_known_dest(const wchar_t *src) {
+  wchar_t dst01[14];
+  wcscpy_s(dst01, src);
+}
+
+//===--===//
+// wmemcpy() - length tests
+//===--===//
+
+void bad_wmemcpy_full_source_length(const wchar_t *src) {
+  wchar_t dest20[13];
+  wmemcpy(dest20, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest20[14];
+  // CHECK-FIXES-NEXT: wcscpy_s(dest20, src);
+}
+
+void good_wmemcpy_full_source_length(const wchar_t *src) {
+  wchar_t dst20[14];
+  wcsc

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

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



Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:377-381
+FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), ") + 1");
+Diag << InsertFirstParenFix << InsertPlusOneAndSecondParenFix;
+  } else {
+const auto InsertPlusOne =
+FixItHint::CreateInsertion(exprLocEnd(LengthExpr, Result), " + 1");

lebedev.ri wrote:
> The fixits are incorrect.
> Increment by `int(1)` can result in overflow, which is then extended to 
> `size_t`
> It should be `+ 1UL`.
> https://godbolt.org/g/4nQiek
Could you show me a true example where it causes a problem? I think it is too 
rare to rewrite all code as you mentioned.



Comment at: 
test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c:158-180
+void bad_memset_1(char *dest, const char *src) {
+  int length = getLengthWithInc(src);
+  memset(dest, '-', length);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'memset' 
is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: memset(dest, '-', length - 1);
+}
+

lebedev.ri wrote:
> ??
> These look like false-negatives.
> How do you make an assumption about `dest` buffer from `src` string length?
My idea here is you do not want to set your null terminator to anything else, 
because then the result is not null-terminated.


https://reviews.llvm.org/D45050



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 149116.
Charusso added a comment.

@xbolva00 idea implemented, doubled the checker's power. Now if the given 
argument is a DeclRefExpr this should handle it as it would be inlined as a 
simple CallExpr.

Results:
I have found 37 `memcpy()` and 2 `memchr()` errors in: curl, FFmpeg, git, 
memcached, OpenSSL, PostGreSQL, Redis, twin.

Example of this new DRE case:
F6291767: postgresql-src-common-md5-c.html 

Found `memchr()` errors:
F6291768: git-vcs-svn-fast_export-c.html 
F6291769: twin-clients-dialog-c.html 

All findings: F6291784: memcpy_and_memchr_errors.rar 



https://reviews.llvm.org/D45050

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tidy/bugprone/NotNullTerminatedResultCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c

Index: test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
@@ -0,0 +1,150 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c11
+
+typedef unsigned int size_t;
+typedef unsigned int wchar_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+wchar_t *wcschr(const wchar_t *, int);
+errno_t *wcsncpy_s(wchar_t *, const wchar_t *, size_t);
+
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+void *wmemcpy(void *, const void *, size_t);
+errno_t wmemcpy_s(void *, size_t, const void *, size_t);
+void *wmemchr(const void *, int, size_t);
+void *wmemmove(void *, const void *, size_t);
+errno_t wmemmove_s(void *, size_t, const void *, size_t);
+void *wmemset(void *, int, size_t);
+
+int wcsncmp(const wchar_t *, const wchar_t *, size_t);
+size_t wcsxfrm(wchar_t *, const wchar_t *, size_t);
+
+
+void bad_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'alloca' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: alloca(wcslen(src) + 1);
+}
+
+void good_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src) + 1);
+}
+
+void bad_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc(wcslen(src), sizeof(wchar_t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'calloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void good_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void bad_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc(wcslen(src) * 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'malloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: malloc((wcslen(src) * 2) + 1);
+}
+
+void good_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc((wcslen(src) * 2) + 1);
+}
+
+void bad_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + wcslen(src)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'realloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void good_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void bad_wmemcpy(wchar_t *dest, const wchar_t *src) {
+  wmemcpy(dest, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_safe(wchar_t *dest, const wchar_t *src) {
+  wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+  wmemcpy_s(dest, wcslen(dest), src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsn

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 149129.
Charusso added a comment.

Clang format.


https://reviews.llvm.org/D45050

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tidy/bugprone/NotNullTerminatedResultCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c

Index: test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
@@ -0,0 +1,150 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c11
+
+typedef unsigned int size_t;
+typedef unsigned int wchar_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+wchar_t *wcschr(const wchar_t *, int);
+errno_t *wcsncpy_s(wchar_t *, const wchar_t *, size_t);
+
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+void *wmemcpy(void *, const void *, size_t);
+errno_t wmemcpy_s(void *, size_t, const void *, size_t);
+void *wmemchr(const void *, int, size_t);
+void *wmemmove(void *, const void *, size_t);
+errno_t wmemmove_s(void *, size_t, const void *, size_t);
+void *wmemset(void *, int, size_t);
+
+int wcsncmp(const wchar_t *, const wchar_t *, size_t);
+size_t wcsxfrm(wchar_t *, const wchar_t *, size_t);
+
+
+void bad_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'alloca' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: alloca(wcslen(src) + 1);
+}
+
+void good_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src) + 1);
+}
+
+void bad_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc(wcslen(src), sizeof(wchar_t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'calloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void good_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void bad_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc(wcslen(src) * 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'malloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: malloc((wcslen(src) * 2) + 1);
+}
+
+void good_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc((wcslen(src) * 2) + 1);
+}
+
+void bad_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + wcslen(src)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'realloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void good_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void bad_wmemcpy(wchar_t *dest, const wchar_t *src) {
+  wmemcpy(dest, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_safe(wchar_t *dest, const wchar_t *src) {
+  wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+  wmemcpy_s(dest, wcslen(dest), src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+  wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemchr(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)wmemchr(src, '\0', wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: the result from calling 'wmemchr' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcschr(src, '\0');
+}
+
+void good_wmemchr(wchar_t *dest, const wchar_t *src) {
+  dest = wcschr(src, '\0');
+}
+
+void bad_wmemmove(wchar_t *dest, const wchar_t *src) {
+  wmemmove(dest, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In https://reviews.llvm.org/D45050#1116361, @xbolva00 wrote:

> memcpy(crypt_buf, passwd, passwd_len); <--- warning
>  memcpy(crypt_buf + passwd_len, salt, salt_len);
>
> This is a false warning since it appends strings using memcpy. But no idea 
> what to do and if it is possible to avoid these false warnings.


I have just tested it because of the `malloc()` function. I'm using CodeChecker 
and leaved the default settings, so `IsSafeFunctionsAreAvailable = 1`. Because 
of the malloc `strncpy_s()` cannot handle this case, but if the check would ran 
with `IsSafeFunctionsAreAvailable = 0`, it rewrites it to `strncpy(crypt_buf, 
passwd, passwd_len + 1)` which is a good transformation, as the official 
`memcpy()`'s result not null-terminated.


https://reviews.llvm.org/D45050



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In https://reviews.llvm.org/D45050#1116446, @lebedev.ri wrote:

> I would like to see more negative tests.
>  What does it do if the len/size is a constant?
>  Variable that wasn't just assigned with `strlen()` return?


Thanks for the comment! What do you mean by negative test?


https://reviews.llvm.org/D45050



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 150643.
Charusso marked 4 inline comments as done.
Charusso added a comment.

`memchr()` revision: it is problematic if the second argument is `'\0'`, there 
is no other case. Added a new type of matcher, to match function calls. More 
static functions and test cases and huge refactor.


https://reviews.llvm.org/D45050

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tidy/bugprone/NotNullTerminatedResultCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c

Index: test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
@@ -0,0 +1,150 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c11
+
+typedef unsigned int size_t;
+typedef unsigned int wchar_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+wchar_t *wcschr(const wchar_t *, int);
+errno_t *wcsncpy_s(wchar_t *, const wchar_t *, size_t);
+
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+void *wmemcpy(void *, const void *, size_t);
+errno_t wmemcpy_s(void *, size_t, const void *, size_t);
+void *wmemchr(const void *, int, size_t);
+void *wmemmove(void *, const void *, size_t);
+errno_t wmemmove_s(void *, size_t, const void *, size_t);
+void *wmemset(void *, int, size_t);
+
+int wcsncmp(const wchar_t *, const wchar_t *, size_t);
+size_t wcsxfrm(wchar_t *, const wchar_t *, size_t);
+
+
+void bad_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'alloca' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: alloca(wcslen(src) + 1);
+}
+
+void good_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src) + 1);
+}
+
+void bad_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc(wcslen(src), sizeof(wchar_t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'calloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void good_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void bad_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc(wcslen(src) * 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'malloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: malloc((wcslen(src) * 2) + 1);
+}
+
+void good_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc((wcslen(src) * 2) + 1);
+}
+
+void bad_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + wcslen(src)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'realloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void good_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void bad_wmemcpy(wchar_t *dest, const wchar_t *src) {
+  wmemcpy(dest, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_safe(wchar_t *dest, const wchar_t *src) {
+  wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+  wmemcpy_s(dest, wcslen(dest), src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+  wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemchr(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)wmemchr(src, '\0', wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: the length is too short for the last '\0' [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcschr(src, '\0');
+}
+
+void g

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

@lebedev.ri there is all the false positive results from the last publicated 
result-dump:

F6334659: curl-lib-curl_path-c.html 
second result: F6334660: ffmpeg-libavformat-sdp.c.html 

all result: F6334663: openssl-apps-ca-c.html 
F6334665: postgresql-src-interfaces-ecpg-preproc-pgc-c.html 

F6334667: redis-src-redis-benchmark-c.html 

(Note: the two `memchr()` result were false positive in that post and there is 
no new result with the new matcher.)

Does the new test cases cover your advice?




Comment at: docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst:105
+
+   An integer non-zero value specifying if the target version implements ``_s``
+   suffixed memory and character handler functions which is safer than older

whisperity wrote:
> Why is this an integer, rather than a bool?
This is how the other Tidy checkers are use their options, I do not know either.


https://reviews.llvm.org/D45050



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 150644.
Charusso added a comment.

Clang format.


https://reviews.llvm.org/D45050

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tidy/bugprone/NotNullTerminatedResultCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c

Index: test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
@@ -0,0 +1,150 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c11
+
+typedef unsigned int size_t;
+typedef unsigned int wchar_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+wchar_t *wcschr(const wchar_t *, int);
+errno_t *wcsncpy_s(wchar_t *, const wchar_t *, size_t);
+
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+void *wmemcpy(void *, const void *, size_t);
+errno_t wmemcpy_s(void *, size_t, const void *, size_t);
+void *wmemchr(const void *, int, size_t);
+void *wmemmove(void *, const void *, size_t);
+errno_t wmemmove_s(void *, size_t, const void *, size_t);
+void *wmemset(void *, int, size_t);
+
+int wcsncmp(const wchar_t *, const wchar_t *, size_t);
+size_t wcsxfrm(wchar_t *, const wchar_t *, size_t);
+
+
+void bad_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'alloca' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: alloca(wcslen(src) + 1);
+}
+
+void good_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src) + 1);
+}
+
+void bad_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc(wcslen(src), sizeof(wchar_t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'calloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void good_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void bad_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc(wcslen(src) * 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'malloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: malloc((wcslen(src) * 2) + 1);
+}
+
+void good_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc((wcslen(src) * 2) + 1);
+}
+
+void bad_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + wcslen(src)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'realloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void good_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void bad_wmemcpy(wchar_t *dest, const wchar_t *src) {
+  wmemcpy(dest, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_safe(wchar_t *dest, const wchar_t *src) {
+  wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+  wmemcpy_s(dest, wcslen(dest), src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+  wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemchr(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)wmemchr(src, '\0', wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: the length is too short for the last '\0' [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcschr(src, '\0');
+}
+
+void good_wmemchr(wchar_t *dest, const wchar_t *src) {
+  dest = wcschr(src, '\0');
+}
+
+void bad_wmemmove(wchar_t *dest, const wchar_t *src) {
+  wmemmove(dest, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen or wcslen

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 150645.
Charusso added a comment.

Fix some comment.


https://reviews.llvm.org/D45050

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tidy/bugprone/NotNullTerminatedResultCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-not-null-terminated-result-in-initialization.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c

Index: test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wcslen-safe.c
@@ -0,0 +1,150 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c11
+
+typedef unsigned int size_t;
+typedef unsigned int wchar_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+wchar_t *wcschr(const wchar_t *, int);
+errno_t *wcsncpy_s(wchar_t *, const wchar_t *, size_t);
+
+void *alloca(size_t);
+void *calloc(size_t, size_t);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+void *wmemcpy(void *, const void *, size_t);
+errno_t wmemcpy_s(void *, size_t, const void *, size_t);
+void *wmemchr(const void *, int, size_t);
+void *wmemmove(void *, const void *, size_t);
+errno_t wmemmove_s(void *, size_t, const void *, size_t);
+void *wmemset(void *, int, size_t);
+
+int wcsncmp(const wchar_t *, const wchar_t *, size_t);
+size_t wcsxfrm(wchar_t *, const wchar_t *, size_t);
+
+
+void bad_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'alloca' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: alloca(wcslen(src) + 1);
+}
+
+void good_alloca(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)alloca(wcslen(src) + 1);
+}
+
+void bad_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc(wcslen(src), sizeof(wchar_t));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'calloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void good_calloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)calloc((wcslen(src) + 1), sizeof(wchar_t));
+}
+
+void bad_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc(wcslen(src) * 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'malloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: malloc((wcslen(src) * 2) + 1);
+}
+
+void good_malloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)malloc((wcslen(src) * 2) + 1);
+}
+
+void bad_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + wcslen(src)));
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: memory allocated by 'realloc' is insufficient to hold the null terminator [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void good_realloc(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)realloc(dest, (wcslen(dest) + (wcslen(src) + 1)));
+}
+
+void bad_wmemcpy(wchar_t *dest, const wchar_t *src) {
+  wmemcpy(dest, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_safe(wchar_t *dest, const wchar_t *src) {
+  wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+  wmemcpy_s(dest, wcslen(dest), src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcsncpy_s(dest, src, wcslen(src));
+}
+
+void good_wmemcpy_s(wchar_t *dest, const wchar_t *src) {
+  wcsncpy_s(dest, src, wcslen(src));
+}
+
+void bad_wmemchr(wchar_t *dest, const wchar_t *src) {
+  dest = (wchar_t *)wmemchr(src, '\0', wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:40: warning: the length is too short for the last '\0' [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcschr(src, '\0');
+}
+
+void good_wmemchr(wchar_t *dest, const wchar_t *src) {
+  dest = wcschr(src, '\0');
+}
+
+void bad_wmemmove(wchar_t *dest, const wchar_t *src) {
+  wmemmove(dest, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result 

[PATCH] D53076: [analyzer] WIP: Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added subscribers: baloghadamsoftware, whisperity.
Charusso added a comment.

In https://reviews.llvm.org/D53076#1260663, @george.karpenkov wrote:

> The change makes sense to me, but:
>
> 1. Note that "Assuming X" directives are useful for the analyzer developers, 
> since they know that's where the checker makes arbitrary assumptions, but to 
> end users that mostly feels like noise ("Taking true branch" is there 
> already, why there should be "Assuming 'i' is > 0" as well?)
> 2. @NoQ do you know why the GDM comparison was there in the first place? The 
> commit was made by Ted in 2011, maybe constraint changes had to be reflected 
> in the GDM at that point (?)


Based on @baloghadamsoftware's idea (https://reviews.llvm.org/D34508) the main 
goal is to print out trivial values for the Clang SA, but lot of time consuming 
nonsense for developers. As you mentioned it is bad to have multiple 
assumptions, but half of the reporters behave like that, and the other half 
does not print any/useful information.

That two separate project based on that current project: **create** the 
reports, and make the style identical, so that we could enhance them. The best 
example here is `test/Analysis/diagnostics/macros.cpp`: on line 27 you could 
see the message `Assuming the condition is true` but the upper conditions miss 
this report.


https://reviews.llvm.org/D53076



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks everyone for the contribution!


https://reviews.llvm.org/D45050



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a subscriber: klimek.
Charusso added a comment.

In https://reviews.llvm.org/D45050#1264435, @JonasToth wrote:

> Unfortunatly this check does not compile on some ARM platforms. It seems that 
> a matcher exceeds the recursion limit in template instantations.
>
> http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/4405/steps/build%20stage%201/logs/stdio


Well, I can do nothing, but ask for @klimek's help.

Manuel, could you review this error please?


https://reviews.llvm.org/D45050



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In https://reviews.llvm.org/D53076#1261134, @NoQ wrote:

> For example, in the `inline-plist.c`'s `bar()` on line 45, Static Analyzer 
> indeed doesn't assume that `p` is equal to null; instead, Static Analyzer 
> *knows* it for sure.


Thanks you! This a great example what I have to cover later on. I have a patch 
where we print out known integers. The basic style is the following: `Assuming 
'x' is not equal to 1`. I would like to emphasize the value and if it a known 
value, make it looks like this: `Variable 'x' is equal to '1'`, or `Variable 
'*ptr' is equal to '1'`. (If this is the situation: `Constant 'x' is equal to 
'1'` would be cool as well.)

I made that patch in a separated file called `BugReporterHelpers.cpp` next to 
the `BugReporterVisitors`. I also would like to move all the helper functions 
from `BugReporterVisitors.cpp` to that source file. My first idea with that to 
create a live documentation, how would a new clang-hacker obtain a value from a 
certain position (me with testing those things out). Also what you mentioned 
with this flow-sensitive chaining, this is could not be a short patch, so I 
think this is the time when we want to have something like this.

What do you think? If this patch goes well, should I attach the mentioned new 
patch to this, or create a new one?


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: 
include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h:202
+  void finalizeConstraints() {
+Constraints.clear();
+  }

george.karpenkov wrote:
> These constraints are conceptually part of the visitor, not part of the 
> constraint manager. Could they be simply stored in the visitor?
My idea was to have a generic constraint map as @NoQ mentioned, then we could 
attach this to other places to reduce noisy reports. But probably this is the 
best place for now.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:244
+if (I == Constraints.end() || !Message.equals(I->second)) {
+  Constraints[Cond] = Message;
+  return true;

george.karpenkov wrote:
> Isn't that equivalent to `Constraints.insert(make_pair(Cond, 
> Message)).second` ?
> I think I have written that before.
We have multiple messages at the same place so we have to update the message. 
The problem with your code is `insert` operates with disjunct keys, not values.


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1853
+
+const auto *Cond = cast(PS->getStmt());
+auto Piece = VisitTrueTest(Cond, tag == tags.first, BRC, BR, N);

george.karpenkov wrote:
> How do we know that it's always an `Expr`?
`VisitTrueTest` operates with `Expr` only, so I just left the type.



Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1970
 
+bool ConditionBRVisitor::insertOrUpdateConstraintMessage(const Stmt *Cond,
+ StringRef Message) {

george.karpenkov wrote:
> 1. What about the refactoring I have previously suggested twice?
> 2. I know you have started with that -- and sorry for spurious changes -- but 
> I also think your original name of `constraintsChanged` is better.
> We have multiple messages at the same place so we have to update the message. 
> The problem with your code is insert operates with disjunct keys, not values.

I have commented this after the last refactor, sorry for the inconvenience.


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks everyone for the review!

@george.karpenkov could you commit it please? I am interested in your ideas in 
the little discussion started with @NoQ at the beginning of the project.


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In https://reviews.llvm.org/D53076#1276270, @george.karpenkov wrote:

> @NoQ has a good point: we need to preserve the distinction between the things 
> analyzer "assumes" and the things analyzer "knows".


Yes, but I think it should be a new patch because I would like to add a 
value-dump method on integers/booleans and stuff I already mentioned in the 
beginning.


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In https://reviews.llvm.org/D53076#1276277, @NoQ wrote:

> I mean, the idea of checking constraints instead of matching program points 
> is generally good, but the example i gave above suggests that there's a bug 
> somewhere.


I think it is an unimplemented feature which appear like 1:500 time, but we 
will see.


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: test/Analysis/MisusedMovedObject.cpp:187
 A a;
-if (i == 1) { // expected-note {{Taking false branch}} expected-note 
{{Taking false branch}}
+if (i == 1) { // expected-note {{Assuming 'i' is not equal to 1}} 
expected-note {{Taking false branch}}
+  // expected-note@-1 {{Assuming 'i' is not equal to 1}} expected-note@-1 
{{Taking false branch}}

NoQ wrote:
> These assumptions were already made on the previous branches. There should be 
> no extra assumptions here.
Agree but only if there is no extra constraint EventPiece between them.



Comment at: test/Analysis/MisusedMovedObject.cpp:221
 }
-if (i > 5) { // expected-note {{Taking true branch}}
+if (i > 5) { // expected-note {{Assuming 'i' is > 5}} expected-note 
{{Taking true branch}}
   a.foo();   // expected-warning {{Method call on a 'moved-from' object 
'a'}} expected-note {{Method call on a 'moved-from' object 'a'}}

NoQ wrote:
> We have assumed that `i` is `>= 10` on the previous branch. It imples that 
> `i` is greater than `5`, so no additional assumption is being made here.
Agree but only if there is no extra constraint EventPiece between them.



Comment at: test/Analysis/NewDelete-path-notes.cpp:10
   if (p)
-// expected-note@-1 {{Taking true branch}}
+// expected-note@-1 {{Assuming 'p' is non-null}}
+// expected-note@-2 {{Taking true branch}}

NoQ wrote:
> Static Analyzer knows that the standard operator new never returns null. 
> Therefore no assumption is being made here.
As I see SA knows nothing. Where to teach it?



Comment at: test/Analysis/inline-plist.c:46
   if (p == 0) {
-// expected-note@-1 {{Taking true branch}}
+// expected-note@-1 {{Assuming 'p' is equal to null}}
+// expected-note@-2 {{Taking true branch}}

NoQ wrote:
> The condition `!!p` above being assumed to false ensures that `p` is equal to 
> `null` here. We are not assuming it again here.
Agree but only if there is no extra constraint EventPiece between them.


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: test/Analysis/Inputs/expected-plists/cxx-for-range.cpp.plist:837
+ message
+ Variable 'fail' is true
+

Double negating is not in standard English, so this behaviour is documented 
here.



Comment at: test/Analysis/Inputs/expected-plists/edges-new.mm.plist:9629
+ Variable 'z' is equal to 0
+
+

@NoQ this is the only place (as I see) where SA works with flow-sensitive 
constraint-handling what you mentioned. Is it a good change?


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks you @NoQ! Luckily it is rely on `ProgramPoints` now. The only problem as 
I see the mentioned Z3-test, where I have no idea what happened.


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

https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 8 inline comments as done.
Charusso added a comment.

@NoQ thanks you for the great explanation! I really wanted to write out known 
constant integers and booleans but I think 'Knowing...' pieces would be more 
cool.

I tried to minimize the changes after a little e-mailing with @george.karpenkov 
on the mailing list. The problem is the following: we go backwards on the 
bug-path, that is why we see range information changes backwards, but the user 
information goes forwards: from the top to the bottom. I have implemented the 
'check upwards' feature, so now everything working fine, but I think it is a 
very time-consuming approach. (The idea is copied from 
`BugReporter.cpp/generateVisitorsDiagnostics()` where I tried to flip the path, 
unsuccessfully: 
http://clang-developers.42468.n3.nabble.com/Visit-nodes-in-the-order-of-the-user-output-to-create-better-reports-td4062898.html
 )


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: test/Analysis/diagnostics/macros.cpp:33
 void testDoubleMacro(double d) {
-  if (d == DBL_MAX) { // expected-note {{Taking true branch}}
+  if (d == DBL_MAX) { // expected-note {{Assuming 'd' is equal to DBL_MAX}}
+  // expected-note@-1 {{Taking true branch}}

NoQ wrote:
> This one's good. Static Analyzer doesn't understand floats, so this branch is 
> indeed non-trivial. There should indeed be an assuming... piece here.
A little bit misunderstandable, because we do not know anything about doubles. 
May the generic message fit better?



Comment at: test/Analysis/diagnostics/undef-value-param.m:56
 SCDynamicStoreRef ref = anotherCreateRef(&err, x);
-if (err) { 
-   //expected-note@-1{{Assuming 'err' is not equal to 0}}
-   //expected-note@-2{{Taking true branch}}
-CFRelease(ref);
-ref = 0; // expected-note{{nil object reference stored to 'ref'}}
+if (err) { //expected-note{{Taking true branch}}
+  CFRelease(ref);

I am not sure why but the range here [1, (2^64)-1]. There is would not be any 
number like 0?



Comment at: test/Analysis/new-ctor-malloc.cpp:11
   void *x = malloc(size); // expected-note {{Memory is allocated}}
-  if (!x) // expected-note{{Assuming 'x' is non-null}}
-  // expected-note@-1 {{Taking false branch}}
+  if (!x) // expected-note {{Taking false branch}}
 return nullptr;

The range information is [1, (2^64)-1] but as I saw `malloc` could return null. 
Who is lying here?



Comment at: test/Analysis/uninit-vals.m:167
   testObj->origin = makePoint(0.0, 0.0);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming the condition is 
false}}
+   // expected-note@-1{{Taking false branch}}

NoQ wrote:
> These are pretty weird. As far as i understand the test, these should be 
> there. But i'm suddenly unable to debug why were they not shown before, 
> because there's either something wrong with exploded graph dumps or with the 
> exploded graph itself; it appears to be missing an edge right after `size > 
> 0` is assumed. I'll look more into those.
We gather information with `clang-analyzer-eval` functions so the conditions 
would be `Knowing...` pieces but the `BugReporter.cpp` does not know about 
these extra assumptations. Where to teach to do not `Assuming...`, but 
`Knowing...` these?



Comment at: test/Analysis/virtualcall.cpp:172
 #endif
   X x(i - 1);
 #if !PUREONLY

Because this `X x(i - 1);` we assume these conditions? This one is tricky to be 
`Knowing...`.


https://reviews.llvm.org/D53076



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

[PATCH] D54560: [analyzer] MoveChecker Pt.3: Improve warning messages a bit.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In https://reviews.llvm.org/D54560#1301870, @NoQ wrote:

> Write down full messages in tests. When the message was updated from `'x' is 
> moved'` to `Object 'x' is moved`, the tests were not updated because they 
> kept passing because the former is still a sub-string of the latter.


It would not be cool to rewrite this `contains` effect to equality like in the 
regex file-checks? I have ran in the same problem and I was very surprised (for 
an hour) why my error-dumps does not show up on rewriting bug-report messages.


https://reviews.llvm.org/D54560



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In https://reviews.llvm.org/D53076#1305209, @Szelethus wrote:

> In the meanwhile we managed to figure out where the problem lays, so if 
> you're interested, I'm going to document it here.
>
> The patch attempts to solve this by inspecting the actual condition, and 
> tries to find out whether the symbols inside that condition has changed in 
> between the states, but this is overkill solution, as comparing the two 
> `ConstraintManager` objects would achieve the same thing.


I tried to investigate what is happening after we entered into a condition, 
which is working well with 'Knowing...' pieces, but with too much overhead. 
Because this patch is all about write out more information, rather than 
removing the GDM-checking I will move forward with 'Knowing...' pieces. The 
current questions are real for that project, just let me simplify the code and 
create that feature.


https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Sorry for the huge noise! It took me a while to see what is the problem.


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

https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

@Szelethus thanks you for the comments!

In D53076#1307336 , @Szelethus wrote:

> I think the reason why the printed message was either along the lines of 
> "this is 0" and "this is non-0", is that we don't necessarily know what 
> constraint solver we're using, and adding more non-general code `BugReporter` 
> is most probably a bad approach.


It was an unimplemented feature to write out known stuff. There is no 
constraint solving, it is rather constant value dumping.

> How about we tinker with `ContraintManager` a little more, maybe add an 
> `explainValueOfVarDecl`, that would return the value of a variable in a given 
> `ProgramState`? We could implement it for `RangeConstraintSolver` 
> (essentially move the code you already wrote there), and leave a `TODO` with 
> a super generic implementation for Z3.

I agree with @george.karpenkov, we want to have the smallest scope.




Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1892-1920
+  const auto CurrentCR = N->getState()->get();
+  const auto PredCR = PredState->get();
+  const auto PredPredCR = PredPredState->get();
+
+  // We only want to check BlockEdges once so we have to determine if the 
change
+  // of the range information is not happened because of dead symbols.
+  //

Szelethus wrote:
> `ConstraintRange`, as far as I know, is the data  `RangedConstraintManager` 
> stores in the GDM. What if we're using a different constraint solver? I think 
> it'd be better to take the more abstract approach, extend 
> `ConstraintManager`'s interface with an `isEqual` or `operator==` pure 
> virtual method, and implement it within it's descendants.
Because we only operate in the `ConditionBRVisitor` it would be useless for now.


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

https://reviews.llvm.org/D53076



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso marked 2 inline comments as done.
Charusso added a comment.

@george.karpenkov thanks you for the comments!

In D53076#1308641 , @george.karpenkov 
wrote:

> What about just dropping the `Knowing` prefix?
>  Just having `arr is null, taking true branch` seems considerably more 
> readable.


I wanted to create something identical to the current reports. If we are about 
to change readability, I would change that two first:

- identical operator printing with `<, <=, >...`, so `equal to` is `=` and `not 
equal to` is `!=`.
- emphasize the value information with quotes, so `Assuming 'i' >= '2'`.




Comment at: test/Analysis/uninit-vals.m:324
   testObj->origin = makeIntPoint2D(1, 2);
-  if (testObj->size > 0) { ; } // expected-note{{Taking false branch}}
+  if (testObj->size > 0) { ; } // expected-note{{Assuming 'testObj->size' is 
<= 0}}
// expected-note@-1{{Taking false branch}}

george.karpenkov wrote:
> That does not seem right: from `calloc` the analyzer should know that the 
> `testObj->size` is actually zero.
That `Assuming...` piece is rely on the change between the last two diffs: I 
just print out more `VarDecls` in `patternMatch()`. I thought everything works 
fine, so I will check that problem if @NoQ will not be faster as he already 
found some problematic code piece in that test file.



Comment at: test/Analysis/virtualcall.cpp:170
+   // expected-note-re@-4 ^}}Assuming 'i' is <= 0}}
+   // expected-note-re@-5 ^}}Taking false branch}}
 #endif

george.karpenkov wrote:
> Could you describe what happens here?
> Why the `assuming` notes weren't there before?
We do not have range information in the *newly seen `Assuming...` pieces. 
Because the analyzer has not learnt new information - as there is no 
information - we have not entered to `VisitTrueTest()` to report these. A good 
reference for that new behaviour is in 
`test/Analysis/Inputs/expected-plists/cxx-for-range.cpp.plist`.

*I have added these with https://reviews.llvm.org/D53076?id=175177


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

https://reviews.llvm.org/D53076



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso requested changes to this revision.
Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1985
 bool partOfParentMacro = false;
+StringRef PName = "";
 if (ParentEx->getBeginLoc().isMacroID()) {

`ParentName`?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1996
+  StringRef MacroName = StartName;
+  while (true) {
 LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart);

I was never cool in algorithms, but I think it is over-complicated. In Hungary 
we are about to build a new nuclear power plant so here it is really 
emphasized: never-ever create an infinite loop.
What about the following?:
```
  while (LocStart.isMacroID()) {
 
StringRef CurrentMacroName = 
Lexer::getImmediateMacroNameForDiagnostics( 
LocStart, BRC.getSourceManager(),   
 
BRC.getASTContext().getLangOpts()); 
 
LocStart = BRC.getSourceManager().getImmediateMacroCallerLoc(LocStart); 
 
if (CurrentMacroName != ParentName) 
  
  MacroName = CurrentMacroName; 
 
  }
```



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2003
+LocStart, BRC.getSourceManager(),
+BRC.getASTContext().getLangOpts());
+if (NextMacroName == PName)

There is too much duplication and it is difficult to understand what is going 
on. May here is the time for deduplicating?
Please note that, there is a function called `getMacroName(SourceLocation, 
BugReporterContext &)`, may you would put your own helper-function above that.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59121



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

[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D58367#1423451 , @NoQ wrote:

> Found a bug! The lambda has to manually make sure that the bug report 
> actually does have something to do with the checker.


I think message-semantic is better than storing some data in two places. 
BugReport has `getCheckName()` and may if the `NoteTag` knows the name of its 
author then it is comparable.
I am not sure but may you could hack the lifetime of that `StringRef` from 
`CheckName` to not to report on something purged out (/not exists?).


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

https://reviews.llvm.org/D58367



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

[PATCH] D54811: [analyzer] ConditionBRVisitor: Remove GDM checking

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC356318: [analyzer] ConditionBRVisitor: Remove GDM checking 
(authored by Charusso, committed by ).
Herald added a subscriber: cfe-commits.

Repository:
  rC Clang

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

https://reviews.llvm.org/D54811

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp


Index: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -588,11 +588,15 @@
   ProgramStateRef getPersistentStateWithGDM(ProgramStateRef FromState,
ProgramStateRef GDMState);
 
-  bool haveEqualEnvironments(ProgramStateRef S1, ProgramStateRef S2) {
+  bool haveEqualConstraints(ProgramStateRef S1, ProgramStateRef S2) const {
+return ConstraintMgr->haveEqualConstraints(S1, S2);
+  }
+
+  bool haveEqualEnvironments(ProgramStateRef S1, ProgramStateRef S2) const {
 return S1->Env == S2->Env;
   }
 
-  bool haveEqualStores(ProgramStateRef S1, ProgramStateRef S2) {
+  bool haveEqualStores(ProgramStateRef S1, ProgramStateRef S2) const {
 return S1->store == S2->store;
   }
 
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
@@ -220,6 +220,11 @@
 OS << nl;
   }
 
+  bool haveEqualConstraints(ProgramStateRef S1,
+ProgramStateRef S2) const override {
+return S1->get() == S2->get();
+  }
+
   bool canReasonAbout(SVal X) const override {
 const TargetInfo &TI = getBasicVals().getContext().getTargetInfo();
 
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
@@ -79,6 +79,9 @@
   ConstraintManager() = default;
   virtual ~ConstraintManager();
 
+  virtual bool haveEqualConstraints(ProgramStateRef S1,
+ProgramStateRef S2) const = 0;
+
   virtual ProgramStateRef assume(ProgramStateRef state,
  DefinedSVal Cond,
  bool Assumption) = 0;
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -1816,13 +1816,10 @@
   BugReporterContext &BRC, BugReport &BR) {
   ProgramPoint progPoint = N->getLocation();
   ProgramStateRef CurrentState = N->getState();
-  ProgramStateRef PrevState = N->getFirstPred()->getState();
+  ProgramStateRef PreviousState = N->getFirstPred()->getState();
 
-  // Compare the GDMs of the state, because that is where constraints
-  // are managed.  Note that ensure that we only look at nodes that
-  // were generated by the analyzer engine proper, not checkers.
-  if (CurrentState->getGDM().getRoot() ==
-  PrevState->getGDM().getRoot())
+  // If the constraint information does not changed there is no assumption.
+  if (BRC.getStateManager().haveEqualConstraints(CurrentState, PreviousState))
 return nullptr;
 
   // If an assumption was made on a branch, it should be caught
Index: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -230,6 +230,11 @@
   // Implementation for interface from ConstraintManager.
   //===--===//
 
+  bool haveEqualConstraints(ProgramStateRef S1,
+ProgramStateRef S2) const override {
+return S1->get() == S2->get();
+  }
+
   bool canReasonAbout(SVal X) const override;
 
   ConditionTruthVal checkNull(ProgramStateRef State, SymbolRef Sym) override;


Index: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
@@ -588,11 +588,15 @@
   ProgramStateRef get

[PATCH] D57896: Variable names rule

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added subscribers: chandlerc, Charusso.
Charusso added a comment.

In D57896#1406812 , @lattner wrote:

> I can understand Zach's position here, but LLDB has historically never 
> conformed to the general LLVM naming or other conventions due to its 
> heritage.  It should not be taken as precedent that the rest of the project 
> should follow.
>
> In any case, I think that it is best for this discussion to take place on the 
> llvm-dev list where it is likely to get the most visibility.  Would you mind 
> moving comments and discussions there?


Hey! Random Clang developer is here after this topic became a little-bit dead 
as not everyone subbed to LLVM dev-list. I think the best solution for every 
difficult question is to let the users decide their own future among all the 
projects. I would announce polls (https://reviews.llvm.org/vote/) and announce 
them on every dev-list.

I do not see any better solution to decide if we would code like `DRE`, `VD` 
versus `expr`, `decl` as @lattner would code. And I am not sure if everyone 
happy with `this_new_styling` as @chandlerc and @zturner would code. E.g. I am 
not happy because I know my if-statements would take two lines of code instead 
of one and it would be super-duper-mega ugly and difficult to read. Here is an 
example:

  static Optional
  getConcreteIntegerValue(const Expr *CondVarExpr, const ExplodedNode *N) {
  //...
if (const auto *DRE = dyn_cast_or_null(CondVarExpr)) {
  if (const auto *VD = dyn_cast_or_null(DRE->getDecl())) {
  //...
  }

would be:

  static Optional 
  |
  getConcreteIntegerValue(const Expr *cond_var_expr, const ExplodedNode *node) 
{  |
  //... 
  |
if (const auto *decl_ref_expr = 
dyn_cast_or_null(cond_var_expr)) {
  if (const auto *var_decl = 
dyn_cast_or_null(decl_ref_expr->getDecl())) {
  //... 
  |
  } whoops 
column-81 ~^

Hungarian notation on members and globals are cool idea. However, the notation 
is made without the `_` part, so I think `mMember` is better than `m_member` as 
we used to 80-column standard and it is waste of space and hurts your 
C-developer eyes. I would recommend `b` prefix to booleans as Unreal Engine 4 
styling is used to do that (`bIsCoolStyle`) and it is handy. It is useful 
because booleans usually has multiple prefixes: `has, have, is` and you would 
list all the booleans together in autocompletion. Yes, there is a problem: if 
the notation is not capital like the pure Hungarian notation then it is 
problematic to list and we are back to the `BIsCapitalLetter` and `MMember` 
CamelCase-world where we started (except one project). I think @lattner could 
say if it is useful as all the Apple projects based on those notations and 
could be annoying.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D57896: Variable names rule

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D57896#1435245 , @lattner wrote:

> FWIW, my suggestion is *not* to expand names like DRE to decl_ref_expr, I 
> agree that doesn't add clarity to the code.  Two possibilities: "dre", or 
> "decl" which is what I would write today.


I totally agree with you, wherever I can I write that out for clarification. 
Please note that in my example there is two `Expr` and that is why I pointed 
out we need acronyms so we cannot really use `expr` and acronyms usually 
capital, that is why we went back to the default CamelCase standard. It was a 
little brainstorming and ping for you guys because I believe you would put 
those polls out and create a better code-base.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57896



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

[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

[Csaba Dabis via Phabricator via cfe-commits]

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

Cool! I have found this message-semantic idea useful in Unity where every 
`GameObject` could talk with each other in a very generic way 
(https://docs.unity3d.com/ScriptReference/GameObject.SendMessage.html).
I am looking forward for more!


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

https://reviews.llvm.org/D58367



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

[PATCH] D59857: [analyzer] PR37501: Disable the assertion for reverse-engineering logical op short circuits.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

There is no connection with my reverse-engineering reverted patch: 
https://reviews.llvm.org/D57410?id=184992 ? It evaluates the left and the right 
side and may it could help.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59857



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

[PATCH] D59861: [analyzer] NFC: Replace Taint API with a usual inter-checker communication API?

[Csaba Dabis via Phabricator via cfe-commits]

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

I like the idea to put connecting stuff together in the same file to create 
more understandable code. LGTM.




Comment at: clang/lib/StaticAnalyzer/Checkers/Taint.h:8
+//
+//===--===//
+//

Outdated header-blurb.



Comment at: clang/lib/StaticAnalyzer/Checkers/Taint.h:81
+
+void dumpTaint(ProgramStateRef State);
+

We left the `ProgramState::dump()' context so what about just `dump()`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59861



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

[PATCH] D59901: [analyzer] PR41239: Fix a crash on invalid source location in NoStoreFuncVisitor.

[Csaba Dabis via Phabricator via cfe-commits]

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

Nice solution.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59901



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D59121#1448367 , @NoQ wrote:

> On second thought, dunno. In the scan-build macro preview it wouldn't show 
> you UINT32_MAX anyway. Maybe let's keep this behavior.
>
> Cleaned up the patch a little bit.


Somehow on the `Assuming ...` pieces we write out the macro name, and my little 
code did that too. We could obtain the value but I think your first intention 
is the correct behaviour. Why are you not using my working example?


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

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D59121#1448386 , @NoQ wrote:

> Mmm, that *is* an `Assuming...` piece, i.e., this is the same code, just the 
> structure of macros is more complicated than usual.


You told me we would like to see a value when we hover over a name, which is 
cool. If I think about C and they do not C# over the complicated macros, I 
would like to C names of macros. It is up to you, but your first intention 
working with my code very well.


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

https://reviews.llvm.org/D59121



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

[PATCH] D59121: [analyzer] Fix macro names in diagnostics within bigger macros.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.

Nice catch!


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

https://reviews.llvm.org/D59121



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

[PATCH] D60107: [analyzer] NoStoreFuncVisitor: Suppress bug reports with no-store in system headers.

[Csaba Dabis via Phabricator via cfe-commits]

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

It is very good to try one improvement in another similar function.




Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:364
   QualType T = PVD->getType();
-  while (const MemRegion *R = S.getAsRegion()) {
-if (RegionOfInterest->isSubRegionOf(R) && !isPointerToConst(T))
-  return notModifiedDiagnostics(N, {}, R, ParamName,
-ParamIsReferenceType, 
IndirectionLevel);
+  while (const MemRegion *MR = S.getAsRegion()) {
+if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))

`R` was cool, that is our unspoken naming convention. What about 
`CallR`/`CallRegion` and possibly `CallSVal`? If I am right usually the 
`BugReport` object is named `BR` because of our regions.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:547
 
   /// \return Diagnostics piece for region not modified in the current 
function.
   std::shared_ptr

Update that comment?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:552
+ const MemRegion *MatchedRegion, StringRef FirstElement,
+ bool FirstIsReferenceType, unsigned IndirectionLevel) {
+// Optimistically suppress uninitialized value bugs that result

What about `tryToEmitNote()`? This 'or' condition is very uncommon for a 
function name. Also there is another `R` what could be `BR`.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:567
+  // other functions that have more paths within them, this suppression
+  // would still apply when we visit these inner functions.
+  if (N->getStackFrame()->getCFG()->size() > 3)

The Phabricator comment is better because it has a cool example. What about 
merging that into this one?



Comment at: clang/test/Analysis/no-store-suppression.cpp:3
+
+// expected-no-diagnostics
+

Could you inject a link for the diff or copy the information for further 
improvements why no diagnostic happen?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60107



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

[PATCH] D53076: [analyzer] ConditionBRVisitor: Enhance to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Thanks you @NoQ for the great idea!

Without the duplicated `BugReport.cpp` reports it could be an on-by-default 
patch, see:

Before:
F8685549: before.html 

After:
F8685550: after.html 

It is not perfect, but I like that and I have already got ideas how to use 
these new pieces.




Comment at: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:187-190
+  // The value may hard-coded.
+  SVal HardCodedSVal = State->getSVal(CondVarExpr, LCtx);
+  if (auto HardCodedCI = HardCodedSVal.getAs())
+return &HardCodedCI->getValue();

NoQ wrote:
> I don't see this triggered on tests. It looks to me that this function is for 
> now only applied to `DeclRefExpr`s that are always glvalues and therefore 
> should never be evaluated to a `nonloc` value.
Good catch!


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

https://reviews.llvm.org/D53076



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

[PATCH] D60732: [analyzer] NFC: Use -verify=... in MoveChecker tests.

[Csaba Dabis via Phabricator via cfe-commits]

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

I think this functionality is unused because you would split the file into six 
to reduce the overhead/scroll and that is it.

It is a cool reveal, could you provide a documentation?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60732



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

[PATCH] D60845: [VerifyDiagnosticConsumer] Document -verify= in doxygen

[Csaba Dabis via Phabricator via cfe-commits]

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

Thanks you!

I really like live working examples, I hope not just me. Could you link 
https://github.com/llvm/llvm-project/blob/master/clang/test/Analysis/use-after-move.cpp
 as well as a live example?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60845



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

[PATCH] D61051: [analyzer] Treat functions without runtime branches as "small".

[Csaba Dabis via Phabricator via cfe-commits]

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

Great patch! There is only a design problem:
You have negated every `isSmall()` condition looks like you could write 
`isLarge()` instead but there is no connection between these functions.
Could you rename or redesign them? The following would be cool: `isLarge()` iff 
`!isSmalll` and `isSmall()` iff `!isLarge()`.


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

https://reviews.llvm.org/D61051



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

[PATCH] D61285: [analyzer] SmartPtrModeling: Fix a null dereference.

[Csaba Dabis via Phabricator via cfe-commits]

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

When I see `->*` I go mad, but we should handle unknown stuff, cool patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61285



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

[PATCH] D61545: [analyzer] Fix a crash in RVO from within blocks.

[Csaba Dabis via Phabricator via cfe-commits]

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

I think the entire `LocationContext` stuff is a huge design issue, and you used 
its functionality without any hack. If you would rename the `getStackFrame` to 
`getNextStackFrame` or something, it is clear it is not a getter and traversing 
every frame until the top-frame.




Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:203
+cast(SFC->getCallSite()), State, CallerSFC,
 RTC->getConstructionContext(), CallOpts);
   } else {

The smallest the scope between the definition and its usage, the better the 
code. Could you put the new code immediately before the return statement?


Repository:
  rC Clang

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

https://reviews.llvm.org/D61545



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 166802.
Charusso marked 28 inline comments as done.
Charusso added a comment.

- Refactor and better English thanks for @whisperity!

- Removed `InjectUL` option so the checker handles the special case if the 
capacity of a buffer is `int.Max()`


https://reviews.llvm.org/D45050

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tidy/bugprone/NotNullTerminatedResultCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-not-null-terminated-result-in-initialization-strlen.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-cxx.cpp
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-other.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen.cpp
  test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp

Index: test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
@@ -0,0 +1,135 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c++11
+
+#define __STDC_WANT_LIB_EXT1__ 1
+
+typedef unsigned int size_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+template 
+errno_t wcsncpy_s(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+errno_t wcsncpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+
+template 
+wchar_t *wcsncpy(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+wchar_t *wcsncpy(wchar_t *, const wchar_t *, size_t);
+
+template 
+errno_t wcscpy_s(wchar_t (&dest)[size], const wchar_t *);
+errno_t wcscpy_s(wchar_t *, size_t, const wchar_t *);
+
+template 
+wchar_t *wcscpy(wchar_t (&dest)[size], const wchar_t *);
+wchar_t *wcscpy(wchar_t *, const wchar_t *);
+
+errno_t wmemcpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+wchar_t *wmemcpy(wchar_t *, const wchar_t *, size_t);
+
+
+//===--===//
+// wmemcpy() - destination array tests
+//===--===//
+
+void bad_wmemcpy_known_dest(const wchar_t *src) {
+  wchar_t dest01[13];
+  wmemcpy(dest01, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest01[14];
+  // CHECK-FIXES-NEXT: wcscpy_s(dest01, src);
+}
+
+void good_wmemcpy_known_dest(const wchar_t *src) {
+  wchar_t dst01[14];
+  wcscpy_s(dst01, src);
+}
+
+//===--===//
+// wmemcpy() - length tests
+//===--===//
+
+void bad_wmemcpy_full_source_length(const wchar_t *src) {
+  wchar_t dest20[13];
+  wmemcpy(dest20, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest20[14];
+  // CHECK-FIXES-NEXT: wcscpy_s(dest20, src);
+}
+
+void good_wmemcpy_full_source_length(const wchar_t *src) {
+  wchar_t dst20[14];
+  wcscpy_s(dst20, src);
+}
+
+void bad_wmemcpy_partial_source_length(const wchar_t *src) {
+  wchar_t dest21[13];
+  wmemcpy(dest21, src, wcslen(src) - 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest21[14];
+  // CHECK-FIXES-NEXT: wcsncpy_s(dest21, src, wcslen(src) - 1);
+}
+
+void good_wmemcpy_partial_source_length(const wchar_t *src) {
+  wchar_t dst21[14];
+  wcsncpy_s(dst21, src, wcslen(src) - 1);
+}
+
+//===--===//
+// wmemcpy_s() - destination array tests
+//===--===//
+
+void bad_wmemcpy_s_unknown_dest(wchar_t *dest40, const wchar_t *src) {
+  wmemcpy_s(dest40, 13, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy_s' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wcscpy_s(dest40, 13, src);
+}
+
+void good_wmemcpy_s_unknown_dest(wchar_t *dst40, const wchar_t *src) {
+  wcscpy_s(dst40, 13, src);
+}
+
+void bad_wmemcpy_s_known_dest(const wchar_t *src) {
+  wchar_t dest41[13];
+  wmemcpy_s(dest41, 13, src, wcslen(src));
+  // CHECK

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

The results are accessible trough the `index.html` in each folder:
F7303310: results.rar 

@aaron.ballman a friendly reminder.


https://reviews.llvm.org/D45050



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

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 167597.
Charusso marked 37 inline comments as done.
Charusso edited the summary of this revision.
Charusso added a comment.

Thanks for the very in-depth review @aaron.ballman!

I have not found a way to obtain the instance of Preprocessor nicely, but it is 
working well. Changes:

- Huge refactor `.`
- Macro definition checking is rely on Preprocessor.
- `isUnjectUL()` is now a function, not a static variable.
- `WantToUseSafeFunctions` is the new integer specifying the 
`AreSafeFunctionsAvailable` variable without the override feature rely on the 
necessary macros has been defined.
- Added a test case in 
`bugprone-not-null-terminated-result-memcpy-before-safe.c` if the user wants to 
use safe functions but it is unavailable.
- Extra: the LOC is now a round number.


https://reviews.llvm.org/D45050

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tidy/bugprone/NotNullTerminatedResultCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-not-null-terminated-result.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-not-null-terminated-result-in-initialization-strlen.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-before-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-cxx.cpp
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe-other.c
  test/clang-tidy/bugprone-not-null-terminated-result-memcpy-safe.c
  test/clang-tidy/bugprone-not-null-terminated-result-strlen.c
  test/clang-tidy/bugprone-not-null-terminated-result-wcslen.cpp
  test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp

Index: test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-not-null-terminated-result-wmemcpy-safe-cxx.cpp
@@ -0,0 +1,136 @@
+// RUN: %check_clang_tidy %s bugprone-not-null-terminated-result %t -- \
+// RUN: -- -std=c++11
+
+#define __STDC_LIB_EXT1__ 1
+#define __STDC_WANT_LIB_EXT1__ 1
+
+typedef unsigned int size_t;
+typedef int errno_t;
+size_t wcslen(const wchar_t *);
+void *malloc(size_t);
+void *realloc(void *, size_t);
+
+template 
+errno_t wcsncpy_s(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+errno_t wcsncpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+
+template 
+wchar_t *wcsncpy(wchar_t (&dest)[size], const wchar_t *src, size_t length);
+wchar_t *wcsncpy(wchar_t *, const wchar_t *, size_t);
+
+template 
+errno_t wcscpy_s(wchar_t (&dest)[size], const wchar_t *);
+errno_t wcscpy_s(wchar_t *, size_t, const wchar_t *);
+
+template 
+wchar_t *wcscpy(wchar_t (&dest)[size], const wchar_t *);
+wchar_t *wcscpy(wchar_t *, const wchar_t *);
+
+errno_t wmemcpy_s(wchar_t *, size_t, const wchar_t *, size_t);
+wchar_t *wmemcpy(wchar_t *, const wchar_t *, size_t);
+
+
+//===--===//
+// wmemcpy() - destination array tests
+//===--===//
+
+void bad_wmemcpy_known_dest(const wchar_t *src) {
+  wchar_t dest01[13];
+  wmemcpy(dest01, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest01[14];
+  // CHECK-FIXES-NEXT: wcscpy_s(dest01, src);
+}
+
+void good_wmemcpy_known_dest(const wchar_t *src) {
+  wchar_t dst01[14];
+  wcscpy_s(dst01, src);
+}
+
+//===--===//
+// wmemcpy() - length tests
+//===--===//
+
+void bad_wmemcpy_full_source_length(const wchar_t *src) {
+  wchar_t dest20[13];
+  wmemcpy(dest20, src, wcslen(src));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest20[14];
+  // CHECK-FIXES-NEXT: wcscpy_s(dest20, src);
+}
+
+void good_wmemcpy_full_source_length(const wchar_t *src) {
+  wchar_t dst20[14];
+  wcscpy_s(dst20, src);
+}
+
+void bad_wmemcpy_partial_source_length(const wchar_t *src) {
+  wchar_t dest21[13];
+  wmemcpy(dest21, src, wcslen(src) - 1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: the result from calling 'wmemcpy' is not null-terminated [bugprone-not-null-terminated-result]
+  // CHECK-FIXES: wchar_t dest21[14];
+  // CHECK-FIXES-NEXT: wcsncpy_s(dest21, src, wcslen(src) - 1);
+}
+
+void good_wmemcpy_partial_source_length(const wchar_t *src) {
+  wchar_t dst21[14];
+  wcsncpy_s(dst21, src, wcslen(src) - 1);
+}
+
+//===--===//
+// wmemcpy_s() - destination array tests
+//===-

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:1038
+
+  SmallString<128> NewAddNullTermExprStr;
+  NewAddNullTermExprStr = "\n";

aaron.ballman wrote:
> JonasToth wrote:
> > whisperity wrote:
> > > aaron.ballman wrote:
> > > > This should be done using a `Twine`.
> > > Can you please give an example of how to well-approach this? We had 
> > > issues with using Twines on the stack and then later on running into 
> > > weird undefined behaviour. The documentation is not clear to a lot of our 
> > > colleagues on where the `Twine`'s usage barriers are... (Asking this not 
> > > just for @Charusso but also for a few more colleagues, 
> > > @baloghadamsoftware e.g.)
> > naive approach:
> > 
> > ```
> > std::string New = 
> > Twine("\n").concat(SpaceBeforeStmtStr).concat(exprToStr(..))...).str();
> > ```
> > I think retrieving a `SmallString` is not supported. Please note, that 
> > `Twine` does support taking integer and floats directly without prior 
> > conversion.
> > You can use `operator+` instead of `concat` too.
> > 
> > Did this help or did you have more specific issues? 
> The important bit is -- don't try to store a Twine (locally or otherwise). 
> It's fine as a parameter in a function, but otherwise you should try to use 
> it only as part of an expression. e.g., `(Twine("foo") + "bar" + baz).str()` 
> is a good pattern to get used to.
+1 for @aaron.ballman's example, it is an unspoken standard.


https://reviews.llvm.org/D45050



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

[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

[Csaba Dabis via Phabricator via cfe-commits]

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

First of all, thanks you for working on this, as I wanted to do the same, but I 
did not know how to. I did not know also that 15 of the checkers already 
implements `BugReporterVisitors`, but I completely shocked it took 10 years of 
pain to rewrite it. It feels like this patch a little-bit brute-force, and I 
believe this should be the future direction of reporting. Also I believe in 
that we could hook a lot more useful information from *all* the checkers, and 
we do not care that much about the core `BugReporterVisitors` as they do their 
job enough well. So with that, I would out-chain this patch from the 
MIG-patches because it is a lot more serious problem-set. I also would like to 
ask you to take it more serious, as all your mentioned benefits will rely on 
this simple and cool approach, so it has to be flexible as possible. I am not 
into the internal stuff that much, so I cannot argue about the lack of 
`CheckerContext` functions, but I think this is a huge problem and breaks the 
flexibility a lot.

I tried out the patch and saw the very recommended source code of 
`MallocChecker`'s so lightweight `BugReporterVisitor` implementation and I 
think your API is too strict about having a lambda function, so here is my 
ideas:

- `CheckerContext.h`: As I see we only work with a `string`, nothing else, so I 
would extend that class:

  ExplodedNode *addNote(ProgramStateRef State, StringRef Message) {
return addTransitionImpl(State, false, nullptr, setNoteTag(Message)); // 
Note the 'setNoteTag()'
  }

`getNoteTag()`: you could differentiate a setter with the true getter what you 
only use in `TagVisitor::VisitNode()` (where you truly hook your `Callback` to 
get extra information later on):

  const NoteTag *setNoteTag(StringRef Message) {
return Eng.getNoteTags().setNoteTag(Message);
  }



- `BugReporterVisitors.h`: basically a `NoteTag` is a string, nothing else, I 
think you would like to hook the `Callback` only when you are about to obtain 
the message:

  class NoteTag {
std::string Msg;
NoteTag(StringRef Message) : ProgramPointTag(&Kind), Msg(Message)
  
class Factory {
  const NoteTag *getNoteTag(Callback &&Cb)
  const NoteTag *setNoteTag(StringRef Message)
};
  };



- Documentation: if we are about the rewrite the entire bug-reporting business, 
it worth some long doxygen comments.

The outcome is that cool I have to be the reviewer in two patches, and I hope 
it helps you creating a better API, even if I got something wrong.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:364
+
+  Optional getNote(BugReporterContext &BRC, BugReport &R) const {
+std::string Note = Cb(BRC, R);

It feels like it returns the `NoteTag`, so I would rename it as `getMessage()`.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:223
+  /// Produce a program point tag that displays an additional path note
+  /// to the user. This is a lightweirght alternative to the
+  /// BugReporterVisitor mechanism: instead of visiting the bug report

It is very randomly spaced, `lightweight`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58367



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

[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a reviewer: Charusso.
Charusso requested changes to this revision.
Charusso added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:113
+  });
+  C.addTransition(C.getState()->set(true), T);
 }

This is a cool approach, but it is difficult to use this API in other checkers. 
If you do not out-chain D58367 I would like to see something like that here:

```
  SmallString<64> Str;
  llvm::raw_svector_ostream OS(Str);
  OS << "Deallocating object passed through parameter '" << PVD->getName()
 << '\'';

  C.addNote(C.getState()->set(true), OS.str());
```


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

https://reviews.llvm.org/D58368



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

[PATCH] D53076: [analyzer] Enhance ConditionBRVisitor to write out more information

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D53076#1403436 , @NoQ wrote:

> I'll take a closer look in a few days, sorry for the delays! The progress 
> you're making is fantastic and i really appreciate your work, but i have an 
> urgent piece of work of my own to deal with this week.


@NoQ, please take your time, that new bug-report logic is the end of my work on 
reporting business (as I mentioned I wanted to create the same patch), so that 
is more important than this.


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

https://reviews.llvm.org/D53076



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

[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:47
 
 REGISTER_TRAIT_WITH_PROGRAMSTATE(ReleasedParameter, bool);
 

`;` is not necessary.


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

https://reviews.llvm.org/D58368



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

[PATCH] D58367: [analyzer] NFC: Improve upon the concept of BugReporterVisitor.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In general I think it would be cool to think all of the problems in the same 
time as these really depends on the `CoreEngine` and how we operates with 
`ExplodedNodes`.

In D58367#1405185 , @NoQ wrote:

> In particular, the implementation you propose does a lot more than adding a 
> note: it also adds, well, a transition.


I think a possible workaround to handle special nodes is adding them in 
`CoreEngine` as successors (instead of predecessors) so when we are traversing 
backwards it immediately known the next (pred) "not-special" node will be its 
parent. This is the same logic in `ConditionBRVisitor` where we only work with 
pair of nodes to see if we would report something.

> The issue here is that the checker doesn't always have an ability to mutate 
> the graph by adding nodes. Now it becomes more of a problem because the 
> checker may want to emit notes more often than it wants to generate nodes. 
> This problem can be easily mitigated by passing a different sort of context 
> (not `CheckerContext` but something else) into these callbacks that will 
> store lambdas in a different manner.
>  I think this is not a big flexibility issue on its own because it should be 
> possible to make lambdas from multiple sources cooperate with each other by 
> sharing a certain amount of arbitrary state information within the BugReport 
> object.

I think it ties with the problem above, so having some special `ExplodedNode` 
could fix both of them because may you would introduce some 
`ExplodedNodeContext` to store more information.

> In D58367#1404722 , @Charusso wrote:
> 
>>   const NoteTag *setNoteTag(StringRef Message) {
>> return Eng.getNoteTags().setNoteTag(Message);
>>   }
>>   
> 
> 
> Mmm, i don't think i understand this part. Like, `getNoteTag()` is not really 
> a getter. It's a factory method on an allocator that causes it to produce an 
> object. What is the semantics of a setter that would correspond to it?

All the problems what you mentioned is not really a job of a factory, so I just 
emphasized we only get the true benefit of the callback in the get part. I 
think leave it as it is now makes sense but later on there would be a lot more 
function to hook crazy lambdas to *set* information.

Please note that it is a very high-level feedback, as I just checked the 
`CoreEngine` and saw why we are splitting states.


Repository:
  rC Clang

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

https://reviews.llvm.org/D58367



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

[PATCH] D58368: [analyzer] MIGChecker: Implement bug reporter visitors.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D58368#1406490 , @NoQ wrote:

> Address comments.
>
> @Charusso: I agreed not to rush for D58367  
> and implemented an old-style visitor here instead :)


Thanks you! I wanted to accept it when you remove it from the review-chain and 
after rewriting the summary. The latter is not that big deal, just wanted to 
let you know it remained the same.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58368



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

[PATCH] D50488: [Analyzer] Checker for non-determinism caused by sorting of pointer-like elements

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

bitcoin:

  sudo apt-get install build-essential libtool autotools-dev automake 
pkg-config libssl-dev libevent-dev bsdmainutils python3 libboost-all-dev 
software-properties-common libminiupnpc-dev libzmq3-dev libqt5gui5 libqt5core5a 
libqt5dbus5 qttools5-dev qttools5-dev-tools libprotobuf-dev protobuf-compiler
  sudo add-apt-repository ppa:bitcoin/bitcoin
  sudo apt-get update
  sudo apt-get install libdb4.8-dev libdb4.8++-dev
  
  git clone https://github.com/bitcoin/bitcoin.git
  cd bitcoin
  
  ./autogen.sh
  ./configure
  CodeChecker log -b "make -j13" -o compile_commands.json

xerces:

  wget http://www-eu.apache.org/dist//xerces/c/3/sources/xerces-c-3.2.2.tar.gz
  tar xf xerces-c-3.2.2.tar.gz
  mv xerces-c-3.2.2 xerces
  rm xerces-c-3.2.2.tar.gz
  cd xerces
  
  ./configure
  CodeChecker log -b "make -j13" -o compile_commands.json

where `-j13` means 13 threads.


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

https://reviews.llvm.org/D50488



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

[PATCH] D62638: [analyzer] A Python script to prettify the ExplodedGraph dumps.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Sorry for that much rewrite request but in a script language you have to name 
your stuff properly or you will be completely lost. I have not written a single 
Python class yet, but trust me.

I really like that `DotDumpVisitor` semantic, but it would be a lot better at 
SVG level. At the moment whatever style/table-data you insert it adds an extra 
text tag per styling which is quite inefficient.

We could use "foreignObject" (somehow) and apply pure HTML tables or I could 
use my `` idea instead 
(https://developer.mozilla.org/en-US/docs/Web/SVG/Element/tspan). I recommend 
the latter as it is better than a fixed table.

With the current representation it would be difficult to apply every styling to 
tspans. I think it also would be difficult to use your `Explorer` idea as we 
are talking about modificating the SVG dynamically.

In D62638#1522357 , @NoQ wrote:

> > I wrote some tests but i'm not really sure they're worth it.
>
> Mmm, on second thought, they probably won't work out of the box, because they 
> might require installing python modules in order to work. I'm actually not 
> sure if all machines have python3. I'll try but it'll most likely fail. I 
> guess i could try excluding them from `make check-all`.


It would be cool to introduce something like `check-scripts`.




Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:5
+import collections
+import json as json_module  # 'json' is a great name for a variable.
+import logging

I think it is not that cool variable name. I would use the appropiate name of 
the object instead of the generic `json` name.



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:12
+class SourceLocation(object):
+def __init__(self, json):
+super(SourceLocation, self).__init__()

`json` -> `loc`



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:22
+class ProgramPoint(object):
+def __init__(self, json):
+super(ProgramPoint, self).__init__()

`json` -> `program_point` and so on...



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:33
+self.pretty = json['pretty']
+self.sloc = SourceLocation(json['location']) \
+if json['location'] is not None else None

What about just `loc`?



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:124
+self.parents = []
+self.children = []
+

What about `predecessors` and `successors`? It is the proper name in the Static 
Analyzer world.



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:126
+
+def set_json(self, node_id, json):
+logging.debug('Adding ' + node_id)

- `set_json()` -> `set()` the given `node`
- `json` -> `node`



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:151
+super(ExplodedGraph, self).__init__()
+self.nodes = collections.defaultdict(ExplodedNode)
+self.root_id = None

`nodes` -> `graph`



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:184
+# even though in a valid dot file everything is escaped.
+raw_json = result[2].replace('\\l', '') \
+.replace(' ', '') \

`raw_json` -> `node_string` which will be a raw JSON after `loads()`.



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:191
+.replace('\\>', '>') \
+.rstrip(',')
+logging.debug(raw_json)

I have removed the trailing `,` from the end yesterday so `rstrip(',')` is not 
needed.

It would be cool to name the `result[]`, like: `pred_id = result[1]` and 
`succ_id = result[2]` or current/previous ID, or something like that.

I also would put the regexes into a function and you could write:
`pred_id, succ_id = parse_edge()` or something more simpler.

What is `result[0]` btw? That confused me a little-bit.



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:233
+else:
+color = 'cyan3'
+

It would be cool to put it into a "switch-statement" 
(https://stackoverflow.com/questions/60208/replacements-for-switch-statement-in-python).



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:237
+if p.sloc is not None:
+self._dump(''
+   '%s:%s:%s:'

I think tables are left aligned by default, so you could remove your left 
alignments.



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:326
+self._dump('Program point:')
+self._dump(''
+   '')

I would c

[PATCH] D62658: [analyzer] print() JSONify: ExplodedNode revision

[Csaba Dabis via Phabricator via cfe-commits]

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

Revert node-ID removal.


Repository:
  rC Clang

https://reviews.llvm.org/D62658

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


Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3075,8 +3075,8 @@
 const unsigned int Space = 1;
 ProgramStateRef State = N->getState();
 
-Out << "{ \"node_id\": \"" << (const void *)N
-<< "\", \"state_id\": " << State->getID()
+Out << "{ \"node_id\": " << N->getID(G) << ", \"pointer\": \""
+<< (const void *)N << "\", \"state_id\": " << State->getID()
 << ", \"has_report\": " << (nodeHasBugReport(N) ? "true" : "false")
 << ",\\l";
 
@@ -3094,7 +3094,7 @@
   else
 Out << "null }";
 },
-   // Adds a comma and a new-line between each program point.
+// Adds a comma and a new-line between each program point.
 [&](const ExplodedNode *) { Out << ",\\l"; },
 [&](const ExplodedNode *) { return false; });
 


Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3075,8 +3075,8 @@
 const unsigned int Space = 1;
 ProgramStateRef State = N->getState();
 
-Out << "{ \"node_id\": \"" << (const void *)N
-<< "\", \"state_id\": " << State->getID()
+Out << "{ \"node_id\": " << N->getID(G) << ", \"pointer\": \""
+<< (const void *)N << "\", \"state_id\": " << State->getID()
 << ", \"has_report\": " << (nodeHasBugReport(N) ? "true" : "false")
 << ",\\l";
 
@@ -3094,7 +3094,7 @@
   else
 Out << "null }";
 },
-	// Adds a comma and a new-line between each program point.
+// Adds a comma and a new-line between each program point.
 [&](const ExplodedNode *) { Out << ",\\l"; },
 [&](const ExplodedNode *) { return false; });
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62716: [analyzer] Fix JSON dumps for dynamic types, add test.

[Csaba Dabis via Phabricator via cfe-commits]

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

Not sure why I was blind here, thanks you!


Repository:
  rC Clang

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

https://reviews.llvm.org/D62716



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

[PATCH] D62658: [analyzer] print() JSONify: ExplodedNode revision

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362249: [analyzer] print() JSONify: ExplodedNode revision 
(authored by Charusso, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62658?vs=202173&id=202462#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62658

Files:
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp


Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3075,8 +3075,8 @@
 const unsigned int Space = 1;
 ProgramStateRef State = N->getState();
 
-Out << "{ \"node_id\": \"" << (const void *)N
-<< "\", \"state_id\": " << State->getID()
+Out << "{ \"node_id\": " << N->getID(G) << ", \"pointer\": \""
+<< (const void *)N << "\", \"state_id\": " << State->getID()
 << ", \"has_report\": " << (nodeHasBugReport(N) ? "true" : "false")
 << ",\\l";
 
@@ -3094,7 +3094,7 @@
   else
 Out << "null }";
 },
-   // Adds a comma and a new-line between each program point.
+// Adds a comma and a new-line between each program point.
 [&](const ExplodedNode *) { Out << ",\\l"; },
 [&](const ExplodedNode *) { return false; });
 


Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3075,8 +3075,8 @@
 const unsigned int Space = 1;
 ProgramStateRef State = N->getState();
 
-Out << "{ \"node_id\": \"" << (const void *)N
-<< "\", \"state_id\": " << State->getID()
+Out << "{ \"node_id\": " << N->getID(G) << ", \"pointer\": \""
+<< (const void *)N << "\", \"state_id\": " << State->getID()
 << ", \"has_report\": " << (nodeHasBugReport(N) ? "true" : "false")
 << ",\\l";
 
@@ -3094,7 +3094,7 @@
   else
 Out << "null }";
 },
-	// Adds a comma and a new-line between each program point.
+// Adds a comma and a new-line between each program point.
 [&](const ExplodedNode *) { Out << ",\\l"; },
 [&](const ExplodedNode *) { return false; });
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62754: [analyzer] Fix JSON dumps for location contexts.

[Csaba Dabis via Phabricator via cfe-commits]

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

That was my feeling as well, but it has been defined like that. Thanks for the 
clarification!


Repository:
  rC Clang

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

https://reviews.llvm.org/D62754



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

[PATCH] D62638: [analyzer] A Python script to prettify the ExplodedGraph dumps.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:151
+super(ExplodedGraph, self).__init__()
+self.nodes = collections.defaultdict(ExplodedNode)
+self.root_id = None

NoQ wrote:
> Charusso wrote:
> > `nodes` -> `graph`
> Mm, why would i have a field called "graph" in an object called "graph".
Because we are constructing the graph object and it would be still much better 
than LLVM's fancy `Graph TheGraph;` logic. I am okay with a bunch of nodes, 
just those are the graph itself.



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:191
+.replace('\\>', '>') \
+.rstrip(',')
+logging.debug(raw_json)

NoQ wrote:
> Charusso wrote:
> > I have removed the trailing `,` from the end yesterday so `rstrip(',')` is 
> > not needed.
> > 
> > It would be cool to name the `result[]`, like: `pred_id = result[1]` and 
> > `succ_id = result[2]` or current/previous ID, or something like that.
> > 
> > I also would put the regexes into a function and you could write:
> > `pred_id, succ_id = parse_edge()` or something more simpler.
> > 
> > What is `result[0]` btw? That confused me a little-bit.
> > `pred_id, succ_id = parse_edge()`
> 
> Not really, i'd also have to return whether the match was successful, which 
> kinda makes it as bad as before.
> 
> > What is `result[0]` btw? That confused me a little-bit.
> 
> That's the entire match. 
> https://docs.python.org/3/library/re.html#match-objects
I see, just I am that newbie. Thanks!



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:237
+if p.sloc is not None:
+self._dump(''
+   '%s:%s:%s:'

NoQ wrote:
> Charusso wrote:
> > I think tables are left aligned by default, so you could remove your left 
> > alignments.
> Unfortunately in GraphViz they're centered by default.
Well, that is unfortunate.


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

https://reviews.llvm.org/D62638



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

[PATCH] D62638: [analyzer] A Python script to prettify the ExplodedGraph dumps.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D62638#1525823 , @NoQ wrote:

> Also switched to python2 because it seems to be the default. The differences 
> are minimal.


What about the state of the patch itself? Planned stuff WIP, could I take it to 
SVGify, or?


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

https://reviews.llvm.org/D62638



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

[PATCH] D62638: [analyzer] A Python script to prettify the ExplodedGraph dumps.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:191
+.replace('\\>', '>') \
+.rstrip(',')
+logging.debug(raw_json)

NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Charusso wrote:
> > > > I have removed the trailing `,` from the end yesterday so `rstrip(',')` 
> > > > is not needed.
> > > > 
> > > > It would be cool to name the `result[]`, like: `pred_id = result[1]` 
> > > > and `succ_id = result[2]` or current/previous ID, or something like 
> > > > that.
> > > > 
> > > > I also would put the regexes into a function and you could write:
> > > > `pred_id, succ_id = parse_edge()` or something more simpler.
> > > > 
> > > > What is `result[0]` btw? That confused me a little-bit.
> > > > `pred_id, succ_id = parse_edge()`
> > > 
> > > Not really, i'd also have to return whether the match was successful, 
> > > which kinda makes it as bad as before.
> > > 
> > > > What is `result[0]` btw? That confused me a little-bit.
> > > 
> > > That's the entire match. 
> > > https://docs.python.org/3/library/re.html#match-objects
> > I see, just I am that newbie. Thanks!
> > I have removed the trailing `,` from the end yesterday so `rstrip(',')` is 
> > not needed.
> 
> It still seems to be there for all nodes except the last node.
> 
> 
Hm, that is why your testing idea is so cool, I am nearly sure I have removed 
it.


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

https://reviews.llvm.org/D62638



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

[PATCH] D62761: [analyzer] exploded-graph-rewriter: Implement a --diff mode.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D62761#1525917 , @NoQ wrote:

> Remove "-" from program point dumps because it resembles a removal marker in 
> diffs.


Could you add an image? I have not seen any problematic stuff, just that.


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

https://reviews.llvm.org/D62761



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

[PATCH] D62638: [analyzer] A Python script to prettify the ExplodedGraph dumps.

[Csaba Dabis via Phabricator via cfe-commits]

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

Let us see those tests!


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

https://reviews.llvm.org/D62638



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

[PATCH] D62638: [analyzer] A Python script to prettify the ExplodedGraph dumps.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: cfe/trunk/utils/analyzer/exploded-graph-rewriter.py:1
+#!/usr/bin/env python
+

```
# something else then
#
#==- exploded-graph-rewriter.py - Simplifies the ExplodedGraph -*- python -*-==#
#
# 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
#
#======#
```
Whoops!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62638



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

[PATCH] D60670: [analyzer] [NFC] PathDiagnostic: Create PathDiagnosticPopUpPiece

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Fix a nit in rL362632 .


Repository:
  rC Clang

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

https://reviews.llvm.org/D60670



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

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

When we traversed backwards on ExplodedNodes to see where we processed the
given statement we `break` too early. With the current approach we do not
miss the CallExitEnd ProgramPoint which stands for an inlined call.


Repository:
  rC Clang

https://reviews.llvm.org/D62926

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/new-ctor-null-throw.cpp
  clang/test/Analysis/new-ctor-null.cpp


Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection 
-analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,7 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection 
-analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,9 +11,11 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer 
unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer 
unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
@@ -22,5 +26,5 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
 }
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -837,9 +837,6 @@
   if (auto CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
 
   Node = Node->getFirstPred();
 } while (Node);


Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,7 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,9 +11,11 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
@@ -22,5 +26,5 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
 }
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass constructing objects to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:835-842
 // First, find when we processed the statement.
 do {
   if (auto CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
 
   Node = Node->getFirstPred();

NoQ wrote:
> This iteration may now take us straight to the root of the graph. I don't 
> think it's supposed to be that slow; i think it's supposed to only skip 
> within maybe a full-expression at most.
> 
> Which statements in the AST are getting peeled off here that weren't before? 
> Which statements are supposed to get peeled off?
> 
> Might it be that we should simply add one more case to `peelOffOuterExpr()` 
> or something like that?
We have the following sequence in the loop:

```
"stmt_kind": "DeclStmt", "stmt_point_kind": "PostStmt"
"stmt_kind": "DeclStmt", "stmt_point_kind": "PostStore"
"stmt_kind": "DeclStmt",  "stmt_point_kind": "PreStmtPurgeDeadSymbols"
"pretty": "S *s = new S [10];"

"stmt_kind": "CXXNewExpr", "stmt_point_kind": "PostStmt"
"stmt_kind": "CXXNewExpr", "stmt_point_kind": "PreStmtPurgeDeadSymbols"
"pretty": "new S [10]"

"stmt_kind": "CXXConstructExpr", "stmt_point_kind": "PostStmt"
"stmt_kind": "CXXConstructExpr", "stmt_point_kind": "PreStmt"
"stmt_kind": "CXXConstructExpr", "stmt_point_kind": "PreStmtPurgeDeadSymbols"
"pretty": null

"kind": "CallExitEnd" - found the ReturnStmt, all good.
```

This ReturnVisitor is totally ProgramPoint-based and I wanted to be super 
generic. Do you know about more problematic constructing-object?


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

https://reviews.llvm.org/D62926



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass constructing objects to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 203324.
Charusso marked 4 inline comments as done.
Charusso retitled this revision from "[analyzer] ReturnVisitor: Bypass 
everything to see inlined calls" to "[analyzer] ReturnVisitor: Bypass 
constructing objects to see inlined calls".
Charusso added a comment.

- More explicit approach.


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

https://reviews.llvm.org/D62926

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/new-ctor-null-throw.cpp
  clang/test/Analysis/new-ctor-null.cpp


Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection 
-analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection 
-analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,9 +11,11 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer 
unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer 
unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
@@ -22,5 +26,6 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -833,13 +833,19 @@
   return;
 
 // First, find when we processed the statement.
+// If we have a 'CXXNewExpr' or a 'CXXConstructExpr' they got purged away
+// before their calls happen and we would catch that purge in the second
+// 'break' so we have to bypass it.
+const bool IsBypass = isa(S) || isa(S);
 do {
-  if (auto CEE = Node->getLocationAs())
+  if (Optional CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
+
+  if (!IsBypass)
+if (Optional SP = Node->getLocationAs())
+  if (SP->getStmt() == S)
+break;
 
   Node = Node->getFirstPred();
 } while (Node);


Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,9 +11,11 @@
 // operator new.
 void *operator new(size_t size) {
   r

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass constructing objects to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 203326.
Charusso added a comment.

- Better wording.


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

https://reviews.llvm.org/D62926

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/new-ctor-null-throw.cpp
  clang/test/Analysis/new-ctor-null.cpp


Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection 
-analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection 
-analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,9 +11,11 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer 
unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer 
unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
@@ -22,5 +26,6 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -833,13 +833,19 @@
   return;
 
 // First, find when we processed the statement.
+// If we have a 'CXXNewExpr' or a 'CXXConstructExpr' they got purged away
+// before their calls happen and we would catch that purge in the second
+// condition as a 'StmtPoint' so we have to bypass it.
+const bool IsBypass = isa(S) || isa(S);
 do {
-  if (auto CEE = Node->getLocationAs())
+  if (Optional CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
+
+  if (!IsBypass)
+if (Optional SP = Node->getLocationAs())
+  if (SP->getStmt() == S)
+break;
 
   Node = Node->getFirstPred();
 } while (Node);


Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,9 +11,11 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warnin

[PATCH] D62946: [analyzer] ProgramPoint: more explicit printJson()

[Csaba Dabis via Phabricator via cfe-commits]

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

Now we print out every possible kinds of ProgramPoints.


Repository:
  rC Clang

https://reviews.llvm.org/D62946

Files:
  clang/lib/Analysis/ProgramPoint.cpp


Index: clang/lib/Analysis/ProgramPoint.cpp
===
--- clang/lib/Analysis/ProgramPoint.cpp
+++ clang/lib/Analysis/ProgramPoint.cpp
@@ -99,12 +99,6 @@
   case ProgramPoint::CallExitEndKind:
 Out << "CallExitEnd\"";
 break;
-  case ProgramPoint::PostStmtPurgeDeadSymbolsKind:
-Out << "PostStmtPurgeDeadSymbols\"";
-break;
-  case ProgramPoint::PreStmtPurgeDeadSymbolsKind:
-Out << "PreStmtPurgeDeadSymbols\"";
-break;
   case ProgramPoint::EpsilonKind:
 Out << "EpsilonPoint\"";
 break;
@@ -210,20 +204,35 @@
 Out << ", ";
 printLocJson(Out, S->getBeginLoc(), SM);
 
-Out << ", \"stmt_point_kind\": ";
-if (getAs())
-  Out << "\"PreStmt\"";
+Out << ", \"stmt_point_kind\": \"";
+if (getAs())
+  Out << "PreLoad";
+else if (getAs())
+  Out << "PreStore";
+else if (getAs())
+  Out << "PostAllocatorCall";
+else if (getAs())
+  Out << "PostCondition";
 else if (getAs())
-  Out << "\"PostLoad\"";
-else if (getAs())
-  Out << "\"PostStore\"";
+  Out << "PostLoad";
 else if (getAs())
-  Out << "\"PostLValue\"";
-else if (getAs())
-  Out << "\"PostAllocatorCall\"";
-else
-  Out << "null";
+  Out << "PostLValue";
+else if (getAs())
+  Out << "PostStore";
+else if (getAs())
+  Out << "PostStmt";
+else if (getAs())
+  Out << "PostStmtPurgeDeadSymbols";
+else if (getAs())
+  Out << "PreStmtPurgeDeadSymbols";
+else if (getAs())
+  Out << "PreStmt";
+else {
+  Out << "\nKind: '" << getKind();
+  llvm_unreachable("' is unhandled StmtPoint kind!");
+}
 
+Out << '\"';
 break;
   }
   }


Index: clang/lib/Analysis/ProgramPoint.cpp
===
--- clang/lib/Analysis/ProgramPoint.cpp
+++ clang/lib/Analysis/ProgramPoint.cpp
@@ -99,12 +99,6 @@
   case ProgramPoint::CallExitEndKind:
 Out << "CallExitEnd\"";
 break;
-  case ProgramPoint::PostStmtPurgeDeadSymbolsKind:
-Out << "PostStmtPurgeDeadSymbols\"";
-break;
-  case ProgramPoint::PreStmtPurgeDeadSymbolsKind:
-Out << "PreStmtPurgeDeadSymbols\"";
-break;
   case ProgramPoint::EpsilonKind:
 Out << "EpsilonPoint\"";
 break;
@@ -210,20 +204,35 @@
 Out << ", ";
 printLocJson(Out, S->getBeginLoc(), SM);
 
-Out << ", \"stmt_point_kind\": ";
-if (getAs())
-  Out << "\"PreStmt\"";
+Out << ", \"stmt_point_kind\": \"";
+if (getAs())
+  Out << "PreLoad";
+else if (getAs())
+  Out << "PreStore";
+else if (getAs())
+  Out << "PostAllocatorCall";
+else if (getAs())
+  Out << "PostCondition";
 else if (getAs())
-  Out << "\"PostLoad\"";
-else if (getAs())
-  Out << "\"PostStore\"";
+  Out << "PostLoad";
 else if (getAs())
-  Out << "\"PostLValue\"";
-else if (getAs())
-  Out << "\"PostAllocatorCall\"";
-else
-  Out << "null";
+  Out << "PostLValue";
+else if (getAs())
+  Out << "PostStore";
+else if (getAs())
+  Out << "PostStmt";
+else if (getAs())
+  Out << "PostStmtPurgeDeadSymbols";
+else if (getAs())
+  Out << "PreStmtPurgeDeadSymbols";
+else if (getAs())
+  Out << "PreStmt";
+else {
+  Out << "\nKind: '" << getKind();
+  llvm_unreachable("' is unhandled StmtPoint kind!");
+}
 
+Out << '\"';
 break;
   }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass constructing objects to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:835-842
 // First, find when we processed the statement.
 do {
   if (auto CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
 
   Node = Node->getFirstPred();

Charusso wrote:
> NoQ wrote:
> > This iteration may now take us straight to the root of the graph. I don't 
> > think it's supposed to be that slow; i think it's supposed to only skip 
> > within maybe a full-expression at most.
> > 
> > Which statements in the AST are getting peeled off here that weren't 
> > before? Which statements are supposed to get peeled off?
> > 
> > Might it be that we should simply add one more case to `peelOffOuterExpr()` 
> > or something like that?
> We have the following sequence in the loop:
> 
> ```
> "stmt_kind": "DeclStmt", "stmt_point_kind": "PostStmt"
> "stmt_kind": "DeclStmt", "stmt_point_kind": "PostStore"
> "stmt_kind": "DeclStmt",  "stmt_point_kind": "PreStmtPurgeDeadSymbols"
> "pretty": "S *s = new S [10];"
> 
> "stmt_kind": "CXXNewExpr", "stmt_point_kind": "PostStmt"
> "stmt_kind": "CXXNewExpr", "stmt_point_kind": "PreStmtPurgeDeadSymbols"
> "pretty": "new S [10]"
> 
> "stmt_kind": "CXXConstructExpr", "stmt_point_kind": "PostStmt"
> "stmt_kind": "CXXConstructExpr", "stmt_point_kind": "PreStmt"
> "stmt_kind": "CXXConstructExpr", "stmt_point_kind": "PreStmtPurgeDeadSymbols"
> "pretty": null
> 
> "kind": "CallExitEnd" - found the ReturnStmt, all good.
> ```
> 
> This ReturnVisitor is totally ProgramPoint-based and I wanted to be super 
> generic. Do you know about more problematic constructing-object?
Btw: this cool dump based on D62946


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

https://reviews.llvm.org/D62926



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

[PATCH] D62978: [analyzer] ReturnVisitor: Handle non-null ReturnStmts

[Csaba Dabis via Phabricator via cfe-commits]

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

Suppressed example: F9091784: return-non-null.html 



If we have a value at the return side do not write out it is being null.


Repository:
  rC Clang

https://reviews.llvm.org/D62978

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/find_last_store.c
  clang/test/Analysis/inlining/false-positive-suppression.cpp


Index: clang/test/Analysis/inlining/false-positive-suppression.cpp
===
--- clang/test/Analysis/inlining/false-positive-suppression.cpp
+++ clang/test/Analysis/inlining/false-positive-suppression.cpp
@@ -1,5 +1,13 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config 
suppress-null-return-paths=false -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify -DSUPPRESSED=1 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -DSUPPRESSED=1 \
+// RUN:  -verify %s
+
+#ifdef SUPPRESSED
+// expected-no-diagnostics
+#endif
 
 namespace rdar12676053 {
   // Delta-reduced from a preprocessed file.
@@ -85,7 +93,10 @@
 
 int *&box2 = m.getValue(i);
 box2 = 0;
-*box2 = 1; // expected-warning {{Dereference of null pointer}}
+*box2 = 1;
+#ifndef SUPPRESSED
+// expected-warning@-2 {{Dereference of null pointer}}
+#endif
   }
 
   SomeClass *&getSomeClass() {
Index: clang/test/Analysis/diagnostics/find_last_store.c
===
--- clang/test/Analysis/diagnostics/find_last_store.c
+++ clang/test/Analysis/diagnostics/find_last_store.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=text 
-verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -analyzer-output=text -verify %s
+
+// FIXME: Make it work with suppress-null-return-paths=true.
+
 typedef struct { float b; } c;
 void *a();
 void *d() {
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -827,8 +827,7 @@
   /// the statement is a call that was inlined, we add the visitor to the
   /// bug report, so it can print a note later.
   static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S,
-BugReport &BR,
-bool InEnableNullFPSuppression) {
+BugReport &BR, bool IsNullFPSuppression) {
 if (!CallEvent::isCallStmt(S))
   return;
 
@@ -877,11 +876,11 @@
 // See if the return value is NULL. If so, suppress the report.
 AnalyzerOptions &Options = State->getAnalysisManager().options;
 
+// If we have a value at the return side do not write out it is being null.
 bool EnableNullFPSuppression = false;
-if (InEnableNullFPSuppression &&
-Options.ShouldSuppressNullReturnPaths)
+if (IsNullFPSuppression && Options.ShouldSuppressNullReturnPaths)
   if (Optional RetLoc = RetVal.getAs())
-EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue();
+EnableNullFPSuppression = true;
 
 BR.markInteresting(CalleeContext);
 BR.addVisitor(llvm::make_unique(CalleeContext,


Index: clang/test/Analysis/inlining/false-positive-suppression.cpp
===
--- clang/test/Analysis/inlining/false-positive-suppression.cpp
+++ clang/test/Analysis/inlining/false-positive-suppression.cpp
@@ -1,5 +1,13 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config suppress-null-return-paths=false -verify %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify -DSUPPRESSED=1 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -DSUPPRESSED=1 \
+// RUN:  -verify %s
+
+#ifdef SUPPRESSED
+// expected-no-diagnostics
+#endif
 
 namespace rdar12676053 {
   // Delta-reduced from a preprocessed file.
@@ -85,7 +93,10 @@
 
 int *&box2 = m.getValue(i);
 box2 = 0;
-*box2 = 1; // expected-warning {{Dereference of null pointer}}
+*box2 = 1;
+#ifndef SUPPRESSED
+// expected-warning@-2 {{Dereference of null pointer}}
+#endif
   }
 
   SomeClass *&getSomeClass() {
Index: clang/test/Analysis/diagnos

[PATCH] D62978: [analyzer] ReturnVisitor: Handle non-null ReturnStmts

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

Suppressed example: F9091784: return-non-null.html 



Repository:
  rC Clang

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

https://reviews.llvm.org/D62978



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass constructing objects to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

If I run `scan-build` on LLVM this is the only non-bypassed case since the 
first diff: F9091851: non-bypassed.html 

I think it is working well.


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

https://reviews.llvm.org/D62926



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

[PATCH] D62946: [analyzer] ProgramPoint: more explicit printJson()

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D62946#1533592 , @NoQ wrote:

> Fair enough!
>
> Tests are welcome.


Is it possible? E.g. I have not found any case for `PostCondition` and may it 
requires 10+ of dot dumps which is very space-consuming. I think test to 
printing something is only required on their respective consumers side, which 
is D62761 , so later.


Repository:
  rC Clang

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

https://reviews.llvm.org/D62946



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass constructing objects to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 203635.
Charusso marked 2 inline comments as done.
Charusso added a comment.

- Added `CallExpr` as being purged away
- More test


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

https://reviews.llvm.org/D62926

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/new-ctor-null-throw.cpp
  clang/test/Analysis/new-ctor-null.cpp

Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -std=c++11 -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -DSUPPRESSED=1 \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,18 +14,41 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
   int x;
   S() : x(1) {}
   ~S() {}
+  int getX() const { return x; }
 };
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
 }
+
+void testCtor() {
+  S *s = new S();
+  s->x = 13;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Access to field 'x' results in a dereference of a null pointer (loaded from variable 's')}}
+#endif
+}
+
+void testMethod() {
+  S *s = new S();
+  const int X = s->getX();
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Called C++ object pointer is null}}
+#endif
+}
+
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -833,13 +833,20 @@
   return;
 
 // First, find when we processed the statement.
+// If we have one of the following cases that is going to be purged away
+// before its call take place. We would catch that purge in the second
+// condition as a 'StmtPoint' so we have to bypass it.
+const bool IsBypass =
+isa(S) || isa(S) || isa(S);
 do {
-  if (auto CEE = Node->getLocationAs())
+  if (Optional CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
+
+  if (!IsBypass)
+if (Optional SP = Node->getLocationAs())
+  if (SP->getStmt() == S)
+break;
 
   Node = Node->getFirstPred();
 } while (Node);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass constructing objects to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D62926#1533560 , @NoQ wrote:

> I'd like to have a test in which the constructor is //inlined//.


What do you mean by that test?


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

https://reviews.llvm.org/D62926



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 203874.
Charusso retitled this revision from "[analyzer] ReturnVisitor: Bypass 
constructing objects to see inlined calls" to "[analyzer] ReturnVisitor: Bypass 
everything to see inlined calls".
Charusso added a comment.

- The most generic approach.


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

https://reviews.llvm.org/D62926

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/new-ctor-null-throw.cpp
  clang/test/Analysis/new-ctor-null.cpp


Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection 
-analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection 
-analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -DSUPPRESSED \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,18 +14,41 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer 
unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer 
unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
   int x;
   S() : x(1) {}
   ~S() {}
+  int getX() const { return x; }
 };
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
 }
+
+void testCtor() {
+  S *s = new S();
+  s->x = 13;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Access to field 'x' results in a dereference of a 
null pointer (loaded from variable 's')}}
+#endif
+}
+
+void testMethod() {
+  S *s = new S();
+  const int X = s->getX();
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Called C++ object pointer is null}}
+#endif
+}
+
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -834,12 +834,9 @@
 
 // First, find when we processed the statement.
 do {
-  if (auto CEE = Node->getLocationAs())
+  if (Optional CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
 
   Node = Node->getFirstPred();
 } while (Node);


Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:842-849
+  if (Optional CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
+
+  if (!IsBypass)
+if (Optional SP = Node->getLocationAs())
+  if (SP->getStmt() == S)

NoQ wrote:
> Comparing statements is usually insufficient because the same statement may 
> appear multiple times due to recursion. When recursion occurs, you may reach 
> the same statement in a different location context. You should think in terms 
> of (statement, location context) pairs to avoid these problems. Your aim here 
> is to find the `CallExitEnd` node that corresponds to returning from an 
> inlined operator new to the current location context. You should stop 
> searching when you find an unrelated statement in the current location 
> context or when you exit the current location context entirely.
I have made a little test when we have a 25-second long Static Analyzer run 
with predefined names and checker. The loop ran 500 times in summary and we 
have some serious performance impacts at other places.

We exit the current context to see inlined calls, so that could not work sadly. 
If you remove that nonsense second condition we run at the same time, so if we 
have not got any problem since 7 years ago I think it is good to go.


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

https://reviews.llvm.org/D62926



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

[PATCH] D62978: [analyzer] ReturnVisitor: Handle unknown ReturnStmts better

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 203876.
Charusso retitled this revision from "[analyzer] ReturnVisitor: Handle non-null 
ReturnStmts" to "[analyzer] ReturnVisitor: Handle unknown ReturnStmts better".
Charusso edited the summary of this revision.
Charusso added a comment.

- The report was too misleading.
- It turns out we have to keep non-nulls.
- Now we do not treat unknown values as known.


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

https://reviews.llvm.org/D62978

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/diagnostics/find_last_store.c
  clang/test/Analysis/uninit-vals.c

Index: clang/test/Analysis/uninit-vals.c
===
--- clang/test/Analysis/uninit-vals.c
+++ clang/test/Analysis/uninit-vals.c
@@ -149,8 +149,6 @@
   RetVoidFuncType f = foo_radar12278788_fp;
   return ((RetIntFuncType)f)(); //expected-warning {{Undefined or garbage value returned to caller}}
 //expected-note@-1 {{Undefined or garbage value returned to caller}}
-//expected-note@-2 {{Calling 'foo_radar12278788_fp'}}
-//expected-note@-3 {{Returning from 'foo_radar12278788_fp'}}
 }
 
 void rdar13665798() {
@@ -164,8 +162,6 @@
 RetVoidFuncType f = foo_radar12278788_fp;
 return ((RetIntFuncType)f)(); //expected-warning {{Undefined or garbage value returned to caller}}
   //expected-note@-1 {{Undefined or garbage value returned to caller}}
-  //expected-note@-2 {{Calling 'foo_radar12278788_fp'}}
-  //expected-note@-3 {{Returning from 'foo_radar12278788_fp'}}
   }();
 }
 
@@ -182,18 +178,14 @@
 void use(struct Point p); 
 
 void testUseHalfPoint() {
-  struct Point p = getHalfPoint(); // expected-note{{Calling 'getHalfPoint'}}
-   // expected-note@-1{{Returning from 'getHalfPoint'}}
-   // expected-note@-2{{'p' initialized here}}
+  struct Point p = getHalfPoint(); // expected-note{{'p' initialized here}}
   use(p); // expected-warning{{uninitialized}}
   // expected-note@-1{{uninitialized}}
 }
 
 void testUseHalfPoint2() {
   struct Point p;
-  p = getHalfPoint(); // expected-note{{Calling 'getHalfPoint'}}
-  // expected-note@-1{{Returning from 'getHalfPoint'}}
-  // expected-note@-2{{Value assigned to 'p'}}
+  p = getHalfPoint(); // expected-note{{Value assigned to 'p'}}
   use(p); // expected-warning{{uninitialized}}
   // expected-note@-1{{uninitialized}}
 }
Index: clang/test/Analysis/diagnostics/find_last_store.c
===
--- clang/test/Analysis/diagnostics/find_last_store.c
+++ clang/test/Analysis/diagnostics/find_last_store.c
@@ -2,13 +2,11 @@
 typedef struct { float b; } c;
 void *a();
 void *d() {
-  return a(); // expected-note{{Returning pointer}}
+  return a();
 }
 
 void no_find_last_store() {
-  c *e = d(); // expected-note{{Calling 'd'}}
-  // expected-note@-1{{Returning from 'd'}}
-  // expected-note@-2{{'e' initialized here}}
+  c *e = d(); // expected-note{{'e' initialized here}}
 
   (void)(e || e->b); // expected-note{{Assuming 'e' is null}}
   // expected-note@-1{{Left side of '||' is false}}
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -874,7 +874,6 @@
   if (Optional RetLoc = RetVal.getAs())
 EnableNullFPSuppression = State->isNull(*RetLoc).isConstrainedTrue();
 
-BR.markInteresting(CalleeContext);
 BR.addVisitor(llvm::make_unique(CalleeContext,
EnableNullFPSuppression,
Options));
@@ -925,16 +924,13 @@
 
 RetE = RetE->IgnoreParenCasts();
 
-// If we're returning 0, we should track where that 0 came from.
-bugreporter::trackExpressionValue(N, RetE, BR, EnableNullFPSuppression);
-
 // Build an appropriate message based on the return value.
 SmallString<64> Msg;
 llvm::raw_svector_ostream Out(Msg);
 
+// Known to be null.
 if (State->isNull(V).isConstrainedTrue()) {
   if (V.getAs()) {
-
 // If we have counter-suppression enabled, make sure we keep visiting
 // future nodes. We want to emit a path note as well, in case
 // the report is resurrected as valid later on.
@@ -950,8 +946,8 @@
   } else {
 Out << "Returning zero";
   }
-
-} else {
+// Known to be non-null.
+} else if (State->isNonNull(V).isConstrainedTrue()) {
   if (auto CI = V.getAs()) {
 Out << "Returning the valu

[PATCH] D62978: [analyzer] ReturnVisitor: Handle unknown ReturnStmts better

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D62978#1533558 , @NoQ wrote:

> Ah, that positive!


Positive == true positive, not false positive, I got it.

> No, i don't think this is a valid way to suppress it.

Bought me, they are worth to report. The misleading reports made me think I 
have to suppress them.

Example (difference starts at note 33, at line 410):
F9159208: before.html 
F9159209: after.html 


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

https://reviews.llvm.org/D62978



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

[PATCH] D63093: [analyzer] WIP: MallocChecker: Release temporary CXXNewExpr

[Csaba Dabis via Phabricator via cfe-commits]

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

-


Repository:
  rC Clang

https://reviews.llvm.org/D63093

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


Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -156,21 +156,14 @@
 
 typedef std::pair LeakInfo;
 
-class MallocChecker : public Checker,
- check::EndFunction,
- check::PreCall,
- check::PostStmt,
- check::PostStmt,
- check::NewAllocator,
- check::PreStmt,
- check::PostStmt,
- check::PostObjCMessage,
- check::Location,
- eval::Assume>
-{
+class MallocChecker
+: public Checker,
+ check::EndFunction, check::PreCall,
+ check::PostStmt, check::PostStmt,
+ check::PostStmt, 
check::NewAllocator,
+ check::PreStmt, check::PostStmt,
+ check::PostObjCMessage, check::Location, eval::Assume> {
 public:
   MallocChecker()
   : II_alloca(nullptr), II_win_alloca(nullptr), II_malloc(nullptr),
@@ -211,6 +204,7 @@
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   void checkPostStmt(const CallExpr *CE, CheckerContext &C) const;
   void checkPostStmt(const CXXNewExpr *NE, CheckerContext &C) const;
+  void checkPostStmt(const CXXBindTemporaryExpr *BTE, CheckerContext &C) const;
   void checkNewAllocator(const CXXNewExpr *NE, SVal Target,
  CheckerContext &C) const;
   void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
@@ -489,7 +483,8 @@
   bool IsReleased = (S && S->isReleased()) &&
 (!SPrev || !SPrev->isReleased());
   assert(!IsReleased ||
- (Stmt && (isa(Stmt) || isa(Stmt))) ||
+ (Stmt && (isa(Stmt) || isa(Stmt) ||
+   isa(Stmt))) ||
  (!Stmt && S->getAllocationFamily() == AF_InnerBuffer));
   return IsReleased;
 }
@@ -1103,6 +1098,34 @@
 processNewAllocation(NE, C, Target);
 }
 
+void MallocChecker::checkPostStmt(const CXXBindTemporaryExpr *BTE,
+  CheckerContext &C) const {
+  if (const auto *CE = dyn_cast(BTE->getSubExpr())) {
+if (CE->getNumArgs() == 0)
+  return;
+
+// If we catch a 'CXXNewExpr' set it is released as it is temporary.
+if (const auto *NE = dyn_cast(CE->getArg(0))) {
+  ProgramStateRef State = C.getState();
+  Optional RetVal = C.getSVal(NE);
+
+  SymbolRef Sym = RetVal->getAsLocSymbol();
+  assert(Sym);
+
+  if (const RefState *RS = State->get(Sym))
+if (RS->isReleased())
+  return;
+
+  // Set the symbol's state to Released.
+  State = State->set(
+  Sym, RefState::getReleased(NE->isArray() ? AF_CXXNewArray : 
AF_CXXNew,
+ NE));
+
+  C.addTransition(State);
+}
+  }
+}
+
 // Sets the extent value of the MemRegion allocated by
 // new expression NE to its size in Bytes.
 //


Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -156,21 +156,14 @@
 
 typedef std::pair LeakInfo;
 
-class MallocChecker : public Checker,
- check::EndFunction,
- check::PreCall,
- check::PostStmt,
- check::PostStmt,
- check::NewAllocator,
- check::PreStmt,
- check::PostStmt,
- check::PostObjCMessage,
- check::Location,
- eval::Assume>
-{
+class MallocChecker
+: public Checker,
+ check::EndFunction, check::PreCall,
+ check::PostStmt, check::PostStmt,
+ check::PostStmt, check::NewAllocator,
+ check::PreStmt, check::PostStmt,
+ check::PostObjCMessage, check::Location, eval::Assume> {
 public:
   MallocChecker()
   : II_alloca(nullptr), II_win_alloca(null

[PATCH] D63093: [analyzer] WIP: MallocChecker: Release temporary CXXNewExpr

[Csaba Dabis via Phabricator via cfe-commits]

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

This is heavily WIP as sometimes we have to release a `new` after we return it 
or a constructor did something with that. The direction is okay?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63093



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:842-849
+  if (Optional CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
+
+  if (!IsBypass)
+if (Optional SP = Node->getLocationAs())
+  if (SP->getStmt() == S)

NoQ wrote:
> Charusso wrote:
> > NoQ wrote:
> > > Comparing statements is usually insufficient because the same statement 
> > > may appear multiple times due to recursion. When recursion occurs, you 
> > > may reach the same statement in a different location context. You should 
> > > think in terms of (statement, location context) pairs to avoid these 
> > > problems. Your aim here is to find the `CallExitEnd` node that 
> > > corresponds to returning from an inlined operator new to the current 
> > > location context. You should stop searching when you find an unrelated 
> > > statement in the current location context or when you exit the current 
> > > location context entirely.
> > I have made a little test when we have a 25-second long Static Analyzer run 
> > with predefined names and checker. The loop ran 500 times in summary and we 
> > have some serious performance impacts at other places.
> > 
> > We exit the current context to see inlined calls, so that could not work 
> > sadly. If you remove that nonsense second condition we run at the same 
> > time, so if we have not got any problem since 7 years ago I think it is 
> > good to go.
> You should break the loop as soon as we go past the new-expression that we've 
> started with in the stack frame that we've started with. That is, you should 
> at most go through the constructor within the new-expression, and then break. 
> You shouldn't explore the whole graph to the root every time the operator-new 
> call isn't inlined.
> 
> This might still be slow in some rare cases, but it should be much faster.
Sorry, I was dumb and I have used `isParentOf()` before, I think it is the most 
simple approach now truly.


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

https://reviews.llvm.org/D62926



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 204111.
Charusso marked an inline comment as done.
Charusso added a comment.

- I have used `LocationContext::isParentOf()`which is not worked well, so I 
thought we are out of our context.
- With that I made some misleading assumptions about our code.


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

https://reviews.llvm.org/D62926

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/new-ctor-null-throw.cpp
  clang/test/Analysis/new-ctor-null.cpp

Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -DSUPPRESSED \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,18 +14,41 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
   int x;
   S() : x(1) {}
   ~S() {}
+  int getX() const { return x; }
 };
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
 }
+
+void testCtor() {
+  S *s = new S();
+  s->x = 13;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Access to field 'x' results in a dereference of a null pointer (loaded from variable 's')}}
+#endif
+}
+
+void testMethod() {
+  S *s = new S();
+  const int X = s->getX();
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Called C++ object pointer is null}}
+#endif
+}
+
Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -833,16 +833,15 @@
   return;
 
 // First, find when we processed the statement.
+const StackFrameContext *BeginSC =
+Node->getLocationContext()->getStackFrame();
 do {
-  if (auto CEE = Node->getLocationAs())
+  if (Optional CEE = Node->getLocationAs())
 if (CEE->getCalleeContext()->getCallSite() == S)
   break;
-  if (auto SP = Node->getLocationAs())
-if (SP->getStmt() == S)
-  break;
 
   Node = Node->getFirstPred();
-} while (Node);
+} while (Node && Node->getLocationContext()->getStackFrame() == BeginSC);
 
 // Next, step over any post-statement checks.
 while (Node && Node->getLocation().getAs())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62978: [analyzer] trackExpressionValue(): Handle unknown values better

[Csaba Dabis via Phabricator via cfe-commits]

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



Comment at: clang/test/Analysis/diagnostics/deref-track-symbolic-region.c:14
   int m = 0;
-  syz.x = foo(); // expected-note{{Value assigned to 'syz.x'}}
 

Here.



Comment at: clang/test/Analysis/diagnostics/find_last_store.c:9
 void no_find_last_store() {
-  c *e = d(); // expected-note{{Calling 'd'}}
-  // expected-note@-1{{Returning from 'd'}}
-  // expected-note@-2{{'e' initialized here}}
+  c *e = d(); // expected-note{{'e' initialized here}}
 

NoQ wrote:
> This remaining note is also unnecessary. You can safely stop tracking the 
> value at `e || ...`. In particular, `ReturnVisitor` is not at fault.
> 
> That said, when we renamed `trackNullOrUndefValue` to `trackExpressionValue`, 
> we kinda assumed that it's not really important whether the value is 
> null/undef or not - the function simply tracks the value. This change would 
> break this invariant, causing null values to be handled in a special manner. 
> I recommend adding another flag argument (similar to 
> `EnableNullFPSuppression`) that would allow the caller to tell whether it's 
> interested in the null or in the "whole" value (defaulting to the latter).
That is a great idea! I tried my best but after a while I give up because it is 
already too complex, so I just removed them. It turns out it makes sense to 
remove those notes, see inline.



Comment at: clang/test/Analysis/diagnostics/macro-null-return-suppression.cpp:53
   int *x = RETURN_NULL();
-  x = returnFreshPointer();  // expected-note{{Value assigned to 'x'}}
   if (x) {} // expected-note{{Taking false branch}}

Here.



Comment at: clang/test/Analysis/loop-widening-notes.cpp:12
// expected-note@-1 {{Loop condition is false. Execution 
continues on line 16}}
-   ++i,// expected-note {{Value assigned to 'p_a'}} 
i < flag_a;

Here.



Comment at: clang/test/Analysis/uninit-const.cpp:59
   int p = f6_1_sub(t); //expected-warning {{Assigned value is garbage or 
undefined}}
-   //expected-note@-1 {{Passing value via 1st parameter 
'p'}}
-   //expected-note@-2 {{Calling 'f6_1_sub'}}

Here we mismatch the parameter.


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

https://reviews.llvm.org/D62978



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

[PATCH] D62761: [analyzer] exploded-graph-rewriter: Implement a --diff mode.

[Csaba Dabis via Phabricator via cfe-commits]

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

I asked for the new behavior, but I think it should be cool.


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

https://reviews.llvm.org/D62761



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

[PATCH] D62761: [analyzer] exploded-graph-rewriter: Implement a --diff mode.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D62761#1539143 , @NoQ wrote:

> In D62761#1539137 , @Charusso wrote:
>
> > I asked for the new behavior, but I think it should be cool.
>
>
> The new behavior is on the original screenshot, i just uploaded the wrong 
> patch initially >.<


No problem, thanks for the screenshot and for the clarification!


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

https://reviews.llvm.org/D62761



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

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 204331.
Charusso marked an inline comment as done.

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

https://reviews.llvm.org/D62926

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/inlining/placement-new-fp-suppression.cpp
  clang/test/Analysis/new-ctor-null-throw.cpp
  clang/test/Analysis/new-ctor-null.cpp

Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -DSUPPRESSED \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,18 +14,41 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
   int x;
   S() : x(1) {}
   ~S() {}
+  int getX() const { return x; }
 };
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
 }
+
+void testCtor() {
+  S *s = new S();
+  s->x = 13;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Access to field 'x' results in a dereference of a null pointer (loaded from variable 's')}}
+#endif
+}
+
+void testMethod() {
+  S *s = new S();
+  const int X = s->getX();
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Called C++ object pointer is null}}
+#endif
+}
+
Index: clang/test/Analysis/inlining/placement-new-fp-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/inlining/placement-new-fp-suppression.cpp
@@ -0,0 +1,137 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core.CallAndMessage \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core.CallAndMessage \
+// RUN:  -DSUPPRESSED \
+// RUN:  -verify %s
+
+#ifdef SUPPRESSED
+// expected-no-diagnostics
+#endif
+
+#include "../Inputs/system-header-simulator-cxx.h"
+
+typedef unsigned long uintptr_t;
+
+void error();
+void *malloc(size_t);
+
+
+// From llvm/include/llvm/Support/MathExtras.h
+inline uintptr_t alignAddr(const void *Addr, size_t Alignment) {
+  return (((uintptr_t)Addr + Alignment - 1) & ~(uintptr_t)(Alignment - 1));
+}
+
+inline size_t alignmentAdjustment(const void *Ptr, size_t Alignment) {
+  return alignAddr(Ptr, Alignment) - (uintptr_t)Ptr;
+}
+
+
+// From llvm/include/llvm/Support/MemAlloc.h
+inline void *safe_malloc(size_t Sz) {
+  void *Result = malloc(Sz);
+  if (Result == nullptr)
+error();
+
+  return Result;
+}
+
+
+// From llvm/include/llvm/Support/Allocator.h
+class MallocAllocator {
+public:
+  void *Allocate(size_t Size, size_t /*Alignment*/) {
+return safe_malloc(Size);
+  }
+};
+
+class BumpPtrAllocator {
+public:
+  void *Allocate(size_t Size, size_t Alignment) {
+BytesAllocated += Size;
+size_t Adjustment = alignmentAdjustment(CurPtr, Alignment);
+size_t SizeToAllocate = Size;
+
+size_t PaddedSize = SizeToAllocate + Alignment - 1;
+uintptr_t AlignedAddr = alignAddr(Allocator.Allocate(PaddedSize, 0),
+  Alignment);
+char *AlignedPtr = (char*)AlignedAddr;
+
+return AlignedPtr;
+  }
+
+private:
+  char *CurPtr = nullptr;
+  size_t BytesAllocated = 0;
+  MallocAllocator Allocator;
+};
+
+
+// From clang/include/clang/AST/ASTContextAllocate.h
+class ASTContext;
+
+void *operator n

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D62926#1539191 , @NoQ wrote:

> All right, it seems that i'm completely misunderstanding this problem and 
> we've been talking past each other this whole time.
>
> The problem is not that we need to skip the `CXXConstructExpr`. The problem 
> is that we need to skip `CXXNewExpr` (!!). CFG elements for an operator-new 
> call go like this:


I really wanted to create a generic approach without any overhead. I have added 
the proper test case what is going on: we enter into another contexts and that 
should be modeled properly. I was not sure about how to do so, but after a 
while it has been born. It contains all of your ideas from all the comments and 
now we only bypassing CXXNewExprs.


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

https://reviews.llvm.org/D62926



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

[PATCH] D62946: [analyzer] ProgramPoint: more explicit printJson()

[Csaba Dabis via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL363187: [analyzer] ProgramPoint: more explicit printJson() 
(authored by Charusso, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62946?vs=203328&id=204334#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62946

Files:
  cfe/trunk/lib/Analysis/ProgramPoint.cpp


Index: cfe/trunk/lib/Analysis/ProgramPoint.cpp
===
--- cfe/trunk/lib/Analysis/ProgramPoint.cpp
+++ cfe/trunk/lib/Analysis/ProgramPoint.cpp
@@ -99,12 +99,6 @@
   case ProgramPoint::CallExitEndKind:
 Out << "CallExitEnd\"";
 break;
-  case ProgramPoint::PostStmtPurgeDeadSymbolsKind:
-Out << "PostStmtPurgeDeadSymbols\"";
-break;
-  case ProgramPoint::PreStmtPurgeDeadSymbolsKind:
-Out << "PreStmtPurgeDeadSymbols\"";
-break;
   case ProgramPoint::EpsilonKind:
 Out << "EpsilonPoint\"";
 break;
@@ -210,20 +204,35 @@
 Out << ", ";
 printLocJson(Out, S->getBeginLoc(), SM);
 
-Out << ", \"stmt_point_kind\": ";
-if (getAs())
-  Out << "\"PreStmt\"";
+Out << ", \"stmt_point_kind\": \"";
+if (getAs())
+  Out << "PreLoad";
+else if (getAs())
+  Out << "PreStore";
+else if (getAs())
+  Out << "PostAllocatorCall";
+else if (getAs())
+  Out << "PostCondition";
 else if (getAs())
-  Out << "\"PostLoad\"";
-else if (getAs())
-  Out << "\"PostStore\"";
+  Out << "PostLoad";
 else if (getAs())
-  Out << "\"PostLValue\"";
-else if (getAs())
-  Out << "\"PostAllocatorCall\"";
-else
-  Out << "null";
+  Out << "PostLValue";
+else if (getAs())
+  Out << "PostStore";
+else if (getAs())
+  Out << "PostStmt";
+else if (getAs())
+  Out << "PostStmtPurgeDeadSymbols";
+else if (getAs())
+  Out << "PreStmtPurgeDeadSymbols";
+else if (getAs())
+  Out << "PreStmt";
+else {
+  Out << "\nKind: '" << getKind();
+  llvm_unreachable("' is unhandled StmtPoint kind!");
+}
 
+Out << '\"';
 break;
   }
   }


Index: cfe/trunk/lib/Analysis/ProgramPoint.cpp
===
--- cfe/trunk/lib/Analysis/ProgramPoint.cpp
+++ cfe/trunk/lib/Analysis/ProgramPoint.cpp
@@ -99,12 +99,6 @@
   case ProgramPoint::CallExitEndKind:
 Out << "CallExitEnd\"";
 break;
-  case ProgramPoint::PostStmtPurgeDeadSymbolsKind:
-Out << "PostStmtPurgeDeadSymbols\"";
-break;
-  case ProgramPoint::PreStmtPurgeDeadSymbolsKind:
-Out << "PreStmtPurgeDeadSymbols\"";
-break;
   case ProgramPoint::EpsilonKind:
 Out << "EpsilonPoint\"";
 break;
@@ -210,20 +204,35 @@
 Out << ", ";
 printLocJson(Out, S->getBeginLoc(), SM);
 
-Out << ", \"stmt_point_kind\": ";
-if (getAs())
-  Out << "\"PreStmt\"";
+Out << ", \"stmt_point_kind\": \"";
+if (getAs())
+  Out << "PreLoad";
+else if (getAs())
+  Out << "PreStore";
+else if (getAs())
+  Out << "PostAllocatorCall";
+else if (getAs())
+  Out << "PostCondition";
 else if (getAs())
-  Out << "\"PostLoad\"";
-else if (getAs())
-  Out << "\"PostStore\"";
+  Out << "PostLoad";
 else if (getAs())
-  Out << "\"PostLValue\"";
-else if (getAs())
-  Out << "\"PostAllocatorCall\"";
-else
-  Out << "null";
+  Out << "PostLValue";
+else if (getAs())
+  Out << "PostStore";
+else if (getAs())
+  Out << "PostStmt";
+else if (getAs())
+  Out << "PostStmtPurgeDeadSymbols";
+else if (getAs())
+  Out << "PreStmtPurgeDeadSymbols";
+else if (getAs())
+  Out << "PreStmt";
+else {
+  Out << "\nKind: '" << getKind();
+  llvm_unreachable("' is unhandled StmtPoint kind!");
+}
 
+Out << '\"';
 break;
   }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 204625.
Charusso marked 3 inline comments as done.
Charusso added a comment.

- Fixed comments.


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

https://reviews.llvm.org/D62926

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/inlining/placement-new-fp-suppression.cpp
  clang/test/Analysis/new-ctor-null-throw.cpp
  clang/test/Analysis/new-ctor-null.cpp

Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -DSUPPRESSED \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,18 +14,41 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
   int x;
   S() : x(1) {}
   ~S() {}
+  int getX() const { return x; }
 };
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
 }
+
+void testCtor() {
+  S *s = new S();
+  s->x = 13;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Access to field 'x' results in a dereference of a null pointer (loaded from variable 's')}}
+#endif
+}
+
+void testMethod() {
+  S *s = new S();
+  const int X = s->getX();
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Called C++ object pointer is null}}
+#endif
+}
+
Index: clang/test/Analysis/inlining/placement-new-fp-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/inlining/placement-new-fp-suppression.cpp
@@ -0,0 +1,137 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core.CallAndMessage \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core.CallAndMessage \
+// RUN:  -DSUPPRESSED \
+// RUN:  -verify %s
+
+#ifdef SUPPRESSED
+// expected-no-diagnostics
+#endif
+
+#include "../Inputs/system-header-simulator-cxx.h"
+
+typedef unsigned long uintptr_t;
+
+void error();
+void *malloc(size_t);
+
+
+// From llvm/include/llvm/Support/MathExtras.h
+inline uintptr_t alignAddr(const void *Addr, size_t Alignment) {
+  return (((uintptr_t)Addr + Alignment - 1) & ~(uintptr_t)(Alignment - 1));
+}
+
+inline size_t alignmentAdjustment(const void *Ptr, size_t Alignment) {
+  return alignAddr(Ptr, Alignment) - (uintptr_t)Ptr;
+}
+
+
+// From llvm/include/llvm/Support/MemAlloc.h
+inline void *safe_malloc(size_t Sz) {
+  void *Result = malloc(Sz);
+  if (Result == nullptr)
+error();
+
+  return Result;
+}
+
+
+// From llvm/include/llvm/Support/Allocator.h
+class MallocAllocator {
+public:
+  void *Allocate(size_t Size, size_t /*Alignment*/) {
+return safe_malloc(Size);
+  }
+};
+
+class BumpPtrAllocator {
+public:
+  void *Allocate(size_t Size, size_t Alignment) {
+BytesAllocated += Size;
+size_t Adjustment = alignmentAdjustment(CurPtr, Alignment);
+size_t SizeToAllocate = Size;
+
+size_t PaddedSize = SizeToAllocate + Alignment - 1;
+uintptr_t AlignedAddr = alignAddr(Allocator.Allocate(PaddedSize, 0),
+  Alignment);
+char *AlignedPtr = (char*)AlignedAddr;
+
+return AlignedPtr;
+  }
+
+private:
+  char *CurPtr = nullptr;
+  size_t BytesAllocated = 0;
+  MallocAllocator Allocator;
+};
+
+
+// From clang/include/clang/AST/ASTContextAll

[PATCH] D62926: [analyzer] ReturnVisitor: Bypass everything to see inlined calls

[Csaba Dabis via Phabricator via cfe-commits]

Charusso updated this revision to Diff 204698.
Charusso marked an inline comment as done.
Charusso added a comment.

- Whoops.


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

https://reviews.llvm.org/D62926

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/inlining/placement-new-fp-suppression.cpp
  clang/test/Analysis/new-ctor-null-throw.cpp
  clang/test/Analysis/new-ctor-null.cpp

Index: clang/test/Analysis/new-ctor-null.cpp
===
--- clang/test/Analysis/new-ctor-null.cpp
+++ clang/test/Analysis/new-ctor-null.cpp
@@ -1,4 +1,6 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,debug.ExprInspection \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_warnIfReached();
@@ -24,7 +26,8 @@
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+  // no-warning: 'Dereference of null pointer' suppressed by ReturnVisitor.
 }
 
 int global;
Index: clang/test/Analysis/new-ctor-null-throw.cpp
===
--- clang/test/Analysis/new-ctor-null-throw.cpp
+++ clang/test/Analysis/new-ctor-null-throw.cpp
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection -analyzer-config c++-allocator-inlining=true -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core \
+// RUN:  -DSUPPRESSED \
+// RUN:  -verify %s
 
 void clang_analyzer_eval(bool);
 
@@ -9,18 +14,41 @@
 // operator new.
 void *operator new(size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 void *operator new[](size_t size) {
   return nullptr;
+  // expected-warning@-1 {{'operator new[]' should not return a null pointer unless it is declared 'throw()' or 'noexcept'}}
 }
 
 struct S {
   int x;
   S() : x(1) {}
   ~S() {}
+  int getX() const { return x; }
 };
 
 void testArrays() {
   S *s = new S[10]; // no-crash
-  s[0].x = 2; // expected-warning{{Dereference of null pointer}}
+  s[0].x = 2;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Dereference of null pointer}}
+#endif
 }
+
+void testCtor() {
+  S *s = new S();
+  s->x = 13;
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Access to field 'x' results in a dereference of a null pointer (loaded from variable 's')}}
+#endif
+}
+
+void testMethod() {
+  S *s = new S();
+  const int X = s->getX();
+#ifndef SUPPRESSED
+  // expected-warning@-2 {{Called C++ object pointer is null}}
+#endif
+}
+
Index: clang/test/Analysis/inlining/placement-new-fp-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/inlining/placement-new-fp-suppression.cpp
@@ -0,0 +1,137 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core.CallAndMessage \
+// RUN:  -analyzer-config suppress-null-return-paths=false \
+// RUN:  -verify %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core.CallAndMessage \
+// RUN:  -DSUPPRESSED \
+// RUN:  -verify %s
+
+#ifdef SUPPRESSED
+// expected-no-diagnostics
+#endif
+
+#include "../Inputs/system-header-simulator-cxx.h"
+
+typedef unsigned long uintptr_t;
+
+void error();
+void *malloc(size_t);
+
+
+// From llvm/include/llvm/Support/MathExtras.h
+inline uintptr_t alignAddr(const void *Addr, size_t Alignment) {
+  return (((uintptr_t)Addr + Alignment - 1) & ~(uintptr_t)(Alignment - 1));
+}
+
+inline size_t alignmentAdjustment(const void *Ptr, size_t Alignment) {
+  return alignAddr(Ptr, Alignment) - (uintptr_t)Ptr;
+}
+
+
+// From llvm/include/llvm/Support/MemAlloc.h
+inline void *safe_malloc(size_t Sz) {
+  void *Result = malloc(Sz);
+  if (Result == nullptr)
+error();
+
+  return Result;
+}
+
+
+// From llvm/include/llvm/Support/Allocator.h
+class MallocAllocator {
+public:
+  void *Allocate(size_t Size, size_t /*Alignment*/) {
+return safe_malloc(Size);
+  }
+};
+
+class BumpPtrAllocator {
+public:
+  void *Allocate(size_t Size, size_t Alignment) {
+BytesAllocated += Size;
+size_t Adjustment = alignmentAdjustment(CurPtr, Alignment);
+size_t SizeToAllocate = Size;
+
+size_t PaddedSize = SizeToAllocate + Alignment - 1;
+uintptr_t AlignedAddr = alignAddr(Allocator.Allocate(PaddedSize, 0),
+  Alignment);
+char *AlignedPtr = (char*)AlignedAddr;
+
+return AlignedPtr;
+  }
+
+private:
+  char *CurPtr = nullptr;
+  size_t BytesAllocated = 0;
+  MallocAllocator Allocator;
+};
+
+
+// From clang/include/clang/AST/ASTContextAllocate.h