[analyzer] Remove some false negatives in StackAddrEscapeChecker #125638
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Flandini
changed the title
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Michael Flanders (Flandini) ChangesFixes #123459. Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in #123459. The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another from Full diff: https://github.com/llvm/llvm-project/pull/125638.diff 3 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
index f4de3b500499c48..86f0949994cf6b9 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -54,8 +54,8 @@ class StackAddrEscapeChecker
CheckerContext &C) const;
void checkAsyncExecutedBlockCaptures(const BlockDataRegion &B,
CheckerContext &C) const;
- void EmitStackError(CheckerContext &C, const MemRegion *R,
- const Expr *RetE) const;
+ void EmitReturnLeakError(CheckerContext &C, const MemRegion *LeakedRegion,
+ const Expr *RetE) const;
bool isSemaphoreCaptured(const BlockDecl &B) const;
static SourceRange genName(raw_ostream &os, const MemRegion *R,
ASTContext &Ctx);
@@ -147,9 +147,22 @@ StackAddrEscapeChecker::getCapturedStackRegions(const BlockDataRegion &B,
return Regions;
}
-void StackAddrEscapeChecker::EmitStackError(CheckerContext &C,
- const MemRegion *R,
- const Expr *RetE) const {
+static void EmitReturnedAsPartOfError(llvm::raw_ostream &OS, SVal ReturnedVal,
+ const MemRegion *LeakedRegion) {
+ if (const MemRegion *ReturnedRegion = ReturnedVal.getAsRegion()) {
+ if (isa<BlockDataRegion>(ReturnedRegion)) {
+ OS << " is captured by a returned block";
+ return;
+ }
+ }
+
+ // Generic message
+ OS << " returned to caller";
+}
+
+void StackAddrEscapeChecker::EmitReturnLeakError(CheckerContext &C,
+ const MemRegion *R,
+ const Expr *RetE) const {
ExplodedNode *N = C.generateNonFatalErrorNode();
if (!N)
return;
@@ -157,11 +170,15 @@ void StackAddrEscapeChecker::EmitStackError(CheckerContext &C,
BT_returnstack = std::make_unique<BugType>(
CheckNames[CK_StackAddrEscapeChecker],
"Return of address to stack-allocated memory");
+
// Generate a report for this bug.
SmallString<128> buf;
llvm::raw_svector_ostream os(buf);
+
+ // Error message formatting
SourceRange range = genName(os, R, C.getASTContext());
- os << " returned to caller";
+ EmitReturnedAsPartOfError(os, C.getSVal(RetE), R);
+
auto report =
std::make_unique<PathSensitiveBugReport>(*BT_returnstack, os.str(), N);
report->addRange(RetE->getSourceRange());
@@ -209,30 +226,6 @@ void StackAddrEscapeChecker::checkAsyncExecutedBlockCaptures(
}
}
-void StackAddrEscapeChecker::checkReturnedBlockCaptures(
- const BlockDataRegion &B, CheckerContext &C) const {
- for (const MemRegion *Region : getCapturedStackRegions(B, C)) {
- if (isNotInCurrentFrame(Region, C))
- continue;
- ExplodedNode *N = C.generateNonFatalErrorNode();
- if (!N)
- continue;
- if (!BT_capturedstackret)
- BT_capturedstackret = std::make_unique<BugType>(
- CheckNames[CK_StackAddrEscapeChecker],
- "Address of stack-allocated memory is captured");
- SmallString<128> Buf;
- llvm::raw_svector_ostream Out(Buf);
- SourceRange Range = genName(Out, Region, C.getASTContext());
- Out << " is captured by a returned block";
- auto Report = std::make_unique<PathSensitiveBugReport>(*BT_capturedstackret,
- Out.str(), N);
- if (Range.isValid())
- Report->addRange(Range);
- C.emitReport(std::move(Report));
- }
-}
-
void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call,
CheckerContext &C) const {
if (!ChecksEnabled[CK_StackAddrAsyncEscapeChecker])
@@ -247,45 +240,134 @@ void StackAddrEscapeChecker::checkPreCall(const CallEvent &Call,
}
}
-void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
- CheckerContext &C) const {
- if (!ChecksEnabled[CK_StackAddrEscapeChecker])
- return;
+/// A visitor made for use with a ScanReachableSymbols scanner, used
+/// for finding stack regions within an SVal that live on the current
+/// stack frame of the given checker context. This visitor excludes
+/// NonParamVarRegion that data is bound to in a BlockDataRegion's
+/// bindings, since these are likely uninteresting, e.g., in case a
+/// temporary is constructed on the stack, but it captures values
+/// that would leak.
+class FindStackRegionsSymbolVisitor final : public SymbolVisitor {
+ CheckerContext &Ctxt;
+ const StackFrameContext *StackFrameContext;
+ SmallVector<const MemRegion *> &EscapingStackRegions;
- const Expr *RetE = RS->getRetValue();
- if (!RetE)
- return;
- RetE = RetE->IgnoreParens();
+public:
+ explicit FindStackRegionsSymbolVisitor(
+ CheckerContext &Ctxt,
+ SmallVector<const MemRegion *> &StorageForStackRegions)
+ : Ctxt(Ctxt), StackFrameContext(Ctxt.getStackFrame()),
+ EscapingStackRegions(StorageForStackRegions) {}
- SVal V = C.getSVal(RetE);
- const MemRegion *R = V.getAsRegion();
- if (!R)
- return;
+ bool VisitSymbol(SymbolRef sym) override { return true; }
- if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R))
- checkReturnedBlockCaptures(*B, C);
+ bool VisitMemRegion(const MemRegion *MR) override {
+ SaveIfEscapes(MR);
- if (!isa<StackSpaceRegion>(R->getMemorySpace()) || isNotInCurrentFrame(R, C))
- return;
+ if (const BlockDataRegion *BDR = MR->getAs<BlockDataRegion>())
+ return VisitBlockDataRegionCaptures(BDR);
+
+ return true;
+ }
+
+private:
+ void SaveIfEscapes(const MemRegion *MR) {
+ const StackSpaceRegion *SSR =
+ MR->getMemorySpace()->getAs<StackSpaceRegion>();
+ if (SSR && SSR->getStackFrame() == StackFrameContext)
+ EscapingStackRegions.push_back(MR);
+ }
+
+ bool VisitBlockDataRegionCaptures(const BlockDataRegion *BDR) {
+ for (auto Var : BDR->referenced_vars()) {
+ SVal Val = Ctxt.getState()->getSVal(Var.getCapturedRegion());
+ const MemRegion *Region = Val.getAsRegion();
+ if (Region) {
+ SaveIfEscapes(Region);
+ VisitMemRegion(Region);
+ }
+ }
+
+ return false;
+ }
+};
+
+/// Given some memory regions that are flagged by FindStackRegionsSymbolVisitor,
+/// this function filters out memory regions that are being returned that are
+/// likely not true leaks:
+/// 1. If returning a block data region that has stack memory space
+/// 2. If returning a constructed object that has stack memory space
+static SmallVector<const MemRegion *>
+FilterReturnExpressionLeaks(const SmallVector<const MemRegion *> &MaybeEscaped,
+ CheckerContext &C, const Expr *RetE, SVal &RetVal) {
+
+ SmallVector<const MemRegion *> WillEscape;
+
+ const MemRegion *RetRegion = RetVal.getAsRegion();
// Returning a record by value is fine. (In this case, the returned
// expression will be a copy-constructor, possibly wrapped in an
// ExprWithCleanups node.)
if (const ExprWithCleanups *Cleanup = dyn_cast<ExprWithCleanups>(RetE))
RetE = Cleanup->getSubExpr();
- if (isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType())
- return;
+ bool IsConstructExpr =
+ isa<CXXConstructExpr>(RetE) && RetE->getType()->isRecordType();
// The CK_CopyAndAutoreleaseBlockObject cast causes the block to be copied
// so the stack address is not escaping here.
+ bool IsCopyAndAutoreleaseBlockObj = false;
if (const auto *ICE = dyn_cast<ImplicitCastExpr>(RetE)) {
- if (isa<BlockDataRegion>(R) &&
- ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject) {
- return;
- }
+ IsCopyAndAutoreleaseBlockObj =
+ isa_and_nonnull<BlockDataRegion>(RetRegion) &&
+ ICE->getCastKind() == CK_CopyAndAutoreleaseBlockObject;
+ }
+
+ for (const MemRegion *MR : MaybeEscaped) {
+ if (RetRegion == MR && (IsCopyAndAutoreleaseBlockObj || IsConstructExpr))
+ continue;
+
+ // If this is a construct expr of an unelided return value copy, then don't
+ // warn about returning a region that currently lives on the stack.
+ if (IsConstructExpr && RetVal.getAs<nonloc::LazyCompoundVal>() &&
+ isa<CXXTempObjectRegion>(MR))
+ continue;
+
+ WillEscape.push_back(MR);
}
- EmitStackError(C, R, RetE);
+ return WillEscape;
+}
+
+/// For use in finding regions that live on the checker context's current
+/// stack frame, deep in the SVal representing the return value.
+static SmallVector<const MemRegion *>
+FindEscapingStackRegions(CheckerContext &C, const Expr *RetE, SVal RetVal) {
+ SmallVector<const MemRegion *> FoundStackRegions;
+
+ FindStackRegionsSymbolVisitor Finder(C, FoundStackRegions);
+ ScanReachableSymbols Scanner(C.getState(), Finder);
+ Scanner.scan(RetVal);
+
+ return FilterReturnExpressionLeaks(FoundStackRegions, C, RetE, RetVal);
+}
+
+void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
+ CheckerContext &C) const {
+ if (!ChecksEnabled[CK_StackAddrEscapeChecker])
+ return;
+
+ const Expr *RetE = RS->getRetValue();
+ if (!RetE)
+ return;
+ RetE = RetE->IgnoreParens();
+
+ SVal V = C.getSVal(RetE);
+
+ SmallVector<const MemRegion *> EscapedStackRegions =
+ FindEscapingStackRegions(C, RetE, V);
+
+ for (const MemRegion *ER : EscapedStackRegions)
+ EmitReturnLeakError(C, ER, RetE);
}
static const MemSpaceRegion *getStackOrGlobalSpaceRegion(const MemRegion *R) {
diff --git a/clang/test/Analysis/stack-addr-ps.cpp b/clang/test/Analysis/stack-addr-ps.cpp
index 73e9dbeca460f60..392982d92a3f14c 100644
--- a/clang/test/Analysis/stack-addr-ps.cpp
+++ b/clang/test/Analysis/stack-addr-ps.cpp
@@ -251,7 +251,7 @@ void* lambda_to_context_direct_pointer_uncalled() {
int local = 42;
p = &local; // no-warning: analyzed only as top-level, ignored explicitly by the checker
};
- return new MyFunction(&lambda);
+ return new MyFunction(&lambda); // expected-warning{{Address of stack memory associated with local variable 'lambda' returned to caller}}
}
void lambda_to_context_direct_pointer_lifetime_extended() {
@@ -410,16 +410,16 @@ void** returned_arr_of_ptr_top() {
int* p = &local;
void** arr = new void*[2];
arr[1] = p;
- return arr;
-} // no-warning False Negative
+ return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}}
+}
void** returned_arr_of_ptr_callee() {
int local = 42;
int* p = &local;
void** arr = new void*[2];
arr[1] = p;
- return arr;
-} // no-warning False Negative
+ return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}}
+}
void returned_arr_of_ptr_caller() {
void** arr = returned_arr_of_ptr_callee();
@@ -466,16 +466,16 @@ void** returned_arr_of_ptr_top(int idx) {
int* p = &local;
void** arr = new void*[2];
arr[idx] = p;
- return arr;
-} // no-warning False Negative
+ return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}}
+}
void** returned_arr_of_ptr_callee(int idx) {
int local = 42;
int* p = &local;
void** arr = new void*[2];
arr[idx] = p;
- return arr;
-} // no-warning False Negative
+ return arr; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}}
+}
void returned_arr_of_ptr_caller(int idx) {
void** arr = returned_arr_of_ptr_callee(idx);
@@ -525,14 +525,25 @@ S returned_struct_with_ptr_top() {
int local = 42;
S s;
s.p = &local;
- return s;
-} // no-warning False Negative, requires traversing returned LazyCompoundVals
+ return s; // expected-warning{{Address of stack memory associated with local variable 'local' returned to caller}}
+}
S returned_struct_with_ptr_callee() {
int local = 42;
S s;
s.p = &local;
- return s; // expected-warning{{'local' is still referred to by the caller variable 's'}}
+ return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}} expected-warning{{Address of stack memory associated with local variable 'local' is still referred to by the caller variable 's' upon returning to the caller. This will be a dangling reference}}
+}
+
+S leak_through_field_of_returned_object() {
+ int local = 14;
+ S s{&local};
+ return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}}
+}
+
+S leak_through_compound_literal() {
+ int local = 0;
+ return (S) { &local }; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}}
}
void returned_struct_with_ptr_caller() {
@@ -555,6 +566,30 @@ void static_struct_with_ptr() {
}
} // namespace leaking_via_struct_with_ptr
+namespace leaking_via_nested_structs_with_ptr {
+struct Inner {
+ int *ptr;
+};
+
+struct Outer {
+ Inner I;
+};
+
+struct Deriving : public Outer {};
+
+Outer leaks_through_nested_objects() {
+ int local = 0;
+ Outer O{&local};
+ return O; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}}
+}
+
+Deriving leaks_through_base_objects() {
+ int local = 0;
+ Deriving D{&local};
+ return D; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}}
+}
+} // namespace leaking_via_nested_structs_with_ptr
+
namespace leaking_via_ref_to_struct_with_ptr {
struct S {
int* p;
@@ -613,15 +648,15 @@ S* returned_ptr_to_struct_with_ptr_top() {
int local = 42;
S* s = new S;
s->p = &local;
- return s;
-} // no-warning False Negative
+ return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}}
+}
S* returned_ptr_to_struct_with_ptr_callee() {
int local = 42;
S* s = new S;
s->p = &local;
- return s;
-} // no-warning False Negative
+ return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}}
+}
void returned_ptr_to_struct_with_ptr_caller() {
S* s = returned_ptr_to_struct_with_ptr_callee();
@@ -676,15 +711,15 @@ S* returned_ptr_to_struct_with_ptr_top() {
int local = 42;
S* s = new S[2];
s[1].p = &local;
- return s;
-} // no-warning False Negative
+ return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}}
+}
S* returned_ptr_to_struct_with_ptr_callee() {
int local = 42;
S* s = new S[2];
s[1].p = &local;
- return s;
-} // no-warning False Negative
+ return s; // expected-warning {{Address of stack memory associated with local variable 'local' returned to caller}}
+}
void returned_ptr_to_struct_with_ptr_caller() {
S* s = returned_ptr_to_struct_with_ptr_callee();
diff --git a/clang/test/Analysis/stackaddrleak.cpp b/clang/test/Analysis/stackaddrleak.cpp
index 3daffb35a6cd9a6..a44fb1f7d2dd121 100644
--- a/clang/test/Analysis/stackaddrleak.cpp
+++ b/clang/test/Analysis/stackaddrleak.cpp
@@ -18,8 +18,8 @@ struct myfunction {
myfunction create_func() {
int n;
auto c = [&n] {};
- return c; // expected-warning {{Address of stack memory associated with local variable 'n' is still referred to by a temporary object on the stack upon returning to the caller. This will be a dangling reference}}
+ return c; // expected-warning {{Address of stack memory associated with local variable 'n' returned to caller}} expected-warning{{Address of stack memory associated with local variable 'n' is still referred to by a temporary object on the stack upon returning to the caller. This will be a dangling reference}}
}
void gh_66221() {
create_func()();
-}
+}
\ No newline at end of file
|
Xazax-hun
reviewed
Feb 4, 2025
|
|
||
| // If this is a construct expr of an unelided return value copy, then don't | ||
| // warn about returning a region that currently lives on the stack. | ||
| if (IsConstructExpr && RetVal.getAs<nonloc::LazyCompoundVal>() && |
There was a problem hiding this comment.
I am wondering if this is the right solution here. I am a bit concerned that there are some other cases that we missed here. I think we are only interested in stack memory regions that are behind a pointer.
But this is also not entirely true. Imagine the following case:
DoesDeepCopy myFunc() {
int local;
DoesDeepCopy l(&local);
return l;
};
Here, if DoesDeepCopy's ctor would copy the value behind the pointer, the code above is fine. On the second though, I wonder if NRVO can actually kick in making this code unsafe.
That being said, if the copy ctor is actually invoked, hopefully we would observe the effects of that, the fields would get invalidated, and we would not see the memory region from the stack.
Overall, I think some scenarios here might need more tests.
There was a problem hiding this comment.
This line is only for removing warnings. It is a little loose and maybe could cause some false negatives. Is there a way here to relate the lazy compound val that would be returned and the temp object region in an example like this from copy-elision.cpp test:
template <typename T> struct AddressVector {
T *buf[20];
int len;
AddressVector() : len(0) {}
void push(T *t) {
buf[len] = t;
++len;
}
};
class ClassWithoutDestructor {
AddressVector<ClassWithoutDestructor> &v;
public:
ClassWithoutDestructor(AddressVector<ClassWithoutDestructor> &v) : v(v) {
push();
}
ClassWithoutDestructor(ClassWithoutDestructor &&c) : v(c.v) { push(); }
ClassWithoutDestructor(const ClassWithoutDestructor &c) : v(c.v) { push(); }
void push() { v.push(this); }
};
ClassWithoutDestructor make1(AddressVector<ClassWithoutDestructor> &v) {
return ClassWithoutDestructor(v);
// no-elide-warning@-1 {{Address of stack memory associated with temporary \
object of type 'ClassWithoutDestructor' is still \
referred to by the caller variable 'v' upon returning to the caller}}
}
Without some check like this, then the line with return ClassWithoutDestructor(v); will warn, and I think it should not under -analyzer-config elide-constructors=false -DNO_ELIDE_FLAG -std=c++11. Is there a better way to relate the lazy compound val of the return expr and the temp object region that is created?
There was a problem hiding this comment.
I think we are only interested in stack memory regions that are behind a pointer.
I think there are still some cases where warning on the returned pointer is also wanted, like this one, or maybe more complicated versions of this that normal compilation warnings wouldn't catch
int *test() {
int x = 14;
return &x;
}
There was a problem hiding this comment.
The local memory region in the function test is behind a pointer so that is matches my description.
But we would not want to warn for:
int test2() {
int x = 14;
return x;
}
Because here x is not behind a pointer.
Could you elaborate why the analyzer thinks the temporary is referred to by v here?
Is there a better way to relate the lazy compound val of the return expr and the temp object region that is created?
Dealing with LazyCompoundVals can be a bit fiddly. You could take a look at FindUninitializedField in CallAndMessageChecker.cpp .
There was a problem hiding this comment.
Overall, I wonder if what we have here is similar to what the analyzer is doing when creating the checkPointerEscape callback. We basically want to collect all the pointers from the returned value a similar way and want to check if any of those point to stack memory. I think collecting the pointers might be an API that we need/want to expose to the checkers.
There was a problem hiding this comment.
We basically want to collect all the pointers from the returned value a similar way and want to check if any of those point to stack memory.
That is what I was hoping this PR was: add a ScanReachableSymbols & SymbolVisitor that takes all stack regions behind the return SVal and warns on those. Previously, this checker only did this for BlockDataRegion captures and it only checked if the return SVal is a stack space region.
IIUC, you would like this instead to be reworked into checkPointerEscape?
There was a problem hiding this comment.
does a v.push(this)
Ah, thanks for the explanation, I missed this detail somehow. So this actually is a true positive warning when copy elision is not happening. I think in that case we should only filter these temporaries out when the temporary is actually elided (we should see this in the AST).
And now I better understand what you mean by relating the returned memory region and the lazy compound val. You already have a RetRegion, would comparing that to LazyCompoundVal::getRegion() work?
There was a problem hiding this comment.
IIUC, you would like this instead to be reworked into checkPointerEscape?
Nope, my bad. Sorry, I misunderstood something. Using ScanReachableSymbols sounds like the right approach to me. I am wondering why is it sufficient to check if a memory region belongs to the current stack. I.e., why we would not warn on:
int test2() {
int x = someFunc();
return x;
}
There was a problem hiding this comment.
I am wondering why is it sufficient to check if a memory region belongs to the current stack. I.e., why we would not warn on:
int test2() { int x = someFunc(); return x; }
Is it because we do this:
void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
CheckerContext &C) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;
const Expr *RetE = RS->getRetValue();
if (!RetE)
return;
RetE = RetE->IgnoreParens();
SVal V = C.getSVal(RetE);
... scan V for escaping stack regions ...
we get the result of the LValueToRValue conversion?
There was a problem hiding this comment.
we get the result of the LValueToRValue conversion?
Ah, I see. Thanks!
I'll do some structural induction approach to what could be returned and what could hold which things, and make sure these all have some test case coverage. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/205/builds/424 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/130/builds/9793 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/28/builds/6734 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/99/builds/4643 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/8/builds/11001 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/75/builds/4974 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/7858 Here is the relevant piece of the build log for the reference |
Xazax-hun
added a commit
that referenced
this pull request
Feb 10, 2025
#126614) …ker (#125638)" This reverts commit 7ba3c55. Co-authored-by: Gabor Horvath <[email protected]>
|
I reverted the change due to the build failure. Could you open a new PR with the error fixed? |
Yes, sorry. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/13004 Here is the relevant piece of the build log for the reference |
This was referenced Feb 10, 2025
|
I wonder how did the "premerge" checks step succeed, if we had build errors. |
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Feb 11, 2025
…s + buildbot fix (#126620) Reapplying changes from llvm/llvm-project#125638 after buildbot failures. Buildbot failures fixed in 029e7e98dc9956086adc6c1dfb0c655a273fbee6, latest commit on this PR. It was a problem with a declared class member with same name as its type. Sorry!
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…m#125638) Fixes llvm#123459. Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in llvm#123459. The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the `checkEndFunction` side of the checker also. Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another from` checkEndFunction` `iterBindings` checking. For these two cases, I prefer the warnings from the return expression checking, but I couldn't figure out a way to drop the `checkEndFunction` without breaking other `checkEndFunction` warnings that we do want. Thoughts here?
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
llvm#126614) …ker (llvm#125638)" This reverts commit 7ba3c55. Co-authored-by: Gabor Horvath <[email protected]>
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…t fix (llvm#126620) Reapplying changes from llvm#125638 after buildbot failures. Buildbot failures fixed in 029e7e9, latest commit on this PR. It was a problem with a declared class member with same name as its type. Sorry!
|
@steakhal, looks like the premerge checks built with clang 20 https://buildkite.com/llvm-project/github-pull-requests/builds/145477#0194f106-3262-4d2b-922c-b3810220acd1/6-4595. I build locally with clang 18. This type of error is caught by g++, not clang++: https://godbolt.org/z/7Y445Pn13. These failed CI builds/tests look like they are built with gcc/g++ maybe? Some explicitly invoke g++, others call |
|
Worth adding a g++ pre-merge build? Can individual subprojects change their pre-merge checks? |
I don't have all the context, as I didn't follow this closely, so I wouldn't raise this. |
|
Did some looking in previous issues. Seems like this was raised when LLVM used bugzilla. This is an ill-formed but no diagnostic required situation #14747 (comment). |
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
…m#125638) Fixes llvm#123459. Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in llvm#123459. The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the `checkEndFunction` side of the checker also. Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another from` checkEndFunction` `iterBindings` checking. For these two cases, I prefer the warnings from the return expression checking, but I couldn't figure out a way to drop the `checkEndFunction` without breaking other `checkEndFunction` warnings that we do want. Thoughts here?
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
llvm#126614) …ker (llvm#125638)" This reverts commit 7ba3c55. Co-authored-by: Gabor Horvath <[email protected]>
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
…t fix (llvm#126620) Reapplying changes from llvm#125638 after buildbot failures. Buildbot failures fixed in 029e7e9, latest commit on this PR. It was a problem with a declared class member with same name as its type. Sorry!
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
…m#125638) Fixes llvm#123459. Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in llvm#123459. The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the `checkEndFunction` side of the checker also. Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another from` checkEndFunction` `iterBindings` checking. For these two cases, I prefer the warnings from the return expression checking, but I couldn't figure out a way to drop the `checkEndFunction` without breaking other `checkEndFunction` warnings that we do want. Thoughts here?
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
llvm#126614) …ker (llvm#125638)" This reverts commit 7ba3c55. Co-authored-by: Gabor Horvath <[email protected]>
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
…t fix (llvm#126620) Reapplying changes from llvm#125638 after buildbot failures. Buildbot failures fixed in 029e7e9, latest commit on this PR. It was a problem with a declared class member with same name as its type. Sorry!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #123459.
Previously, when the StackAddrEscapeChecker checked return values, it did not scan into the structure of the return SVal. Now it does, and we can catch some more false negatives that were already mocked out in the tests in addition to those mentioned in #123459.
The warning message at the moment for these newly caught leaks is not great. I think they would be better if they had a better trace of why and how the region leaks. If y'all are happy with these changes, I would try to improve these warnings and work on normalizing this SVal checking on the
checkEndFunctionside of the checker also.Two of the stack address leak test cases now have two warnings, one warning from return expression checking and another fromThey're fine, some test cases will warn twice depending on if checkEndFunction and checkPreStmt both emit warnings, like if leaking through an arg object and also returning the arg object.checkEndFunctioniterBindingschecking. For these two cases, I prefer the warnings from the return expression checking, but I couldn't figure out a way to drop thecheckEndFunctionwithout breaking othercheckEndFunctionwarnings that we do want. Thoughts here?