[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/102602
>From 7b4f999b39f4308cab253204e6be41ea7a70f695 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH 1/9] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+ StateSuccess = StateSuccess->bindLoc(*L, RetVal, C.getLocationContext());
+
+C.addTransition(StateSuccess);
+ }
+
+ // Handle overflow case.
+ {
+C.addTransition(
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
+ }
+}
+
bool BuiltinFunctionChecker::isBuiltinLikeFunction(
const CallEvent &Call) const {
const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
@@ -82,10 +171,41 @@ bool BuiltinFunctionChe
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -50,6 +118,75 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+std::pair
+BuiltinFunctionChecker::checkOverflow(CheckerContext &C, SVal RetVal,
+ QualType Res) const {
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ ASTContext &ACtx = C.getASTContext();
+
+ // Calling a builtin with a non-integer type result produces compiler error.
+ assert(Res->isIntegerType());
+
+ unsigned BitWidth = ACtx.getIntWidth(Res);
+ auto MinVal =
+ llvm::APSInt::getMinValue(BitWidth, Res->isUnsignedIntegerType());
+ auto MaxVal =
+ llvm::APSInt::getMaxValue(BitWidth, Res->isUnsignedIntegerType());
+
+ SVal IsLeMax =
+ SVB.evalBinOp(State, BO_LE, RetVal, nonloc::ConcreteInt(MaxVal), Res);
+ SVal IsGeMin =
+ SVB.evalBinOp(State, BO_GE, RetVal, nonloc::ConcreteInt(MinVal), Res);
+
+ auto [MayNotOverflow, MayOverflow] =
+ State->assume(IsLeMax.castAs());
+ auto [MayNotUnderflow, MayUnderflow] =
+ State->assume(IsGeMin.castAs());
+
+ return {MayOverflow || MayUnderflow, MayNotOverflow && MayNotUnderflow};
+}
+
+void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // Calling a builtin with an incorrect argument count produces compiler
error.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetValMax = SVB.evalBinOp(State, Op, Arg1, Arg2,
+ getSufficientTypeForOverflowOp(C,
ResultType));
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ auto [Overflow, NotOverflow] = checkOverflow(C, RetValMax, ResultType);
+ if (NotOverflow) {
+ProgramStateRef StateNoOverflow =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs()) {
+ StateNoOverflow =
+ StateNoOverflow->bindLoc(*L, RetVal, C.getLocationContext());
+
+ // Propagate taint if any of the argumets were tainted
+ if (isTainted(State, Arg1) || isTainted(State, Arg2))
+StateNoOverflow = addTaint(StateNoOverflow, *L);
+}
+
+C.addTransition(StateNoOverflow);
pskrgag wrote:
Added notes for overflow case with prunable flag in the latest version, thanks!
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] fix crash on binding to symbolic region with `void *` type (PR #107572)
https://github.com/pskrgag created
https://github.com/llvm/llvm-project/pull/107572
As reported in
https://github.com/llvm/llvm-project/pull/103714#issuecomment-2295769193. CSA
crashes on trying to bind value to symbolic region with `void *`. This happens
when such region gets passed as inline asm input and engine tries to bind
`UnknownVal` to that region.
Fix it by changing type from void to char before calling `GetElementZeroRegion`
>From 0e8db855a1bde0692260f5aa26c245328a358a50 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 6 Sep 2024 15:15:52 +0300
Subject: [PATCH] clang/csa: fix crash on bind to symbolic region with void *
---
clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 12 ++--
clang/test/Analysis/asm.cpp | 9 +
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index ba29c123139016..a523daad28736f 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2380,8 +2380,16 @@ RegionStoreManager::bind(RegionBindingsConstRef B, Loc
L, SVal V) {
// Binding directly to a symbolic region should be treated as binding
// to element 0.
- if (const SymbolicRegion *SR = dyn_cast(R))
-R = GetElementZeroRegion(SR, SR->getPointeeStaticType());
+ if (const SymbolicRegion *SR = dyn_cast(R)) {
+// Symbolic region with void * type may appear as input for inline asm
+// block. In such case CSA cannot reason about region content and just
+// assumes it has UnknownVal()
+QualType PT = SR->getPointeeStaticType();
+if (PT->isVoidType())
+ PT = StateMgr.getContext().CharTy;
+
+R = GetElementZeroRegion(SR, PT);
+ }
assert((!isa(R) || !B.lookup(R)) &&
"'this' pointer is not an l-value and is not assignable");
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
index b17ab04994d249..0abaa96b0ae79e 100644
--- a/clang/test/Analysis/asm.cpp
+++ b/clang/test/Analysis/asm.cpp
@@ -40,3 +40,12 @@ void testInlineAsmMemcpyUninit(void)
MyMemcpy(&a[1], &b[1], sizeof(b) - sizeof(b[1]));
c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
}
+
+void *globalPtr;
+
+void testNoCrash()
+{
+ // Use global pointer to make it symbolic. Then engine will try to bind
+ // value to first element of type void * and should not crash.
+ asm ("" : : "a"(globalPtr)); // no crash
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] fix crash on binding to symbolic region with `void *` type (PR #107572)
pskrgag wrote: CC @steakhal @NagyDonat https://github.com/llvm/llvm-project/pull/107572 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] fix crash on binding to symbolic region with `void *` type (PR #107572)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/107572
>From 0e8db855a1bde0692260f5aa26c245328a358a50 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 6 Sep 2024 15:15:52 +0300
Subject: [PATCH 1/3] clang/csa: fix crash on bind to symbolic region with void
*
---
clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 12 ++--
clang/test/Analysis/asm.cpp | 9 +
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index ba29c123139016..a523daad28736f 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2380,8 +2380,16 @@ RegionStoreManager::bind(RegionBindingsConstRef B, Loc
L, SVal V) {
// Binding directly to a symbolic region should be treated as binding
// to element 0.
- if (const SymbolicRegion *SR = dyn_cast(R))
-R = GetElementZeroRegion(SR, SR->getPointeeStaticType());
+ if (const SymbolicRegion *SR = dyn_cast(R)) {
+// Symbolic region with void * type may appear as input for inline asm
+// block. In such case CSA cannot reason about region content and just
+// assumes it has UnknownVal()
+QualType PT = SR->getPointeeStaticType();
+if (PT->isVoidType())
+ PT = StateMgr.getContext().CharTy;
+
+R = GetElementZeroRegion(SR, PT);
+ }
assert((!isa(R) || !B.lookup(R)) &&
"'this' pointer is not an l-value and is not assignable");
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
index b17ab04994d249..0abaa96b0ae79e 100644
--- a/clang/test/Analysis/asm.cpp
+++ b/clang/test/Analysis/asm.cpp
@@ -40,3 +40,12 @@ void testInlineAsmMemcpyUninit(void)
MyMemcpy(&a[1], &b[1], sizeof(b) - sizeof(b[1]));
c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
}
+
+void *globalPtr;
+
+void testNoCrash()
+{
+ // Use global pointer to make it symbolic. Then engine will try to bind
+ // value to first element of type void * and should not crash.
+ asm ("" : : "a"(globalPtr)); // no crash
+}
>From 106e2d4a50b1208829e70317a059136de0354a47 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 9 Sep 2024 16:06:41 +0200
Subject: [PATCH 2/3] Apply suggestions from code review
---
clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 14 +-
clang/test/Analysis/asm.cpp | 13 +++--
2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index a523daad28736f..c257a87dff385b 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2380,15 +2380,11 @@ RegionStoreManager::bind(RegionBindingsConstRef B, Loc
L, SVal V) {
// Binding directly to a symbolic region should be treated as binding
// to element 0.
- if (const SymbolicRegion *SR = dyn_cast(R)) {
-// Symbolic region with void * type may appear as input for inline asm
-// block. In such case CSA cannot reason about region content and just
-// assumes it has UnknownVal()
-QualType PT = SR->getPointeeStaticType();
-if (PT->isVoidType())
- PT = StateMgr.getContext().CharTy;
-
-R = GetElementZeroRegion(SR, PT);
+ if (const auto *SymReg = dyn_cast(R)) {
+QualType Ty = SymReg->getPointeeStaticType();
+if (Ty->isVoidType())
+ Ty = StateMgr.getContext().CharTy;
+R = GetElementZeroRegion(SymReg, Ty);
}
assert((!isa(R) || !B.lookup(R)) &&
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
index 0abaa96b0ae79e..4ec119400d2a04 100644
--- a/clang/test/Analysis/asm.cpp
+++ b/clang/test/Analysis/asm.cpp
@@ -41,11 +41,12 @@ void testInlineAsmMemcpyUninit(void)
c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
}
-void *globalPtr;
-
-void testNoCrash()
+void testAsmWithVoidPtrArgument()
{
- // Use global pointer to make it symbolic. Then engine will try to bind
- // value to first element of type void * and should not crash.
- asm ("" : : "a"(globalPtr)); // no crash
+ extern void *globalVoidPtr;
+ clang_analyzer_dump(*(int *)globalVoidPtr); // expected-warning-re
{{reg_${{[0-9]+}}},0 S64b,int}>}}
+ clang_analyzer_dump_ptr(globalVoidPtr); // expected-warning-re
{{&SymRegion{reg_${{[0-9]+}
+ asm ("" : : "a"(globalVoidPtr)); // no crash
+ clang_analyzer_dump(*(int *)globalVoidPtr); // expected-warning {{Unknown}}
+ clang_analyzer_dump_ptr(globalVoidPtr); // expected-warning-re
{{&SymRegion{reg_${{[0-9]+}
}
>From 39a1411d31302c39f1ba288872311982192afcca Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Mon, 9 Sep 2024 17:43:53 +0300
Subject: [PATCH 3/3] add missing declarations
---
clang/test/Analysis/asm.cpp | 3 +++
1 file changed, 3 insertions(+)
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
inde
[clang] [analyzer] fix crash on binding to symbolic region with `void *` type (PR #107572)
pskrgag wrote: added missing declarations. should fix the CI https://github.com/llvm/llvm-project/pull/107572 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] fix crash on binding to symbolic region with `void *` type (PR #107572)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/107572
>From 0e8db855a1bde0692260f5aa26c245328a358a50 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 6 Sep 2024 15:15:52 +0300
Subject: [PATCH 1/4] clang/csa: fix crash on bind to symbolic region with void
*
---
clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 12 ++--
clang/test/Analysis/asm.cpp | 9 +
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index ba29c123139016..a523daad28736f 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2380,8 +2380,16 @@ RegionStoreManager::bind(RegionBindingsConstRef B, Loc
L, SVal V) {
// Binding directly to a symbolic region should be treated as binding
// to element 0.
- if (const SymbolicRegion *SR = dyn_cast(R))
-R = GetElementZeroRegion(SR, SR->getPointeeStaticType());
+ if (const SymbolicRegion *SR = dyn_cast(R)) {
+// Symbolic region with void * type may appear as input for inline asm
+// block. In such case CSA cannot reason about region content and just
+// assumes it has UnknownVal()
+QualType PT = SR->getPointeeStaticType();
+if (PT->isVoidType())
+ PT = StateMgr.getContext().CharTy;
+
+R = GetElementZeroRegion(SR, PT);
+ }
assert((!isa(R) || !B.lookup(R)) &&
"'this' pointer is not an l-value and is not assignable");
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
index b17ab04994d249..0abaa96b0ae79e 100644
--- a/clang/test/Analysis/asm.cpp
+++ b/clang/test/Analysis/asm.cpp
@@ -40,3 +40,12 @@ void testInlineAsmMemcpyUninit(void)
MyMemcpy(&a[1], &b[1], sizeof(b) - sizeof(b[1]));
c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
}
+
+void *globalPtr;
+
+void testNoCrash()
+{
+ // Use global pointer to make it symbolic. Then engine will try to bind
+ // value to first element of type void * and should not crash.
+ asm ("" : : "a"(globalPtr)); // no crash
+}
>From 106e2d4a50b1208829e70317a059136de0354a47 Mon Sep 17 00:00:00 2001
From: Balazs Benics
Date: Mon, 9 Sep 2024 16:06:41 +0200
Subject: [PATCH 2/4] Apply suggestions from code review
---
clang/lib/StaticAnalyzer/Core/RegionStore.cpp | 14 +-
clang/test/Analysis/asm.cpp | 13 +++--
2 files changed, 12 insertions(+), 15 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
index a523daad28736f..c257a87dff385b 100644
--- a/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2380,15 +2380,11 @@ RegionStoreManager::bind(RegionBindingsConstRef B, Loc
L, SVal V) {
// Binding directly to a symbolic region should be treated as binding
// to element 0.
- if (const SymbolicRegion *SR = dyn_cast(R)) {
-// Symbolic region with void * type may appear as input for inline asm
-// block. In such case CSA cannot reason about region content and just
-// assumes it has UnknownVal()
-QualType PT = SR->getPointeeStaticType();
-if (PT->isVoidType())
- PT = StateMgr.getContext().CharTy;
-
-R = GetElementZeroRegion(SR, PT);
+ if (const auto *SymReg = dyn_cast(R)) {
+QualType Ty = SymReg->getPointeeStaticType();
+if (Ty->isVoidType())
+ Ty = StateMgr.getContext().CharTy;
+R = GetElementZeroRegion(SymReg, Ty);
}
assert((!isa(R) || !B.lookup(R)) &&
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
index 0abaa96b0ae79e..4ec119400d2a04 100644
--- a/clang/test/Analysis/asm.cpp
+++ b/clang/test/Analysis/asm.cpp
@@ -41,11 +41,12 @@ void testInlineAsmMemcpyUninit(void)
c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
}
-void *globalPtr;
-
-void testNoCrash()
+void testAsmWithVoidPtrArgument()
{
- // Use global pointer to make it symbolic. Then engine will try to bind
- // value to first element of type void * and should not crash.
- asm ("" : : "a"(globalPtr)); // no crash
+ extern void *globalVoidPtr;
+ clang_analyzer_dump(*(int *)globalVoidPtr); // expected-warning-re
{{reg_${{[0-9]+}}},0 S64b,int}>}}
+ clang_analyzer_dump_ptr(globalVoidPtr); // expected-warning-re
{{&SymRegion{reg_${{[0-9]+}
+ asm ("" : : "a"(globalVoidPtr)); // no crash
+ clang_analyzer_dump(*(int *)globalVoidPtr); // expected-warning {{Unknown}}
+ clang_analyzer_dump_ptr(globalVoidPtr); // expected-warning-re
{{&SymRegion{reg_${{[0-9]+}
}
>From 39a1411d31302c39f1ba288872311982192afcca Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Mon, 9 Sep 2024 17:43:53 +0300
Subject: [PATCH 3/4] add missing declarations
---
clang/test/Analysis/asm.cpp | 3 +++
1 file changed, 3 insertions(+)
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
inde
[clang] [analyzer] fix crash on binding to symbolic region with `void *` type (PR #107572)
@@ -3,6 +3,9 @@ int clang_analyzer_eval(int); pskrgag wrote: fixed, thanks! https://github.com/llvm/llvm-project/pull/107572 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] clang/csa: add initial support for builtin overflow (PR #102602)
https://github.com/pskrgag created
https://github.com/llvm/llvm-project/pull/102602
Add basic support for `builtin_*_overflow` primitives.
These helps a lot for checking custom calloc-like functions with inlinable
body. Without such support code like
```c
#include
#include
static void *myMalloc(size_t a1, size_t a2)
{
size_t res;
if (__builtin_smul_overflow(a1, a2, &res))
return NULL;
return malloc(res);
}
void test(void)
{
char *ptr = myMalloc(10, 1);
ptr[20] = 10;
}
does not trigger any warnings.
>From 3e0fcffa8fbea5f89ab927fd897c451bcafd8e5e Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+
[clang] [clang] [Static analyzer]: add initial support for builtin overflow (PR #102602)
https://github.com/pskrgag edited https://github.com/llvm/llvm-project/pull/102602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Static analyzer]: add initial support for builtin overflow (PR #102602)
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
pskrgag wrote:
I didn't find a way to check this. We could do old-style thing like `res < 0 ?
lhs > rhs : lhs < rhs` , but I guess, `SVB` should take care of it?
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Static analyzer]: add initial support for builtin overflow (PR #102602)
https://github.com/pskrgag edited https://github.com/llvm/llvm-project/pull/102602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [Static analyzer]: add initial support for builtin overflow (PR #102602)
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
pskrgag wrote:
Hm, I thought that CSA won't run on code with compile errors. Or there is a
mode which will try to run even on invalid AST?
I've tried to catch this assert locally by calling `__builtin_*_overflow` with
2 arguments, but clang just rejects it.
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
pskrgag wrote:
> Is it because its not always present in the declaration of the builting?
Yeah. If I try `cast(Call.getDecl())->getParamDecl(2)`, then CSA
crashes, since `Decl` for `builtin` has 0 params.
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -278,6 +278,23 @@ int *mallocRegion(void) {
return mem;
}
+int *custom_calloc(size_t a, size_t b) {
+ size_t res;
+ if (__builtin_mul_overflow(a, b, &res))
+return 0;
+
+ return malloc(res);
+}
+
+int *mallocRegionOverflow(void) {
+ int *mem = (int*)custom_calloc(4, 10);
+
+ mem[20] = 10;
pskrgag wrote:
Thanks for note! I've just copy-pasted logic from tests above =)
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/102602
>From 3e0fcffa8fbea5f89ab927fd897c451bcafd8e5e Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH 1/2] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+ StateSuccess = StateSuccess->bindLoc(*L, RetVal, C.getLocationContext());
+
+C.addTransition(StateSuccess);
+ }
+
+ // Handle overflow case.
+ {
+C.addTransition(
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
+ }
+}
+
bool BuiltinFunctionChecker::isBuiltinLikeFunction(
const CallEvent &Call) const {
const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
@@ -82,10 +171,41 @@ bool BuiltinFunctionChe
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
pskrgag wrote:
So I've pushed what I came up with for handling overflow, but during test
writing I found smth I don't understand. I've decided to push current state,
since it's easier to show code than describe it =)
My current problem is following code:
```c
if (a > 10)
return;
if (b > 10)
return;
// clang_analyzer_eval(a + b < 30); <--- Prints 1 and 0, but why ???
For some reason constraints do not work as expected. And because of that my
overflow checker splits state where it shouldn't I'd really appreciate tips
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
pskrgag wrote:
So I've pushed what I came up with for handling overflow, but during test
writing I found smth I don't understand. I've decided to push current state,
since it's easier to show code than describe it =)
My current problem is following code:
```c
if (a > 10)
return;
if (b > 10)
return;
// clang_analyzer_eval(a + b < 30); <--- Prints 1 and 0, but why ???
For some reason constraints do not work as expected. And because of that my
overflow checker splits state where it shouldn't I'd really appreciate tips
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag deleted https://github.com/llvm/llvm-project/pull/102602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag edited https://github.com/llvm/llvm-project/pull/102602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
pskrgag wrote:
... which turns out to be bad idea, since it depends on `{i,u}128` support.
Will redo with Min/Max
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag deleted https://github.com/llvm/llvm-project/pull/102602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/102602
>From 3e0fcffa8fbea5f89ab927fd897c451bcafd8e5e Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH 1/3] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+ StateSuccess = StateSuccess->bindLoc(*L, RetVal, C.getLocationContext());
+
+C.addTransition(StateSuccess);
+ }
+
+ // Handle overflow case.
+ {
+C.addTransition(
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
+ }
+}
+
bool BuiltinFunctionChecker::isBuiltinLikeFunction(
const CallEvent &Call) const {
const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
@@ -82,10 +171,41 @@ bool BuiltinFunctionChe
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/102602
>From 3e0fcffa8fbea5f89ab927fd897c451bcafd8e5e Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH 1/4] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+ StateSuccess = StateSuccess->bindLoc(*L, RetVal, C.getLocationContext());
+
+C.addTransition(StateSuccess);
+ }
+
+ // Handle overflow case.
+ {
+C.addTransition(
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
+ }
+}
+
bool BuiltinFunctionChecker::isBuiltinLikeFunction(
const CallEvent &Call) const {
const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
@@ -82,10 +171,41 @@ bool BuiltinFunctionChe
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/102602
>From 7b4f999b39f4308cab253204e6be41ea7a70f695 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH 1/4] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+ StateSuccess = StateSuccess->bindLoc(*L, RetVal, C.getLocationContext());
+
+C.addTransition(StateSuccess);
+ }
+
+ // Handle overflow case.
+ {
+C.addTransition(
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
+ }
+}
+
bool BuiltinFunctionChecker::isBuiltinLikeFunction(
const CallEvent &Call) const {
const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
@@ -82,10 +171,41 @@ bool BuiltinFunctionChe
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/102602
>From 7b4f999b39f4308cab253204e6be41ea7a70f695 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH 1/5] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+ StateSuccess = StateSuccess->bindLoc(*L, RetVal, C.getLocationContext());
+
+C.addTransition(StateSuccess);
+ }
+
+ // Handle overflow case.
+ {
+C.addTransition(
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
+ }
+}
+
bool BuiltinFunctionChecker::isBuiltinLikeFunction(
const CallEvent &Call) const {
const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
@@ -82,10 +171,41 @@ bool BuiltinFunctionChe
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
pskrgag wrote:
> Assuming that a and b are signed integers
Oh, sorry, I forgot to mention that `a` and `b` are unsigned. I've showed this
in `test_uadd_overflow_contraints` test in this pr
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/102602
>From 7b4f999b39f4308cab253204e6be41ea7a70f695 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH 1/6] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+ StateSuccess = StateSuccess->bindLoc(*L, RetVal, C.getLocationContext());
+
+C.addTransition(StateSuccess);
+ }
+
+ // Handle overflow case.
+ {
+C.addTransition(
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
+ }
+}
+
bool BuiltinFunctionChecker::isBuiltinLikeFunction(
const CallEvent &Call) const {
const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
@@ -82,10 +171,41 @@ bool BuiltinFunctionChe
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
pskrgag wrote:
Added comments to the asserts (about number of arguments and result type) and
added taint propagation
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
pskrgag wrote: > Try using the Github merge workflow to avoid doing force-pushes. Those are > destructive for inline comments done for the PR. On force-push, GH can't > follow to which line it should migrate the existing inline comments, thus > drops them. > Sorry again for fp =(. I am getting used to rebase+fp workflow, since I've never worked with gh flow. I am trying to switch my brain while developing llvm, but sometimes I mess up. https://github.com/llvm/llvm-project/pull/102602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/102602
>From 7b4f999b39f4308cab253204e6be41ea7a70f695 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH 1/7] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+ StateSuccess = StateSuccess->bindLoc(*L, RetVal, C.getLocationContext());
+
+C.addTransition(StateSuccess);
+ }
+
+ // Handle overflow case.
+ {
+C.addTransition(
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
+ }
+}
+
bool BuiltinFunctionChecker::isBuiltinLikeFunction(
const CallEvent &Call) const {
const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
@@ -82,10 +171,41 @@ bool BuiltinFunctionChe
[clang] [analyzer] Do not reason about locations passed as inline asm input (PR #103714)
https://github.com/pskrgag created
https://github.com/llvm/llvm-project/pull/103714
If pointer is passed as input operand for inline assembly, it's possible that
asm block will change memory behind this pointer. So if pointer is passed
inside inline asm block, it's better to not guess and assume memory has unknown
state.
Without such change, we observed a lot of FP with hand-written `memcpy` and
friends.
>From e528b0ded1a9815195e33d141a9e8ce05fb26cd1 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 14 Aug 2024 10:50:24 +0300
Subject: [PATCH] clang/csa: stop reasoning about pointers passed inside inline
assembly
---
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 8 +
clang/test/Analysis/asm.cpp | 36 +++-
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 686310d38ebd58..d64134102d96c7 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3807,6 +3807,14 @@ void ExprEngine::VisitGCCAsmStmt(const GCCAsmStmt *A,
ExplodedNode *Pred,
state = state->bindLoc(*LV, UnknownVal(), Pred->getLocationContext());
}
+ // Do not reason about locations passed inside inline assembly.
+ for (const Expr *O : A->inputs()) {
+SVal X = state->getSVal(O, Pred->getLocationContext());
+
+if (std::optional LV = X.getAs())
+ state = state->bindLoc(*LV, UnknownVal(), Pred->getLocationContext());
+ }
+
Bldr.generateNode(A, Pred, state);
}
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
index 1180063502168f..5d158b62e7221f 100644
--- a/clang/test/Analysis/asm.cpp
+++ b/clang/test/Analysis/asm.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker debug.ExprInspection
-fheinous-gnu-extensions -w %s -verify
+// RUN: %clang_analyze_cc1 -triple=x86_64-unknown-unknown \
+// RUN: -analyzer-checker debug.ExprInspection,core
-fheinous-gnu-extensions -w %s -verify
int clang_analyzer_eval(int);
@@ -10,3 +11,36 @@ void testRValueOutput() {
clang_analyzer_eval(global == 1); // expected-warning{{UNKNOWN}}
clang_analyzer_eval(ref == 1);// expected-warning{{UNKNOWN}}
}
+
+void *MyMemcpy(void *d, const void *s, const int n) {
+ asm volatile (
+"cld\n rep movsb\n"
+:: "S" (s), "D" (d), "c" (n) : "memory"
+ );
+
+ return d;
+}
+
+void testInlineAsmMemcpy(void)
+{
+int a, b = 10, c;
+
+MyMemcpy(&a, &b, sizeof(b));
+c = a; // no-warning
+}
+
+void testInlineAsmMemcpyArray(void)
+{
+int a[10], b[10] = {}, c;
+
+MyMemcpy(&a, &b, sizeof(b));
+c = a[8]; // no-warning
+}
+
+void testInlineAsmMemcpyUninit(void)
+{
+int a[10], b[10] = {}, c;
+
+MyMemcpy(&a[1], &b[1], sizeof(b) - sizeof(b[1]));
+c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Do not reason about locations passed as inline asm input (PR #103714)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/103714
>From e528b0ded1a9815195e33d141a9e8ce05fb26cd1 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 14 Aug 2024 10:50:24 +0300
Subject: [PATCH 1/2] clang/csa: stop reasoning about pointers passed inside
inline assembly
---
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 8 +
clang/test/Analysis/asm.cpp | 36 +++-
2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index 686310d38ebd58..d64134102d96c7 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3807,6 +3807,14 @@ void ExprEngine::VisitGCCAsmStmt(const GCCAsmStmt *A,
ExplodedNode *Pred,
state = state->bindLoc(*LV, UnknownVal(), Pred->getLocationContext());
}
+ // Do not reason about locations passed inside inline assembly.
+ for (const Expr *O : A->inputs()) {
+SVal X = state->getSVal(O, Pred->getLocationContext());
+
+if (std::optional LV = X.getAs())
+ state = state->bindLoc(*LV, UnknownVal(), Pred->getLocationContext());
+ }
+
Bldr.generateNode(A, Pred, state);
}
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
index 1180063502168f..5d158b62e7221f 100644
--- a/clang/test/Analysis/asm.cpp
+++ b/clang/test/Analysis/asm.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker debug.ExprInspection
-fheinous-gnu-extensions -w %s -verify
+// RUN: %clang_analyze_cc1 -triple=x86_64-unknown-unknown \
+// RUN: -analyzer-checker debug.ExprInspection,core
-fheinous-gnu-extensions -w %s -verify
int clang_analyzer_eval(int);
@@ -10,3 +11,36 @@ void testRValueOutput() {
clang_analyzer_eval(global == 1); // expected-warning{{UNKNOWN}}
clang_analyzer_eval(ref == 1);// expected-warning{{UNKNOWN}}
}
+
+void *MyMemcpy(void *d, const void *s, const int n) {
+ asm volatile (
+"cld\n rep movsb\n"
+:: "S" (s), "D" (d), "c" (n) : "memory"
+ );
+
+ return d;
+}
+
+void testInlineAsmMemcpy(void)
+{
+int a, b = 10, c;
+
+MyMemcpy(&a, &b, sizeof(b));
+c = a; // no-warning
+}
+
+void testInlineAsmMemcpyArray(void)
+{
+int a[10], b[10] = {}, c;
+
+MyMemcpy(&a, &b, sizeof(b));
+c = a[8]; // no-warning
+}
+
+void testInlineAsmMemcpyUninit(void)
+{
+int a[10], b[10] = {}, c;
+
+MyMemcpy(&a[1], &b[1], sizeof(b) - sizeof(b[1]));
+c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
+}
>From 70f9323c183fd1703f31e098bf3b3ac43dc7dc7d Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 14 Aug 2024 15:49:21 +0300
Subject: [PATCH 2/2] fix naming and remove newlines after declarations in test
---
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp | 4 ++--
clang/test/Analysis/asm.cpp | 4
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
index d64134102d96c7..493914f88ba9a5 100644
--- a/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ b/clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -3808,8 +3808,8 @@ void ExprEngine::VisitGCCAsmStmt(const GCCAsmStmt *A,
ExplodedNode *Pred,
}
// Do not reason about locations passed inside inline assembly.
- for (const Expr *O : A->inputs()) {
-SVal X = state->getSVal(O, Pred->getLocationContext());
+ for (const Expr *I : A->inputs()) {
+SVal X = state->getSVal(I, Pred->getLocationContext());
if (std::optional LV = X.getAs())
state = state->bindLoc(*LV, UnknownVal(), Pred->getLocationContext());
diff --git a/clang/test/Analysis/asm.cpp b/clang/test/Analysis/asm.cpp
index 5d158b62e7221f..3181aea870c8aa 100644
--- a/clang/test/Analysis/asm.cpp
+++ b/clang/test/Analysis/asm.cpp
@@ -17,14 +17,12 @@ void *MyMemcpy(void *d, const void *s, const int n) {
"cld\n rep movsb\n"
:: "S" (s), "D" (d), "c" (n) : "memory"
);
-
return d;
}
void testInlineAsmMemcpy(void)
{
int a, b = 10, c;
-
MyMemcpy(&a, &b, sizeof(b));
c = a; // no-warning
}
@@ -32,7 +30,6 @@ void testInlineAsmMemcpy(void)
void testInlineAsmMemcpyArray(void)
{
int a[10], b[10] = {}, c;
-
MyMemcpy(&a, &b, sizeof(b));
c = a[8]; // no-warning
}
@@ -40,7 +37,6 @@ void testInlineAsmMemcpyArray(void)
void testInlineAsmMemcpyUninit(void)
{
int a[10], b[10] = {}, c;
-
MyMemcpy(&a[1], &b[1], sizeof(b) - sizeof(b[1]));
c = a[0]; // expected-warning{{Assigned value is garbage or undefined}}
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Do not reason about locations passed as inline asm input (PR #103714)
pskrgag wrote: Thank you for review! Deleted newline and renamed `O` -> `I`. > It's a pitty that ProgramState::invalidateRegions accepts an Expr instead of > a Stmt and then later inside it just conjures the result of the invalidation > for a Stmt I saw that API, but thought there was a reason why it accepts `Stmt`... https://github.com/llvm/llvm-project/pull/103714 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
pskrgag wrote: Could you, please, check the lattest vestion, if you have time? https://github.com/llvm/llvm-project/pull/102602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
pskrgag wrote: Oh, sorry for ping then. PR is not urgent at all. Have a fun vacation! =) https://github.com/llvm/llvm-project/pull/102602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
https://github.com/pskrgag created
https://github.com/llvm/llvm-project/pull/104599
Current MalloChecker logic suppresses FP caused by refcounting only for C++
destructors. The same pattern occurs a lot in C in objects with intrusive
refcounting. See #104229 for code example.
To extend current logic to C, suspect all release functions as candidate for
suppression.
Closes: #104229
>From 913036ab795d6b91d6bb74d82aa2d329fe689535 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 16 Aug 2024 17:45:57 +0300
Subject: [PATCH] clang/csa: suspect all functions as those that may do
refcount release
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 57 ++---
clang/test/Analysis/malloc-refcounted.c | 80 +++
2 files changed, 110 insertions(+), 27 deletions(-)
create mode 100644 clang/test/Analysis/malloc-refcounted.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 3ddcb7e94ae5d6..77b9913a904700 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -904,16 +904,16 @@ class MallocBugVisitor final : public BugReporterVisitor {
// A symbol from when the primary region should have been reallocated.
SymbolRef FailedReallocSymbol;
- // A C++ destructor stack frame in which memory was released. Used for
+ // A release function stack frame in which memory was released. Used for
// miscellaneous false positive suppression.
- const StackFrameContext *ReleaseDestructorLC;
+ const StackFrameContext *ReleaseFunctionLC;
bool IsLeak;
public:
MallocBugVisitor(SymbolRef S, bool isLeak = false)
: Sym(S), Mode(Normal), FailedReallocSymbol(nullptr),
-ReleaseDestructorLC(nullptr), IsLeak(isLeak) {}
+ReleaseFunctionLC(nullptr), IsLeak(isLeak) {}
static void *getTag() {
static int Tag = 0;
@@ -3558,8 +3558,8 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const
ExplodedNode *N,
// original reference count is positive, we should not report use-after-frees
// on objects deleted in such destructors. This can probably be improved
// through better shared pointer modeling.
- if (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC ||
- ReleaseDestructorLC->isParentOf(CurrentLC))) {
+ if (ReleaseFunctionLC && (ReleaseFunctionLC == CurrentLC ||
+ReleaseFunctionLC->isParentOf(CurrentLC))) {
if (const auto *AE = dyn_cast(S)) {
// Check for manual use of atomic builtins.
AtomicExpr::AtomicOp Op = AE->getOp();
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-if (const auto *DD = dyn_cast(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
-// This immediately looks like a reference-counting destructor.
-// We're bad at guessing the original reference count of the
object,
-// so suppress the report for now.
-BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
-assert(!ReleaseDestructorLC &&
+// See if we're releasing memory while inlining a destructor or
+// functions that decrement reference counters (or one of its callees).
+// This turns on various common false positive suppressions.
+bool FoundAnyReleaseFunction = false;
+for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast(LC->getDecl())) {
+if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+}
+ }
+
+ if (!FoundAnyReleaseFunction) {
+assert(!ReleaseFunctionLC &&
"There can be only one release point!");
-// Suspect that it's a reference counting pointer destructor.
-// On one of the next nodes might find out that it has atomic
-// reference counting operations within it (see the code above),
-// and if so, we'd conclude that it likely is a reference counting
-// pointer destructor.
-ReleaseDestructorLC = LC->getStackFrame();
+// Suspect that it's a reference counting pointer
+// destructor/function. On one of the next
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
@@ -0,0 +1,80 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -verify %s
+//
+
+typedef unsigned long size_t;
+
+typedef enum memory_order {
+ memory_order_relaxed = __ATOMIC_RELAXED,
+} memory_order;
+
+void *calloc(size_t, size_t);
+void free(void *);
+
+struct SomeData {
+ int i;
+ _Atomic int ref;
+};
+
+static struct SomeData *alloc_data(void)
+{
+ struct SomeData *data = calloc(sizeof(*data), 1);
+
+ __c11_atomic_store(&data->ref, 2, memory_order_relaxed);
+ return data;
+}
+
+static void put_data(struct SomeData *data)
+{
+ if (__c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed) == 1)
+ free(data);
+}
+
+static int dec_refcounter(struct SomeData *data)
+{
+ return __c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed) == 1;
+}
+
+static void put_data_nested(struct SomeData *data)
+{
+ if (dec_refcounter(data))
+free(data);
+}
+
+static void put_data_uncond(struct SomeData *data)
+{
+ free(data);
+}
+
+static void put_data_unrelated_atomic(struct SomeData *data)
+{
+ free(data);
+ __c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed);
pskrgag wrote:
One thing that bothers me a bit is:
```c
static void put_data_unrelated_atomic(struct SomeData *data)
{
__c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed);
free(data);
}
```
following will be suppressed, but atomic operation is unrelated to `free()`. I
think, it's caused by `LocationContext::isParentOf` call.
https://github.com/llvm/llvm-project/pull/104599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/104599
>From 913036ab795d6b91d6bb74d82aa2d329fe689535 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 16 Aug 2024 17:45:57 +0300
Subject: [PATCH 1/2] clang/csa: suspect all functions as those that may do
refcount release
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 57 ++---
clang/test/Analysis/malloc-refcounted.c | 80 +++
2 files changed, 110 insertions(+), 27 deletions(-)
create mode 100644 clang/test/Analysis/malloc-refcounted.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 3ddcb7e94ae5d6..77b9913a904700 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -904,16 +904,16 @@ class MallocBugVisitor final : public BugReporterVisitor {
// A symbol from when the primary region should have been reallocated.
SymbolRef FailedReallocSymbol;
- // A C++ destructor stack frame in which memory was released. Used for
+ // A release function stack frame in which memory was released. Used for
// miscellaneous false positive suppression.
- const StackFrameContext *ReleaseDestructorLC;
+ const StackFrameContext *ReleaseFunctionLC;
bool IsLeak;
public:
MallocBugVisitor(SymbolRef S, bool isLeak = false)
: Sym(S), Mode(Normal), FailedReallocSymbol(nullptr),
-ReleaseDestructorLC(nullptr), IsLeak(isLeak) {}
+ReleaseFunctionLC(nullptr), IsLeak(isLeak) {}
static void *getTag() {
static int Tag = 0;
@@ -3558,8 +3558,8 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const
ExplodedNode *N,
// original reference count is positive, we should not report use-after-frees
// on objects deleted in such destructors. This can probably be improved
// through better shared pointer modeling.
- if (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC ||
- ReleaseDestructorLC->isParentOf(CurrentLC))) {
+ if (ReleaseFunctionLC && (ReleaseFunctionLC == CurrentLC ||
+ReleaseFunctionLC->isParentOf(CurrentLC))) {
if (const auto *AE = dyn_cast(S)) {
// Check for manual use of atomic builtins.
AtomicExpr::AtomicOp Op = AE->getOp();
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-if (const auto *DD = dyn_cast(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
-// This immediately looks like a reference-counting destructor.
-// We're bad at guessing the original reference count of the
object,
-// so suppress the report for now.
-BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
-assert(!ReleaseDestructorLC &&
+// See if we're releasing memory while inlining a destructor or
+// functions that decrement reference counters (or one of its callees).
+// This turns on various common false positive suppressions.
+bool FoundAnyReleaseFunction = false;
+for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast(LC->getDecl())) {
+if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+}
+ }
+
+ if (!FoundAnyReleaseFunction) {
+assert(!ReleaseFunctionLC &&
"There can be only one release point!");
-// Suspect that it's a reference counting pointer destructor.
-// On one of the next nodes might find out that it has atomic
-// reference counting operations within it (see the code above),
-// and if so, we'd conclude that it likely is a reference counting
-// pointer destructor.
-ReleaseDestructorLC = LC->getStackFrame();
+// Suspect that it's a reference counting pointer
+// destructor/function. On one of the next nodes might find out
that
+// it has atomic reference counting operations within it (see the
+// code above), and if so, we'd conclude that it likely is a
+// reference counting pointer destructor.
+ReleaseFunctionLC = LC->getStackFrame();
[clang] [clang][analyzer] Support `ownership_{returns,takes}` attributes (PR #98941)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/98941
>From c1746eec0e985bb394ecd604129cd0c30d5c66ca Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 16:41:20 +0300
Subject: [PATCH 1/9] clang/sema: disallow more than one 'onweship_takes' with
different classes
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 4
clang/lib/Sema/SemaDeclAttr.cpp | 10 ++
clang/test/Sema/attr-ownership.c | 4
3 files changed, 18 insertions(+)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b8d97a6b14fe6..bdb7fadda7743 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3331,6 +3331,10 @@ def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
"declared with index %0 here">;
+def err_ownership_takes_class_mismatch : Error<
+ "'ownership_takes' attribute class does not match; here it is '%0'">;
+def note_ownership_takes_class_mismatch : Note<
+ "declared with class '%0' here">;
def err_format_strftime_third_parameter : Error<
"strftime format attribute requires 3rd parameter to be 0">;
def err_format_attribute_not : Error<"format argument not a string type">;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 5fd8622c90dd8..39675422e3f9f 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1537,6 +1537,16 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
<< Idx.getSourceIndex() << Ex->getSourceRange();
return;
}
+ } else if (K == OwnershipAttr::Takes &&
+ I->getOwnKind() == OwnershipAttr::Takes) {
+if (I->getModule()->getName() != ModuleName) {
+ S.Diag(I->getLocation(), diag::err_ownership_takes_class_mismatch)
+ << I->getModule()->getName();
+ S.Diag(AL.getLoc(), diag::note_ownership_takes_class_mismatch)
+ << ModuleName << Ex->getSourceRange();
+
+ return;
+}
}
}
OwnershipArgs.push_back(Idx);
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 8157ba7145a24..084624353315c 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -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&R-style 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}}
>From 47793876e74c5773ae6c8464fddb95c760d84507 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 18:38:28 +0300
Subject: [PATCH 2/9] csa/MallocChecker: turn llvm_unreachable into asserts
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 22 ---
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index fe202c79ed620..8ff71318073ce 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1293,7 +1293,8 @@ void MallocChecker::checkCXXNewOrCXXDelete(const
CallEvent &Call,
AF_CXXNewArray);
break;
default:
-llvm_unreachable("not a new/delete operator");
+assert(false && "not a new/delete operator");
+return;
}
C.addTransition(State);
@@ -1489,8 +1490,10 @@ ProgramStateRef MallocChecker::ProcessZeroAllocCheck(
} else {
return State;
}
- } else
-llvm_unreachable("not a CallExpr or CXXNewExpr");
+ } else {
+assert(false && "not a CallExpr or CXXNewExpr");
+return nullptr;
+ }
assert(Arg);
@@ -1925,7 +1928,7 @@ static void printExpectedAllocName(raw_ostream &os,
AllocationFamily Family) {
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");
+case AF_None: assert(false && "not a deallocation expression");
}
}
@@ -1937,7 +1940,7 @@ static void printExpectedDeallocName(raw_ostream &os,
AllocationFamily Family) {
case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
case AF_InnerBuffer: os << "container-specific deall
[clang] [clang][analyzer] Support `ownership_{returns,takes}` attributes (PR #98941)
pskrgag wrote: Rebased on top of d89f3e8df3160b3afc07bc742c81aa4738ea9646 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][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/99564
>From 6b7ec7c95df16de5eb0fecf2d69befb5461d98a5 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Thu, 18 Jul 2024 18:48:47 +0300
Subject: [PATCH 1/4] clang/sema: disallow ownership_returns for functions that
return non-pointers
ownership_takes expects an argument to a pointer and clang reports an
error if it is not the case.
Since pointers consumed by ownership_takes are produced by functions
with ownership_returns attribute, it make sence to report an error
if function does not return a pointer type.
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
clang/lib/Sema/SemaDeclAttr.cpp | 11 +++
clang/test/AST/attr-print-emit.cpp | 4 ++--
clang/test/Sema/attr-ownership.c | 5 -
clang/test/Sema/attr-ownership.cpp | 6 +++---
5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 810abe4f23e31..9e6ea80ac4e40 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3330,6 +3330,8 @@ def err_attribute_invalid_implicit_this_argument : Error<
"%0 attribute is invalid for the implicit this argument">;
def err_ownership_type : Error<
"%0 attribute only applies to %select{pointer|integer}1 arguments">;
+def err_ownership_takes_return_type : Error<
+ "'ownership_returns' attribute only applies to functions that return
pointers">;
def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 39675422e3f9f..810f0ddfa06fd 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1481,6 +1481,17 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
break;
}
+ // Allow only pointers to be return type for functions with ownership_takes
+ // attribute. This matches with current OwnershipAttr::Takes semantics
+ if (K == OwnershipAttr::Returns) {
+QualType RetType = getFunctionOrMethodResultType(D);
+
+if (!RetType->isPointerType()) {
+ S.Diag(AL.getLoc(), diag::err_ownership_takes_return_type) << AL;
+ return;
+}
+ }
+
IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident;
StringRef ModuleName = Module->getName();
diff --git a/clang/test/AST/attr-print-emit.cpp
b/clang/test/AST/attr-print-emit.cpp
index 8c8a2b2080599..9c89764a3cac2 100644
--- a/clang/test/AST/attr-print-emit.cpp
+++ b/clang/test/AST/attr-print-emit.cpp
@@ -33,7 +33,7 @@ void ownt(int *, int *) __attribute__((ownership_takes(foo,
1, 2)));
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 1)));
-void ownr(int) __attribute__((ownership_returns(foo, 1)));
+void *ownr(int) __attribute__((ownership_returns(foo, 1)));
// CHECK: void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo,
3, 2)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 3, 2)));
@@ -66,7 +66,7 @@ class C {
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 2,
3)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 2, 3)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 2)));
- void ownr(int) __attribute__((ownership_returns(foo, 2)));
+ void *ownr(int) __attribute__((ownership_returns(foo, 2)));
// CHECK: void awtt(int, int, ...)
__attribute__((argument_with_type_tag(foo, 4, 3)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 4, 3)));
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 084624353315c..5fb28a1b4e66f 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -18,7 +18,7 @@ void *f12(float i, int k, int f, int *j)
__attribute__((ownership_returns(foo, 4
void f13(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_takes(foo, 2)));
void f14(int i, int j, int *k) __attribute__((ownership_holds(foo, 3)))
__attribute__((ownership_takes(foo, 3))); // expected-error
{{'ownership_takes' and 'ownership_holds' attributes are not compatible}}
-void f15(int, int)
+void *f15(int, int)
__attribute__((ownership_returns(foo, 1))) // expected-error
{{'ownership_returns' attribute index does not match; here it is 1}}
__attribute__((ownership_returns(foo, 2))); // expected-note {{declared with
index 2 here}}
void f16(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_holds(foo, 1))); // OK, same index
@@ -28,3 +28,6 @@ void f1
[clang] [Clang][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
pskrgag wrote: Rebased on top of 2ce865d4905970c07477a9c4e37159664a785c81 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][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/99564
>From 6b7ec7c95df16de5eb0fecf2d69befb5461d98a5 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Thu, 18 Jul 2024 18:48:47 +0300
Subject: [PATCH 1/5] clang/sema: disallow ownership_returns for functions that
return non-pointers
ownership_takes expects an argument to a pointer and clang reports an
error if it is not the case.
Since pointers consumed by ownership_takes are produced by functions
with ownership_returns attribute, it make sence to report an error
if function does not return a pointer type.
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
clang/lib/Sema/SemaDeclAttr.cpp | 11 +++
clang/test/AST/attr-print-emit.cpp | 4 ++--
clang/test/Sema/attr-ownership.c | 5 -
clang/test/Sema/attr-ownership.cpp | 6 +++---
5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 810abe4f23e31..9e6ea80ac4e40 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3330,6 +3330,8 @@ def err_attribute_invalid_implicit_this_argument : Error<
"%0 attribute is invalid for the implicit this argument">;
def err_ownership_type : Error<
"%0 attribute only applies to %select{pointer|integer}1 arguments">;
+def err_ownership_takes_return_type : Error<
+ "'ownership_returns' attribute only applies to functions that return
pointers">;
def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 39675422e3f9f..810f0ddfa06fd 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1481,6 +1481,17 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
break;
}
+ // Allow only pointers to be return type for functions with ownership_takes
+ // attribute. This matches with current OwnershipAttr::Takes semantics
+ if (K == OwnershipAttr::Returns) {
+QualType RetType = getFunctionOrMethodResultType(D);
+
+if (!RetType->isPointerType()) {
+ S.Diag(AL.getLoc(), diag::err_ownership_takes_return_type) << AL;
+ return;
+}
+ }
+
IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident;
StringRef ModuleName = Module->getName();
diff --git a/clang/test/AST/attr-print-emit.cpp
b/clang/test/AST/attr-print-emit.cpp
index 8c8a2b2080599..9c89764a3cac2 100644
--- a/clang/test/AST/attr-print-emit.cpp
+++ b/clang/test/AST/attr-print-emit.cpp
@@ -33,7 +33,7 @@ void ownt(int *, int *) __attribute__((ownership_takes(foo,
1, 2)));
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 1)));
-void ownr(int) __attribute__((ownership_returns(foo, 1)));
+void *ownr(int) __attribute__((ownership_returns(foo, 1)));
// CHECK: void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo,
3, 2)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 3, 2)));
@@ -66,7 +66,7 @@ class C {
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 2,
3)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 2, 3)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 2)));
- void ownr(int) __attribute__((ownership_returns(foo, 2)));
+ void *ownr(int) __attribute__((ownership_returns(foo, 2)));
// CHECK: void awtt(int, int, ...)
__attribute__((argument_with_type_tag(foo, 4, 3)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 4, 3)));
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 084624353315c..5fb28a1b4e66f 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -18,7 +18,7 @@ void *f12(float i, int k, int f, int *j)
__attribute__((ownership_returns(foo, 4
void f13(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_takes(foo, 2)));
void f14(int i, int j, int *k) __attribute__((ownership_holds(foo, 3)))
__attribute__((ownership_takes(foo, 3))); // expected-error
{{'ownership_takes' and 'ownership_holds' attributes are not compatible}}
-void f15(int, int)
+void *f15(int, int)
__attribute__((ownership_returns(foo, 1))) // expected-error
{{'ownership_returns' attribute index does not match; here it is 1}}
__attribute__((ownership_returns(foo, 2))); // expected-note {{declared with
index 2 here}}
void f16(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_holds(foo, 1))); // OK, same index
@@ -28,3 +28,6 @@ void f1
[clang] [Clang][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
pskrgag wrote: @AaronBallman sorry for Saturday ping, but could you, please, take a look? 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][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/99564
>From 6b7ec7c95df16de5eb0fecf2d69befb5461d98a5 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Thu, 18 Jul 2024 18:48:47 +0300
Subject: [PATCH 1/6] clang/sema: disallow ownership_returns for functions that
return non-pointers
ownership_takes expects an argument to a pointer and clang reports an
error if it is not the case.
Since pointers consumed by ownership_takes are produced by functions
with ownership_returns attribute, it make sence to report an error
if function does not return a pointer type.
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
clang/lib/Sema/SemaDeclAttr.cpp | 11 +++
clang/test/AST/attr-print-emit.cpp | 4 ++--
clang/test/Sema/attr-ownership.c | 5 -
clang/test/Sema/attr-ownership.cpp | 6 +++---
5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 810abe4f23e31..9e6ea80ac4e40 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3330,6 +3330,8 @@ def err_attribute_invalid_implicit_this_argument : Error<
"%0 attribute is invalid for the implicit this argument">;
def err_ownership_type : Error<
"%0 attribute only applies to %select{pointer|integer}1 arguments">;
+def err_ownership_takes_return_type : Error<
+ "'ownership_returns' attribute only applies to functions that return
pointers">;
def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 39675422e3f9f..810f0ddfa06fd 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1481,6 +1481,17 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
break;
}
+ // Allow only pointers to be return type for functions with ownership_takes
+ // attribute. This matches with current OwnershipAttr::Takes semantics
+ if (K == OwnershipAttr::Returns) {
+QualType RetType = getFunctionOrMethodResultType(D);
+
+if (!RetType->isPointerType()) {
+ S.Diag(AL.getLoc(), diag::err_ownership_takes_return_type) << AL;
+ return;
+}
+ }
+
IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident;
StringRef ModuleName = Module->getName();
diff --git a/clang/test/AST/attr-print-emit.cpp
b/clang/test/AST/attr-print-emit.cpp
index 8c8a2b2080599..9c89764a3cac2 100644
--- a/clang/test/AST/attr-print-emit.cpp
+++ b/clang/test/AST/attr-print-emit.cpp
@@ -33,7 +33,7 @@ void ownt(int *, int *) __attribute__((ownership_takes(foo,
1, 2)));
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 1)));
-void ownr(int) __attribute__((ownership_returns(foo, 1)));
+void *ownr(int) __attribute__((ownership_returns(foo, 1)));
// CHECK: void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo,
3, 2)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 3, 2)));
@@ -66,7 +66,7 @@ class C {
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 2,
3)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 2, 3)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 2)));
- void ownr(int) __attribute__((ownership_returns(foo, 2)));
+ void *ownr(int) __attribute__((ownership_returns(foo, 2)));
// CHECK: void awtt(int, int, ...)
__attribute__((argument_with_type_tag(foo, 4, 3)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 4, 3)));
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 084624353315c..5fb28a1b4e66f 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -18,7 +18,7 @@ void *f12(float i, int k, int f, int *j)
__attribute__((ownership_returns(foo, 4
void f13(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_takes(foo, 2)));
void f14(int i, int j, int *k) __attribute__((ownership_holds(foo, 3)))
__attribute__((ownership_takes(foo, 3))); // expected-error
{{'ownership_takes' and 'ownership_holds' attributes are not compatible}}
-void f15(int, int)
+void *f15(int, int)
__attribute__((ownership_returns(foo, 1))) // expected-error
{{'ownership_returns' attribute index does not match; here it is 1}}
__attribute__((ownership_returns(foo, 2))); // expected-note {{declared with
index 2 here}}
void f16(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_holds(foo, 1))); // OK, same index
@@ -28,3 +28,6 @@ void f1
[clang] [Clang][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/99564
>From 6b7ec7c95df16de5eb0fecf2d69befb5461d98a5 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Thu, 18 Jul 2024 18:48:47 +0300
Subject: [PATCH 1/7] clang/sema: disallow ownership_returns for functions that
return non-pointers
ownership_takes expects an argument to a pointer and clang reports an
error if it is not the case.
Since pointers consumed by ownership_takes are produced by functions
with ownership_returns attribute, it make sence to report an error
if function does not return a pointer type.
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
clang/lib/Sema/SemaDeclAttr.cpp | 11 +++
clang/test/AST/attr-print-emit.cpp | 4 ++--
clang/test/Sema/attr-ownership.c | 5 -
clang/test/Sema/attr-ownership.cpp | 6 +++---
5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 810abe4f23e31..9e6ea80ac4e40 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3330,6 +3330,8 @@ def err_attribute_invalid_implicit_this_argument : Error<
"%0 attribute is invalid for the implicit this argument">;
def err_ownership_type : Error<
"%0 attribute only applies to %select{pointer|integer}1 arguments">;
+def err_ownership_takes_return_type : Error<
+ "'ownership_returns' attribute only applies to functions that return
pointers">;
def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 39675422e3f9f..810f0ddfa06fd 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1481,6 +1481,17 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
break;
}
+ // Allow only pointers to be return type for functions with ownership_takes
+ // attribute. This matches with current OwnershipAttr::Takes semantics
+ if (K == OwnershipAttr::Returns) {
+QualType RetType = getFunctionOrMethodResultType(D);
+
+if (!RetType->isPointerType()) {
+ S.Diag(AL.getLoc(), diag::err_ownership_takes_return_type) << AL;
+ return;
+}
+ }
+
IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident;
StringRef ModuleName = Module->getName();
diff --git a/clang/test/AST/attr-print-emit.cpp
b/clang/test/AST/attr-print-emit.cpp
index 8c8a2b2080599..9c89764a3cac2 100644
--- a/clang/test/AST/attr-print-emit.cpp
+++ b/clang/test/AST/attr-print-emit.cpp
@@ -33,7 +33,7 @@ void ownt(int *, int *) __attribute__((ownership_takes(foo,
1, 2)));
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 1)));
-void ownr(int) __attribute__((ownership_returns(foo, 1)));
+void *ownr(int) __attribute__((ownership_returns(foo, 1)));
// CHECK: void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo,
3, 2)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 3, 2)));
@@ -66,7 +66,7 @@ class C {
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 2,
3)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 2, 3)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 2)));
- void ownr(int) __attribute__((ownership_returns(foo, 2)));
+ void *ownr(int) __attribute__((ownership_returns(foo, 2)));
// CHECK: void awtt(int, int, ...)
__attribute__((argument_with_type_tag(foo, 4, 3)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 4, 3)));
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 084624353315c..5fb28a1b4e66f 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -18,7 +18,7 @@ void *f12(float i, int k, int f, int *j)
__attribute__((ownership_returns(foo, 4
void f13(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_takes(foo, 2)));
void f14(int i, int j, int *k) __attribute__((ownership_holds(foo, 3)))
__attribute__((ownership_takes(foo, 3))); // expected-error
{{'ownership_takes' and 'ownership_holds' attributes are not compatible}}
-void f15(int, int)
+void *f15(int, int)
__attribute__((ownership_returns(foo, 1))) // expected-error
{{'ownership_returns' attribute index does not match; here it is 1}}
__attribute__((ownership_returns(foo, 2))); // expected-note {{declared with
index 2 here}}
void f16(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_holds(foo, 1))); // OK, same index
@@ -28,3 +28,6 @@ void f1
[clang] [Clang][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
pskrgag wrote: Applied style and spelling fixes, thank you for review! 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 SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
pskrgag wrote: Hello @steakhal, could you, please, take a look? I just found linked issue quite interesting and decided to give it a try. I am new to llvm world, so I would really appreciate your review 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 *); pskrgag wrote: I also find this quite strange, but can't say if there are any real-world examples of function that takes ownership of 2 different classes. Maybe it would be better to just disallow 2 different classes? 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,
pskrgag wrote:
Marking with `explicit` would cause compile errors for code like `family ==
AF_Smth`. We could wrap all `AF_*` with direct constructor call, but, I think,
it would be less clean. What do you think?
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 &os,
CheckerContext &C, const Expr *E) {
static void printExpectedAllocName(raw_ostream &os, 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");
pskrgag wrote:
I think, all `llvm_unreachable` s in this file could be turned in `assert` ,
since they just check sanity of calling contract. Will clean up all of them in
separate patch, thanks!
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'}}
pskrgag wrote:
"malloc" and "malloc1" here means allocation classes (i.e. argument of
`ownership_{returns,takes}`, so I decided to wrap them with quotes to make them
look different from function names.
Do you think it's worth wrapping function names as well?
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 *); pskrgag wrote: Disallowed 2 ownership_takes with different classes 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'}}
pskrgag wrote:
So I added quotes around all functions and operators. Output looks a way more
consistent. Thanks!
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); pskrgag wrote: Seems like it works fine. At least, new test case passes. I guess, clang picks the most specific declaration 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/pskrgag updated
https://github.com/llvm/llvm-project/pull/98941
>From 3d362cfb1e197713a51ce798996ff4308e7ab5aa Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 16:41:20 +0300
Subject: [PATCH 1/6] clang/sema: disallow more than one 'onweship_takes' with
different classes
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 4
clang/lib/Sema/SemaDeclAttr.cpp | 10 ++
clang/test/Sema/attr-ownership.c | 4
3 files changed, 18 insertions(+)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 0ea3677355169..77f830b3b4aee 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3328,6 +3328,10 @@ def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
"declared with index %0 here">;
+def err_ownership_takes_class_mismatch : Error<
+ "'ownership_takes' attribute class does not match; here it is '%0'">;
+def note_ownership_takes_class_mismatch : Note<
+ "declared with class '%0' here">;
def err_format_strftime_third_parameter : Error<
"strftime format attribute requires 3rd parameter to be 0">;
def err_format_attribute_not : Error<"format argument not a string type">;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 20f46c003a464..12f8f2ab8954d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1537,6 +1537,16 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
<< Idx.getSourceIndex() << Ex->getSourceRange();
return;
}
+ } else if (K == OwnershipAttr::Takes &&
+ I->getOwnKind() == OwnershipAttr::Takes) {
+if (I->getModule()->getName() != ModuleName) {
+ S.Diag(I->getLocation(), diag::err_ownership_takes_class_mismatch)
+ << I->getModule()->getName();
+ S.Diag(AL.getLoc(), diag::note_ownership_takes_class_mismatch)
+ << ModuleName << Ex->getSourceRange();
+
+ return;
+}
}
}
OwnershipArgs.push_back(Idx);
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 8157ba7145a24..084624353315c 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -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&R-style 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}}
>From 964262763a6d48410d4f83ba63d434d2824a2c87 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 18:38:28 +0300
Subject: [PATCH 2/6] csa/MallocChecker: turn llvm_unreachable into asserts
---
.../lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index fe202c79ed620..3b558aa27e341 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1293,7 +1293,7 @@ void MallocChecker::checkCXXNewOrCXXDelete(const
CallEvent &Call,
AF_CXXNewArray);
break;
default:
-llvm_unreachable("not a new/delete operator");
+assert(false && "not a new/delete operator");
}
C.addTransition(State);
@@ -1490,7 +1490,7 @@ ProgramStateRef MallocChecker::ProcessZeroAllocCheck(
return State;
}
} else
-llvm_unreachable("not a CallExpr or CXXNewExpr");
+assert(false && "not a CallExpr or CXXNewExpr");
assert(Arg);
@@ -1925,7 +1925,7 @@ static void printExpectedAllocName(raw_ostream &os,
AllocationFamily Family) {
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");
+case AF_None: assert(false && "not a deallocation expression");
}
}
@@ -1937,7 +1937,7 @@ static void printExpectedDeallocName(raw_ostream &os,
AllocationFamily Family) {
case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
case AF_InnerBuffer: os << "container-specific deallocator"; return;
case AF_Alloca:
-case AF_None: llvm_unrea
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/98941
>From 3d362cfb1e197713a51ce798996ff4308e7ab5aa Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 16:41:20 +0300
Subject: [PATCH 1/6] clang/sema: disallow more than one 'onweship_takes' with
different classes
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 4
clang/lib/Sema/SemaDeclAttr.cpp | 10 ++
clang/test/Sema/attr-ownership.c | 4
3 files changed, 18 insertions(+)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 0ea3677355169..77f830b3b4aee 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3328,6 +3328,10 @@ def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
"declared with index %0 here">;
+def err_ownership_takes_class_mismatch : Error<
+ "'ownership_takes' attribute class does not match; here it is '%0'">;
+def note_ownership_takes_class_mismatch : Note<
+ "declared with class '%0' here">;
def err_format_strftime_third_parameter : Error<
"strftime format attribute requires 3rd parameter to be 0">;
def err_format_attribute_not : Error<"format argument not a string type">;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 20f46c003a464..12f8f2ab8954d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1537,6 +1537,16 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
<< Idx.getSourceIndex() << Ex->getSourceRange();
return;
}
+ } else if (K == OwnershipAttr::Takes &&
+ I->getOwnKind() == OwnershipAttr::Takes) {
+if (I->getModule()->getName() != ModuleName) {
+ S.Diag(I->getLocation(), diag::err_ownership_takes_class_mismatch)
+ << I->getModule()->getName();
+ S.Diag(AL.getLoc(), diag::note_ownership_takes_class_mismatch)
+ << ModuleName << Ex->getSourceRange();
+
+ return;
+}
}
}
OwnershipArgs.push_back(Idx);
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 8157ba7145a24..084624353315c 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -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&R-style 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}}
>From 426b1918f354835550e853a1f875af8dc3c04617 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 18:38:28 +0300
Subject: [PATCH 2/6] csa/MallocChecker: turn llvm_unreachable into asserts
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 22 ---
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index fe202c79ed620..8ff71318073ce 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1293,7 +1293,8 @@ void MallocChecker::checkCXXNewOrCXXDelete(const
CallEvent &Call,
AF_CXXNewArray);
break;
default:
-llvm_unreachable("not a new/delete operator");
+assert(false && "not a new/delete operator");
+return;
}
C.addTransition(State);
@@ -1489,8 +1490,10 @@ ProgramStateRef MallocChecker::ProcessZeroAllocCheck(
} else {
return State;
}
- } else
-llvm_unreachable("not a CallExpr or CXXNewExpr");
+ } else {
+assert(false && "not a CallExpr or CXXNewExpr");
+return nullptr;
+ }
assert(Arg);
@@ -1925,7 +1928,7 @@ static void printExpectedAllocName(raw_ostream &os,
AllocationFamily Family) {
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");
+case AF_None: assert(false && "not a deallocation expression");
}
}
@@ -1937,7 +1940,7 @@ static void printExpectedDeallocName(raw_ostream &os,
AllocationFamily Family) {
case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
case AF_InnerBuffer: os << "container-specific deall
[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 &Call,
CheckerContext &C,
if (TrueState && !FalseState) {
SVal ZeroVal = C.getSValBuilder().makeZeroVal(Ctx.CharTy);
return MallocMemAux(C, Call, Call.getArgExpr(0), ZeroVal, TrueState,
-AF_Malloc);
+AllocationFamily(AF_Malloc));
pskrgag wrote:
I like current variant more. Just because it makes code more straightforward, I
believe
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;
+}
pskrgag wrote:
Sorry, I over-engineered things.
I redo a bit. Now callers pass `AF_Custom` with `customName` set, instead of
`AF_Malloc`
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 &Other) const {
+return std::tie(Kind, CustomName) == std::tie(Other.Kind,
Other.CustomName);
+ }
+
+ bool operator!=(const AllocationFamily &Other) const {
+return !(*this == Other);
+ }
+
+ void Profile(llvm::FoldingSetNodeID &ID) const {
+ID.AddInteger(Kind);
+
+if (Kind == AF_Custom)
+ ID.AddString(CustomName.value());
+ }
pskrgag wrote:
Why? If it's not custom class, then unwrapping `CustomName` would cause an
exception, isn't it?
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&R-style 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}}
pskrgag wrote:
Hm It does not detect. As well as for other attributes.
I think, we should fix it, but maybe all of them in scope of separate PR?
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)
pskrgag wrote: > 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. I am would be happy to give it a try, but I think we need to fix couple of things to truly follow the semantics. As you have already noticed, attributes conflicts on different declarations are not reported. Also I found out that code like ``` void f21(void) __attribute__((ownership_returns(foo))); ``` does not cause an error. I will fill an issue for these cases. Also, thank you so much for review! 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/pskrgag updated
https://github.com/llvm/llvm-project/pull/98941
>From 3d362cfb1e197713a51ce798996ff4308e7ab5aa Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 16:41:20 +0300
Subject: [PATCH 1/7] clang/sema: disallow more than one 'onweship_takes' with
different classes
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 4
clang/lib/Sema/SemaDeclAttr.cpp | 10 ++
clang/test/Sema/attr-ownership.c | 4
3 files changed, 18 insertions(+)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 0ea3677355169..77f830b3b4aee 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3328,6 +3328,10 @@ def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
"declared with index %0 here">;
+def err_ownership_takes_class_mismatch : Error<
+ "'ownership_takes' attribute class does not match; here it is '%0'">;
+def note_ownership_takes_class_mismatch : Note<
+ "declared with class '%0' here">;
def err_format_strftime_third_parameter : Error<
"strftime format attribute requires 3rd parameter to be 0">;
def err_format_attribute_not : Error<"format argument not a string type">;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 20f46c003a464..12f8f2ab8954d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1537,6 +1537,16 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
<< Idx.getSourceIndex() << Ex->getSourceRange();
return;
}
+ } else if (K == OwnershipAttr::Takes &&
+ I->getOwnKind() == OwnershipAttr::Takes) {
+if (I->getModule()->getName() != ModuleName) {
+ S.Diag(I->getLocation(), diag::err_ownership_takes_class_mismatch)
+ << I->getModule()->getName();
+ S.Diag(AL.getLoc(), diag::note_ownership_takes_class_mismatch)
+ << ModuleName << Ex->getSourceRange();
+
+ return;
+}
}
}
OwnershipArgs.push_back(Idx);
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 8157ba7145a24..084624353315c 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -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&R-style 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}}
>From 426b1918f354835550e853a1f875af8dc3c04617 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 18:38:28 +0300
Subject: [PATCH 2/7] csa/MallocChecker: turn llvm_unreachable into asserts
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 22 ---
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index fe202c79ed620..8ff71318073ce 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1293,7 +1293,8 @@ void MallocChecker::checkCXXNewOrCXXDelete(const
CallEvent &Call,
AF_CXXNewArray);
break;
default:
-llvm_unreachable("not a new/delete operator");
+assert(false && "not a new/delete operator");
+return;
}
C.addTransition(State);
@@ -1489,8 +1490,10 @@ ProgramStateRef MallocChecker::ProcessZeroAllocCheck(
} else {
return State;
}
- } else
-llvm_unreachable("not a CallExpr or CXXNewExpr");
+ } else {
+assert(false && "not a CallExpr or CXXNewExpr");
+return nullptr;
+ }
assert(Arg);
@@ -1925,7 +1928,7 @@ static void printExpectedAllocName(raw_ostream &os,
AllocationFamily Family) {
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");
+case AF_None: assert(false && "not a deallocation expression");
}
}
@@ -1937,7 +1940,7 @@ static void printExpectedDeallocName(raw_ostream &os,
AllocationFamily Family) {
case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
case AF_InnerBuffer: os << "container-specific deall
[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)
pskrgag wrote: Thank you for review! I've applied your suggestions and fixed test failures. Messed up a bit with shadowing in constructor. > 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 Oh, ok. I've never really worked with gh workflow, so didn't know that. Thank you for guidance! 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)
pskrgag 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. Do you mean that link https://clang.llvm.org/docs/AttributeReference.html#ownership-holds-ownership-returns-ownership-takes? 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)
pskrgag wrote: Added release note and added basic example to `clang/docs/analyzer/checkers/mismatched_deallocator_example.cpp` Thanks! I learned a lot about llvm workflow. 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/pskrgag updated
https://github.com/llvm/llvm-project/pull/98941
>From bb8a806c919715637e9d4877d02a8fc735c488a6 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 16:41:20 +0300
Subject: [PATCH 1/9] clang/sema: disallow more than one 'onweship_takes' with
different classes
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 4
clang/lib/Sema/SemaDeclAttr.cpp | 10 ++
clang/test/Sema/attr-ownership.c | 4
3 files changed, 18 insertions(+)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d60f32674ca3a..96a391d821f72 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3328,6 +3328,10 @@ def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
"declared with index %0 here">;
+def err_ownership_takes_class_mismatch : Error<
+ "'ownership_takes' attribute class does not match; here it is '%0'">;
+def note_ownership_takes_class_mismatch : Note<
+ "declared with class '%0' here">;
def err_format_strftime_third_parameter : Error<
"strftime format attribute requires 3rd parameter to be 0">;
def err_format_attribute_not : Error<"format argument not a string type">;
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 41295bfb3b94f..6364a58627085 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1537,6 +1537,16 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
<< Idx.getSourceIndex() << Ex->getSourceRange();
return;
}
+ } else if (K == OwnershipAttr::Takes &&
+ I->getOwnKind() == OwnershipAttr::Takes) {
+if (I->getModule()->getName() != ModuleName) {
+ S.Diag(I->getLocation(), diag::err_ownership_takes_class_mismatch)
+ << I->getModule()->getName();
+ S.Diag(AL.getLoc(), diag::note_ownership_takes_class_mismatch)
+ << ModuleName << Ex->getSourceRange();
+
+ return;
+}
}
}
OwnershipArgs.push_back(Idx);
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 8157ba7145a24..084624353315c 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -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&R-style 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}}
>From 22e8c6bbb84860cdf207dae729b0abe78254ad0d Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Wed, 17 Jul 2024 18:38:28 +0300
Subject: [PATCH 2/9] csa/MallocChecker: turn llvm_unreachable into asserts
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 22 ---
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index fe202c79ed620..8ff71318073ce 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1293,7 +1293,8 @@ void MallocChecker::checkCXXNewOrCXXDelete(const
CallEvent &Call,
AF_CXXNewArray);
break;
default:
-llvm_unreachable("not a new/delete operator");
+assert(false && "not a new/delete operator");
+return;
}
C.addTransition(State);
@@ -1489,8 +1490,10 @@ ProgramStateRef MallocChecker::ProcessZeroAllocCheck(
} else {
return State;
}
- } else
-llvm_unreachable("not a CallExpr or CXXNewExpr");
+ } else {
+assert(false && "not a CallExpr or CXXNewExpr");
+return nullptr;
+ }
assert(Arg);
@@ -1925,7 +1928,7 @@ static void printExpectedAllocName(raw_ostream &os,
AllocationFamily Family) {
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");
+case AF_None: assert(false && "not a deallocation expression");
}
}
@@ -1937,7 +1940,7 @@ static void printExpectedDeallocName(raw_ostream &os,
AllocationFamily Family) {
case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
case AF_InnerBuffer: os << "container-specific deall
[clang] [Clang][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
https://github.com/pskrgag created
https://github.com/llvm/llvm-project/pull/99564
`onwership_returns` works only with pointers, since it models user-defined
memory allocation functions. Make semantics more clear and report an error if
attribute is attached to wrong function.
Closes #99501
>From f517162e629535446f5a261b81aa11b3155de6ed Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Thu, 18 Jul 2024 18:48:47 +0300
Subject: [PATCH 1/2] clang/sema: disallow ownership_returns for functions that
return non-pointers
ownership_takes expects an argument to a pointer and clang reports an
error if it is not the case.
Since pointers consumed by ownership_takes are produced by functions
with ownership_returns attribute, it make sence to report an error
if function does not return a pointer type.
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
clang/lib/Sema/SemaDeclAttr.cpp | 11 +++
clang/test/AST/attr-print-emit.cpp | 4 ++--
clang/test/Sema/attr-ownership.c | 5 -
clang/test/Sema/attr-ownership.cpp | 6 +++---
5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d60f32674ca3a..09d19e6e4b4fc 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3324,6 +3324,8 @@ def err_attribute_invalid_implicit_this_argument : Error<
"%0 attribute is invalid for the implicit this argument">;
def err_ownership_type : Error<
"%0 attribute only applies to %select{pointer|integer}1 arguments">;
+def err_ownership_takes_return_type : Error<
+ "'ownership_returns' attribute only applies to functions that return
pointers">;
def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 41295bfb3b94f..06fd5c756a94d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1481,6 +1481,17 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
break;
}
+ // Allow only pointers to be return type for functions with ownership_takes
+ // attribute. This matches with current OwnershipAttr::Takes semantics
+ if (K == OwnershipAttr::Returns) {
+QualType RetType = getFunctionOrMethodResultType(D);
+
+if (!RetType->isPointerType()) {
+ S.Diag(AL.getLoc(), diag::err_ownership_takes_return_type) << AL;
+ return;
+}
+ }
+
IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident;
StringRef ModuleName = Module->getName();
diff --git a/clang/test/AST/attr-print-emit.cpp
b/clang/test/AST/attr-print-emit.cpp
index 8c8a2b2080599..9c89764a3cac2 100644
--- a/clang/test/AST/attr-print-emit.cpp
+++ b/clang/test/AST/attr-print-emit.cpp
@@ -33,7 +33,7 @@ void ownt(int *, int *) __attribute__((ownership_takes(foo,
1, 2)));
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 1)));
-void ownr(int) __attribute__((ownership_returns(foo, 1)));
+void *ownr(int) __attribute__((ownership_returns(foo, 1)));
// CHECK: void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo,
3, 2)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 3, 2)));
@@ -66,7 +66,7 @@ class C {
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 2,
3)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 2, 3)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 2)));
- void ownr(int) __attribute__((ownership_returns(foo, 2)));
+ void *ownr(int) __attribute__((ownership_returns(foo, 2)));
// CHECK: void awtt(int, int, ...)
__attribute__((argument_with_type_tag(foo, 4, 3)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 4, 3)));
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 8157ba7145a24..28b7cf25a21d5 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -18,9 +18,12 @@ void *f12(float i, int k, int f, int *j)
__attribute__((ownership_returns(foo, 4
void f13(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_takes(foo, 2)));
void f14(int i, int j, int *k) __attribute__((ownership_holds(foo, 3)))
__attribute__((ownership_takes(foo, 3))); // expected-error
{{'ownership_takes' and 'ownership_holds' attributes are not compatible}}
-void f15(int, int)
+void *f15(int, int)
__attribute__((ownership_returns(foo, 1))) // expected-error
{{'ownership_returns' attribute index does not match; here it is 1}}
__attribute__((ownership_returns
[clang] [Clang][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/99564
>From f517162e629535446f5a261b81aa11b3155de6ed Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Thu, 18 Jul 2024 18:48:47 +0300
Subject: [PATCH 1/4] clang/sema: disallow ownership_returns for functions that
return non-pointers
ownership_takes expects an argument to a pointer and clang reports an
error if it is not the case.
Since pointers consumed by ownership_takes are produced by functions
with ownership_returns attribute, it make sence to report an error
if function does not return a pointer type.
---
clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
clang/lib/Sema/SemaDeclAttr.cpp | 11 +++
clang/test/AST/attr-print-emit.cpp | 4 ++--
clang/test/Sema/attr-ownership.c | 5 -
clang/test/Sema/attr-ownership.cpp | 6 +++---
5 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index d60f32674ca3a..09d19e6e4b4fc 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3324,6 +3324,8 @@ def err_attribute_invalid_implicit_this_argument : Error<
"%0 attribute is invalid for the implicit this argument">;
def err_ownership_type : Error<
"%0 attribute only applies to %select{pointer|integer}1 arguments">;
+def err_ownership_takes_return_type : Error<
+ "'ownership_returns' attribute only applies to functions that return
pointers">;
def err_ownership_returns_index_mismatch : Error<
"'ownership_returns' attribute index does not match; here it is %0">;
def note_ownership_returns_index_mismatch : Note<
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 41295bfb3b94f..06fd5c756a94d 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -1481,6 +1481,17 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
break;
}
+ // Allow only pointers to be return type for functions with ownership_takes
+ // attribute. This matches with current OwnershipAttr::Takes semantics
+ if (K == OwnershipAttr::Returns) {
+QualType RetType = getFunctionOrMethodResultType(D);
+
+if (!RetType->isPointerType()) {
+ S.Diag(AL.getLoc(), diag::err_ownership_takes_return_type) << AL;
+ return;
+}
+ }
+
IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident;
StringRef ModuleName = Module->getName();
diff --git a/clang/test/AST/attr-print-emit.cpp
b/clang/test/AST/attr-print-emit.cpp
index 8c8a2b2080599..9c89764a3cac2 100644
--- a/clang/test/AST/attr-print-emit.cpp
+++ b/clang/test/AST/attr-print-emit.cpp
@@ -33,7 +33,7 @@ void ownt(int *, int *) __attribute__((ownership_takes(foo,
1, 2)));
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 1, 2)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 1)));
-void ownr(int) __attribute__((ownership_returns(foo, 1)));
+void *ownr(int) __attribute__((ownership_returns(foo, 1)));
// CHECK: void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo,
3, 2)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 3, 2)));
@@ -66,7 +66,7 @@ class C {
// CHECK: void ownh(int *, int *) __attribute__((ownership_holds(foo, 2,
3)));
void ownh(int *, int *) __attribute__((ownership_holds(foo, 2, 3)));
// CHECK: void ownr(int) __attribute__((ownership_returns(foo, 2)));
- void ownr(int) __attribute__((ownership_returns(foo, 2)));
+ void *ownr(int) __attribute__((ownership_returns(foo, 2)));
// CHECK: void awtt(int, int, ...)
__attribute__((argument_with_type_tag(foo, 4, 3)));
void awtt(int, int, ...) __attribute__((argument_with_type_tag(foo, 4, 3)));
diff --git a/clang/test/Sema/attr-ownership.c b/clang/test/Sema/attr-ownership.c
index 8157ba7145a24..28b7cf25a21d5 100644
--- a/clang/test/Sema/attr-ownership.c
+++ b/clang/test/Sema/attr-ownership.c
@@ -18,9 +18,12 @@ void *f12(float i, int k, int f, int *j)
__attribute__((ownership_returns(foo, 4
void f13(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_takes(foo, 2)));
void f14(int i, int j, int *k) __attribute__((ownership_holds(foo, 3)))
__attribute__((ownership_takes(foo, 3))); // expected-error
{{'ownership_takes' and 'ownership_holds' attributes are not compatible}}
-void f15(int, int)
+void *f15(int, int)
__attribute__((ownership_returns(foo, 1))) // expected-error
{{'ownership_returns' attribute index does not match; here it is 1}}
__attribute__((ownership_returns(foo, 2))); // expected-note {{declared with
index 2 here}}
void f16(int *i, int *j) __attribute__((ownership_holds(foo, 1)))
__attribute__((ownership_holds(foo, 1))); // OK, same index
void f17(void*) __attri
[clang] [Clang][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
@@ -1481,6 +1481,17 @@ static void handleOwnershipAttr(Sema &S, Decl *D, const
ParsedAttr &AL) {
break;
}
+ // Allow only pointers to be return type for functions with ownership_takes
+ // attribute. This matches with current OwnershipAttr::Takes semantics
+ if (K == OwnershipAttr::Returns) {
pskrgag wrote:
Yeah, typo. Thanks for spotting!
Fixed.
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][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
pskrgag wrote: Fixed test and applied suggestions. Thank you for review! 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][Sema] Disallow applying `onwership_returns` to functions that return non-pointers (PR #99564)
pskrgag wrote: I think, it would be even better to drop second argument for `ownership_returns`, since it mirrors the behavior of `alloc_size`. And also allow any return type. But I am not sure if it's possible to make such change directly. Maybe we should start with deprecation for couple of releases and eventually come to scheme where `ownership_*` annotates classes of resources, while `alloc_size` annotates only memory allocations. And users will be able to combine them as they like. If this sounds sane, I will fill up an issue. 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] [WIP] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
@@ -67,19 +67,6 @@ void testGlobalNoThrowPlacementExprNewBeforeOverload() {
int *p = new(std::nothrow) int;
} // leak-warning{{Potential leak of memory pointed to by 'p'}}
-//- Standard pointer placement operators
-void testGlobalPointerPlacementNew() {
pskrgag wrote:
Sure! This tests checks that CSA looks into `new` implementation and sees that
it just returns some constant pointer. Since PR changes this behavior, test no
longer works and CSA reports memory leaks
https://github.com/llvm/llvm-project/pull/106081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WIP] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
@@ -1736,6 +1816,25 @@ MallocChecker::MallocMemReturnsAttr(CheckerContext &C,
const CallEvent &Call,
return MallocMemAux(C, Call, UnknownVal(), UndefinedVal(), State, Family);
}
+ProgramStateRef MallocChecker::MallocBindRetval(CheckerContext &C,
+const CallEvent &Call,
+ProgramStateRef State,
+bool isAlloca) const {
+ const Expr *CE = Call.getOriginExpr();
+
+ // We expect the allocation functions to return a pointer.
+ if (!Loc::isLocType(CE->getType()))
+return nullptr;
+
+ unsigned Count = C.blockCount();
+ SValBuilder &SVB = C.getSValBuilder();
+ const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+ DefinedSVal RetVal = (isAlloca ? SVB.getAllocaRegionVal(CE, LCtx, Count)
+ : SVB.getConjuredHeapSymbolVal(CE, LCtx,
Count)
+ .castAs());
pskrgag wrote:
`getConjuredHeapSymbolVal` always returns `DefinedSVal` as far as I can see,
but there is a problem with `SValBuilder::makeZeroVal` which may return
`UnknownVal` in case of fp numbers.
So, I guess, it could be fixed on type level if we add API like
`makeZeroValNoFP`
https://github.com/llvm/llvm-project/pull/106081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WIP] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
@@ -1736,6 +1816,25 @@ MallocChecker::MallocMemReturnsAttr(CheckerContext &C,
const CallEvent &Call,
return MallocMemAux(C, Call, UnknownVal(), UndefinedVal(), State, Family);
}
+ProgramStateRef MallocChecker::MallocBindRetval(CheckerContext &C,
+const CallEvent &Call,
+ProgramStateRef State,
+bool isAlloca) const {
+ const Expr *CE = Call.getOriginExpr();
+
+ // We expect the allocation functions to return a pointer.
+ if (!Loc::isLocType(CE->getType()))
+return nullptr;
+
+ unsigned Count = C.blockCount();
+ SValBuilder &SVB = C.getSValBuilder();
+ const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+ DefinedSVal RetVal = (isAlloca ? SVB.getAllocaRegionVal(CE, LCtx, Count)
+ : SVB.getConjuredHeapSymbolVal(CE, LCtx,
Count)
+ .castAs());
pskrgag wrote:
Ignore previous comment... We can just cast `DefinedOrUnknownSVal` to
`DefinedSVal` inside `getConjuredHeapSymbolVal`, since we sure there won't be
FP number.
Will update the PR, thanks!
https://github.com/llvm/llvm-project/pull/106081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WIP] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
@@ -2815,7 +2906,7 @@ MallocChecker::ReallocMemAux(CheckerContext &C, const CallEvent &Call, // Get the from and to pointer symbols as in toPtr = realloc(fromPtr, size). SymbolRef FromPtr = arg0Val.getLocSymbolInBase(); -SVal RetVal = C.getSVal(CE); +SVal RetVal = stateRealloc->getSVal(CE, C.getLocationContext()); SymbolRef ToPtr = RetVal.getAsSymbol(); assert(FromPtr && ToPtr && "By this point, FreeMemAux and MallocMemAux should have checked " pskrgag wrote: Based on code inspection and basic tests (like passing undefined values to realloc) this assertation still holds https://github.com/llvm/llvm-project/pull/106081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WIP] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/106081
>From 82e3d871766b132d0ce0b9e8e74371d8598d2431 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Tue, 6 Aug 2024 19:12:01 +0300
Subject: [PATCH 1/4] wip
---
.../Core/PathSensitive/DynamicExtent.h| 2 +-
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 355 +++---
.../Checkers/VLASizeChecker.cpp | 3 +-
.../lib/StaticAnalyzer/Core/DynamicExtent.cpp | 2 +-
.../Core/ExprEngineCallAndReturn.cpp | 3 +-
.../test/Analysis/NewDelete-checker-test.cpp | 13 -
.../test/Analysis/NewDelete-intersections.mm | 6 +-
clang/test/Analysis/malloc-interprocedural.c | 35 --
8 files changed, 233 insertions(+), 186 deletions(-)
diff --git
a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h
b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h
index 50d5d254415af3..1a9bef06b15a44 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h
@@ -36,7 +36,7 @@ DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef
State,
/// Set the dynamic extent \p Extent of the region \p MR.
ProgramStateRef setDynamicExtent(ProgramStateRef State, const MemRegion *MR,
- DefinedOrUnknownSVal Extent, SValBuilder
&SVB);
+ DefinedOrUnknownSVal Extent);
/// Get the dynamic extent for a symbolic value that represents a buffer. If
/// there is an offsetting to the underlying buffer we consider that too.
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 3ddcb7e94ae5d6..1f524481049fa4 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -315,13 +315,24 @@ struct ReallocPair {
REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair)
-/// Tells if the callee is one of the builtin new/delete operators, including
-/// placement operators and other standard overloads.
-static bool isStandardNewDelete(const FunctionDecl *FD);
-static bool isStandardNewDelete(const CallEvent &Call) {
+static bool isStandardNew(const FunctionDecl *FD);
+static bool isStandardNew(const CallEvent &Call) {
+ if (!Call.getDecl() || !isa(Call.getDecl()))
+return false;
+ return isStandardNew(cast(Call.getDecl()));
+}
+
+static bool isStandardDelete(const FunctionDecl *FD);
+static bool isStandardDelete(const CallEvent &Call) {
if (!Call.getDecl() || !isa(Call.getDecl()))
return false;
- return isStandardNewDelete(cast(Call.getDecl()));
+ return isStandardDelete(cast(Call.getDecl()));
+}
+
+/// Tells if the callee is one of the builtin new/delete operators, including
+/// placement operators and other standard overloads.
+template static bool isStandardNewDelete(const T &FD) {
+ return isStandardDelete(FD) || isStandardNew(FD);
}
//===--===//
@@ -334,8 +345,9 @@ class MallocChecker
: public Checker,
check::EndFunction, check::PreCall, check::PostCall,
- check::NewAllocator, check::PostStmt,
- check::PostObjCMessage, check::Location, eval::Assume> {
+ eval::Call, check::NewAllocator,
+ check::PostStmt, check::PostObjCMessage,
+ check::Location, eval::Assume> {
public:
/// In pessimistic mode, the checker assumes that it does not know which
/// functions might free the memory.
@@ -367,6 +379,7 @@ class MallocChecker
void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
void checkPostCall(const CallEvent &Call, CheckerContext &C) const;
+ bool evalCall(const CallEvent &Call, CheckerContext &C) const;
void checkNewAllocator(const CXXAllocatorCall &Call, CheckerContext &C)
const;
void checkPostObjCMessage(const ObjCMethodCall &Call, CheckerContext &C)
const;
void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const;
@@ -403,7 +416,8 @@ class MallocChecker
mutable std::unique_ptr BT_TaintedAlloc;
#define CHECK_FN(NAME)
\
- void NAME(const CallEvent &Call, CheckerContext &C) const;
+ void NAME(ProgramStateRef State, const CallEvent &Call, CheckerContext &C)
\
+ const;
CHECK_FN(checkFree)
CHECK_FN(checkIfNameIndex)
@@ -423,11 +437,12 @@ class MallocChecker
CHECK_FN(checkReallocN)
CHECK_FN(checkOwnershipAttr)
- void checkRealloc(const CallEvent &Call, CheckerContext &C,
-bool ShouldFreeOnFail) const;
+ void checkRealloc(ProgramStateRef State, const CallEvent &Call,
+CheckerContext &C, bool ShouldFreeOnFail) const;
- using CheckFn = std::function;
+ using CheckFn =
+ std::f
[clang] [WIP] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
pskrgag wrote: Thank you so much for review! After invalidating location in `FreeMemAux` everything started working as it should. Also changed `getConjuredHeapSymbolVal` to return `DefinedSVal`. https://github.com/llvm/llvm-project/pull/106081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WIP] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
pskrgag wrote: Since all pipelines has successfully finished, I am changing state to normal pr https://github.com/llvm/llvm-project/pull/106081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [WIP] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
https://github.com/pskrgag ready_for_review https://github.com/llvm/llvm-project/pull/106081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
https://github.com/pskrgag edited https://github.com/llvm/llvm-project/pull/106081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
pskrgag wrote: seems patch got lost CC: @steakhal @NagyDonat https://github.com/llvm/llvm-project/pull/104599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
@@ -58,14 +60,14 @@ void testFreeOpNew() {
void *p = operator new(0);
free(p);
// mismatch-warning@-1{{Memory allocated by 'operator new' should be
deallocated by 'delete', not 'free()'}}
-} // leak-warning{{Potential leak of memory pointed to by 'p'}}
+}
pskrgag wrote:
To be clear, these `leak-warning` s were added during WIP stage of this PR and
then I dropped it after I added invalidation of freed regions.
I will anyway check if it is be possible to add them, tho
https://github.com/llvm/llvm-project/pull/106081
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-if (const auto *DD = dyn_cast(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
-// This immediately looks like a reference-counting destructor.
-// We're bad at guessing the original reference count of the
object,
-// so suppress the report for now.
-BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
-assert(!ReleaseDestructorLC &&
+// See if we're releasing memory while inlining a destructor or
+// functions that decrement reference counters (or one of its callees).
+// This turns on various common false positive suppressions.
+bool FoundAnyReleaseFunction = false;
+for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast(LC->getDecl())) {
+if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+}
+ }
+
+ if (!FoundAnyReleaseFunction) {
pskrgag wrote:
Trying to reproduce the behavior you mention, but I cannot. So I wrote
following test:
```cpp
// Also IntrusivePtr but release is happening out-of-line in desctructor
template
class IntrusivePtrOutlineRelease {
T *Ptr;
public:
IntrusivePtrOutlineRelease(T *Ptr) : Ptr(Ptr) {
Ptr->incRef();
}
IntrusivePtrOutlineRelease(const IntrusivePtrOutlineRelease &Other) :
Ptr(Other.Ptr) {
Ptr->incRef();
}
void releasePtr(void) {
delete Ptr;
}
~IntrusivePtrOutlineRelease() {
// We should not take the path on which the object is deleted.
if (Ptr->decRef() == 1)
releasePtr();
}
T *getPtr() const { return Ptr; } // no-warning
};
void testDestroyLocalRefPtrWithOutlineRelease() {
IntrusivePtrOutlineRelease p1(new RawObj());
{
IntrusivePtrOutlineRelease p2(p1);
}
// p1 still maintains ownership. The object is not deleted.
p1.getPtr()->foo(); // no-warning
}
```
And there is no warning. Did I get it wrong?
https://github.com/llvm/llvm-project/pull/104599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
https://github.com/pskrgag edited https://github.com/llvm/llvm-project/pull/104599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
pskrgag wrote: > To me, a switch from eval-call to post-call should be NFC for the most part. It is, but it causes `MallocChecker` to no longer look into body of the functions, because of `evalCall` semantics, which is breaking change. So I am not quite sure how to split this PR into two https://github.com/llvm/llvm-project/pull/106081 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-if (const auto *DD = dyn_cast(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
-// This immediately looks like a reference-counting destructor.
-// We're bad at guessing the original reference count of the
object,
-// so suppress the report for now.
-BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
-assert(!ReleaseDestructorLC &&
+// See if we're releasing memory while inlining a destructor or
+// functions that decrement reference counters (or one of its callees).
+// This turns on various common false positive suppressions.
+bool FoundAnyReleaseFunction = false;
+for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast(LC->getDecl())) {
+if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+}
+ }
+
+ if (!FoundAnyReleaseFunction) {
pskrgag wrote:
Indeed! I totally forgot about suppression logic by name.
Hm, ok then. I will try to rework the logic to avoid it. Thank you!
https://github.com/llvm/llvm-project/pull/104599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
https://github.com/pskrgag edited https://github.com/llvm/llvm-project/pull/104599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/104599
>From 913036ab795d6b91d6bb74d82aa2d329fe689535 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 16 Aug 2024 17:45:57 +0300
Subject: [PATCH 1/3] clang/csa: suspect all functions as those that may do
refcount release
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 57 ++---
clang/test/Analysis/malloc-refcounted.c | 80 +++
2 files changed, 110 insertions(+), 27 deletions(-)
create mode 100644 clang/test/Analysis/malloc-refcounted.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 3ddcb7e94ae5d6..77b9913a904700 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -904,16 +904,16 @@ class MallocBugVisitor final : public BugReporterVisitor {
// A symbol from when the primary region should have been reallocated.
SymbolRef FailedReallocSymbol;
- // A C++ destructor stack frame in which memory was released. Used for
+ // A release function stack frame in which memory was released. Used for
// miscellaneous false positive suppression.
- const StackFrameContext *ReleaseDestructorLC;
+ const StackFrameContext *ReleaseFunctionLC;
bool IsLeak;
public:
MallocBugVisitor(SymbolRef S, bool isLeak = false)
: Sym(S), Mode(Normal), FailedReallocSymbol(nullptr),
-ReleaseDestructorLC(nullptr), IsLeak(isLeak) {}
+ReleaseFunctionLC(nullptr), IsLeak(isLeak) {}
static void *getTag() {
static int Tag = 0;
@@ -3558,8 +3558,8 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const
ExplodedNode *N,
// original reference count is positive, we should not report use-after-frees
// on objects deleted in such destructors. This can probably be improved
// through better shared pointer modeling.
- if (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC ||
- ReleaseDestructorLC->isParentOf(CurrentLC))) {
+ if (ReleaseFunctionLC && (ReleaseFunctionLC == CurrentLC ||
+ReleaseFunctionLC->isParentOf(CurrentLC))) {
if (const auto *AE = dyn_cast(S)) {
// Check for manual use of atomic builtins.
AtomicExpr::AtomicOp Op = AE->getOp();
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-if (const auto *DD = dyn_cast(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
-// This immediately looks like a reference-counting destructor.
-// We're bad at guessing the original reference count of the
object,
-// so suppress the report for now.
-BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
-assert(!ReleaseDestructorLC &&
+// See if we're releasing memory while inlining a destructor or
+// functions that decrement reference counters (or one of its callees).
+// This turns on various common false positive suppressions.
+bool FoundAnyReleaseFunction = false;
+for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast(LC->getDecl())) {
+if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+}
+ }
+
+ if (!FoundAnyReleaseFunction) {
+assert(!ReleaseFunctionLC &&
"There can be only one release point!");
-// Suspect that it's a reference counting pointer destructor.
-// On one of the next nodes might find out that it has atomic
-// reference counting operations within it (see the code above),
-// and if so, we'd conclude that it likely is a reference counting
-// pointer destructor.
-ReleaseDestructorLC = LC->getStackFrame();
+// Suspect that it's a reference counting pointer
+// destructor/function. On one of the next nodes might find out
that
+// it has atomic reference counting operations within it (see the
+// code above), and if so, we'd conclude that it likely is a
+// reference counting pointer destructor.
+ReleaseFunctionLC = LC->getStackFrame();
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-if (const auto *DD = dyn_cast(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
-// This immediately looks like a reference-counting destructor.
-// We're bad at guessing the original reference count of the
object,
-// so suppress the report for now.
-BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
-assert(!ReleaseDestructorLC &&
+// See if we're releasing memory while inlining a destructor or
+// functions that decrement reference counters (or one of its callees).
+// This turns on various common false positive suppressions.
+bool FoundAnyReleaseFunction = false;
+for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast(LC->getDecl())) {
+if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+}
+ }
+
+ if (!FoundAnyReleaseFunction) {
pskrgag wrote:
So, based on my local testings:
Following does not produce warnings before and after this PR
```cpp
template class DifferentlyNamedWithInlineRef {
T *Ptr;
_Atomic int refs;
public:
DifferentlyNamedWithInlineRef(T *Ptr) : Ptr(Ptr), refs(1) {}
DifferentlyNamedWithInlineRef(const DifferentlyNamedWithInlineRef &Other)
: Ptr(Other.Ptr),
refs(__c11_atomic_fetch_add(&refs, 1, memory_order_relaxed) + 1) {}
void releasePtr() { delete Ptr; }
~DifferentlyNamedWithInlineRef() {
if (__c11_atomic_fetch_sub(&refs, 1, memory_order_relaxed) == 1)
releasePtr();
}
T *getPtr() const { return Ptr; } // no-warning
};
```
but following gives regression
```cpp
template
class DifferentlyNamedOutlineRelease {
T *Ptr;
public:
DifferentlyNamedOutlineRelease(T *Ptr) : Ptr(Ptr) {
Ptr->incRef();
}
DifferentlyNamedOutlineRelease(const DifferentlyNamedOutlineRelease &Other) :
Ptr(Other.Ptr) {
Ptr->incRef();
}
void releasePtr(void) {
delete Ptr;
}
~DifferentlyNamedOutlineRelease() {
if (Ptr->decRef() == 1)
releasePtr();
}
T *getPtr() const { return Ptr; } // Warning here
};
```
https://github.com/llvm/llvm-project/pull/104599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
pskrgag wrote: gentle ping https://github.com/llvm/llvm-project/pull/102602 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -0,0 +1,157 @@
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-unknown -verify %s \
+// RUN: -analyzer-checker=core,debug.ExprInspection
+
+#define __UINT_MAX__ (__INT_MAX__ * 2U + 1U)
+#define __INT_MIN__ (-__INT_MAX__ - 1)
+
+void clang_analyzer_dump_int(int);
+void clang_analyzer_dump_long(long);
+void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached(void);
+
+void test_add_nooverflow(void)
+{
+ int res;
+
+ if (__builtin_add_overflow(10, 20, &res)) {
+ clang_analyzer_warnIfReached();
+ return;
+ }
+
+ clang_analyzer_dump_int(res); //expected-warning{{30 S32b}}
+}
+
+void test_add_overflow(void)
+{
+ int res;
+
+ if (__builtin_add_overflow(__INT_MAX__, 1, &res)) {
+ clang_analyzer_dump_int(res); //expected-warning{{1st function call
argument is an uninitialized value}}
+ return;
+ }
+
+ clang_analyzer_warnIfReached();
+}
+
+void test_add_underoverflow(void)
+{
+ int res;
+
+ if (__builtin_add_overflow(__INT_MIN__, -1, &res)) {
+ clang_analyzer_dump_int(res); //expected-warning{{1st function call
argument is an uninitialized value}}
+ return;
+ }
+
+ clang_analyzer_warnIfReached();
+}
+
+void test_sub_underflow(void)
+{
+ int res;
+
+ if (__builtin_sub_overflow(__INT_MIN__, 10, &res)) {
+ return;
+ }
+
+ clang_analyzer_warnIfReached();
+}
+
+void test_sub_overflow(void)
+{
+ int res;
+
+ if (__builtin_sub_overflow(__INT_MAX__, -1, &res)) {
+ return;
+ }
+
+ clang_analyzer_warnIfReached();
+}
+
+void test_sub_nooverflow(void)
+{
+ int res;
+
+ if (__builtin_sub_overflow(__INT_MAX__, 1, &res)) {
+ clang_analyzer_warnIfReached();
+ return;
+ }
+
+ clang_analyzer_dump_int(res); //expected-warning{{2147483646 S32b}}
+}
+
+void test_mul_overrflow(void)
+{
+ int res;
+
+ if (__builtin_mul_overflow(__INT_MAX__, 2, &res)) {
+ return;
+ }
+
+ clang_analyzer_warnIfReached();
+}
+
+void test_mul_underrflow(void)
+{
+ int res;
+
+ if (__builtin_mul_overflow(__INT_MIN__, -2, &res)) {
+ return;
+ }
+
+ clang_analyzer_warnIfReached();
+}
+
+void test_mul_nooverflow(void)
+{
+ int res;
+
+ if (__builtin_mul_overflow(10, -2, &res)) {
+ clang_analyzer_warnIfReached();
+ return;
+ }
+
+ clang_analyzer_dump_int(res); //expected-warning{{-20 S32b}}
+}
+
+void test_nooverflow_diff_types(void)
+{
+ long res;
+
+ // This is not an overflow, since result type is long.
+ if (__builtin_add_overflow(__INT_MAX__, 1, &res)) {
+ clang_analyzer_warnIfReached();
+ return;
+ }
+
+ clang_analyzer_dump_long(res); //expected-warning{{2147483648 S64b}}
+}
+
+void test_uaddll_overflow_contraints(unsigned long a, unsigned long b)
+{
+ unsigned long long res;
+
+ if (a != 10)
+ return;
+ if (b != 10)
pskrgag wrote:
I've tried to add smth like this, but it fails even for more simple constraint
like `if (a > 10 || b > 10) return`; for unsigned case. See
https://github.com/llvm/llvm-project/pull/102602#discussion_r1712211946.
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/104599
>From 913036ab795d6b91d6bb74d82aa2d329fe689535 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 16 Aug 2024 17:45:57 +0300
Subject: [PATCH 1/4] clang/csa: suspect all functions as those that may do
refcount release
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 57 ++---
clang/test/Analysis/malloc-refcounted.c | 80 +++
2 files changed, 110 insertions(+), 27 deletions(-)
create mode 100644 clang/test/Analysis/malloc-refcounted.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 3ddcb7e94ae5d6..77b9913a904700 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -904,16 +904,16 @@ class MallocBugVisitor final : public BugReporterVisitor {
// A symbol from when the primary region should have been reallocated.
SymbolRef FailedReallocSymbol;
- // A C++ destructor stack frame in which memory was released. Used for
+ // A release function stack frame in which memory was released. Used for
// miscellaneous false positive suppression.
- const StackFrameContext *ReleaseDestructorLC;
+ const StackFrameContext *ReleaseFunctionLC;
bool IsLeak;
public:
MallocBugVisitor(SymbolRef S, bool isLeak = false)
: Sym(S), Mode(Normal), FailedReallocSymbol(nullptr),
-ReleaseDestructorLC(nullptr), IsLeak(isLeak) {}
+ReleaseFunctionLC(nullptr), IsLeak(isLeak) {}
static void *getTag() {
static int Tag = 0;
@@ -3558,8 +3558,8 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const
ExplodedNode *N,
// original reference count is positive, we should not report use-after-frees
// on objects deleted in such destructors. This can probably be improved
// through better shared pointer modeling.
- if (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC ||
- ReleaseDestructorLC->isParentOf(CurrentLC))) {
+ if (ReleaseFunctionLC && (ReleaseFunctionLC == CurrentLC ||
+ReleaseFunctionLC->isParentOf(CurrentLC))) {
if (const auto *AE = dyn_cast(S)) {
// Check for manual use of atomic builtins.
AtomicExpr::AtomicOp Op = AE->getOp();
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-if (const auto *DD = dyn_cast(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
-// This immediately looks like a reference-counting destructor.
-// We're bad at guessing the original reference count of the
object,
-// so suppress the report for now.
-BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
-assert(!ReleaseDestructorLC &&
+// See if we're releasing memory while inlining a destructor or
+// functions that decrement reference counters (or one of its callees).
+// This turns on various common false positive suppressions.
+bool FoundAnyReleaseFunction = false;
+for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast(LC->getDecl())) {
+if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+}
+ }
+
+ if (!FoundAnyReleaseFunction) {
+assert(!ReleaseFunctionLC &&
"There can be only one release point!");
-// Suspect that it's a reference counting pointer destructor.
-// On one of the next nodes might find out that it has atomic
-// reference counting operations within it (see the code above),
-// and if so, we'd conclude that it likely is a reference counting
-// pointer destructor.
-ReleaseDestructorLC = LC->getStackFrame();
+// Suspect that it's a reference counting pointer
+// destructor/function. On one of the next nodes might find out
that
+// it has atomic reference counting operations within it (see the
+// code above), and if so, we'd conclude that it likely is a
+// reference counting pointer destructor.
+ReleaseFunctionLC = LC->getStackFrame();
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -50,6 +118,75 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+std::pair
+BuiltinFunctionChecker::checkOverflow(CheckerContext &C, SVal RetVal,
+ QualType Res) const {
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ ASTContext &ACtx = C.getASTContext();
+
+ // Calling a builtin with a non-integer type result produces compiler error.
+ assert(Res->isIntegerType());
+
+ unsigned BitWidth = ACtx.getIntWidth(Res);
+ auto MinVal =
+ llvm::APSInt::getMinValue(BitWidth, Res->isUnsignedIntegerType());
+ auto MaxVal =
+ llvm::APSInt::getMaxValue(BitWidth, Res->isUnsignedIntegerType());
+
+ SVal IsLeMax =
+ SVB.evalBinOp(State, BO_LE, RetVal, nonloc::ConcreteInt(MaxVal), Res);
+ SVal IsGeMin =
+ SVB.evalBinOp(State, BO_GE, RetVal, nonloc::ConcreteInt(MinVal), Res);
+
+ auto [MayNotOverflow, MayOverflow] =
+ State->assume(IsLeMax.castAs());
+ auto [MayNotUnderflow, MayUnderflow] =
+ State->assume(IsGeMin.castAs());
+
+ return {MayOverflow || MayUnderflow, MayNotOverflow && MayNotUnderflow};
+}
+
+void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // Calling a builtin with an incorrect argument count produces compiler
error.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetValMax = SVB.evalBinOp(State, Op, Arg1, Arg2,
+ getSufficientTypeForOverflowOp(C,
ResultType));
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ auto [Overflow, NotOverflow] = checkOverflow(C, RetValMax, ResultType);
+ if (NotOverflow) {
+ProgramStateRef StateNoOverflow =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs()) {
+ StateNoOverflow =
+ StateNoOverflow->bindLoc(*L, RetVal, C.getLocationContext());
+
+ // Propagate taint if any of the argumets were tainted
+ if (isTainted(State, Arg1) || isTainted(State, Arg2))
+StateNoOverflow = addTaint(StateNoOverflow, *L);
+}
+
+C.addTransition(StateNoOverflow);
pskrgag wrote:
Sorry, I am not sure I get it correctly (I am still learning internals, so
might be dumb question).
As far as I see, `markInteresting` is called on bug reported object. And there
is no bug report object, since there is no bug here, we just do assumtions. I
tried to find an example in other checkers, but I don't see it.
(Like I see manipulations with `markInteresting` only during construction of
bug report).
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/102602
>From 7b4f999b39f4308cab253204e6be41ea7a70f695 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 9 Aug 2024 14:37:47 +0300
Subject: [PATCH 1/8] clang/csa: add initial support for builtin overflow
---
.../Checkers/BuiltinFunctionChecker.cpp | 124 +-
clang/test/Analysis/builtin_overflow.c| 65 +
.../test/Analysis/out-of-bounds-diagnostics.c | 17 +++
3 files changed, 204 insertions(+), 2 deletions(-)
create mode 100644 clang/test/Analysis/builtin_overflow.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
index b198b1c2ff4d11..0c8b9fa3d947b0 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -21,16 +21,67 @@
#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/CheckerHelpers.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
using namespace clang;
using namespace ento;
namespace {
+QualType getOverflowBuiltinResultType(const CallEvent &Call) {
+ assert(Call.getNumArgs() == 3);
+
+ return Call.getArgExpr(2)->getType()->getPointeeType();
+}
+
+QualType getOverflowBuiltinResultType(const CallEvent &Call, CheckerContext &C,
+ unsigned BI) {
+ assert(Call.getNumArgs() == 3);
+
+ ASTContext &Ast = C.getASTContext();
+
+ switch (BI) {
+ case Builtin::BI__builtin_smul_overflow:
+ case Builtin::BI__builtin_ssub_overflow:
+ case Builtin::BI__builtin_sadd_overflow:
+return Ast.IntTy;
+ case Builtin::BI__builtin_smull_overflow:
+ case Builtin::BI__builtin_ssubl_overflow:
+ case Builtin::BI__builtin_saddl_overflow:
+return Ast.LongTy;
+ case Builtin::BI__builtin_smulll_overflow:
+ case Builtin::BI__builtin_ssubll_overflow:
+ case Builtin::BI__builtin_saddll_overflow:
+return Ast.LongLongTy;
+ case Builtin::BI__builtin_umul_overflow:
+ case Builtin::BI__builtin_usub_overflow:
+ case Builtin::BI__builtin_uadd_overflow:
+return Ast.UnsignedIntTy;
+ case Builtin::BI__builtin_umull_overflow:
+ case Builtin::BI__builtin_usubl_overflow:
+ case Builtin::BI__builtin_uaddl_overflow:
+return Ast.UnsignedLongTy;
+ case Builtin::BI__builtin_umulll_overflow:
+ case Builtin::BI__builtin_usubll_overflow:
+ case Builtin::BI__builtin_uaddll_overflow:
+return Ast.UnsignedLongLongTy;
+ case Builtin::BI__builtin_mul_overflow:
+ case Builtin::BI__builtin_sub_overflow:
+ case Builtin::BI__builtin_add_overflow:
+return getOverflowBuiltinResultType(Call);
+ default:
+assert(false && "Unknown overflow builtin");
+ }
+}
+
class BuiltinFunctionChecker : public Checker {
public:
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void HandleOverflowBuiltin(const CallEvent &Call, CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const;
private:
// From: clang/include/clang/Basic/Builtins.def
@@ -50,6 +101,44 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+void BuiltinFunctionChecker::HandleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // All __builtin_*_overflow functions take 3 argumets.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ // TODO: Handle overflows with values that known to overflow. Like INT_MAX +
1
+ // should not produce state for non-overflow case and threat it as
+ // unreachable.
+
+ // Handle non-overflow case.
+ {
+ProgramStateRef StateSuccess =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs())
+ StateSuccess = StateSuccess->bindLoc(*L, RetVal, C.getLocationContext());
+
+C.addTransition(StateSuccess);
+ }
+
+ // Handle overflow case.
+ {
+C.addTransition(
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(true)));
+ }
+}
+
bool BuiltinFunctionChecker::isBuiltinLikeFunction(
const CallEvent &Call) const {
const auto *FD = llvm::dyn_cast_or_null(Call.getDecl());
@@ -82,10 +171,41 @@ bool BuiltinFunctionChe
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -50,6 +118,75 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+std::pair
+BuiltinFunctionChecker::checkOverflow(CheckerContext &C, SVal RetVal,
+ QualType Res) const {
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ ASTContext &ACtx = C.getASTContext();
+
+ // Calling a builtin with a non-integer type result produces compiler error.
+ assert(Res->isIntegerType());
+
+ unsigned BitWidth = ACtx.getIntWidth(Res);
+ auto MinVal =
+ llvm::APSInt::getMinValue(BitWidth, Res->isUnsignedIntegerType());
+ auto MaxVal =
+ llvm::APSInt::getMaxValue(BitWidth, Res->isUnsignedIntegerType());
+
+ SVal IsLeMax =
+ SVB.evalBinOp(State, BO_LE, RetVal, nonloc::ConcreteInt(MaxVal), Res);
+ SVal IsGeMin =
+ SVB.evalBinOp(State, BO_GE, RetVal, nonloc::ConcreteInt(MinVal), Res);
+
+ auto [MayNotOverflow, MayOverflow] =
+ State->assume(IsLeMax.castAs());
+ auto [MayNotUnderflow, MayUnderflow] =
+ State->assume(IsGeMin.castAs());
+
+ return {MayOverflow || MayUnderflow, MayNotOverflow && MayNotUnderflow};
+}
+
+void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // Calling a builtin with an incorrect argument count produces compiler
error.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetValMax = SVB.evalBinOp(State, Op, Arg1, Arg2,
+ getSufficientTypeForOverflowOp(C,
ResultType));
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ auto [Overflow, NotOverflow] = checkOverflow(C, RetValMax, ResultType);
+ if (NotOverflow) {
+ProgramStateRef StateNoOverflow =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs()) {
+ StateNoOverflow =
+ StateNoOverflow->bindLoc(*L, RetVal, C.getLocationContext());
+
+ // Propagate taint if any of the argumets were tainted
+ if (isTainted(State, Arg1) || isTainted(State, Arg2))
+StateNoOverflow = addTaint(StateNoOverflow, *L);
+}
+
+C.addTransition(StateNoOverflow);
pskrgag wrote:
Thank you for explanation!
Ok, so based on my local testing, if overflow happens, then result does not end
up in `InterestingSymbols` in bug reporter, so there is no need to note that
condition. I tried different tests, but anyway it does not appear in this list.
I ended up just covering non-overflow case in callback.
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] [MallocChecker] suspect all release functions as candidate for suppression (PR #104599)
https://github.com/pskrgag updated
https://github.com/llvm/llvm-project/pull/104599
>From 913036ab795d6b91d6bb74d82aa2d329fe689535 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin
Date: Fri, 16 Aug 2024 17:45:57 +0300
Subject: [PATCH 1/5] clang/csa: suspect all functions as those that may do
refcount release
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 57 ++---
clang/test/Analysis/malloc-refcounted.c | 80 +++
2 files changed, 110 insertions(+), 27 deletions(-)
create mode 100644 clang/test/Analysis/malloc-refcounted.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 3ddcb7e94ae5d6..77b9913a904700 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -904,16 +904,16 @@ class MallocBugVisitor final : public BugReporterVisitor {
// A symbol from when the primary region should have been reallocated.
SymbolRef FailedReallocSymbol;
- // A C++ destructor stack frame in which memory was released. Used for
+ // A release function stack frame in which memory was released. Used for
// miscellaneous false positive suppression.
- const StackFrameContext *ReleaseDestructorLC;
+ const StackFrameContext *ReleaseFunctionLC;
bool IsLeak;
public:
MallocBugVisitor(SymbolRef S, bool isLeak = false)
: Sym(S), Mode(Normal), FailedReallocSymbol(nullptr),
-ReleaseDestructorLC(nullptr), IsLeak(isLeak) {}
+ReleaseFunctionLC(nullptr), IsLeak(isLeak) {}
static void *getTag() {
static int Tag = 0;
@@ -3558,8 +3558,8 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const
ExplodedNode *N,
// original reference count is positive, we should not report use-after-frees
// on objects deleted in such destructors. This can probably be improved
// through better shared pointer modeling.
- if (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC ||
- ReleaseDestructorLC->isParentOf(CurrentLC))) {
+ if (ReleaseFunctionLC && (ReleaseFunctionLC == CurrentLC ||
+ReleaseFunctionLC->isParentOf(CurrentLC))) {
if (const auto *AE = dyn_cast(S)) {
// Check for manual use of atomic builtins.
AtomicExpr::AtomicOp Op = AE->getOp();
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-if (const auto *DD = dyn_cast(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
-// This immediately looks like a reference-counting destructor.
-// We're bad at guessing the original reference count of the
object,
-// so suppress the report for now.
-BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
-assert(!ReleaseDestructorLC &&
+// See if we're releasing memory while inlining a destructor or
+// functions that decrement reference counters (or one of its callees).
+// This turns on various common false positive suppressions.
+bool FoundAnyReleaseFunction = false;
+for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast(LC->getDecl())) {
+if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+ continue;
+}
+ }
+
+ if (!FoundAnyReleaseFunction) {
+assert(!ReleaseFunctionLC &&
"There can be only one release point!");
-// Suspect that it's a reference counting pointer destructor.
-// On one of the next nodes might find out that it has atomic
-// reference counting operations within it (see the code above),
-// and if so, we'd conclude that it likely is a reference counting
-// pointer destructor.
-ReleaseDestructorLC = LC->getStackFrame();
+// Suspect that it's a reference counting pointer
+// destructor/function. On one of the next nodes might find out
that
+// it has atomic reference counting operations within it (see the
+// code above), and if so, we'd conclude that it likely is a
+// reference counting pointer destructor.
+ReleaseFunctionLC = LC->getStackFrame();
[clang] [analyzer] [MallocChecker] suspect all release functions as candidate for suppression (PR #104599)
@@ -3648,35 +3652,53 @@ PathDiagnosticPieceRef
MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor
- // (or one of its callees). This turns on various common
- // false positive suppressions.
- bool FoundAnyDestructor = false;
- for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
-if (const auto *DD = dyn_cast(LC->getDecl())) {
- if (isReferenceCountingPointerDestructor(DD)) {
-// This immediately looks like a reference-counting destructor.
-// We're bad at guessing the original reference count of the
object,
-// so suppress the report for now.
-BR.markInvalid(getTag(), DD);
- } else if (!FoundAnyDestructor) {
-assert(!ReleaseDestructorLC &&
- "There can be only one release point!");
-// Suspect that it's a reference counting pointer destructor.
-// On one of the next nodes might find out that it has atomic
-// reference counting operations within it (see the code above),
-// and if so, we'd conclude that it likely is a reference counting
-// pointer destructor.
-ReleaseDestructorLC = LC->getStackFrame();
-// It is unlikely that releasing memory is delegated to a
destructor
-// inside a destructor of a shared pointer, because it's fairly
hard
-// to pass the information that the pointer indeed needs to be
-// released into it. So we're only interested in the innermost
-// destructor.
-FoundAnyDestructor = true;
+// Save the first destructor/function as release point.
+assert(!ReleaseFunctionLC && "There should be only one release point");
+ReleaseFunctionLC = CurrentLC->getStackFrame();
+
+// See if we're releasing memory while inlining a destructor that
+// decrement reference counters (or one of its callees).
+// This turns on various common false positive suppressions.
+for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (const auto *DD = dyn_cast(LC->getDecl())) {
+if (isReferenceCountingPointerDestructor(DD)) {
+ // This immediately looks like a reference-counting destructor.
+ // We're bad at guessing the original reference count of the
+ // object, so suppress the report for now.
+ BR.markInvalid(getTag(), DD);
+
+ // After report is considered invalid there is no need to proceed
+ // futher.
+ return nullptr;
+}
+
+// Switch suspection to outer destructor to catch patterns like:
+//
+// SmartPointr::~SmartPointr() {
+// if (__c11_atomic_fetch_sub(refcount, 1, memory_order_relaxed)
==
pskrgag wrote:
It will compile (with weird cast to `volatile _Atomic type *`), but I made
comment more c++-like anyway.
thanks!
https://github.com/llvm/llvm-project/pull/104599
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [analyzer] Model overflow builtins (PR #102602)
@@ -50,6 +118,75 @@ class BuiltinFunctionChecker : public Checker {
} // namespace
+std::pair
+BuiltinFunctionChecker::checkOverflow(CheckerContext &C, SVal RetVal,
+ QualType Res) const {
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ ASTContext &ACtx = C.getASTContext();
+
+ // Calling a builtin with a non-integer type result produces compiler error.
+ assert(Res->isIntegerType());
+
+ unsigned BitWidth = ACtx.getIntWidth(Res);
+ auto MinVal =
+ llvm::APSInt::getMinValue(BitWidth, Res->isUnsignedIntegerType());
+ auto MaxVal =
+ llvm::APSInt::getMaxValue(BitWidth, Res->isUnsignedIntegerType());
+
+ SVal IsLeMax =
+ SVB.evalBinOp(State, BO_LE, RetVal, nonloc::ConcreteInt(MaxVal), Res);
+ SVal IsGeMin =
+ SVB.evalBinOp(State, BO_GE, RetVal, nonloc::ConcreteInt(MinVal), Res);
+
+ auto [MayNotOverflow, MayOverflow] =
+ State->assume(IsLeMax.castAs());
+ auto [MayNotUnderflow, MayUnderflow] =
+ State->assume(IsGeMin.castAs());
+
+ return {MayOverflow || MayUnderflow, MayNotOverflow && MayNotUnderflow};
+}
+
+void BuiltinFunctionChecker::handleOverflowBuiltin(const CallEvent &Call,
+ CheckerContext &C,
+ BinaryOperator::Opcode Op,
+ QualType ResultType) const {
+ // Calling a builtin with an incorrect argument count produces compiler
error.
+ assert(Call.getNumArgs() == 3);
+
+ ProgramStateRef State = C.getState();
+ SValBuilder &SVB = C.getSValBuilder();
+ const Expr *CE = Call.getOriginExpr();
+
+ SVal Arg1 = Call.getArgSVal(0);
+ SVal Arg2 = Call.getArgSVal(1);
+
+ SVal RetValMax = SVB.evalBinOp(State, Op, Arg1, Arg2,
+ getSufficientTypeForOverflowOp(C,
ResultType));
+ SVal RetVal = SVB.evalBinOp(State, Op, Arg1, Arg2, ResultType);
+
+ auto [Overflow, NotOverflow] = checkOverflow(C, RetValMax, ResultType);
+ if (NotOverflow) {
+ProgramStateRef StateNoOverflow =
+State->BindExpr(CE, C.getLocationContext(), SVB.makeTruthVal(false));
+
+if (auto L = Call.getArgSVal(2).getAs()) {
+ StateNoOverflow =
+ StateNoOverflow->bindLoc(*L, RetVal, C.getLocationContext());
+
+ // Propagate taint if any of the argumets were tainted
+ if (isTainted(State, Arg1) || isTainted(State, Arg2))
+StateNoOverflow = addTaint(StateNoOverflow, *L);
+}
+
+C.addTransition(StateNoOverflow);
pskrgag wrote:
> Oops, I wanted to refer to the result of the operation (which is written to
> the memory area specified by the third argument), not the boolean "did
> overflow happen?"
Yes, that was clear and I implemented it in the lasted version, but anyway in
case of say
```c
int res;
if (__builtin_add_oveflow(a, b, &res))
doStmh(res); // <-- read of uninit value
```
`res` is somehow not interesting... =(
https://github.com/llvm/llvm-project/pull/102602
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
