[clang] [analyzer] Fix crash on using `bitcast(, )` as array subscript (PR #101647)
steakhal wrote: Backporting to clang-19 in https://github.com/llvm/llvm-project/pull/101684 https://github.com/llvm/llvm-project/pull/101647 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash on using `bitcast(, )` as array subscript (PR #101647)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/101647 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash on using `bitcast(, )` as array subscript (PR #101647)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/101647 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Restore recognition of mutex methods (PR #101511)
=?utf-8?q?Don=C3=A1t?= Nagy Message-ID: In-Reply-To: steakhal wrote: Backport PR in https://github.com/llvm/llvm-project/pull/101651 https://github.com/llvm/llvm-project/pull/101511 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Restore recognition of mutex methods (PR #101511)
=?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/101511 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Restore recognition of mutex methods (PR #101511)
=?utf-8?q?Don=C3=A1t?= Nagy Message-ID: In-Reply-To: https://github.com/steakhal approved this pull request. Let's merge this, and backport it into clang-19. I'll deal with that. https://github.com/llvm/llvm-project/pull/101511 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][Static analyzer] fix crash on using `bitcast(, )` as array subscript (PR #101647)
@@ -472,7 +472,19 @@ SVal StoreManager::getLValueElement(QualType elementType,
NonLoc Offset,
const auto *ElemR = dyn_cast(BaseRegion);
// Convert the offset to the appropriate size and signedness.
- Offset = svalBuilder.convertToArrayIndex(Offset).castAs();
+ auto Off = svalBuilder.convertToArrayIndex(Offset).getAs();
+ if (!Off) {
+// Handle cases when LazyCompoundVal is used for an array index.
+// Such case is possible if code does:
+//
+// char b[4];
+// a[__builtin_bitcast(int, b)];
+//
steakhal wrote:
```suggestion
// char b[4];
// a[__builtin_bitcast(int, b)];
```
https://github.com/llvm/llvm-project/pull/101647
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][Static analyzer] fix crash on using `bitcast(, )` as array subscript (PR #101647)
@@ -472,7 +472,19 @@ SVal StoreManager::getLValueElement(QualType elementType, NonLoc Offset, const auto *ElemR = dyn_cast(BaseRegion); // Convert the offset to the appropriate size and signedness. - Offset = svalBuilder.convertToArrayIndex(Offset).castAs(); + auto Off = svalBuilder.convertToArrayIndex(Offset).getAs(); steakhal wrote: ```suggestion Offset = svalBuilder.convertToArrayIndex(Offset).getAs().value_or(UnknownVal()); ``` https://github.com/llvm/llvm-project/pull/101647 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][Static analyzer] fix crash on using `bitcast(, )` as array subscript (PR #101647)
@@ -30,3 +30,10 @@ void f3(void *dest) {
void *src = __builtin_alloca(5);
memcpy(dest, src, 1); // expected-warning{{2nd function call argument is a
pointer to uninitialized value}}
}
+
+// Reproduce crash from GH#94496. When array is used as subcript to another
array, CSA cannot model it
+// and should just assume it's unknown and do not crash.
+void f4(char *array) {
+ char b[4] = {0};
+ array[__builtin_bit_cast(int, b)] = 0x10; // no crash
steakhal wrote:
Please enable the `debug.ExprInspection` checker too, and then forward declare
(but not define) the `void clang_analyzer_dump_int(int);
```suggestion
clang_analyzer_dump_int(__builtin_bit_cast(int, b)); // expected-warning
{{Unknown}}
array[__builtin_bit_cast(int, b)] = 0x10; // no crash
```
Hold on. Shouldn't we pin the target triple to be able to safely assume that a
sizeof `int` is 4 chars? Consider pinning the target using `-triple xxx` in the
RUN line.
https://github.com/llvm/llvm-project/pull/101647
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][Static analyzer] fix crash on using `bitcast(, )` as array subscript (PR #101647)
https://github.com/steakhal requested changes to this pull request. https://github.com/llvm/llvm-project/pull/101647 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Assert Callee is FunctionDecl in doesFnIntendToHandleOwnership() (PR #101066)
steakhal wrote: > > Already under fix in #100719, I'm just a lazy bum and haven't fixed the > > buildbots. I'll try to speed things up! > > @Szelethus, would it possible to speed up the PR#100719? Thanks Why? https://github.com/llvm/llvm-project/pull/101066 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [NFCI][clang][analyzer] Make ProgramStatePartialTrait a template definition (PR #98150)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/98150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] add support for handling assume attributes (PR #101063)
steakhal wrote: No, its cool. You are on the right track. Ill come back to you tomorrow. https://github.com/llvm/llvm-project/pull/101063 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Don't invalidate the super region when a std object ctor runs (PR #100405)
@@ -923,12 +923,31 @@ SVal AnyCXXConstructorCall::getCXXThisVal() const {
return UnknownVal();
}
+static bool isWithinStdNamespace(const Decl *D) {
steakhal wrote:
AFAIK not quite. Mine works even if the class a nested subclass within the std
namespace, such as "std::vector::iterator". So directly using
`isInStdNamespace` wouldn't quite cut it.
https://github.com/llvm/llvm-project/pull/100405
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Don't invalidate the super region when a std object ctor runs (PR #100405)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/100405 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Don't invalidate the super region when a std object ctor runs (PR #100405)
@@ -923,12 +923,31 @@ SVal AnyCXXConstructorCall::getCXXThisVal() const {
return UnknownVal();
}
+static bool isWithinStdNamespace(const Decl *D) {
steakhal wrote:
@AaronBallman Do you think we clang already has something like this? Or it
would make sense to have this part of Decl?
https://github.com/llvm/llvm-project/pull/100405
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/100990 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
steakhal wrote: > This looks good now, documentation could be a bit more exact in that > operations on standard streams are not checked by the checker, like any other > operation on streams that are not opened on the analysis path. Elaborated the docs section. https://github.com/llvm/llvm-project/pull/100990 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
https://github.com/steakhal updated
https://github.com/llvm/llvm-project/pull/100990
>From 12ff3274fce8dfe534e71abb9511d6549e32f74c Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 29 Jul 2024 10:33:55 +0200
Subject: [PATCH 1/7] [analyzer] Fix crash of StreamChecker when eval calling
'fopen'
Actually, on the failure branch of `fopen`, the resulting pointer could
alias with `stdout` iff `stdout` is already known to be null.
We crashed in this case as the implementation assumed that the
state-split for creating the success and failure branches both should be
viable; thus dereferenced both of those states - leading to the crash.
To fix this, let's just only add this no-alias property for the success
path, and that's it :)
Fixes #100901
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 3 ++-
clang/test/Analysis/stream.c| 10 +-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 53770532609d5..942e7541a83cd 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -938,7 +938,6 @@ void StreamChecker::evalFopen(const FnDescription *Desc,
const CallEvent ,
assert(RetSym && "RetVal must be a symbol here.");
State = State->BindExpr(CE, C.getLocationContext(), RetVal);
- State = assumeNoAliasingWithStdStreams(State, RetVal, C);
// Bifurcate the state into two: one with a valid FILE* pointer, the other
// with a NULL.
@@ -951,6 +950,8 @@ void StreamChecker::evalFopen(const FnDescription *Desc,
const CallEvent ,
StateNull =
StateNull->set(RetSym, StreamState::getOpenFailed(Desc));
+ StateNotNull = assumeNoAliasingWithStdStreams(StateNotNull, RetVal, C);
+
C.addTransition(StateNotNull,
constructLeakNoteTag(C, RetSym, "Stream opened here"));
C.addTransition(StateNull);
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index b3a47ce4153d3..575b32d97be31 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -502,11 +502,19 @@ void gh_93408_regression_ZeroSized(struct ZeroSized
*buffer) {
extern FILE *stdout_like_ptr;
void no_aliasing(void) {
FILE *f = fopen("file", "r");
+ if (!f) return;
clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}}
expected-warning {{TRUE}}
- if (f && f != stdout) {
+ if (f != stdout) {
fclose(f);
}
} // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.
+
+void only_success_path_does_not_alias_with_stdout(void) {
+ if (stdout) return;
+ FILE *f = fopen("/tmp/foof", "r"); // no-crash
+ if (!f) return;
+ fclose(f);
+}
>From 7453968ac1764adbc70f867d43fa3d63473b09f3 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 29 Jul 2024 10:39:03 +0200
Subject: [PATCH 2/7] NFC Fixup previous PR comment
Addresses:
https://github.com/llvm/llvm-project/pull/100085#discussion_r1688051166
---
clang/test/Analysis/stream.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 575b32d97be31..aff884f190c8e 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -499,14 +499,14 @@ void gh_93408_regression_ZeroSized(struct ZeroSized
*buffer) {
fclose(f);
}
-extern FILE *stdout_like_ptr;
-void no_aliasing(void) {
+extern FILE *non_standard_stream_ptr;
+void test_fopen_does_not_alias_with_standard_streams(void) {
FILE *f = fopen("file", "r");
if (!f) return;
clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}}
no-TRUE
- clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}}
expected-warning {{TRUE}}
+ clang_analyzer_eval(f == non_standard_stream_ptr); // expected-warning
{{FALSE}} expected-warning {{TRUE}}
if (f != stdout) {
fclose(f);
}
>From db92245ab09f195cbb859d85f2b93a81fd1b2617 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 29 Jul 2024 10:46:16 +0200
Subject: [PATCH 3/7] NFC Fixup previous PR comment
Addresses
https://github.com/llvm/llvm-project/pull/100085#discussion_r1688054004
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 25 ++-
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 942e7541a83cd..4454f30630e27 100644
---
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
@@ -1766,13 +1770,6 @@ are assumed to succeed.) fclose(p); } -**Limitations** - -The checker does not track the correspondence between integer file descriptors -and ``FILE *`` pointers. Operations on standard streams like ``stdin`` are not -treated specially and are therefore often not recognized (because these streams -are usually not opened explicitly by the program, and are global variables). steakhal wrote: Ah indeed. So basically opening a file with file descriptor "ints" and then using `fdopen()` to get the corresponding `FILE *` - and back with `fileno()`. Yea, that seems weird to do :D Probably has its use somewhere though. Anyways, makes sense now. https://github.com/llvm/llvm-project/pull/100990 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
steakhal wrote: @balazske Could you please have a look? I wanna make sure everyone is aligned. https://github.com/llvm/llvm-project/pull/100990 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
https://github.com/steakhal updated
https://github.com/llvm/llvm-project/pull/100990
>From 12ff3274fce8dfe534e71abb9511d6549e32f74c Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 29 Jul 2024 10:33:55 +0200
Subject: [PATCH 1/6] [analyzer] Fix crash of StreamChecker when eval calling
'fopen'
Actually, on the failure branch of `fopen`, the resulting pointer could
alias with `stdout` iff `stdout` is already known to be null.
We crashed in this case as the implementation assumed that the
state-split for creating the success and failure branches both should be
viable; thus dereferenced both of those states - leading to the crash.
To fix this, let's just only add this no-alias property for the success
path, and that's it :)
Fixes #100901
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 3 ++-
clang/test/Analysis/stream.c| 10 +-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 53770532609d5..942e7541a83cd 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -938,7 +938,6 @@ void StreamChecker::evalFopen(const FnDescription *Desc,
const CallEvent ,
assert(RetSym && "RetVal must be a symbol here.");
State = State->BindExpr(CE, C.getLocationContext(), RetVal);
- State = assumeNoAliasingWithStdStreams(State, RetVal, C);
// Bifurcate the state into two: one with a valid FILE* pointer, the other
// with a NULL.
@@ -951,6 +950,8 @@ void StreamChecker::evalFopen(const FnDescription *Desc,
const CallEvent ,
StateNull =
StateNull->set(RetSym, StreamState::getOpenFailed(Desc));
+ StateNotNull = assumeNoAliasingWithStdStreams(StateNotNull, RetVal, C);
+
C.addTransition(StateNotNull,
constructLeakNoteTag(C, RetSym, "Stream opened here"));
C.addTransition(StateNull);
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index b3a47ce4153d3..575b32d97be31 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -502,11 +502,19 @@ void gh_93408_regression_ZeroSized(struct ZeroSized
*buffer) {
extern FILE *stdout_like_ptr;
void no_aliasing(void) {
FILE *f = fopen("file", "r");
+ if (!f) return;
clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}}
expected-warning {{TRUE}}
- if (f && f != stdout) {
+ if (f != stdout) {
fclose(f);
}
} // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.
+
+void only_success_path_does_not_alias_with_stdout(void) {
+ if (stdout) return;
+ FILE *f = fopen("/tmp/foof", "r"); // no-crash
+ if (!f) return;
+ fclose(f);
+}
>From 7453968ac1764adbc70f867d43fa3d63473b09f3 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 29 Jul 2024 10:39:03 +0200
Subject: [PATCH 2/6] NFC Fixup previous PR comment
Addresses:
https://github.com/llvm/llvm-project/pull/100085#discussion_r1688051166
---
clang/test/Analysis/stream.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 575b32d97be31..aff884f190c8e 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -499,14 +499,14 @@ void gh_93408_regression_ZeroSized(struct ZeroSized
*buffer) {
fclose(f);
}
-extern FILE *stdout_like_ptr;
-void no_aliasing(void) {
+extern FILE *non_standard_stream_ptr;
+void test_fopen_does_not_alias_with_standard_streams(void) {
FILE *f = fopen("file", "r");
if (!f) return;
clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}}
no-TRUE
- clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}}
expected-warning {{TRUE}}
+ clang_analyzer_eval(f == non_standard_stream_ptr); // expected-warning
{{FALSE}} expected-warning {{TRUE}}
if (f != stdout) {
fclose(f);
}
>From db92245ab09f195cbb859d85f2b93a81fd1b2617 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 29 Jul 2024 10:46:16 +0200
Subject: [PATCH 3/6] NFC Fixup previous PR comment
Addresses
https://github.com/llvm/llvm-project/pull/100085#discussion_r1688054004
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 25 ++-
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 942e7541a83cd..4454f30630e27 100644
---
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
@@ -1766,13 +1770,6 @@ are assumed to succeed.) fclose(p); } -**Limitations** - -The checker does not track the correspondence between integer file descriptors -and ``FILE *`` pointers. Operations on standard streams like ``stdin`` are not -treated specially and are therefore often not recognized (because these streams -are usually not opened explicitly by the program, and are global variables). steakhal wrote: Yea, but I didn't know exactly what it refers to. We could add an example for such a case there. That would help. https://github.com/llvm/llvm-project/pull/100990 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
@@ -1703,7 +1703,11 @@ are detected:
* Invalid 3rd ("``whence``") argument to ``fseek``.
The stream operations are by this checker usually split into two cases, a
success
-and a failure case. However, in the case of write operations (like ``fwrite``,
+and a failure case.
+On the success case it also assumes that the current value of ``stdout``,
+``stderr``, or ``stdin`` can't be equal to the file pointer returned by
``fopen``.
+
+In the case of write operations (like ``fwrite``,
steakhal wrote:
Same feeling. But I decided to opt for minimizing the diff. I still think that
I made the right call.
https://github.com/llvm/llvm-project/pull/100990
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Avoid crashes in the stream checker (PR #100901)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/100901 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Avoid crashes in the stream checker (PR #100901)
steakhal wrote: Thanks for the really valuable reduced case @vabridgers. I'll close this in favor of #100990 if you don't mind. Thanks again for reporting the crash. https://github.com/llvm/llvm-project/pull/100901 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
steakhal wrote: I decided to put the fixup NFC changes along with this PR (the ones were submitted after I merged the original commit), but on hindsight probably it would be better to merge those NFC changes separately. If you request, I'll split the PR. https://github.com/llvm/llvm-project/pull/100990 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
@@ -1766,13 +1770,6 @@ are assumed to succeed.) fclose(p); } -**Limitations** - -The checker does not track the correspondence between integer file descriptors -and ``FILE *`` pointers. Operations on standard streams like ``stdin`` are not -treated specially and are therefore often not recognized (because these streams -are usually not opened explicitly by the program, and are global variables). steakhal wrote: I wasn't sure of this section, so I just removed it. Let me know if it's still relevant or not. https://github.com/llvm/llvm-project/pull/100990 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Fix crash of StreamChecker when eval calling 'fopen' (PR #100990)
https://github.com/steakhal created
https://github.com/llvm/llvm-project/pull/100990
Actually, on the failure branch of `fopen`, the resulting pointer could
alias with `stdout` iff `stdout` is already known to be null.
We crashed in this case as the implementation assumed that the
state-split for creating the success and failure branches both should be
viable; thus dereferenced both of those states - leading to the crash.
To fix this, let's just only add this no-alias property for the success
path, and that's it :)
Fixes #100901
>From 12ff3274fce8dfe534e71abb9511d6549e32f74c Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 29 Jul 2024 10:33:55 +0200
Subject: [PATCH 1/5] [analyzer] Fix crash of StreamChecker when eval calling
'fopen'
Actually, on the failure branch of `fopen`, the resulting pointer could
alias with `stdout` iff `stdout` is already known to be null.
We crashed in this case as the implementation assumed that the
state-split for creating the success and failure branches both should be
viable; thus dereferenced both of those states - leading to the crash.
To fix this, let's just only add this no-alias property for the success
path, and that's it :)
Fixes #100901
---
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 3 ++-
clang/test/Analysis/stream.c| 10 +-
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 53770532609d5..942e7541a83cd 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -938,7 +938,6 @@ void StreamChecker::evalFopen(const FnDescription *Desc,
const CallEvent ,
assert(RetSym && "RetVal must be a symbol here.");
State = State->BindExpr(CE, C.getLocationContext(), RetVal);
- State = assumeNoAliasingWithStdStreams(State, RetVal, C);
// Bifurcate the state into two: one with a valid FILE* pointer, the other
// with a NULL.
@@ -951,6 +950,8 @@ void StreamChecker::evalFopen(const FnDescription *Desc,
const CallEvent ,
StateNull =
StateNull->set(RetSym, StreamState::getOpenFailed(Desc));
+ StateNotNull = assumeNoAliasingWithStdStreams(StateNotNull, RetVal, C);
+
C.addTransition(StateNotNull,
constructLeakNoteTag(C, RetSym, "Stream opened here"));
C.addTransition(StateNull);
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index b3a47ce4153d3..575b32d97be31 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -502,11 +502,19 @@ void gh_93408_regression_ZeroSized(struct ZeroSized
*buffer) {
extern FILE *stdout_like_ptr;
void no_aliasing(void) {
FILE *f = fopen("file", "r");
+ if (!f) return;
clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}}
expected-warning {{TRUE}}
- if (f && f != stdout) {
+ if (f != stdout) {
fclose(f);
}
} // no-leak: 'fclose()' is always called because 'f' cannot be 'stdout'.
+
+void only_success_path_does_not_alias_with_stdout(void) {
+ if (stdout) return;
+ FILE *f = fopen("/tmp/foof", "r"); // no-crash
+ if (!f) return;
+ fclose(f);
+}
>From 7453968ac1764adbc70f867d43fa3d63473b09f3 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 29 Jul 2024 10:39:03 +0200
Subject: [PATCH 2/5] NFC Fixup previous PR comment
Addresses:
https://github.com/llvm/llvm-project/pull/100085#discussion_r1688051166
---
clang/test/Analysis/stream.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index 575b32d97be31..aff884f190c8e 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -499,14 +499,14 @@ void gh_93408_regression_ZeroSized(struct ZeroSized
*buffer) {
fclose(f);
}
-extern FILE *stdout_like_ptr;
-void no_aliasing(void) {
+extern FILE *non_standard_stream_ptr;
+void test_fopen_does_not_alias_with_standard_streams(void) {
FILE *f = fopen("file", "r");
if (!f) return;
clang_analyzer_eval(f == stdin); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}}
no-TRUE
clang_analyzer_eval(f == stderr); // expected-warning {{FALSE}}
no-TRUE
- clang_analyzer_eval(f == stdout_like_ptr); // expected-warning {{FALSE}}
expected-warning {{TRUE}}
+ clang_analyzer_eval(f == non_standard_stream_ptr); // expected-warning
{{FALSE}} expected-warning {{TRUE}}
if (f != stdout) {
fclose(f);
}
>From db92245ab09f195cbb859d85f2b93a81fd1b2617 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 29 Jul 2024 10:46:16
[clang] [analyzer] Avoid crashes in the stream checker (PR #100901)
steakhal wrote: > In the state dump I see that `stdout` seems to be NULL (last line in > "constraints"). This explains why the `StateNull` becomes NULL, because call > to `assumeNoAliasingWithStdStreams` was called already. I think the better > solution is to check NULL-ness of the std stream variable > `assumeNoAliasingWithStdStreams` and do not assume if it is NULL. There is > not a case when `fopen` returns non-null for sure, but at least not in the > current situation, so the current fix is not as good. We could add an > `assert` to check if both `StateNull` and `StateNotNull` are non-zero. Exactly. I didn't want to rush too much, but I can share that my current idea is to call `assumeNoAliasingWithStdStreams` only on the success path. https://github.com/llvm/llvm-project/pull/100901 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Keep alive short-circuiting condition subexpressions in a conditional (PR #100745)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/100745 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Keep alive short-circuiting condition subexpressions in a conditional (PR #100745)
steakhal wrote: We'll merge this on Monday. https://github.com/llvm/llvm-project/pull/100745 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Eliminate a dyn_cast (PR #100719)
https://github.com/steakhal approved this pull request. https://github.com/llvm/llvm-project/pull/100719 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Don't invalidate the super region when a std object ctor runs (PR #100405)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/100405 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Don't invalidate the super region when a std object ctor runs (PR #100405)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/100405 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Don't invalidate the super region when a std object ctor runs (PR #100405)
@@ -923,12 +923,31 @@ SVal AnyCXXConstructorCall::getCXXThisVal() const {
return UnknownVal();
}
+static bool isWithinStdNamespace(const Decl *D) {
steakhal wrote:
Makes sense. I think I'd prefer moving this utility in a followup patch, as it
may take a while to reach consensus where to put it. And I don't want to block
this PR until that happens.
https://github.com/llvm/llvm-project/pull/100405
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
https://github.com/steakhal approved this pull request. LGTM, thanks. https://github.com/llvm/llvm-project/pull/99564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] MmapWriteExecChecker improvements (PR #97078)
=?utf-8?q?Bal=C3=A1zs_K=C3=A9ri?= , =?utf-8?q?Bal=C3=A1zs_K=C3=A9ri?= Message-ID: In-Reply-To: steakhal wrote: Please make sure that the premerge bots are happy before merging. https://github.com/llvm/llvm-project/pull/97078 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Don't invalidate the super region when a std object ctor runs (PR #100405)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/100405 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Don't invalidate super region when a std object ctor runs (PR #100405)
https://github.com/steakhal created
https://github.com/llvm/llvm-project/pull/100405
CPP-5269
>From 95c0cd7f5d1c1a5c87707a5a63141dc3008191ed Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Wed, 24 Jul 2024 17:11:54 +0200
Subject: [PATCH] [analyzer] Don't invalidate super region when a std object
ctor runs
CPP-5269
---
clang/lib/StaticAnalyzer/Core/CallEvent.cpp | 19
clang/test/Analysis/call-invalidation.cpp | 115
2 files changed, 134 insertions(+)
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index 0e317ec765ec0..eba224b8ec01c 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -923,12 +923,31 @@ SVal AnyCXXConstructorCall::getCXXThisVal() const {
return UnknownVal();
}
+static bool isWithinStdNamespace(const Decl *D) {
+ const DeclContext *DC = D->getDeclContext();
+ while (DC) {
+if (const auto *NS = dyn_cast(DC);
+NS && NS->isStdNamespace())
+ return true;
+DC = DC->getParent();
+ }
+ return false;
+}
+
void AnyCXXConstructorCall::getExtraInvalidatedValues(ValueList ,
RegionAndSymbolInvalidationTraits *ETraits) const {
SVal V = getCXXThisVal();
if (SymbolRef Sym = V.getAsSymbol(true))
ETraits->setTrait(Sym,
RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+
+ // Standard classes don't reinterpret-cast and modify super regions.
+ const bool IsStdClassCtor = isWithinStdNamespace(getDecl());
+ if (const MemRegion *Obj = V.getAsRegion(); Obj && IsStdClassCtor) {
+ETraits->setTrait(
+Obj, RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
+ }
+
Values.push_back(V);
}
diff --git a/clang/test/Analysis/call-invalidation.cpp
b/clang/test/Analysis/call-invalidation.cpp
index 727217f228b05..fb2b892b31a1f 100644
--- a/clang/test/Analysis/call-invalidation.cpp
+++ b/clang/test/Analysis/call-invalidation.cpp
@@ -1,5 +1,6 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify
-analyzer-config eagerly-assume=false %s
+template void clang_analyzer_dump(T);
void clang_analyzer_eval(bool);
void usePointer(int * const *);
@@ -165,3 +166,117 @@ void testMixedConstNonConstCalls() {
useFirstNonConstSecondConst(&(s2.x), &(s2.y));
clang_analyzer_eval(s2.y == 1); // expected-warning{{UNKNOWN}}
}
+
+namespace std {
+class Opaque {
+public:
+ Opaque();
+ int nested_member;
+};
+} // namespace std
+
+struct StdWrappingOpaque {
+ std::Opaque o; // first member
+ int uninit;
+};
+struct StdWrappingOpaqueSwapped {
+ int uninit; // first member
+ std::Opaque o;
+};
+
+int testStdCtorDoesNotInvalidateParentObject() {
+ StdWrappingOpaque obj;
+ int x = obj.o.nested_member; // no-garbage: std::Opaque::ctor might
initialized this
+ int y = obj.uninit; // FIXME: We should have a garbage read here. Read the
details.
+ // As the first member ("obj.o") is invalidated, a conjured default binding
is bound
+ // to the offset 0 within cluster "obj", and this masks every uninitialized
fields
+ // that follows. We need a better store with extents to fix this.
+ return x + y;
+}
+
+int testStdCtorDoesNotInvalidateParentObjectSwapped() {
+ StdWrappingOpaqueSwapped obj;
+ int x = obj.o.nested_member; // no-garbage: std::Opaque::ctor might
initialized this
+ int y = obj.uninit; // expected-warning {{Assigned value is garbage or
undefined}}
+ return x + y;
+}
+
+class UserProvidedOpaque {
+public:
+ UserProvidedOpaque(); // might reinterpret_cast(this)
+ int nested_member;
+};
+
+struct WrappingUserProvidedOpaque {
+ UserProvidedOpaque o; // first member
+ int uninit;
+};
+struct WrappingUserProvidedOpaqueSwapped {
+ int uninit; // first member
+ UserProvidedOpaque o;
+};
+
+int testUserProvidedCtorInvalidatesParentObject() {
+ WrappingUserProvidedOpaque obj;
+ int x = obj.o.nested_member; // no-garbage: UserProvidedOpaque::ctor might
initialized this
+ int y = obj.uninit; // no-garbage: UserProvidedOpaque::ctor might
reinterpret_cast(this) and write to the "uninit" member.
+ return x + y;
+}
+
+int testUserProvidedCtorInvalidatesParentObjectSwapped() {
+ WrappingUserProvidedOpaqueSwapped obj;
+ int x = obj.o.nested_member; // no-garbage: same as above
+ int y = obj.uninit; // no-garbage: same as above
+ return x + y;
+}
+
+struct WrappingStdWrappingOpaqueOuterInits {
+ int first = 1;
+ std::Opaque second;
+ int third = 3;
+ WrappingStdWrappingOpaqueOuterInits() {
+clang_analyzer_dump(first); // expected-warning {{1 S32b}}
+clang_analyzer_dump(second.nested_member); // expected-warning {{derived_}}
+clang_analyzer_dump(third); // expected-warning {{3 S32b}}
+ }
+};
+
+struct WrappingUserProvidedOpaqueOuterInits {
+ int first = 1; // Potentially overwritten by UserProvidedOpaque::ctor
+ UserProvidedOpaque second; // Invalidates the object so far.
+ int third =
[clang] [clang][analyzer] Support `ownership_{returns,takes}` attributes (PR #98941)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Support `ownership_{returns,takes}` attributes (PR #98941)
steakhal wrote: Should I merge this PR, or you want to wait some? https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Support `ownership_{returns,takes}` attributes (PR #98941)
https://github.com/steakhal approved this pull request. https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' (PR #100085)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/100085 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' (PR #100085)
@@ -451,6 +462,10 @@ class StreamChecker : public Checker StdFileStreamDecls = {};
void evalFopen(const FnDescription *Desc, const CallEvent ,
CheckerContext ) const;
@@ -919,9 +918,8 @@ ProgramStateRef
StreamChecker::assumeNoAliasingWithStdStreams(
};
assert(State);
- State = assumeRetNE(State, StdinDecl);
- State = assumeRetNE(State, StdoutDecl);
- State = assumeRetNE(State, StderrDecl);
+ for (const VarDecl *VD : StdFileStreamDecls)
+State = assumeRetNE(State, VD);
assert(State);
return State;
}
@@ -2082,9 +2080,11 @@ getGlobalStreamPointerByName(const TranslationUnitDecl
*TU, StringRef VarName) {
void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,
AnalysisManager &, BugReporter &) const {
- StdinDecl = getGlobalStreamPointerByName(TU, "stdin");
- StdoutDecl = getGlobalStreamPointerByName(TU, "stdout");
- StderrDecl = getGlobalStreamPointerByName(TU, "stderr");
+ std::array DeclNames{"stdin", "stdout", "stderr"};
+ for (const auto &[DeclSlot, DeclName] :
+ llvm::zip_equal(StdFileStreamDecls, DeclNames)) {
+DeclSlot = getGlobalStreamPointerByName(TU, DeclName);
+ }
}
//===--===//
```
In total, it replaces 9 lines with other 9 lines of code that is a lot more
complicated and does an indirection. I'd say, not worth it.
https://github.com/llvm/llvm-project/pull/100085
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' (PR #100085)
https://github.com/steakhal updated
https://github.com/llvm/llvm-project/pull/100085
>From c8148c88a39434501b9dacb374b3ca9d81ee8fdf Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Tue, 23 Jul 2024 08:55:16 +0200
Subject: [PATCH 1/2] [analyzer] Assume the result of 'fopen' can't alias with
'std{in,out,err}'
'fopen' should return a new FILE handle, thus we should assume it can't
alias with commonly used FILE handles, such as with 'stdin', 'stdout' or
'stderr'.
This problem appears in code that handles either some input/output file
with stdin or stdout, as the business logic is basically the same no
matter the stream being used.
However, one would should only close the stream if it was opened via
'fopen'. Consequently, such code usually has a condition like
`if (f && f != stdout)` to guard the `fclose()` call.
This patch brings this assumption, thus eliminates FPs for not taking
the guarded branch.
CPP-5306
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 72 ++-
clang/test/Analysis/stream.c | 11 +++
2 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index e8d538388e56c..53770532609d5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -254,7 +254,8 @@ inline void assertStreamStateOpened(const StreamState *SS) {
}
class StreamChecker : public Checker
{
+ check::DeadSymbols, check::PointerEscape,
+ check::ASTDecl> {
BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
BugType BT_UseAfterOpenFailed{this, "Invalid stream",
@@ -276,11 +277,21 @@ class StreamChecker : public Checker ProgramStateRef {
+if (!Var)
+ return State;
+const auto *LCtx = C.getLocationContext();
+auto = C.getStoreManager();
+auto = C.getSValBuilder();
+SVal VarValue = State->getSVal(StoreMgr.getLValueVar(Var, LCtx));
+auto NoAliasState =
+SVB.evalBinOp(State, BO_NE, RetVal, VarValue, SVB.getConditionType())
+.castAs();
+return State->assume(NoAliasState, true);
+ };
+
+ assert(State);
+ State = assumeRetNE(State, StdinDecl);
+ State = assumeRetNE(State, StdoutDecl);
+ State = assumeRetNE(State, StderrDecl);
+ assert(State);
+ return State;
+}
+
void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent ,
CheckerContext ) const {
ProgramStateRef State = C.getState();
@@ -899,6 +938,7 @@ void StreamChecker::evalFopen(const FnDescription *Desc,
const CallEvent ,
assert(RetSym && "RetVal must be a symbol here.");
State = State->BindExpr(CE, C.getLocationContext(), RetVal);
+ State = assumeNoAliasingWithStdStreams(State, RetVal, C);
// Bifurcate the state into two: one with a valid FILE* pointer, the other
// with a NULL.
@@ -2017,6 +2057,36 @@ ProgramStateRef StreamChecker::checkPointerEscape(
return State;
}
+static const VarDecl *
+getGlobalStreamPointerByName(const TranslationUnitDecl *TU, StringRef VarName)
{
+ ASTContext = TU->getASTContext();
+ const auto = Ctx.getSourceManager();
+ const QualType FileTy = Ctx.getFILEType();
+
+ if (FileTy.isNull())
+return nullptr;
+
+ const QualType FilePtrTy = Ctx.getPointerType(FileTy).getCanonicalType();
+
+ auto LookupRes = TU->lookup((VarName));
+ for (const Decl *D : LookupRes) {
+if (auto *VD = dyn_cast_or_null(D)) {
+ if (SM.isInSystemHeader(VD->getLocation()) && VD->hasExternalStorage() &&
+ VD->getType().getCanonicalType() == FilePtrTy) {
+return VD;
+ }
+}
+ }
+ return nullptr;
+}
+
+void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,
+ AnalysisManager &, BugReporter &) const {
+ StdinDecl = getGlobalStreamPointerByName(TU, "stdin");
+ StdoutDecl = getGlobalStreamPointerByName(TU, "stdout");
+ StderrDecl = getGlobalStreamPointerByName(TU, "stderr");
+}
+
//===--===//
// Checker registration.
//===--===//
diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c
index c924cbd36f759..c518820df6ed6 100644
--- a/clang/test/Analysis/stream.c
+++ b/clang/test/Analysis/stream.c
@@ -498,3 +498,14 @@ void gh_93408_regression_ZeroSized(struct ZeroSized
*buffer) {
fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}}
no-crash
fclose(f);
}
+
+extern FILE *stdout_like_ptr;
+void no_aliasing(void) {
+ FILE *f = fopen("file", "r");
+ clang_analyzer_eval(f == stdout); // expected-warning {{FALSE}}
no-TRUE
+ clang_analyzer_eval(f == stderr); //
[clang] [analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' (PR #100085)
steakhal wrote: I didn't mention this in the release notes as I don't think it's that impactful. https://github.com/llvm/llvm-project/pull/100085 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Assume the result of 'fopen' can't alias with 'std{in,out,err}' (PR #100085)
https://github.com/steakhal created
https://github.com/llvm/llvm-project/pull/100085
'fopen' should return a new FILE handle, thus we should assume it can't alias
with commonly used FILE handles, such as with 'stdin', 'stdout' or 'stderr'.
This problem appears in code that handles either some input/output file with
stdin or stdout, as the business logic is basically the same no matter the
stream being used.
However, one would should only close the stream if it was opened via 'fopen'.
Consequently, such code usually has a condition like `if (f && f != stdout)` to
guard the `fclose()` call.
This patch brings this assumption, thus eliminates FPs for not taking the
guarded branch.
CPP-5306
>From c8148c88a39434501b9dacb374b3ca9d81ee8fdf Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Tue, 23 Jul 2024 08:55:16 +0200
Subject: [PATCH] [analyzer] Assume the result of 'fopen' can't alias with
'std{in,out,err}'
'fopen' should return a new FILE handle, thus we should assume it can't
alias with commonly used FILE handles, such as with 'stdin', 'stdout' or
'stderr'.
This problem appears in code that handles either some input/output file
with stdin or stdout, as the business logic is basically the same no
matter the stream being used.
However, one would should only close the stream if it was opened via
'fopen'. Consequently, such code usually has a condition like
`if (f && f != stdout)` to guard the `fclose()` call.
This patch brings this assumption, thus eliminates FPs for not taking
the guarded branch.
CPP-5306
---
.../StaticAnalyzer/Checkers/StreamChecker.cpp | 72 ++-
clang/test/Analysis/stream.c | 11 +++
2 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index e8d538388e56c..53770532609d5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -254,7 +254,8 @@ inline void assertStreamStateOpened(const StreamState *SS) {
}
class StreamChecker : public Checker
{
+ check::DeadSymbols, check::PointerEscape,
+ check::ASTDecl> {
BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};
BugType BT_UseAfterOpenFailed{this, "Invalid stream",
@@ -276,11 +277,21 @@ class StreamChecker : public Checker ProgramStateRef {
+if (!Var)
+ return State;
+const auto *LCtx = C.getLocationContext();
+auto = C.getStoreManager();
+auto = C.getSValBuilder();
+SVal VarValue = State->getSVal(StoreMgr.getLValueVar(Var, LCtx));
+auto NoAliasState =
+SVB.evalBinOp(State, BO_NE, RetVal, VarValue, SVB.getConditionType())
+.castAs();
+return State->assume(NoAliasState, true);
+ };
+
+ assert(State);
+ State = assumeRetNE(State, StdinDecl);
+ State = assumeRetNE(State, StdoutDecl);
+ State = assumeRetNE(State, StderrDecl);
+ assert(State);
+ return State;
+}
+
void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent ,
CheckerContext ) const {
ProgramStateRef State = C.getState();
@@ -899,6 +938,7 @@ void StreamChecker::evalFopen(const FnDescription *Desc,
const CallEvent ,
assert(RetSym && "RetVal must be a symbol here.");
State = State->BindExpr(CE, C.getLocationContext(), RetVal);
+ State = assumeNoAliasingWithStdStreams(State, RetVal, C);
// Bifurcate the state into two: one with a valid FILE* pointer, the other
// with a NULL.
@@ -2017,6 +2057,36 @@ ProgramStateRef StreamChecker::checkPointerEscape(
return State;
}
+static const VarDecl *
+getGlobalStreamPointerByName(const TranslationUnitDecl *TU, StringRef VarName)
{
+ ASTContext = TU->getASTContext();
+ const auto = Ctx.getSourceManager();
+ const QualType FileTy = Ctx.getFILEType();
+
+ if (FileTy.isNull())
+return nullptr;
+
+ const QualType FilePtrTy = Ctx.getPointerType(FileTy).getCanonicalType();
+
+ auto LookupRes = TU->lookup((VarName));
+ for (const Decl *D : LookupRes) {
+if (auto *VD = dyn_cast_or_null(D)) {
+ if (SM.isInSystemHeader(VD->getLocation()) && VD->hasExternalStorage() &&
+ VD->getType().getCanonicalType() == FilePtrTy) {
+return VD;
+ }
+}
+ }
+ return nullptr;
+}
+
+void StreamChecker::checkASTDecl(const TranslationUnitDecl *TU,
+ AnalysisManager &, BugReporter &) const {
+ StdinDecl = getGlobalStreamPointerByName(TU, "stdin");
+ StdoutDecl = getGlobalStreamPointerByName(TU, "stdout");
+ StderrDecl = getGlobalStreamPointerByName(TU, "stderr");
+}
+
//===--===//
// Checker registration.
[clang] [analyzer] Model builtin-like functions as builtin functions (PR #99886)
https://github.com/steakhal created
https://github.com/llvm/llvm-project/pull/99886
Some template function instantiations don't have a body, even though their
templates did have a body.
Examples are: `std::move`, `std::forward`, `std::addressof` etc.
They had bodies before
https://github.com/llvm/llvm-project/commit/72315d02c432a0fe0acae9c96c69eac8d8e1a9f6
After that change, the sentiment was that these special functions should be
considered and treated as builtin functions.
Fixes #94193
CPP-5358
>From 3f25761041417de42330b348d46f35ac5af99426 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 22 Jul 2024 15:59:23 +0200
Subject: [PATCH] [analyzer] Model builtin-like functions as builtin functions
Some template function instantiations don't have a body, even though
their templates did have a body.
Examples are: `std::move`, `std::forward`, `std::addressof` etc.
They had bodies before
https://github.com/llvm/llvm-project/commit/72315d02c432a0fe0acae9c96c69eac8d8e1a9f6
After that change, the sentiment was that these special funtions
should be considered and treated as builtin functions.
Fixes #94193
CPP-5358
---
clang/docs/ReleaseNotes.rst | 4 ++
.../Checkers/BuiltinFunctionChecker.cpp | 38 +
.../Inputs/system-header-simulator-cxx.h | 14 ---
clang/test/Analysis/builtin-functions.cpp | 20 +
.../diagnostics/explicit-suppression.cpp | 2 +-
clang/test/Analysis/issue-94193.cpp | 41 +++
clang/test/Analysis/use-after-move.cpp| 9 +---
7 files changed, 114 insertions(+), 14 deletions(-)
create mode 100644 clang/test/Analysis/issue-94193.cpp
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 61629ff30aeeb..4ddb6baa49824 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1326,6 +1326,10 @@ Crash and bug fixes
- Z3 crosschecking (aka. Z3 refutation) is now bounded, and can't consume
more total time than the eymbolic execution itself. (#GH97298)
+- ``std::addressof``, ``std::as_const``, ``std::forward``,
+ ``std::forward_like``, ``std::move``, ``std::move_if_noexcept``, are now
+ modeled just like their builtin counterpart. (#GH94193)
+
Improvements
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index 1a75d7b52ad6e..b198b1c2ff4d1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -18,6 +18,7 @@
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
@@ -30,8 +31,40 @@ namespace {
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent , CheckerContext ) const;
+
+private:
+ // From: clang/include/clang/Basic/Builtins.def
+ // C++ standard library builtins in namespace 'std'.
+ const CallDescriptionSet BuiltinLikeStdFunctions{
+ {CDM::SimpleFunc, {"std", "addressof"}},//
+ {CDM::SimpleFunc, {"std", "__addressof"}}, //
+ {CDM::SimpleFunc, {"std", "as_const"}}, //
+ {CDM::SimpleFunc, {"std", "forward"}}, //
+ {CDM::SimpleFunc, {"std", "forward_like"}}, //
+ {CDM::SimpleFunc, {"std", "move"}}, //
+ {CDM::SimpleFunc, {"std", "move_if_noexcept"}}, //
+ };
+
+ bool isBuiltinLikeFunction(const CallEvent ) const;
};
+} // namespace
+
+bool BuiltinFunctionChecker::isBuiltinLikeFunction(
+const CallEvent ) const {
+ const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
+ if (!FD || FD->getNumParams() != 1)
+return false;
+
+ if (QualType RetTy = FD->getReturnType();
+ !RetTy->isPointerType() && !RetTy->isReferenceType())
+return false;
+
+ if (QualType ParmTy = FD->getParamDecl(0)->getType();
+ !ParmTy->isPointerType() && !ParmTy->isReferenceType())
+return false;
+
+ return BuiltinLikeStdFunctions.contains(Call);
}
bool BuiltinFunctionChecker::evalCall(const CallEvent ,
@@ -44,6 +77,11 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent ,
const LocationContext *LCtx = C.getLocationContext();
const Expr *CE = Call.getOriginExpr();
+ if (isBuiltinLikeFunction(Call)) {
+C.addTransition(state->BindExpr(CE, LCtx, Call.getArgSVal(0)));
+return true;
+ }
+
switch (FD->getBuiltinID()) {
default:
return false;
diff --git a/clang/test/Analysis/Inputs/system-header-simulator-cxx.h
b/clang/test/Analysis/Inputs/system-header-simulator-cxx.h
index 29326ec1f9280..a379a47515668
[clang] [analyzer][NFC] Add some docs for LazyCompoundValue (PR #97407)
=?utf-8?q?Krist=C3=B3f?= Umann , =?utf-8?q?Krist=C3=B3f?= Umann , =?utf-8?q?Krist=C3=B3f?= Umann , =?utf-8?q?Krist=C3=B3f?= Umann , =?utf-8?q?Krist=C3=B3f?= Umann Message-ID: In-Reply-To: https://github.com/steakhal approved this pull request. https://github.com/llvm/llvm-project/pull/97407 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Improve bug report hashing, merge similar reports (PR #98621)
=?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/98621 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Improve bug report hashing, merge similar reports (PR #98621)
=?utf-8?q?Donát?= Nagy
Message-ID:
In-Reply-To:
@@ -2213,7 +2213,7 @@ void BasicBugReport::Profile(llvm::FoldingSetNodeID&
hash) const {
void PathSensitiveBugReport::Profile(llvm::FoldingSetNodeID ) const {
hash.AddInteger(static_cast(getKind()));
hash.AddPointer();
- hash.AddString(Description);
+ hash.AddString(getShortDescription());
steakhal wrote:
Is it intentional that you use a member function here instead of the member
variable?
https://github.com/llvm/llvm-project/pull/98621
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Improve bug report hashing, merge similar reports (PR #98621)
=?utf-8?q?Don=C3=A1t?= Nagy Message-ID: In-Reply-To: https://github.com/steakhal approved this pull request. LGTM. I only had one minor question inline. https://github.com/llvm/llvm-project/pull/98621 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Support `ownership_{returns,takes}` attributes (PR #98941)
steakhal wrote: > > and you can read the docs for the checker, and the attribute at the links. > > I can't find any docs for this attribute. As I mentioned, I will fill new > issues to fix couple of frontend issues and after that we can write down > correct semantics of these attrs. Thats fine. > Do you mean that link > https://clang.llvm.org/docs/AttributeReference.html#ownership-holds-ownership-returns-ownership-takes? Yes. Its generated from a tablegen file. Thats why you probably didnt find it. Look for files with the "td" extension, and "attr" in name. May have a different casing though. Watch out. https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Support `ownership_{returns,takes}` attributes (PR #98941)
steakhal wrote: FYI This PR is still not mentioned in the `clang/docs/ReleaseNotes.rst`. We should have two entries there: 1) In [Attribute Changes in Clang](https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst#id34), mentioning that calls can have at most one `ownership_return` attribute. 2) In [Static Analyzer > New features](https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst#id47), that the MallocChecker now checks this attribute, and you can read the docs for the checker, and the attribute at the links. https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][analyzer] Support `ownership_{returns,takes}` attributes (PR #98941)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1877,16 +1923,43 @@ static bool didPreviousFreeFail(ProgramStateRef State,
return false;
}
+static void printOwnershipTakesList(raw_ostream , CheckerContext ,
+const Expr *E) {
+ if (const CallExpr *CE = dyn_cast(E)) {
+const FunctionDecl *FD = CE->getDirectCallee();
+if (!FD)
+ return;
+
+if (!FD->hasAttrs())
+ return;
+
+// Only one ownership_takes attribute is allowed
+for (const auto *I : FD->specific_attrs()) {
+ OwnershipAttr::OwnershipKind OwnKind = I->getOwnKind();
+
+ if (OwnKind != OwnershipAttr::Takes)
steakhal wrote:
```suggestion
if (I->getOwnKind() != OwnershipAttr::Takes)
```
It seems like inlining this variable would make it more readable actually.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1877,16 +1923,43 @@ static bool didPreviousFreeFail(ProgramStateRef State,
return false;
}
+static void printOwnershipTakesList(raw_ostream , CheckerContext ,
+const Expr *E) {
+ if (const CallExpr *CE = dyn_cast(E)) {
steakhal wrote:
When I advocated for early-returns, I wanted something like this:
```c++
const auto *CE = dyn_cast(E);
if (!CE)
return;
const FunctionDecl *FD = CE->getDirectCallee();
if (!FD)
return;
...
```
On the same token, the `FD->hasAttrs()` appears to be not strictly necessary as
in that case the loop simply wouldn't take any iterations at all, right?
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1877,16 +1923,43 @@ static bool didPreviousFreeFail(ProgramStateRef State,
return false;
}
+static void printOwnershipTakesList(raw_ostream , CheckerContext ,
+const Expr *E) {
+ if (const CallExpr *CE = dyn_cast(E)) {
+const FunctionDecl *FD = CE->getDirectCallee();
+if (!FD)
+ return;
+
+if (!FD->hasAttrs())
+ return;
+
+// Only one ownership_takes attribute is allowed
steakhal wrote:
```suggestion
// Only one ownership_takes attribute is allowed.
```
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1877,16 +1923,43 @@ static bool didPreviousFreeFail(ProgramStateRef State,
return false;
}
+static void printOwnershipTakesList(raw_ostream , CheckerContext ,
+const Expr *E) {
+ if (const CallExpr *CE = dyn_cast(E)) {
+const FunctionDecl *FD = CE->getDirectCallee();
+if (!FD)
+ return;
+
+if (!FD->hasAttrs())
+ return;
+
+// Only one ownership_takes attribute is allowed
+for (const auto *I : FD->specific_attrs()) {
+ OwnershipAttr::OwnershipKind OwnKind = I->getOwnKind();
+
+ if (OwnKind != OwnershipAttr::Takes)
+continue;
+
+ os << ", which takes ownership of " << '\'' << I->getModule()->getName()
+ << '\'';
steakhal wrote:
```suggestion
os << ", which takes ownership of '" << I->getModule()->getName() << '\'';
```
This way it would even fit into a single line.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
https://github.com/steakhal approved this pull request. I think it looks great. Only a few more stylistic remarks are left, but it should be already in a good shape functional-wise. I'll let you decide if you wanna apply my suggestions or not. One more remark - this time w.r.t. the workflow. Usually, GitHub PRs prefer "merges" over "force-pushes". Whenever you force-push, all the inline remarks could get lost, as it fails to track the lines where the comments were made. Thus, when working with PRs, one usually just "merges" the tracked branch, and just builds more and more commits on top of your PR branch. Once the review is done, one is free to destroy the history and squash the commits, or do some reordering - because there are no more pending comments or remarks left. https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -103,14 +103,49 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+struct AllocationFamily {
+ AllocationFamilyKind Kind;
+ std::optional CustomName;
+
+ explicit AllocationFamily(AllocationFamilyKind kind,
+std::optional name = std::nullopt)
+ : Kind(kind), CustomName(name) {
+assert(kind != AF_Custom || name != std::nullopt);
+
+// Preseve previous behavior when "malloc" class means AF_Malloc
+if (Kind == AF_Malloc && CustomName) {
+ if (CustomName.value() == "malloc")
+CustomName = std::nullopt;
+ else
+Kind = AF_Custom;
+}
+ }
+
+ bool operator==(const AllocationFamily ) const {
+return std::tie(Kind, CustomName) == std::tie(Other.Kind,
Other.CustomName);
+ }
+
+ bool operator!=(const AllocationFamily ) const {
+return !(*this == Other);
+ }
+
+ void Profile(llvm::FoldingSetNodeID ) const {
+ID.AddInteger(Kind);
+
+if (Kind == AF_Custom)
+ ID.AddString(CustomName.value());
+ }
steakhal wrote:
Ah indeed. nvm.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1122,7 +1157,7 @@ MallocChecker::performKernelMalloc(const CallEvent ,
CheckerContext ,
if (TrueState && !FalseState) {
SVal ZeroVal = C.getSValBuilder().makeZeroVal(Ctx.CharTy);
return MallocMemAux(C, Call, Call.getArgExpr(0), ZeroVal, TrueState,
-AF_Malloc);
+AllocationFamily(AF_Malloc));
steakhal wrote:
Now I get what you meant when pushed back on adding `explicit`. I didn't expect
this to cause such a widespread effect. Sorry. Now that you made all the
necessary changes, we can keep it as-is. You decide if you prefer reverting
these back. I'm good either way.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -103,14 +103,49 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+struct AllocationFamily {
+ AllocationFamilyKind Kind;
+ std::optional CustomName;
+
+ explicit AllocationFamily(AllocationFamilyKind kind,
+std::optional name = std::nullopt)
+ : Kind(kind), CustomName(name) {
+assert(kind != AF_Custom || name != std::nullopt);
+
+// Preseve previous behavior when "malloc" class means AF_Malloc
+if (Kind == AF_Malloc && CustomName) {
+ if (CustomName.value() == "malloc")
+CustomName = std::nullopt;
+ else
+Kind = AF_Custom;
+}
+ }
+
+ bool operator==(const AllocationFamily ) const {
+return std::tie(Kind, CustomName) == std::tie(Other.Kind,
Other.CustomName);
+ }
+
+ bool operator!=(const AllocationFamily ) const {
+return !(*this == Other);
+ }
+
+ void Profile(llvm::FoldingSetNodeID ) const {
+ID.AddInteger(Kind);
+
+if (Kind == AF_Custom)
+ ID.AddString(CustomName.value());
+ }
steakhal wrote:
I think you could also do this unconditionally.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -103,14 +103,49 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+struct AllocationFamily {
+ AllocationFamilyKind Kind;
+ std::optional CustomName;
+
+ explicit AllocationFamily(AllocationFamilyKind kind,
+std::optional name = std::nullopt)
+ : Kind(kind), CustomName(name) {
+assert(kind != AF_Custom || name != std::nullopt);
+
+// Preseve previous behavior when "malloc" class means AF_Malloc
+if (Kind == AF_Malloc && CustomName) {
+ if (CustomName.value() == "malloc")
+CustomName = std::nullopt;
+ else
+Kind = AF_Custom;
+}
steakhal wrote:
Probably I don't understand the semantics of this one.
I was expecting that the `CustomName` field is only used when the `Kind` is
`AF_Custom`. Yet, here it appears to be used even for `Kind == AF_Malloc`.
What I originally thought that when `AF_Custom` is specified with `"malloc"`,
then it should consider the case as `AF_Malloc` instead.
Could you elaborate here?
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1490,7 +1532,7 @@ ProgramStateRef MallocChecker::ProcessZeroAllocCheck(
return State;
}
} else
-llvm_unreachable("not a CallExpr or CXXNewExpr");
+assert(false && "not a CallExpr or CXXNewExpr");
steakhal wrote:
```suggestion
} else {
assert(false && "not a CallExpr or CXXNewExpr");
return nullptr;
}
```
Just to be on the safe side and we have a NDEBUG build (with assertions
disabled), let's avoid undefined behavior by safely backing off.
The same principle should apply to all the other cases when previously we had
an `llvm_unreachable`.
This is what I wanted to imply by `assert and return`.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -103,14 +103,49 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+struct AllocationFamily {
+ AllocationFamilyKind Kind;
+ std::optional CustomName;
+
+ explicit AllocationFamily(AllocationFamilyKind kind,
+std::optional name = std::nullopt)
steakhal wrote:
Please use upper camel case for variable names. Shadowing is not a problem for
constructor parameters and init-lists, and within the ctor body the semantics
are anyways well-defined.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -14,6 +14,13 @@ void free(void *); void __attribute((ownership_takes(malloc, 1))) my_free(void *); +void __attribute((ownership_returns(malloc1))) *my_malloc1(size_t); steakhal wrote: Interesting. https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -103,14 +103,49 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+struct AllocationFamily {
+ AllocationFamilyKind Kind;
+ std::optional CustomName;
+
+ explicit AllocationFamily(AllocationFamilyKind kind,
+std::optional name = std::nullopt)
+ : Kind(kind), CustomName(name) {
+assert(kind != AF_Custom || name != std::nullopt);
steakhal wrote:
I'd prefer to have a message here to be able to immediately tell what this
assert is about when fires.
```suggestion
assert((kind != AF_Custom || name.has_value()) && "Custom family must
specify also the name");
```
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -24,3 +24,7 @@ void f15(int, int)
void f16(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_holds(foo, 1))); // OK, same index
void f17(void*) __attribute__((ownership_takes(__, 1)));
void f18() __attribute__((ownership_takes(foo, 1))); // expected-warning
{{'ownership_takes' attribute only applies to non-K functions}}
+
+int f19(void *)
+ __attribute__((ownership_takes(foo, 1)))// expected-error
{{'ownership_takes' attribute class does not match; here it is 'foo'}}
+ __attribute__((ownership_takes(foo1, 1))); // expected-note {{declared with
class 'foo1' here}}
steakhal wrote:
What happens if I want to redeclare the function with a different
"ownership_takes" attribute in a subsequent line? Will it detect the conflict?
```
int f20(void *) __attribute__((ownership_takes(foo, 1)));
int f20(void *) __attribute__((ownership_takes(foo1, 1))); // redecl with
different attr
```
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
https://github.com/steakhal commented: It looks great! I only had one more thing to test. Besides that it would be nice to advertise this in the `clang/docs/ReleaseNotes.rst`. Usually, from that release notes we also link to the checker or the relevant doc pages - in this case it would be nice to link to the docs of this attribute - which does not really elaborate on the attribute and the semantics. Do you want to extend the scope of this PR to add some minimal docs to the attribute? If not, that's also fine, we will create a separate ticket for adding them later. https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -60,6 +67,41 @@ void testMalloc8() {
operator delete[](p); // expected-warning{{Memory allocated by malloc()
should be deallocated by free(), not operator delete[]}}
}
+void testMalloc9() {
+ int *p = (int *)my_malloc(sizeof(int));
+ my_free(p); // no warning
+}
+
+void testMalloc10() {
+ int *p = (int *)my_malloc1(sizeof(int));
+ my_free1(p); // no warning
+}
+
+void testMalloc11() {
+ int *p = (int *)my_malloc2(sizeof(int));
+ my_free23(p); // no warning
+}
+
+void testMalloc12() {
+ int *p = (int *)my_malloc1(sizeof(int));
+ my_free(p); // expected-warning{{Memory allocated by my_malloc1() should be
deallocated by function that takes ownership of 'malloc1', not my_free(), which
takes ownership of 'malloc'}}
steakhal wrote:
Ah, I see. The phrasing wasn't clear to me. Maybe because we don't have a clear
spec for this attribute, thus it's difficult to make out things that have
special meaning.
I think we should quote function names, yes.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -103,14 +103,46 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+class AllocationFamily {
+public:
+ AllocationFamily(AllocationFamilyKind kind,
steakhal wrote:
I'd prefer having this explicit.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -14,6 +14,13 @@ void free(void *); void __attribute((ownership_takes(malloc, 1))) my_free(void *); +void __attribute((ownership_returns(malloc1))) *my_malloc1(size_t); +void __attribute((ownership_takes(malloc1, 1))) my_free1(void *); + +void __attribute((ownership_returns(malloc2))) *my_malloc2(size_t); +void __attribute((ownership_returns(malloc2))) *my_malloc3(size_t); +void __attribute((ownership_takes(malloc2, 1))) __attribute((ownership_takes(malloc3, 1))) my_free23(void *); steakhal wrote: To me, it would make more sense to allow at most one of this attribute to be present. https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Add some docs for LazyCompoundValue (PR #97407)
=?utf-8?q?Kristóf?= Umann ,
=?utf-8?q?Kristóf?= Umann ,
=?utf-8?q?Kristóf?= Umann ,
=?utf-8?q?Kristóf?= Umann
Message-ID:
In-Reply-To:
@@ -346,6 +350,36 @@ class CompoundVal : public NonLoc {
static bool classof(SVal V) { return V.getKind() == CompoundValKind; }
};
+/// While nonloc::CompoundVal covers a few simple use cases,
+/// nonloc::LazyCompoundVal is a more performant and flexible way to represent
+/// an rvalue of record type, so it shows up much more frequently during
+/// analysis. This value is an r-value that represents a snapshot of any
+/// structure "as a whole" at a given moment during the analysis. Such value is
+/// already quite far from being referred to as "concrete", as many fields
+/// inside it would be unknown or symbolic. nonloc::LazyCompoundVal operates by
+/// storing two things:
+/// * a reference to the TypedValueRegion being snapshotted (yes, it is
always
+/// typed), and also
+/// * a reference to the whole Store object, obtained from the ProgramState in
+///which the nonloc::LazyCompoundVal was created.
steakhal wrote:
These bulletpoints are indented by different levels.
https://github.com/llvm/llvm-project/pull/97407
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Add some docs for LazyCompoundValue (PR #97407)
=?utf-8?q?Kristóf?= Umann ,
=?utf-8?q?Kristóf?= Umann ,
=?utf-8?q?Kristóf?= Umann ,
=?utf-8?q?Kristóf?= Umann
Message-ID:
In-Reply-To:
@@ -346,6 +350,36 @@ class CompoundVal : public NonLoc {
static bool classof(SVal V) { return V.getKind() == CompoundValKind; }
};
+/// While nonloc::CompoundVal covers a few simple use cases,
+/// nonloc::LazyCompoundVal is a more performant and flexible way to represent
+/// an rvalue of record type, so it shows up much more frequently during
+/// analysis. This value is an r-value that represents a snapshot of any
+/// structure "as a whole" at a given moment during the analysis. Such value is
+/// already quite far from being referred to as "concrete", as many fields
+/// inside it would be unknown or symbolic. nonloc::LazyCompoundVal operates by
+/// storing two things:
+/// * a reference to the TypedValueRegion being snapshotted (yes, it is
always
+/// typed), and also
+/// * a reference to the whole Store object, obtained from the ProgramState in
+///which the nonloc::LazyCompoundVal was created.
+///
+/// Note that the old ProgramState and its Store is kept alive during the
+/// analysis because these are immutable functional data structures and each
new
+/// Store value is represented as "earlier Store" + "additional binding".
+///
+/// Essentially, nonloc::LazyCompoundVal is a performance optimization for the
+/// analyzer. Because Store is immutable, creating a nonloc::LazyCompoundVal is
+/// a very cheap operation. Note that the Store contains all region bindings in
+/// the program state, not only related to the region. Later, if necessary,
such
+/// value can be unpacked -- eg. when it is assigned to another variable.
+///
+/// If you ever need inspect the contents of the LazyCompoundVal, you can use
steakhal wrote:
```suggestion
/// If you ever need to inspect the contents of the LazyCompoundVal, you can use
```
https://github.com/llvm/llvm-project/pull/97407
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Add some docs for LazyCompoundValue (PR #97407)
=?utf-8?q?Kristóf?= Umann , =?utf-8?q?Kristóf?= Umann , =?utf-8?q?Kristóf?= Umann , =?utf-8?q?Kristóf?= Umann Message-ID: In-Reply-To: https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/97407 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Add some docs for LazyCompoundValue (PR #97407)
=?utf-8?q?Kristóf?= Umann ,
=?utf-8?q?Kristóf?= Umann ,
=?utf-8?q?Kristóf?= Umann ,
=?utf-8?q?Kristóf?= Umann
Message-ID:
In-Reply-To:
@@ -363,6 +397,18 @@ class LazyCompoundVal : public NonLoc {
/// It might return null.
const void *getStore() const;
+ /// This function itself is immaterial. It is only an implementation detail.
+ /// LazyCompoundVal represents only the rvalue, the data (known or unknown)
+ /// that *was* stored in that region *at some point in the past*. The region
+ /// should not be used for any purpose other than figuring out what part of
+ /// the frozen Store you're interested in. The value does not represent the
+ /// *current* value of that region. Sometimes it may, but this should not be
+ /// relied upon. Instead, if you want to figure out what region it
represents,
+ /// you typically need to see where you got it from in the first place. The
+ /// region is absolutely not analogous to the C++ "this" pointer. It is also
+ /// not a valid way to "materialize" the prvalue into a glvalue in C++,
+ /// because the region represents the *old* storage (sometimes very old), not
+ /// the *future* storage.
steakhal wrote:
To be fair, I don't really understand this.
To me, this region is the region that one would need to query from the
associated `Store` to get the value this `LazyCompoundValue` actually boils
down to.
So, a region, like every other region, only defines a memory location (an
l-value), and there is not much one could do with such a region. But I find
this true for any other `MemRegion` anywhere in the engine.
Probably I miss something, as I don't usually deal with LCVs, and it's been a
while I last touched them.
So, to summarize, I don't really understand what `immaterial` means in this
context or understand why we mention it for `LCV::getRegion()` instead of
mention this for a `MemRegion`.
https://github.com/llvm/llvm-project/pull/97407
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][NFC] Add some docs for LazyCompoundValue (PR #97407)
=?utf-8?q?Kristóf?= Umann , =?utf-8?q?Kristóf?= Umann , =?utf-8?q?Kristóf?= Umann , =?utf-8?q?Kristóf?= Umann Message-ID: In-Reply-To: https://github.com/steakhal approved this pull request. I think it's already in a good shape. I found barely any problems with the wording. https://github.com/llvm/llvm-project/pull/97407 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -3483,53 +3578,54 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
Sym, "Returned allocated memory");
} else if (isReleased(RSCurr, RSPrev, S)) {
const auto Family = RSCurr->getAllocationFamily();
- switch (Family) {
-case AF_Alloca:
-case AF_Malloc:
-case AF_CXXNew:
-case AF_CXXNewArray:
-case AF_IfNameIndex:
- Msg = "Memory is released";
+ switch (Family.kind()) {
+ case AF_Alloca:
+ case AF_Malloc:
+ case AF_Custom:
+ case AF_CXXNew:
+ case AF_CXXNewArray:
+ case AF_IfNameIndex:
+Msg = "Memory is released";
+StackHint = std::make_unique(
+Sym, "Returning; memory was released");
+break;
+ case AF_InnerBuffer: {
+const MemRegion *ObjRegion =
+allocation_state::getContainerObjRegion(statePrev, Sym);
+const auto *TypedRegion = cast(ObjRegion);
+QualType ObjTy = TypedRegion->getValueType();
+OS << "Inner buffer of '" << ObjTy << "' ";
+
+if (N->getLocation().getKind() == ProgramPoint::PostImplicitCallKind) {
+ OS << "deallocated by call to destructor";
StackHint = std::make_unique(
- Sym, "Returning; memory was released");
- break;
-case AF_InnerBuffer: {
- const MemRegion *ObjRegion =
- allocation_state::getContainerObjRegion(statePrev, Sym);
- const auto *TypedRegion = cast(ObjRegion);
- QualType ObjTy = TypedRegion->getValueType();
- OS << "Inner buffer of '" << ObjTy << "' ";
-
- if (N->getLocation().getKind() ==
ProgramPoint::PostImplicitCallKind) {
-OS << "deallocated by call to destructor";
-StackHint = std::make_unique(
-Sym, "Returning; inner buffer was deallocated");
- } else {
-OS << "reallocated by call to '";
-const Stmt *S = RSCurr->getStmt();
-if (const auto *MemCallE = dyn_cast(S)) {
- OS << MemCallE->getMethodDecl()->getDeclName();
-} else if (const auto *OpCallE = dyn_cast(S))
{
- OS << OpCallE->getDirectCallee()->getDeclName();
-} else if (const auto *CallE = dyn_cast(S)) {
- auto = BRC.getStateManager().getCallEventManager();
- CallEventRef<> Call =
- CEMgr.getSimpleCall(CallE, state, CurrentLC, {nullptr, 0});
- if (const auto *D = dyn_cast_or_null(Call->getDecl()))
-OS << D->getDeclName();
- else
-OS << "unknown";
-}
-OS << "'";
-StackHint = std::make_unique(
-Sym, "Returning; inner buffer was reallocated");
+ Sym, "Returning; inner buffer was deallocated");
+} else {
+ OS << "reallocated by call to '";
+ const Stmt *S = RSCurr->getStmt();
+ if (const auto *MemCallE = dyn_cast(S)) {
+OS << MemCallE->getMethodDecl()->getDeclName();
+ } else if (const auto *OpCallE = dyn_cast(S)) {
+OS << OpCallE->getDirectCallee()->getDeclName();
+ } else if (const auto *CallE = dyn_cast(S)) {
+auto = BRC.getStateManager().getCallEventManager();
+CallEventRef<> Call =
+CEMgr.getSimpleCall(CallE, state, CurrentLC, {nullptr, 0});
+if (const auto *D = dyn_cast_or_null(Call->getDecl()))
+ OS << D->getDeclName();
+else
+ OS << "unknown";
}
- Msg = OS.str();
- break;
+ OS << "'";
+ StackHint = std::make_unique(
+ Sym, "Returning; inner buffer was reallocated");
}
+Msg = OS.str();
+break;
+ }
case AF_None:
llvm_unreachable("Unhandled allocation family!");
steakhal wrote:
This isn't part of your current patch, but I'd suggest turning this into an
assert and return too.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -14,6 +14,13 @@ void free(void *); void __attribute((ownership_takes(malloc, 1))) my_free(void *); +void __attribute((ownership_returns(malloc1))) *my_malloc1(size_t); steakhal wrote: What happens if you have a forward declaration to a malloc function without the attribute, then later have another redeclaration with the `ownership_returns` attribute? A similar case to test would be the opposite: 1st declaration has the attribute, the second redeclaration would not have the attribute. I suspect that the FunctionDecl of the callees will refer to the last seen declaration of the function, thus, will not recognize the attribute for the second case I mentioned here. Could you test these cases? https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1877,6 +1914,33 @@ static bool didPreviousFreeFail(ProgramStateRef State,
return false;
}
+static void printOwnershipTakesList(raw_ostream , CheckerContext ,
+const Expr *E) {
+ if (const CallExpr *CE = dyn_cast(E)) {
+const FunctionDecl *FD = CE->getDirectCallee();
+if (!FD)
+ return;
+
+if (FD->hasAttrs()) {
steakhal wrote:
You could switch this into an early-return to reduce indentation for the rest
of the code.
Same applies to the `if (const CallExpr *CE = dyn_cast(E))` a few
lines above.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -14,6 +14,13 @@ void free(void *); void __attribute((ownership_takes(malloc, 1))) my_free(void *); +void __attribute((ownership_returns(malloc1))) *my_malloc1(size_t); +void __attribute((ownership_takes(malloc1, 1))) my_free1(void *); + +void __attribute((ownership_returns(malloc2))) *my_malloc2(size_t); +void __attribute((ownership_returns(malloc2))) *my_malloc3(size_t); +void __attribute((ownership_takes(malloc2, 1))) __attribute((ownership_takes(malloc3, 1))) my_free23(void *); steakhal wrote: I'm surprised to see that a function can have two different `ownership_takes` attributes. Are you sure that this is the case? If so, why is that? Have you considered adding some documentation to the attributes page, and to the Malloc checker to highlight this attribute? https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -103,14 +103,46 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+class AllocationFamily {
+public:
+ AllocationFamily(AllocationFamilyKind kind,
+ std::optional name = std::nullopt)
+ : _kind(kind), _customName(name) {
+assert(kind != AF_Custom || name != std::nullopt);
+ }
+
+ bool operator==(const AllocationFamily ) const {
+if (Other.kind() == this->kind()) {
+ return this->kind() == AF_Custom ? this->_customName == Other._customName
+ : true;
+} else {
+ return false;
+}
+ }
+
+ bool operator==(const AllocationFamilyKind ) {
+return this->kind() == kind;
+ }
+
+ bool operator!=(const AllocationFamilyKind ) { return !(*this == kind);
}
steakhal wrote:
Could you define this for the class type instead?
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1918,26 +1982,54 @@ static bool printMemFnName(raw_ostream ,
CheckerContext , const Expr *E) {
static void printExpectedAllocName(raw_ostream , AllocationFamily Family) {
- switch(Family) {
-case AF_Malloc: os << "malloc()"; return;
-case AF_CXXNew: os << "'new'"; return;
-case AF_CXXNewArray: os << "'new[]'"; return;
-case AF_IfNameIndex: os << "'if_nameindex()'"; return;
-case AF_InnerBuffer: os << "container-specific allocator"; return;
-case AF_Alloca:
-case AF_None: llvm_unreachable("not a deallocation expression");
+ switch (Family.kind()) {
+ case AF_Malloc:
+os << "malloc()";
+return;
+ case AF_CXXNew:
+os << "'new'";
+return;
+ case AF_CXXNewArray:
+os << "'new[]'";
+return;
+ case AF_IfNameIndex:
+os << "'if_nameindex()'";
+return;
+ case AF_InnerBuffer:
+os << "container-specific allocator";
+return;
+ case AF_Custom:
+os << Family.name().value();
+return;
+ case AF_Alloca:
+ case AF_None:
+llvm_unreachable("not a deallocation expression");
}
}
static void printExpectedDeallocName(raw_ostream , AllocationFamily Family)
{
- switch(Family) {
-case AF_Malloc: os << "free()"; return;
-case AF_CXXNew: os << "'delete'"; return;
-case AF_CXXNewArray: os << "'delete[]'"; return;
-case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
-case AF_InnerBuffer: os << "container-specific deallocator"; return;
-case AF_Alloca:
-case AF_None: llvm_unreachable("suspicious argument");
+ switch (Family.kind()) {
+ case AF_Malloc:
+os << "free()";
+return;
+ case AF_CXXNew:
+os << "'delete'";
+return;
+ case AF_CXXNewArray:
+os << "'delete[]'";
+return;
+ case AF_IfNameIndex:
+os << "'if_freenameindex()'";
+return;
+ case AF_InnerBuffer:
+os << "container-specific deallocator";
+return;
+ case AF_Custom:
+os << "function that takes ownership of '" << Family.name().value() <<
"\'";
+return;
+ case AF_Alloca:
+ case AF_None:
+llvm_unreachable("suspicious argument");
steakhal wrote:
Same goes here, use a regular `assert` and `return`.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -103,14 +103,46 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+class AllocationFamily {
+public:
+ AllocationFamily(AllocationFamilyKind kind,
+ std::optional name = std::nullopt)
+ : _kind(kind), _customName(name) {
+assert(kind != AF_Custom || name != std::nullopt);
+ }
+
+ bool operator==(const AllocationFamily ) const {
+if (Other.kind() == this->kind()) {
+ return this->kind() == AF_Custom ? this->_customName == Other._customName
+ : true;
+} else {
+ return false;
+}
+ }
+
+ bool operator==(const AllocationFamilyKind ) {
+return this->kind() == kind;
+ }
+
+ bool operator!=(const AllocationFamilyKind ) { return !(*this == kind);
}
+
+ std::optional name() const { return _customName; }
+ AllocationFamilyKind kind() const { return _kind; }
+
+private:
+ AllocationFamilyKind _kind;
+ std::optional _customName;
steakhal wrote:
I think it should be fine to have these fields as public members, given the
complexity of this class. This is basically a std::pair. Feel free to use
`struct` instead of `class`.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1877,6 +1914,33 @@ static bool didPreviousFreeFail(ProgramStateRef State,
return false;
}
+static void printOwnershipTakesList(raw_ostream , CheckerContext ,
+const Expr *E) {
+ if (const CallExpr *CE = dyn_cast(E)) {
+const FunctionDecl *FD = CE->getDirectCallee();
+if (!FD)
+ return;
+
+if (FD->hasAttrs()) {
+ bool attrPrinted = false;
+
+ for (const auto *I : FD->specific_attrs()) {
+OwnershipAttr::OwnershipKind OwnKind = I->getOwnKind();
+
+if (OwnKind == OwnershipAttr::Takes) {
+ if (attrPrinted)
+os << ", ";
+ else
+os << ", which takes ownership of ";
+
+ os << '\'' << I->getModule()->getName() << "'";
steakhal wrote:
You use single quoted char and double quoted char. I just highlight this
inconsistency.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -194,7 +226,7 @@ class RefState {
void Profile(llvm::FoldingSetNodeID ) const {
ID.AddInteger(K);
ID.AddPointer(S);
-ID.AddInteger(Family);
+ID.AddInteger(Family.kind());
steakhal wrote:
This Profile wouldn't take the custom allocation function name into account.
I'd suggest letting the Family define its own Profile method and just pass the
ID to it.
That is the canonical way of doing this.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
https://github.com/steakhal requested changes to this pull request. Looks pretty good. Thanks for working on the issue. I didn't see any major blocking issue with the PR, so good job on that! Overall, be sure to follow the llvm [coding style](https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly), including using upper case variable names. Thanks again for working on this :) https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -1918,26 +1982,54 @@ static bool printMemFnName(raw_ostream ,
CheckerContext , const Expr *E) {
static void printExpectedAllocName(raw_ostream , AllocationFamily Family) {
- switch(Family) {
-case AF_Malloc: os << "malloc()"; return;
-case AF_CXXNew: os << "'new'"; return;
-case AF_CXXNewArray: os << "'new[]'"; return;
-case AF_IfNameIndex: os << "'if_nameindex()'"; return;
-case AF_InnerBuffer: os << "container-specific allocator"; return;
-case AF_Alloca:
-case AF_None: llvm_unreachable("not a deallocation expression");
+ switch (Family.kind()) {
+ case AF_Malloc:
+os << "malloc()";
+return;
+ case AF_CXXNew:
+os << "'new'";
+return;
+ case AF_CXXNewArray:
+os << "'new[]'";
+return;
+ case AF_IfNameIndex:
+os << "'if_nameindex()'";
+return;
+ case AF_InnerBuffer:
+os << "container-specific allocator";
+return;
+ case AF_Custom:
+os << Family.name().value();
+return;
+ case AF_Alloca:
+ case AF_None:
+llvm_unreachable("not a deallocation expression");
steakhal wrote:
I'd suggest swapping this `llvm_unreachable` to an `assert(false && "not a
deallocation expression"); return;` as this code is probably not in the hot
path, thus the use of `llvm_unreachable` is unjustified.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -14,6 +14,13 @@ void free(void *); void __attribute((ownership_takes(malloc, 1))) my_free(void *); +void __attribute((ownership_returns(malloc1))) *my_malloc1(size_t); +void __attribute((ownership_takes(malloc1, 1))) my_free1(void *); + +void __attribute((ownership_returns(malloc2))) *my_malloc2(size_t); +void __attribute((ownership_returns(malloc2))) *my_malloc3(size_t); steakhal wrote: This looks suspiciously similar to `my_malloc2`, are you sure you don't have a type somewhere? https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
@@ -103,14 +103,46 @@ using namespace std::placeholders;
namespace {
// Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
AF_None,
AF_Malloc,
AF_CXXNew,
AF_CXXNewArray,
AF_IfNameIndex,
AF_Alloca,
- AF_InnerBuffer
+ AF_InnerBuffer,
+ AF_Custom,
+};
+
+class AllocationFamily {
+public:
+ AllocationFamily(AllocationFamilyKind kind,
steakhal wrote:
I'd suggest marking this `explicit`.
https://github.com/llvm/llvm-project/pull/98941
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
https://github.com/steakhal edited https://github.com/llvm/llvm-project/pull/98941 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Improve bug report hashing, merge similar reports (PR #98621)
=?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: steakhal wrote: > _(Technical detail: I'll be on vacation during the next week, so I won't see > updates on this PR until the 22nd of July. If you want to merge this PR, feel > free to do so.)_ No need to rush. Have a nice one! https://github.com/llvm/llvm-project/pull/98621 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Don't display the offset value in underflows (PR #98621)
https://github.com/steakhal commented: This makes sense. I wonder if we could have something in between. I'm thinking of having the concrete offset as a separate note, instead of having it part of the primary message. That way after BR selection, we would still deterministically pick the shortest parh, and also have the offset in the path notes. I find this better than dropping the concrete offset you worked so hard acquiring. WDYT? https://github.com/llvm/llvm-project/pull/98621 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer][docs] Add clang-19 release notes for CSA (PR #97418)
https://github.com/steakhal closed https://github.com/llvm/llvm-project/pull/97418 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Split TaintPropagation checker into reporting and modeling checkers (PR #98157)
@@ -1046,10 +1044,7 @@ bool GenericTaintChecker::generateReportIfTainted(const Expr *E, StringRef Msg, return false; // Generate diagnostic. steakhal wrote: If we are there, maybe we should also assert that inside the register call, before emplacing it was empty. https://github.com/llvm/llvm-project/pull/98157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
