[clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/94642
In that scalar case, the Init should initialize the auto var before use.
The Init might use uninitialized memory from other sources (e.g., heap)
but auto-init did not help us in that case because the auto-init would
have been overwritten by the Init before use.
For non-scalars e.g., classes, the Init expr might be a ctor call that
leaves uninitialized members, so we leave the auto-init there.
The motivation is to have less IR for the optimizer to reduce, which
may not happen until a fairly late pass (DSE) or may not get optimized away
in lower optimization levels like O1 (no DSE). This is ~10% less left-over
auto-init in O1 in a few examples checked.
>From 23ee93af279360dc94cc34f47f9bbef2ba40f815 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 6 Jun 2024 16:32:20 +
Subject: [PATCH] Skip auto-init on scalar vars that have a non-constant Init
and no self-ref
In that scalar case, the Init should initialize the auto var before use.
The Init might use uninitialized memory from other sources (e.g., heap)
but auto-init did not help us in that case because the auto-init would
have been overwritten by the Init before use.
For non-scalars e.g., classes, the Init expr might be a ctor call that
leaves uninitialized members, so we leave the auto-init there.
The motivation is to have less IR for the optimizer to reduce, which
may be in a fairly late pass (DSE) or may not get optimized in lower
optimization levels like O1 (no DSe). This is ~10% less left-over
auto-init in O1 in a few examples checked.
---
clang/lib/CodeGen/CGDecl.cpp | 15 ++-
.../CodeGenCXX/auto-var-init-max-size.cpp | 2 +-
.../CodeGenCXX/auto-var-init-stop-after.cpp | 2 +-
clang/test/CodeGenCXX/auto-var-init.cpp | 8 --
...ar-init-skip-scalar-with-nonconst-init.cpp | 112 ++
5 files changed, 128 insertions(+), 11 deletions(-)
create mode 100644
clang/test/CodeGenCXX/trivial-auto-var-init-skip-scalar-with-nonconst-init.cpp
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 4a213990d1e36..49e97a23cb0a9 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -1972,7 +1972,20 @@ void CodeGenFunction::EmitAutoVarInit(const
AutoVarEmission &emission) {
}
if (!constant) {
-initializeWhatIsTechnicallyUninitialized(Loc);
+if (trivialAutoVarInit !=
+LangOptions::TrivialAutoVarInitKind::Uninitialized) {
+ // At this point, we know D has an Init expression, but isn't a constant.
+ // - If D is not a scalar, auto-var-init conservatively (members may be
+ // left uninitialized by constructor Init expressions for example).
+ // - If D is a scalar, we only need to auto-var-init if there is a
+ // self-reference. Otherwise, the Init expression should be sufficient.
+ // It may be that the Init expression uses other uninitialized memory,
+ // but auto-var-init here would not help, as auto-init would get
+ // overwritten by Init.
+ if (!D.getType()->isScalarType() || isAccessedBy(D, Init)) {
+initializeWhatIsTechnicallyUninitialized(Loc);
+ }
+}
LValue lv = MakeAddrLValue(Loc, type);
lv.setNonGC(true);
return EmitExprAsInit(Init, &D, lv, capturedByInit);
diff --git a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
index ef38b8227a9a1..f4db297a07be8 100644
--- a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
@@ -15,7 +15,7 @@ struct Foo {
int foo(unsigned n) {
bool var_size_1;
- long var_size_8 = 123;
+ long var_size_8;
void *var_size_8p;
int var_size_1024[256];
Foo var_size_1028;
diff --git a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
index a782692d0127e..f1dc0e3a068e7 100644
--- a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
@@ -18,7 +18,7 @@ typedef struct {
int foo(unsigned n) {
// scalar variable
- long a = 888;
+ long a;
// array
S arr[ARRLEN];
// VLA
diff --git a/clang/test/CodeGenCXX/auto-var-init.cpp
b/clang/test/CodeGenCXX/auto-var-init.cpp
index e1568bee136e5..e697731b0cdf1 100644
--- a/clang/test/CodeGenCXX/auto-var-init.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init.cpp
@@ -146,16 +146,8 @@ struct notlockfree { long long a[4]; };
// PATTERN-O1-NOT: @__const.test_atomictailpad_uninit.uninit
// PATTERN-O0: @__const.test_complexfloat_uninit.uninit = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
// PATTERN-O1-NOT: @__const.test_complexfloat_uninit.uninit
-// PATTERN-O0: @__const.test_complexfloat_braces.braces = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
[clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/94642
>From 23ee93af279360dc94cc34f47f9bbef2ba40f815 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 6 Jun 2024 16:32:20 +
Subject: [PATCH 1/2] Skip auto-init on scalar vars that have a non-constant
Init and no self-ref
In that scalar case, the Init should initialize the auto var before use.
The Init might use uninitialized memory from other sources (e.g., heap)
but auto-init did not help us in that case because the auto-init would
have been overwritten by the Init before use.
For non-scalars e.g., classes, the Init expr might be a ctor call that
leaves uninitialized members, so we leave the auto-init there.
The motivation is to have less IR for the optimizer to reduce, which
may be in a fairly late pass (DSE) or may not get optimized in lower
optimization levels like O1 (no DSe). This is ~10% less left-over
auto-init in O1 in a few examples checked.
---
clang/lib/CodeGen/CGDecl.cpp | 15 ++-
.../CodeGenCXX/auto-var-init-max-size.cpp | 2 +-
.../CodeGenCXX/auto-var-init-stop-after.cpp | 2 +-
clang/test/CodeGenCXX/auto-var-init.cpp | 8 --
...ar-init-skip-scalar-with-nonconst-init.cpp | 112 ++
5 files changed, 128 insertions(+), 11 deletions(-)
create mode 100644
clang/test/CodeGenCXX/trivial-auto-var-init-skip-scalar-with-nonconst-init.cpp
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 4a213990d1e36..49e97a23cb0a9 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -1972,7 +1972,20 @@ void CodeGenFunction::EmitAutoVarInit(const
AutoVarEmission &emission) {
}
if (!constant) {
-initializeWhatIsTechnicallyUninitialized(Loc);
+if (trivialAutoVarInit !=
+LangOptions::TrivialAutoVarInitKind::Uninitialized) {
+ // At this point, we know D has an Init expression, but isn't a constant.
+ // - If D is not a scalar, auto-var-init conservatively (members may be
+ // left uninitialized by constructor Init expressions for example).
+ // - If D is a scalar, we only need to auto-var-init if there is a
+ // self-reference. Otherwise, the Init expression should be sufficient.
+ // It may be that the Init expression uses other uninitialized memory,
+ // but auto-var-init here would not help, as auto-init would get
+ // overwritten by Init.
+ if (!D.getType()->isScalarType() || isAccessedBy(D, Init)) {
+initializeWhatIsTechnicallyUninitialized(Loc);
+ }
+}
LValue lv = MakeAddrLValue(Loc, type);
lv.setNonGC(true);
return EmitExprAsInit(Init, &D, lv, capturedByInit);
diff --git a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
index ef38b8227a9a1..f4db297a07be8 100644
--- a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
@@ -15,7 +15,7 @@ struct Foo {
int foo(unsigned n) {
bool var_size_1;
- long var_size_8 = 123;
+ long var_size_8;
void *var_size_8p;
int var_size_1024[256];
Foo var_size_1028;
diff --git a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
index a782692d0127e..f1dc0e3a068e7 100644
--- a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
@@ -18,7 +18,7 @@ typedef struct {
int foo(unsigned n) {
// scalar variable
- long a = 888;
+ long a;
// array
S arr[ARRLEN];
// VLA
diff --git a/clang/test/CodeGenCXX/auto-var-init.cpp
b/clang/test/CodeGenCXX/auto-var-init.cpp
index e1568bee136e5..e697731b0cdf1 100644
--- a/clang/test/CodeGenCXX/auto-var-init.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init.cpp
@@ -146,16 +146,8 @@ struct notlockfree { long long a[4]; };
// PATTERN-O1-NOT: @__const.test_atomictailpad_uninit.uninit
// PATTERN-O0: @__const.test_complexfloat_uninit.uninit = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
// PATTERN-O1-NOT: @__const.test_complexfloat_uninit.uninit
-// PATTERN-O0: @__const.test_complexfloat_braces.braces = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
-// PATTERN-O1-NOT: @__const.test_complexfloat_braces.braces
-// PATTERN-O0: @__const.test_complexfloat_custom.custom = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
-// PATTERN-O1-NOT: @__const.test_complexfloat_custom.custom
// PATTERN-O0: @__const.test_complexdouble_uninit.uninit = private
unnamed_addr constant { double, double } { double 0x, double
0x }, align 8
// PATTERN-O1-NOT: @__const.test_complexdouble_uninit.uninit
-// PATTERN-O0: @__const.test_complexdouble_braces.braces = private
unnamed_addr constant { double, double } { double 0x
[clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/94642 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung ready_for_review https://github.com/llvm/llvm-project/pull/94642 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/94642 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
jvoung wrote: +cc @dwblaikie @aeubanks https://github.com/llvm/llvm-project/pull/94642 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/94642
>From 23ee93af279360dc94cc34f47f9bbef2ba40f815 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 6 Jun 2024 16:32:20 +
Subject: [PATCH 1/3] Skip auto-init on scalar vars that have a non-constant
Init and no self-ref
In that scalar case, the Init should initialize the auto var before use.
The Init might use uninitialized memory from other sources (e.g., heap)
but auto-init did not help us in that case because the auto-init would
have been overwritten by the Init before use.
For non-scalars e.g., classes, the Init expr might be a ctor call that
leaves uninitialized members, so we leave the auto-init there.
The motivation is to have less IR for the optimizer to reduce, which
may be in a fairly late pass (DSE) or may not get optimized in lower
optimization levels like O1 (no DSe). This is ~10% less left-over
auto-init in O1 in a few examples checked.
---
clang/lib/CodeGen/CGDecl.cpp | 15 ++-
.../CodeGenCXX/auto-var-init-max-size.cpp | 2 +-
.../CodeGenCXX/auto-var-init-stop-after.cpp | 2 +-
clang/test/CodeGenCXX/auto-var-init.cpp | 8 --
...ar-init-skip-scalar-with-nonconst-init.cpp | 112 ++
5 files changed, 128 insertions(+), 11 deletions(-)
create mode 100644
clang/test/CodeGenCXX/trivial-auto-var-init-skip-scalar-with-nonconst-init.cpp
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 4a213990d1e36..49e97a23cb0a9 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -1972,7 +1972,20 @@ void CodeGenFunction::EmitAutoVarInit(const
AutoVarEmission &emission) {
}
if (!constant) {
-initializeWhatIsTechnicallyUninitialized(Loc);
+if (trivialAutoVarInit !=
+LangOptions::TrivialAutoVarInitKind::Uninitialized) {
+ // At this point, we know D has an Init expression, but isn't a constant.
+ // - If D is not a scalar, auto-var-init conservatively (members may be
+ // left uninitialized by constructor Init expressions for example).
+ // - If D is a scalar, we only need to auto-var-init if there is a
+ // self-reference. Otherwise, the Init expression should be sufficient.
+ // It may be that the Init expression uses other uninitialized memory,
+ // but auto-var-init here would not help, as auto-init would get
+ // overwritten by Init.
+ if (!D.getType()->isScalarType() || isAccessedBy(D, Init)) {
+initializeWhatIsTechnicallyUninitialized(Loc);
+ }
+}
LValue lv = MakeAddrLValue(Loc, type);
lv.setNonGC(true);
return EmitExprAsInit(Init, &D, lv, capturedByInit);
diff --git a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
index ef38b8227a9a1..f4db297a07be8 100644
--- a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
@@ -15,7 +15,7 @@ struct Foo {
int foo(unsigned n) {
bool var_size_1;
- long var_size_8 = 123;
+ long var_size_8;
void *var_size_8p;
int var_size_1024[256];
Foo var_size_1028;
diff --git a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
index a782692d0127e..f1dc0e3a068e7 100644
--- a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
@@ -18,7 +18,7 @@ typedef struct {
int foo(unsigned n) {
// scalar variable
- long a = 888;
+ long a;
// array
S arr[ARRLEN];
// VLA
diff --git a/clang/test/CodeGenCXX/auto-var-init.cpp
b/clang/test/CodeGenCXX/auto-var-init.cpp
index e1568bee136e5..e697731b0cdf1 100644
--- a/clang/test/CodeGenCXX/auto-var-init.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init.cpp
@@ -146,16 +146,8 @@ struct notlockfree { long long a[4]; };
// PATTERN-O1-NOT: @__const.test_atomictailpad_uninit.uninit
// PATTERN-O0: @__const.test_complexfloat_uninit.uninit = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
// PATTERN-O1-NOT: @__const.test_complexfloat_uninit.uninit
-// PATTERN-O0: @__const.test_complexfloat_braces.braces = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
-// PATTERN-O1-NOT: @__const.test_complexfloat_braces.braces
-// PATTERN-O0: @__const.test_complexfloat_custom.custom = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
-// PATTERN-O1-NOT: @__const.test_complexfloat_custom.custom
// PATTERN-O0: @__const.test_complexdouble_uninit.uninit = private
unnamed_addr constant { double, double } { double 0x, double
0x }, align 8
// PATTERN-O1-NOT: @__const.test_complexdouble_uninit.uninit
-// PATTERN-O0: @__const.test_complexdouble_braces.braces = private
unnamed_addr constant { double, double } { double 0x
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
@@ -15,7 +15,7 @@ struct Foo {
int foo(unsigned n) {
bool var_size_1;
- long var_size_8 = 123;
jvoung wrote:
Sure! Added those cases.
https://github.com/llvm/llvm-project/pull/94642
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
jvoung wrote: > Drive-by comment: can you please either merge the two commits together, or > change their descriptions so that they are not 100% identical? Oops, changed the later commit description! https://github.com/llvm/llvm-project/pull/94642 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
@@ -0,0 +1,110 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -emit-llvm -o - |
FileCheck %s -check-prefix=UNINIT
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown
-ftrivial-auto-var-init=pattern %s -emit-llvm -o - | FileCheck %s
-check-prefix=PATTERN
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=zero
%s -emit-llvm -o - | FileCheck %s -check-prefix=ZERO
+
+template void used(T &) noexcept;
+
+extern "C" {
+
+extern int get_int(int) noexcept;
+struct C {
+ int x;
+ int y;
+};
+extern C make_c() noexcept;
+
+// Scalar with a self-reference: does need auto-init.
+// UNINIT-LABEL: test_selfinit_call(
+// ZERO-LABEL:test_selfinit_call(
+// ZERO: store i32 0, ptr %self, align 4, !annotation [[AUTO_INIT:!.+]]
+// PATTERN-LABEL: test_selfinit_call(
+// PATTERN: store i32 -1431655766, ptr %self, align 4, !annotation
[[AUTO_INIT:!.+]]
+void test_selfinit_call() {
+ int self = get_int(self);
+ used(self);
+}
+
+// Scalar without a self-reference: no auto-init needed.
+// UNINIT-LABEL: test_nonself_call(
+// ZERO-LABEL:test_nonself_call(
+// ZERO-NOT: !annotation [[AUTO_INIT:!.+]]
+// PATTERN-LABEL: test_nonself_call(
+// PATTERN-NOT: !annotation [[AUTO_INIT:!.+]]
+void test_nonself_call() {
+ int x = get_int(2);
+ used(x);
+}
+
+// Scalar with a self-reference: does need auto-init.
+// UNINIT-LABEL: test_selfinit_lambda_call(
+// ZERO-LABEL:test_selfinit_lambda_call(
jvoung wrote:
I'm not sure if a common "CHECK-LABEL:" would have the same semantics and help
split the stream for matching when there is a different prefix like "ZERO: "
(or "PATTERN:") that follows, or if the CHECK and the ZERO end up as a
independent scans. If it's an independent scan, then "CHECK-LABEL" wouldn't
have the same semantics as "ZERO-LABEL:" (which helps split the stream for the
"ZERO:" checks that follow -- e.g., in case there a multiple places that could
match something generic like "store i32 0, ", the preceding ZERO-LABEL help
disambiguate which to match).
I'd prefer to keep the redundancy just to be safe and more clear that the
X-LABEL are for the X that it's associated with, unless you're pretty sure it
can cross over different check prefixes.
https://github.com/llvm/llvm-project/pull/94642
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/94642
>From 23ee93af279360dc94cc34f47f9bbef2ba40f815 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 6 Jun 2024 16:32:20 +
Subject: [PATCH 1/4] Skip auto-init on scalar vars that have a non-constant
Init and no self-ref
In that scalar case, the Init should initialize the auto var before use.
The Init might use uninitialized memory from other sources (e.g., heap)
but auto-init did not help us in that case because the auto-init would
have been overwritten by the Init before use.
For non-scalars e.g., classes, the Init expr might be a ctor call that
leaves uninitialized members, so we leave the auto-init there.
The motivation is to have less IR for the optimizer to reduce, which
may be in a fairly late pass (DSE) or may not get optimized in lower
optimization levels like O1 (no DSe). This is ~10% less left-over
auto-init in O1 in a few examples checked.
---
clang/lib/CodeGen/CGDecl.cpp | 15 ++-
.../CodeGenCXX/auto-var-init-max-size.cpp | 2 +-
.../CodeGenCXX/auto-var-init-stop-after.cpp | 2 +-
clang/test/CodeGenCXX/auto-var-init.cpp | 8 --
...ar-init-skip-scalar-with-nonconst-init.cpp | 112 ++
5 files changed, 128 insertions(+), 11 deletions(-)
create mode 100644
clang/test/CodeGenCXX/trivial-auto-var-init-skip-scalar-with-nonconst-init.cpp
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 4a213990d1e36..49e97a23cb0a9 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -1972,7 +1972,20 @@ void CodeGenFunction::EmitAutoVarInit(const
AutoVarEmission &emission) {
}
if (!constant) {
-initializeWhatIsTechnicallyUninitialized(Loc);
+if (trivialAutoVarInit !=
+LangOptions::TrivialAutoVarInitKind::Uninitialized) {
+ // At this point, we know D has an Init expression, but isn't a constant.
+ // - If D is not a scalar, auto-var-init conservatively (members may be
+ // left uninitialized by constructor Init expressions for example).
+ // - If D is a scalar, we only need to auto-var-init if there is a
+ // self-reference. Otherwise, the Init expression should be sufficient.
+ // It may be that the Init expression uses other uninitialized memory,
+ // but auto-var-init here would not help, as auto-init would get
+ // overwritten by Init.
+ if (!D.getType()->isScalarType() || isAccessedBy(D, Init)) {
+initializeWhatIsTechnicallyUninitialized(Loc);
+ }
+}
LValue lv = MakeAddrLValue(Loc, type);
lv.setNonGC(true);
return EmitExprAsInit(Init, &D, lv, capturedByInit);
diff --git a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
index ef38b8227a9a1..f4db297a07be8 100644
--- a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
@@ -15,7 +15,7 @@ struct Foo {
int foo(unsigned n) {
bool var_size_1;
- long var_size_8 = 123;
+ long var_size_8;
void *var_size_8p;
int var_size_1024[256];
Foo var_size_1028;
diff --git a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
index a782692d0127e..f1dc0e3a068e7 100644
--- a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
@@ -18,7 +18,7 @@ typedef struct {
int foo(unsigned n) {
// scalar variable
- long a = 888;
+ long a;
// array
S arr[ARRLEN];
// VLA
diff --git a/clang/test/CodeGenCXX/auto-var-init.cpp
b/clang/test/CodeGenCXX/auto-var-init.cpp
index e1568bee136e5..e697731b0cdf1 100644
--- a/clang/test/CodeGenCXX/auto-var-init.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init.cpp
@@ -146,16 +146,8 @@ struct notlockfree { long long a[4]; };
// PATTERN-O1-NOT: @__const.test_atomictailpad_uninit.uninit
// PATTERN-O0: @__const.test_complexfloat_uninit.uninit = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
// PATTERN-O1-NOT: @__const.test_complexfloat_uninit.uninit
-// PATTERN-O0: @__const.test_complexfloat_braces.braces = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
-// PATTERN-O1-NOT: @__const.test_complexfloat_braces.braces
-// PATTERN-O0: @__const.test_complexfloat_custom.custom = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
-// PATTERN-O1-NOT: @__const.test_complexfloat_custom.custom
// PATTERN-O0: @__const.test_complexdouble_uninit.uninit = private
unnamed_addr constant { double, double } { double 0x, double
0x }, align 8
// PATTERN-O1-NOT: @__const.test_complexdouble_uninit.uninit
-// PATTERN-O0: @__const.test_complexdouble_braces.braces = private
unnamed_addr constant { double, double } { double 0x
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
@@ -1972,7 +1972,20 @@ void CodeGenFunction::EmitAutoVarInit(const
AutoVarEmission &emission) {
}
if (!constant) {
-initializeWhatIsTechnicallyUninitialized(Loc);
+if (trivialAutoVarInit !=
+LangOptions::TrivialAutoVarInitKind::Uninitialized) {
+ // At this point, we know D has an Init expression, but isn't a constant.
+ // - If D is not a scalar, auto-var-init conservatively (members may be
+ // left uninitialized by constructor Init expressions for example).
+ // - If D is a scalar, we only need to auto-var-init if there is a
+ // self-reference. Otherwise, the Init expression should be sufficient.
+ // It may be that the Init expression uses other uninitialized memory,
+ // but auto-var-init here would not help, as auto-init would get
+ // overwritten by Init.
+ if (!D.getType()->isScalarType() || isAccessedBy(D, Init)) {
jvoung wrote:
Great question, thank you for reviewing!
I tried that example and it does appear to get auto-init on the "calls" var
(`isAccessedBy(D, Init)` is true at the same time that `capturedByInit` is
true).
There are some existing test cases that are similar and were passing:
https://github.com/llvm/llvm-project/blob/0c0281130ed51fea06cf20a2db37c5bca257ad31/clang/test/CodeGenCXX/trivial-auto-var-init.cpp#L33
However, perhaps it's safer to just check `capturedByInit || isAccessedBy...`.
WDYT? The capturedByInit could short-circuit in limited cases to avoid a second
recursive pass over the `Init` too.
Also added another test case for stmt exprs (see below).
===
Looking at the implementation of `isCapturedBy`
https://github.com/llvm/llvm-project/blob/0c0281130ed51fea06cf20a2db37c5bca257ad31/clang/lib/CodeGen/CGDecl.cpp#L1699
and `isAccessedBy`:
https://github.com/llvm/llvm-project/blob/0c0281130ed51fea06cf20a2db37c5bca257ad31/clang/lib/CodeGen/CGDecl.cpp#L685
They are similar (both check BlockDecl captures), but do have some slight
differences:
- isCapturedBy checks StmtExprs for VarDecl Inits that might recursively capture
- isAccessedBy checks DeclRefExprs while isCapturedBy does not
https://github.com/llvm/llvm-project/pull/94642
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/94642 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/94642
>From 23ee93af279360dc94cc34f47f9bbef2ba40f815 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 6 Jun 2024 16:32:20 +
Subject: [PATCH 1/4] Skip auto-init on scalar vars that have a non-constant
Init and no self-ref
In that scalar case, the Init should initialize the auto var before use.
The Init might use uninitialized memory from other sources (e.g., heap)
but auto-init did not help us in that case because the auto-init would
have been overwritten by the Init before use.
For non-scalars e.g., classes, the Init expr might be a ctor call that
leaves uninitialized members, so we leave the auto-init there.
The motivation is to have less IR for the optimizer to reduce, which
may be in a fairly late pass (DSE) or may not get optimized in lower
optimization levels like O1 (no DSe). This is ~10% less left-over
auto-init in O1 in a few examples checked.
---
clang/lib/CodeGen/CGDecl.cpp | 15 ++-
.../CodeGenCXX/auto-var-init-max-size.cpp | 2 +-
.../CodeGenCXX/auto-var-init-stop-after.cpp | 2 +-
clang/test/CodeGenCXX/auto-var-init.cpp | 8 --
...ar-init-skip-scalar-with-nonconst-init.cpp | 112 ++
5 files changed, 128 insertions(+), 11 deletions(-)
create mode 100644
clang/test/CodeGenCXX/trivial-auto-var-init-skip-scalar-with-nonconst-init.cpp
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 4a213990d1e36..49e97a23cb0a9 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -1972,7 +1972,20 @@ void CodeGenFunction::EmitAutoVarInit(const
AutoVarEmission &emission) {
}
if (!constant) {
-initializeWhatIsTechnicallyUninitialized(Loc);
+if (trivialAutoVarInit !=
+LangOptions::TrivialAutoVarInitKind::Uninitialized) {
+ // At this point, we know D has an Init expression, but isn't a constant.
+ // - If D is not a scalar, auto-var-init conservatively (members may be
+ // left uninitialized by constructor Init expressions for example).
+ // - If D is a scalar, we only need to auto-var-init if there is a
+ // self-reference. Otherwise, the Init expression should be sufficient.
+ // It may be that the Init expression uses other uninitialized memory,
+ // but auto-var-init here would not help, as auto-init would get
+ // overwritten by Init.
+ if (!D.getType()->isScalarType() || isAccessedBy(D, Init)) {
+initializeWhatIsTechnicallyUninitialized(Loc);
+ }
+}
LValue lv = MakeAddrLValue(Loc, type);
lv.setNonGC(true);
return EmitExprAsInit(Init, &D, lv, capturedByInit);
diff --git a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
index ef38b8227a9a1..f4db297a07be8 100644
--- a/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-max-size.cpp
@@ -15,7 +15,7 @@ struct Foo {
int foo(unsigned n) {
bool var_size_1;
- long var_size_8 = 123;
+ long var_size_8;
void *var_size_8p;
int var_size_1024[256];
Foo var_size_1028;
diff --git a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
index a782692d0127e..f1dc0e3a068e7 100644
--- a/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
@@ -18,7 +18,7 @@ typedef struct {
int foo(unsigned n) {
// scalar variable
- long a = 888;
+ long a;
// array
S arr[ARRLEN];
// VLA
diff --git a/clang/test/CodeGenCXX/auto-var-init.cpp
b/clang/test/CodeGenCXX/auto-var-init.cpp
index e1568bee136e5..e697731b0cdf1 100644
--- a/clang/test/CodeGenCXX/auto-var-init.cpp
+++ b/clang/test/CodeGenCXX/auto-var-init.cpp
@@ -146,16 +146,8 @@ struct notlockfree { long long a[4]; };
// PATTERN-O1-NOT: @__const.test_atomictailpad_uninit.uninit
// PATTERN-O0: @__const.test_complexfloat_uninit.uninit = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
// PATTERN-O1-NOT: @__const.test_complexfloat_uninit.uninit
-// PATTERN-O0: @__const.test_complexfloat_braces.braces = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
-// PATTERN-O1-NOT: @__const.test_complexfloat_braces.braces
-// PATTERN-O0: @__const.test_complexfloat_custom.custom = private unnamed_addr
constant { float, float } { float 0xE000, float 0xE000
}, align 4
-// PATTERN-O1-NOT: @__const.test_complexfloat_custom.custom
// PATTERN-O0: @__const.test_complexdouble_uninit.uninit = private
unnamed_addr constant { double, double } { double 0x, double
0x }, align 8
// PATTERN-O1-NOT: @__const.test_complexdouble_uninit.uninit
-// PATTERN-O0: @__const.test_complexdouble_braces.braces = private
unnamed_addr constant { double, double } { double 0x
[clang] [llvm] [LTO] Reduce memory usage for import lists (PR #106772)
https://github.com/jvoung approved this pull request. Nice! LG https://github.com/llvm/llvm-project/pull/106772 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [LTO] Reduce memory usage for import lists (PR #106772)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/106772 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [LTO] Reduce memory usage for import lists (PR #106772)
@@ -174,51 +174,33 @@ std::string llvm::computeLTOCacheKey(
for (auto GUID : ExportsGUID)
Hasher.update(ArrayRef((uint8_t *)&GUID, sizeof(GUID)));
- // Include the hash for every module we import functions from. The set of
- // imported symbols for each module may affect code generation and is
- // sensitive to link order, so include that as well.
- using ImportMapIteratorTy =
- FunctionImporter::ImportMapTy::ImportMapTyImpl::const_iterator;
- struct ImportModule {
-ImportMapIteratorTy ModIt;
-const ModuleSummaryIndex::ModuleInfo *ModInfo;
-
-StringRef getIdentifier() const { return ModIt->getFirst(); }
-const FunctionImporter::FunctionsToImportTy &getFunctions() const {
- return ModIt->second;
-}
-
-const ModuleHash &getHash() const { return ModInfo->second; }
- };
-
- std::vector ImportModulesVector;
- ImportModulesVector.reserve(ImportList.getImportMap().size());
-
- for (ImportMapIteratorTy It = ImportList.getImportMap().begin();
- It != ImportList.getImportMap().end(); ++It) {
-ImportModulesVector.push_back({It, Index.getModule(It->getFirst())});
- }
// Order using module hash, to be both independent of module name and
// module order.
- llvm::sort(ImportModulesVector,
- [](const ImportModule &Lhs, const ImportModule &Rhs) -> bool {
- return Lhs.getHash() < Rhs.getHash();
- });
- std::vector> ImportedGUIDs;
- for (const ImportModule &Entry : ImportModulesVector) {
-auto ModHash = Entry.getHash();
-Hasher.update(ArrayRef((uint8_t *)&ModHash[0], sizeof(ModHash)));
-
-AddUint64(Entry.getFunctions().size());
-
-ImportedGUIDs.clear();
-for (auto &[Fn, ImportType] : Entry.getFunctions())
- ImportedGUIDs.push_back(std::make_pair(Fn, ImportType));
-llvm::sort(ImportedGUIDs);
-for (auto &[GUID, Type] : ImportedGUIDs) {
- AddUint64(GUID);
- AddUint8(Type);
+ auto Comp = [&](const std::pair &L,
+ const std::pair &R) {
+return std::make_pair(Index.getModule(L.first)->second, L.second) <
jvoung wrote:
It seems like there are more Index.getModule() lookups, but maybe that is fast
enough.
https://github.com/llvm/llvm-project/pull/106772
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Skip auto-init on scalar vars that have a non-constant Init and no self-ref (PR #94642)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/94642 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
jvoung wrote: Friendly ping =) https://github.com/llvm/llvm-project/pull/111006 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/111006
>From f82e63e470f704f29f4c161579fd592c27301868 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 3 Oct 2024 15:21:32 +
Subject: [PATCH 1/4] [clang][dataflow] Add a lattice to help represent cache
const accessor methods
By caching const accessor methods we can sometimes treat method call
results as stable (e.g., for issue
https://github.com/llvm/llvm-project/issues/58510).
Users can clear the cache when a non-const method is called that may
modify the state of an object.
This is represented as mixin.
It will be used in a follow on patch to change
bugprone-unchecked-optional-access's lattice from
NoopLattice to CachedConstAccessorsLattice,
along with some additional transfer functions.
---
.../CachedConstAccessorsLattice.h | 221 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 217 +
3 files changed, 439 insertions(+)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
create mode 100644
clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
new file mode 100644
index 00..b8517fabd1f550
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -0,0 +1,221 @@
+//===-- CachedConstAccessorsLattice.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const
method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible
for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+/// const std::optional& getFoo() const;
+/// void clear();
+/// };
+//
+/// void func(Bar& s) {
+/// if (s.getFoo().has_value()) {
+/// use(s.getFoo().value()); // safe (checked earlier getFoo())
+/// s.clear();
+/// use(s.getFoo().value()); // unsafe (invalidate cache for s)
+/// }
+/// }
+template
+class CachedConstAccessorsLattice : public Base {
+public:
+ using Base::Base; // inherit all constructors
+
+ /// Creates or returns a previously created `Value` associated with a const
+ /// method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a value (not a reference or record type)
+ Value *
+ getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+const CallExpr *CE, Environment &Env);
+
+ /// Creates or returns a previously created `StorageLocation` associated a
+ /// const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a location (GLValue or a record type).
+ StorageLocation *getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const CallExpr *CE,
+ Environment &Env,
+ llvm::function_ref Initialize);
+
+ void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnValues.erase(&RecordLoc);
+ }
+
+ void clearConstMethodReturnStorageLocations(
+ const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnStorageLocations.erase(&RecordLoc);
+ }
+
+ bool op
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/111006 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (PR #112605)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/112605
>From 57a913c8870b338fa127f323be65fda972d65a96 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 16 Oct 2024 19:38:45 +
Subject: [PATCH 1/3] [clang][dataflow] Cache accessors for
bugprone-unchecked-optional-access
Treat calls to zero-param const methods as having stable return values
(with a cache) to address issue #58510. The cache is invalidated when
non-const methods are called. This uses the infrastructure from PR #111006.
For now we cache methods returning:
- ref to optional
- optional by value
- booleans
We can extend that to pointers to optional in a next change.
---
.../bugprone/unchecked-optional-access.rst| 10 +
.../Models/UncheckedOptionalAccessModel.h | 17 +-
.../Models/UncheckedOptionalAccessModel.cpp | 137 +-
.../UncheckedOptionalAccessModelTest.cpp | 177 +-
4 files changed, 330 insertions(+), 11 deletions(-)
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
index 97fe37b5353560..815b5cdeeebe24 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
+++
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
@@ -76,6 +76,16 @@ For example:
}
}
+Exception: accessor methods
+```
+
+The check assumes *accessor* methods of a class are stable, with a heuristic to
+determine which methods are accessors. Specifically, parameter-free ``const``
+methods are treated as accessors. Note that this is not guaranteed to be safe
+-- but, it is widely used (safely) in practice, and so we have chosen to treat
+it as generally safe. Calls to non ``const`` methods are assumed to modify
+the state of the object and affect the stability of earlier accessor calls.
+
Rely on invariants of uncommon APIs
---
diff --git
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 09eb8b93822612..9d81cacb507351 100644
---
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -17,6 +17,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/NoopLattice.h"
@@ -39,23 +40,28 @@ struct UncheckedOptionalAccessModelOptions {
bool IgnoreSmartPointerDereference = false;
};
+using UncheckedOptionalAccessLattice =
CachedConstAccessorsLattice;
+
/// Dataflow analysis that models whether optionals hold values or not.
///
/// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
class UncheckedOptionalAccessModel
-: public DataflowAnalysis {
+: public DataflowAnalysis {
public:
UncheckedOptionalAccessModel(ASTContext &Ctx, dataflow::Environment &Env);
/// Returns a matcher for the optional classes covered by this model.
static ast_matchers::DeclarationMatcher optionalClassDecl();
- static NoopLattice initialElement() { return {}; }
+ static UncheckedOptionalAccessLattice initialElement() { return {}; }
- void transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env);
+ void transfer(const CFGElement &Elt, UncheckedOptionalAccessLattice &L,
+Environment &Env);
private:
- CFGMatchSwitch> TransferMatchSwitch;
+ CFGMatchSwitch>
+ TransferMatchSwitch;
};
class UncheckedOptionalAccessDiagnoser {
@@ -65,7 +71,8 @@ class UncheckedOptionalAccessDiagnoser {
llvm::SmallVector
operator()(const CFGElement &Elt, ASTContext &Ctx,
- const TransferStateForDiagnostics &State) {
+ const TransferStateForDiagnostics
+ &State) {
return DiagnoseMatchSwitch(Elt, Ctx, State.Env);
}
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 70ffe92753e053..563fe6a6625d44 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -17,13 +17,14 @@
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/ASTMatchers/ASTMatchersMacros.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
#include
[clang] [clang-tools-extra] [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (PR #112605)
@@ -523,6 +544,99 @@ void transferCallReturningOptional(const CallExpr *E,
setHasValue(*Loc, State.Env.makeAtomicBoolValue(), State.Env);
}
+void handleConstMemberCall(const CallExpr *CE,
+ dataflow::RecordStorageLocation *RecordLoc,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ // If the const method returns an optional or reference to an optional.
+ if (RecordLoc != nullptr && isSupportedOptionalType(CE->getType())) {
+StorageLocation *Loc =
+State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+*RecordLoc, CE, State.Env, [&](StorageLocation &Loc) {
+ setHasValue(cast(Loc),
+ State.Env.makeAtomicBoolValue(), State.Env);
+});
+if (Loc == nullptr)
+ return;
+if (CE->isGLValue()) {
+ // If the call to the const method returns a reference to an optional,
+ // link the call expression to the cached StorageLocation.
+ State.Env.setStorageLocation(*CE, *Loc);
+} else {
+ // If the call to the const method returns an optional by value, we
+ // need to use CopyRecord to link the optional to the result object
+ // of the call expression.
+ auto &ResultLoc = State.Env.getResultObjectLocation(*CE);
+ copyRecord(*cast(Loc), ResultLoc, State.Env);
+}
+return;
+ }
+
+ // Cache if the const method returns a boolean type.
+ // We may decide to cache other return types in the future.
+ if (RecordLoc != nullptr && CE->getType()->isBooleanType()) {
+Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc,
CE,
+ State.Env);
+if (Val == nullptr)
+ return;
+State.Env.setValue(*CE, *Val);
+return;
+ }
+
+ // Perform default handling if the call returns an optional
+ // but wasn't handled above (if RecordLoc is nullptr).
+ if (isSupportedOptionalType(CE->getType())) {
+transferCallReturningOptional(CE, Result, State);
+ }
+}
+
+void transferValue_ConstMemberCall(const CXXMemberCallExpr *MCE,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ handleConstMemberCall(
+ MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result,
State);
+}
+
+void handleNonConstMemberCall(const CallExpr *CE,
+ dataflow::RecordStorageLocation *RecordLoc,
+ const MatchFinder::MatchResult &Result,
+ LatticeTransferState &State) {
+ // When a non-const member function is called, reset some state.
+ if (RecordLoc != nullptr) {
+for (const auto [Field, FieldLoc] : RecordLoc->children()) {
+ if (isSupportedOptionalType(Field->getType())) {
+auto *FieldRecordLoc = cast_or_null(FieldLoc);
+if (FieldRecordLoc) {
+ setHasValue(*FieldRecordLoc, State.Env.makeAtomicBoolValue(),
+ State.Env);
+}
+ }
+}
jvoung wrote:
Yes this is new and independent of the accessors feature (overlap is handling
non-const method calls now).
Sounds good! Added a test.
https://github.com/llvm/llvm-project/pull/112605
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (PR #112605)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/112605
>From 57a913c8870b338fa127f323be65fda972d65a96 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 16 Oct 2024 19:38:45 +
Subject: [PATCH 1/4] [clang][dataflow] Cache accessors for
bugprone-unchecked-optional-access
Treat calls to zero-param const methods as having stable return values
(with a cache) to address issue #58510. The cache is invalidated when
non-const methods are called. This uses the infrastructure from PR #111006.
For now we cache methods returning:
- ref to optional
- optional by value
- booleans
We can extend that to pointers to optional in a next change.
---
.../bugprone/unchecked-optional-access.rst| 10 +
.../Models/UncheckedOptionalAccessModel.h | 17 +-
.../Models/UncheckedOptionalAccessModel.cpp | 137 +-
.../UncheckedOptionalAccessModelTest.cpp | 177 +-
4 files changed, 330 insertions(+), 11 deletions(-)
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
index 97fe37b5353560..815b5cdeeebe24 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
+++
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
@@ -76,6 +76,16 @@ For example:
}
}
+Exception: accessor methods
+```
+
+The check assumes *accessor* methods of a class are stable, with a heuristic to
+determine which methods are accessors. Specifically, parameter-free ``const``
+methods are treated as accessors. Note that this is not guaranteed to be safe
+-- but, it is widely used (safely) in practice, and so we have chosen to treat
+it as generally safe. Calls to non ``const`` methods are assumed to modify
+the state of the object and affect the stability of earlier accessor calls.
+
Rely on invariants of uncommon APIs
---
diff --git
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 09eb8b93822612..9d81cacb507351 100644
---
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -17,6 +17,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/NoopLattice.h"
@@ -39,23 +40,28 @@ struct UncheckedOptionalAccessModelOptions {
bool IgnoreSmartPointerDereference = false;
};
+using UncheckedOptionalAccessLattice =
CachedConstAccessorsLattice;
+
/// Dataflow analysis that models whether optionals hold values or not.
///
/// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
class UncheckedOptionalAccessModel
-: public DataflowAnalysis {
+: public DataflowAnalysis {
public:
UncheckedOptionalAccessModel(ASTContext &Ctx, dataflow::Environment &Env);
/// Returns a matcher for the optional classes covered by this model.
static ast_matchers::DeclarationMatcher optionalClassDecl();
- static NoopLattice initialElement() { return {}; }
+ static UncheckedOptionalAccessLattice initialElement() { return {}; }
- void transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env);
+ void transfer(const CFGElement &Elt, UncheckedOptionalAccessLattice &L,
+Environment &Env);
private:
- CFGMatchSwitch> TransferMatchSwitch;
+ CFGMatchSwitch>
+ TransferMatchSwitch;
};
class UncheckedOptionalAccessDiagnoser {
@@ -65,7 +71,8 @@ class UncheckedOptionalAccessDiagnoser {
llvm::SmallVector
operator()(const CFGElement &Elt, ASTContext &Ctx,
- const TransferStateForDiagnostics &State) {
+ const TransferStateForDiagnostics
+ &State) {
return DiagnoseMatchSwitch(Elt, Ctx, State.Env);
}
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 70ffe92753e053..563fe6a6625d44 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -17,13 +17,14 @@
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/ASTMatchers/ASTMatchersMacros.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
#include
[clang] [clang-tools-extra] [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (PR #112605)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/112605
>From 57a913c8870b338fa127f323be65fda972d65a96 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 16 Oct 2024 19:38:45 +
Subject: [PATCH 1/2] [clang][dataflow] Cache accessors for
bugprone-unchecked-optional-access
Treat calls to zero-param const methods as having stable return values
(with a cache) to address issue #58510. The cache is invalidated when
non-const methods are called. This uses the infrastructure from PR #111006.
For now we cache methods returning:
- ref to optional
- optional by value
- booleans
We can extend that to pointers to optional in a next change.
---
.../bugprone/unchecked-optional-access.rst| 10 +
.../Models/UncheckedOptionalAccessModel.h | 17 +-
.../Models/UncheckedOptionalAccessModel.cpp | 137 +-
.../UncheckedOptionalAccessModelTest.cpp | 177 +-
4 files changed, 330 insertions(+), 11 deletions(-)
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
index 97fe37b5353560..815b5cdeeebe24 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
+++
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
@@ -76,6 +76,16 @@ For example:
}
}
+Exception: accessor methods
+```
+
+The check assumes *accessor* methods of a class are stable, with a heuristic to
+determine which methods are accessors. Specifically, parameter-free ``const``
+methods are treated as accessors. Note that this is not guaranteed to be safe
+-- but, it is widely used (safely) in practice, and so we have chosen to treat
+it as generally safe. Calls to non ``const`` methods are assumed to modify
+the state of the object and affect the stability of earlier accessor calls.
+
Rely on invariants of uncommon APIs
---
diff --git
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 09eb8b93822612..9d81cacb507351 100644
---
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -17,6 +17,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/NoopLattice.h"
@@ -39,23 +40,28 @@ struct UncheckedOptionalAccessModelOptions {
bool IgnoreSmartPointerDereference = false;
};
+using UncheckedOptionalAccessLattice =
CachedConstAccessorsLattice;
+
/// Dataflow analysis that models whether optionals hold values or not.
///
/// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
class UncheckedOptionalAccessModel
-: public DataflowAnalysis {
+: public DataflowAnalysis {
public:
UncheckedOptionalAccessModel(ASTContext &Ctx, dataflow::Environment &Env);
/// Returns a matcher for the optional classes covered by this model.
static ast_matchers::DeclarationMatcher optionalClassDecl();
- static NoopLattice initialElement() { return {}; }
+ static UncheckedOptionalAccessLattice initialElement() { return {}; }
- void transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env);
+ void transfer(const CFGElement &Elt, UncheckedOptionalAccessLattice &L,
+Environment &Env);
private:
- CFGMatchSwitch> TransferMatchSwitch;
+ CFGMatchSwitch>
+ TransferMatchSwitch;
};
class UncheckedOptionalAccessDiagnoser {
@@ -65,7 +71,8 @@ class UncheckedOptionalAccessDiagnoser {
llvm::SmallVector
operator()(const CFGElement &Elt, ASTContext &Ctx,
- const TransferStateForDiagnostics &State) {
+ const TransferStateForDiagnostics
+ &State) {
return DiagnoseMatchSwitch(Elt, Ctx, State.Env);
}
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 70ffe92753e053..563fe6a6625d44 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -17,13 +17,14 @@
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/ASTMatchers/ASTMatchersMacros.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
#include
[clang] [clang-tools-extra] [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (PR #112605)
https://github.com/jvoung ready_for_review https://github.com/llvm/llvm-project/pull/112605 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (PR #112605)
https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/112605
Treat calls to zero-param const methods as having stable return values
(with a cache) to address issue #58510. The cache is invalidated when
non-const methods are called. This uses the infrastructure from PR #111006.
For now we cache methods returning:
- ref to optional
- optional by value
- booleans
We can extend that to pointers to optional in a next change.
>From 57a913c8870b338fa127f323be65fda972d65a96 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 16 Oct 2024 19:38:45 +
Subject: [PATCH] [clang][dataflow] Cache accessors for
bugprone-unchecked-optional-access
Treat calls to zero-param const methods as having stable return values
(with a cache) to address issue #58510. The cache is invalidated when
non-const methods are called. This uses the infrastructure from PR #111006.
For now we cache methods returning:
- ref to optional
- optional by value
- booleans
We can extend that to pointers to optional in a next change.
---
.../bugprone/unchecked-optional-access.rst| 10 +
.../Models/UncheckedOptionalAccessModel.h | 17 +-
.../Models/UncheckedOptionalAccessModel.cpp | 137 +-
.../UncheckedOptionalAccessModelTest.cpp | 177 +-
4 files changed, 330 insertions(+), 11 deletions(-)
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
index 97fe37b5353560..815b5cdeeebe24 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
+++
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
@@ -76,6 +76,16 @@ For example:
}
}
+Exception: accessor methods
+```
+
+The check assumes *accessor* methods of a class are stable, with a heuristic to
+determine which methods are accessors. Specifically, parameter-free ``const``
+methods are treated as accessors. Note that this is not guaranteed to be safe
+-- but, it is widely used (safely) in practice, and so we have chosen to treat
+it as generally safe. Calls to non ``const`` methods are assumed to modify
+the state of the object and affect the stability of earlier accessor calls.
+
Rely on invariants of uncommon APIs
---
diff --git
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 09eb8b93822612..9d81cacb507351 100644
---
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -17,6 +17,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/NoopLattice.h"
@@ -39,23 +40,28 @@ struct UncheckedOptionalAccessModelOptions {
bool IgnoreSmartPointerDereference = false;
};
+using UncheckedOptionalAccessLattice =
CachedConstAccessorsLattice;
+
/// Dataflow analysis that models whether optionals hold values or not.
///
/// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
class UncheckedOptionalAccessModel
-: public DataflowAnalysis {
+: public DataflowAnalysis {
public:
UncheckedOptionalAccessModel(ASTContext &Ctx, dataflow::Environment &Env);
/// Returns a matcher for the optional classes covered by this model.
static ast_matchers::DeclarationMatcher optionalClassDecl();
- static NoopLattice initialElement() { return {}; }
+ static UncheckedOptionalAccessLattice initialElement() { return {}; }
- void transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env);
+ void transfer(const CFGElement &Elt, UncheckedOptionalAccessLattice &L,
+Environment &Env);
private:
- CFGMatchSwitch> TransferMatchSwitch;
+ CFGMatchSwitch>
+ TransferMatchSwitch;
};
class UncheckedOptionalAccessDiagnoser {
@@ -65,7 +71,8 @@ class UncheckedOptionalAccessDiagnoser {
llvm::SmallVector
operator()(const CFGElement &Elt, ASTContext &Ctx,
- const TransferStateForDiagnostics &State) {
+ const TransferStateForDiagnostics
+ &State) {
return DiagnoseMatchSwitch(Elt, Ctx, State.Env);
}
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 70ffe92753e053..563fe6a6625d44 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/Unch
[clang] [clang][dataflow] Add a test demonstrating an issue in unchecked-optional-access-check (PR #110870)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/110870 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (PR #112605)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/112605
>From 57a913c8870b338fa127f323be65fda972d65a96 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 16 Oct 2024 19:38:45 +
Subject: [PATCH 1/4] [clang][dataflow] Cache accessors for
bugprone-unchecked-optional-access
Treat calls to zero-param const methods as having stable return values
(with a cache) to address issue #58510. The cache is invalidated when
non-const methods are called. This uses the infrastructure from PR #111006.
For now we cache methods returning:
- ref to optional
- optional by value
- booleans
We can extend that to pointers to optional in a next change.
---
.../bugprone/unchecked-optional-access.rst| 10 +
.../Models/UncheckedOptionalAccessModel.h | 17 +-
.../Models/UncheckedOptionalAccessModel.cpp | 137 +-
.../UncheckedOptionalAccessModelTest.cpp | 177 +-
4 files changed, 330 insertions(+), 11 deletions(-)
diff --git
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
index 97fe37b5353560..815b5cdeeebe24 100644
---
a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
+++
b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
@@ -76,6 +76,16 @@ For example:
}
}
+Exception: accessor methods
+```
+
+The check assumes *accessor* methods of a class are stable, with a heuristic to
+determine which methods are accessors. Specifically, parameter-free ``const``
+methods are treated as accessors. Note that this is not guaranteed to be safe
+-- but, it is widely used (safely) in practice, and so we have chosen to treat
+it as generally safe. Calls to non ``const`` methods are assumed to modify
+the state of the object and affect the stability of earlier accessor calls.
+
Rely on invariants of uncommon APIs
---
diff --git
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 09eb8b93822612..9d81cacb507351 100644
---
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -17,6 +17,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
#include "clang/Analysis/FlowSensitive/DataflowAnalysis.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/NoopLattice.h"
@@ -39,23 +40,28 @@ struct UncheckedOptionalAccessModelOptions {
bool IgnoreSmartPointerDereference = false;
};
+using UncheckedOptionalAccessLattice =
CachedConstAccessorsLattice;
+
/// Dataflow analysis that models whether optionals hold values or not.
///
/// Models the `std::optional`, `absl::optional`, and `base::Optional` types.
class UncheckedOptionalAccessModel
-: public DataflowAnalysis {
+: public DataflowAnalysis {
public:
UncheckedOptionalAccessModel(ASTContext &Ctx, dataflow::Environment &Env);
/// Returns a matcher for the optional classes covered by this model.
static ast_matchers::DeclarationMatcher optionalClassDecl();
- static NoopLattice initialElement() { return {}; }
+ static UncheckedOptionalAccessLattice initialElement() { return {}; }
- void transfer(const CFGElement &Elt, NoopLattice &L, Environment &Env);
+ void transfer(const CFGElement &Elt, UncheckedOptionalAccessLattice &L,
+Environment &Env);
private:
- CFGMatchSwitch> TransferMatchSwitch;
+ CFGMatchSwitch>
+ TransferMatchSwitch;
};
class UncheckedOptionalAccessDiagnoser {
@@ -65,7 +71,8 @@ class UncheckedOptionalAccessDiagnoser {
llvm::SmallVector
operator()(const CFGElement &Elt, ASTContext &Ctx,
- const TransferStateForDiagnostics &State) {
+ const TransferStateForDiagnostics
+ &State) {
return DiagnoseMatchSwitch(Elt, Ctx, State.Env);
}
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 70ffe92753e053..563fe6a6625d44 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -17,13 +17,14 @@
#include "clang/AST/Expr.h"
#include "clang/AST/ExprCXX.h"
#include "clang/AST/Stmt.h"
+#include "clang/AST/Type.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/ASTMatchers/ASTMatchersMacros.h"
#include "clang/Analysis/CFG.h"
#include "clang/Analysis/FlowSensitive/CFGMatchSwitch.h"
#include
[clang] [clang-tools-extra] [clang][dataflow] Cache accessors for bugprone-unchecked-optional-access (PR #112605)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/112605 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
@@ -0,0 +1,218 @@
+//===-- CachedConstAccessorsLattice.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const
method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible
for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+/// const std::optional& getFoo() const;
+/// void clear();
+/// };
+//
+/// void func(Bar& s) {
+/// if (s.getFoo().has_value()) {
+/// use(s.getFoo().value()); // safe (checked earlier getFoo())
+/// s.clear();
+/// use(s.getFoo().value()); // unsafe (invalidate cache for s)
+/// }
+/// }
+template class CachedConstAccessorsLattice : public Base {
+public:
+ using Base::Base; // inherit all constructors
+
+ /// Creates or returns a previously created `Value` associated with a const
+ /// method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a value (not a reference or record type)
+ Value *
+ getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+const CallExpr *CE, Environment &Env);
+
+ /// Creates or returns a previously created `StorageLocation` associated with
+ /// a const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a location (GLValue or a record type).
+ StorageLocation *getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const CallExpr *CE,
+ Environment &Env, llvm::function_ref
Initialize);
+
+ void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnValues.erase(&RecordLoc);
+ }
+
+ void clearConstMethodReturnStorageLocations(
+ const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnStorageLocations.erase(&RecordLoc);
+ }
+
+ bool operator==(const CachedConstAccessorsLattice &Other) const {
+return Base::operator==(Other);
+ }
+
+ LatticeJoinEffect join(const CachedConstAccessorsLattice &Other);
+
+private:
+ // Maps a record storage location and const method to the value to return
+ // from that const method.
+ using ConstMethodReturnValuesType =
+ llvm::SmallDenseMap>;
+ ConstMethodReturnValuesType ConstMethodReturnValues;
+
+ // Maps a record storage location and const method to the record storage
+ // location to return from that const method.
+ using ConstMethodReturnStorageLocationsType = llvm::SmallDenseMap<
+ const RecordStorageLocation *,
+ llvm::SmallDenseMap>;
+ ConstMethodReturnStorageLocationsType ConstMethodReturnStorageLocations;
+};
+
+namespace internal {
jvoung wrote:
Ah -- Taking a look!
https://github.com/llvm/llvm-project/pull/111006
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Disambiguate a ref to "internal" in CachedConstAccessorsLattice (PR #113601)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/113601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Disambiguate a ref to "internal" in CachedConstAccessorsLattice (PR #113601)
https://github.com/jvoung created https://github.com/llvm/llvm-project/pull/113601 Disambiguate to fix a build error (e.g., on windows with clang-cl) >From f5470d4bee2ab991942d6640dd48631ca343bc85 Mon Sep 17 00:00:00 2001 From: Jan Voung Date: Thu, 24 Oct 2024 17:37:03 + Subject: [PATCH] [clang][dataflow] Disambiguate a ref to "internal" in CachedConstAccessorsLattice Disambiguate to fix a build error (e.g., on windows with clang-cl) --- .../Analysis/FlowSensitive/CachedConstAccessorsLattice.h | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h index 3402d105746e88..48c5287367739a 100644 --- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h +++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h @@ -154,11 +154,12 @@ LatticeEffect CachedConstAccessorsLattice::join( // are non-identical but equivalent. This is likely to be sufficient in // practice, and it reduces implementation complexity considerably. - ConstMethodReturnValues = internal::joinConstMethodMap( - ConstMethodReturnValues, Other.ConstMethodReturnValues, Effect); + ConstMethodReturnValues = + clang::dataflow::internal::joinConstMethodMap( + ConstMethodReturnValues, Other.ConstMethodReturnValues, Effect); ConstMethodReturnStorageLocations = - internal::joinConstMethodMap( + clang::dataflow::internal::joinConstMethodMap( ConstMethodReturnStorageLocations, Other.ConstMethodReturnStorageLocations, Effect); ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
@@ -0,0 +1,218 @@
+//===-- CachedConstAccessorsLattice.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const
method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible
for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+/// const std::optional& getFoo() const;
+/// void clear();
+/// };
+//
+/// void func(Bar& s) {
+/// if (s.getFoo().has_value()) {
+/// use(s.getFoo().value()); // safe (checked earlier getFoo())
+/// s.clear();
+/// use(s.getFoo().value()); // unsafe (invalidate cache for s)
+/// }
+/// }
+template class CachedConstAccessorsLattice : public Base {
+public:
+ using Base::Base; // inherit all constructors
+
+ /// Creates or returns a previously created `Value` associated with a const
+ /// method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a value (not a reference or record type)
+ Value *
+ getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+const CallExpr *CE, Environment &Env);
+
+ /// Creates or returns a previously created `StorageLocation` associated with
+ /// a const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a location (GLValue or a record type).
+ StorageLocation *getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const CallExpr *CE,
+ Environment &Env, llvm::function_ref
Initialize);
+
+ void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnValues.erase(&RecordLoc);
+ }
+
+ void clearConstMethodReturnStorageLocations(
+ const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnStorageLocations.erase(&RecordLoc);
+ }
+
+ bool operator==(const CachedConstAccessorsLattice &Other) const {
+return Base::operator==(Other);
+ }
+
+ LatticeJoinEffect join(const CachedConstAccessorsLattice &Other);
+
+private:
+ // Maps a record storage location and const method to the value to return
+ // from that const method.
+ using ConstMethodReturnValuesType =
+ llvm::SmallDenseMap>;
+ ConstMethodReturnValuesType ConstMethodReturnValues;
+
+ // Maps a record storage location and const method to the record storage
+ // location to return from that const method.
+ using ConstMethodReturnStorageLocationsType = llvm::SmallDenseMap<
+ const RecordStorageLocation *,
+ llvm::SmallDenseMap>;
+ ConstMethodReturnStorageLocationsType ConstMethodReturnStorageLocations;
+};
+
+namespace internal {
jvoung wrote:
Thanks! Created https://github.com/llvm/llvm-project/pull/113601
Though is there a way to validate this, similar to this clang-cl compile?
https://github.com/llvm/llvm-project/pull/111006
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung ready_for_review https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/115051
>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/2] [clang-tidy] Filter out googletest TUs in
bugprone-unchecked-optional-access
We currently do not handle ASSERT_TRUE(opt.has_value()) macros from
googletest (and other macros), so would see lots of false positives
in test TUs.
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
.../bugprone/UncheckedOptionalAccessCheck.h | 8 ++-
.../unchecked-optional-access/gtest/gtest.h | 24 +++
...cked-optional-access-ignore-googletest.cpp | 24 +++
4 files changed, 73 insertions(+), 2 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 0a0e212f345ed8..06deea0a446809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -40,10 +40,27 @@ void
UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
this);
}
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ is_test_tu_ = false;
+}
+
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
- if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ // The googletest assertion macros are not currently recognized, so we have
+ // many false positives in tests. So, do not check functions in a test TU.
+ if (is_test_tu_ ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+return;
+
+ // Look for two (public) googletest macros; if found, we'll mark this TU as a
+ // test TU. We look for ASSERT_TRUE because it is a problematic macro that
+ // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
+ if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
+is_test_tu_ = true;
return;
+ }
const auto *FuncDecl = Result.Nodes.getNodeAs(FuncID);
if (FuncDecl->isTemplated())
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 2d1570f7df8abb..3bd7ff9867b145 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -12,7 +12,6 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include
namespace clang::tidy::bugprone {
@@ -30,6 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)}
{}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
@@ -40,6 +40,12 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // Records whether the current TU includes the googletest headers, in which
+ // case we assume it is a test TU of some sort. The googletest assertion
+ // macros are not currently recognized, so we have many false positives in
+ // tests. So, we disable checking for all test TUs.
+ bool is_test_tu_ = false;
};
} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
new file mode 100644
index 00..8a9eeac94dbb77
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+
+// Mock version of googletest macros.
+
+#define GTEST_TEST(test_suite_name, test_name)
\
+ class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {
\
+ public:
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/115051
We currently do not handle ASSERT_TRUE(opt.has_value()) macros from
googletest (and other macros), so would see lots of false positives
in test TUs.
>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH] [clang-tidy] Filter out googletest TUs in
bugprone-unchecked-optional-access
We currently do not handle ASSERT_TRUE(opt.has_value()) macros from
googletest (and other macros), so would see lots of false positives
in test TUs.
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
.../bugprone/UncheckedOptionalAccessCheck.h | 8 ++-
.../unchecked-optional-access/gtest/gtest.h | 24 +++
...cked-optional-access-ignore-googletest.cpp | 24 +++
4 files changed, 73 insertions(+), 2 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 0a0e212f345ed8..06deea0a446809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -40,10 +40,27 @@ void
UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
this);
}
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ is_test_tu_ = false;
+}
+
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
- if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ // The googletest assertion macros are not currently recognized, so we have
+ // many false positives in tests. So, do not check functions in a test TU.
+ if (is_test_tu_ ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+return;
+
+ // Look for two (public) googletest macros; if found, we'll mark this TU as a
+ // test TU. We look for ASSERT_TRUE because it is a problematic macro that
+ // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
+ if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
+is_test_tu_ = true;
return;
+ }
const auto *FuncDecl = Result.Nodes.getNodeAs(FuncID);
if (FuncDecl->isTemplated())
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 2d1570f7df8abb..3bd7ff9867b145 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -12,7 +12,6 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include
namespace clang::tidy::bugprone {
@@ -30,6 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)}
{}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
@@ -40,6 +40,12 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // Records whether the current TU includes the googletest headers, in which
+ // case we assume it is a test TU of some sort. The googletest assertion
+ // macros are not currently recognized, so we have many false positives in
+ // tests. So, we disable checking for all test TUs.
+ bool is_test_tu_ = false;
};
} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
new file mode 100644
index 00..8a9eeac94dbb77
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+
+// Mock version of googletest macros.
+
+#define GTEST_TEST(test_suite_name, test_name)
\
+ class
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
jvoung wrote: > A quick middle-point solution is to add an option to allow ignoring code that > is executed from within macros, or even allow the user to specify which > macros to ignore. Unfortunately, the problem is exactly that we're not (fully) understanding the content of `ASSERT_TRUE` and related macros (`ASSERT_FALSE`, `ASSERT_THAT(..., IsTrue(opt.has_value()))`, etc.), which then affect code outside and following the macros. So, ignoring them would not solve anything. Until we can understand the googletest macros, there is little value in check any tests that use them. That is what this patch accomplishes. https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
jvoung wrote: > Relying on Google-test internal implementation details that are outside of > our control and may change at any point in time does not feel good. > > The patch should instead fix the root cause of the problem. I agree that it's not good to rely on implementation details, but these are not internal implementation details - these are key elements of the *public API* , which trip up the analysis. It's not just any macros that we have a problem with, it's specifically the googletest macros, which we don't properly understand, resulting in false positives. https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Cache accessors returning pointers in bugprone-unchecked-optional-access (PR #113922)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/113922
>From 21f15146b8a7941781b6d728cdbb0d0be50b02fc Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Mon, 28 Oct 2024 14:45:39 +
Subject: [PATCH 1/2] [clang][dataflow] Cache accessors returning pointers in
bugprone-unchecked-optional-access
Previously, we covered returning refs, or copies of optional, and bools.
Now cover returning pointers (to any type).
This is useful for cases like operator-> of smart pointers.
Addresses more of issue llvm#58510
---
.../Models/UncheckedOptionalAccessModel.h | 8 +++
.../Models/UncheckedOptionalAccessModel.cpp | 20 +-
.../UncheckedOptionalAccessModelTest.cpp | 72 ---
3 files changed, 87 insertions(+), 13 deletions(-)
diff --git
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 9d81cacb507351..713494178b97bd 100644
---
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -37,6 +37,14 @@ struct UncheckedOptionalAccessModelOptions {
/// can't identify when their results are used safely (across calls),
/// resulting in false positives in all such cases. Note: this option does
not
/// cover access through `operator[]`.
+ /// FIXME: we currently cache and equate the result of const accessors
+ /// returning pointers, so cover the case of operator-> followed by
+ /// operator->, which covers the common case of smart pointers. We also cover
+ /// some limited cases of returning references (if return type is an optional
+ /// type), so cover some cases of operator* followed by operator*. We don't
+ /// cover mixing operator-> and operator*. Once we are confident in this
const
+ /// accessor caching, we shouldn't need the IgnoreSmartPointerDereference
+ /// option anymore.
bool IgnoreSmartPointerDereference = false;
};
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 31ae2b94f5b617..da5dda063344f9 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -338,6 +338,11 @@ auto isZeroParamConstMemberCall() {
callee(cxxMethodDecl(parameterCountIs(0), isConst(;
}
+auto isZeroParamConstMemberOperatorCall() {
+ return cxxOperatorCallExpr(
+ callee(cxxMethodDecl(parameterCountIs(0), isConst(;
+}
+
auto isNonConstMemberCall() {
return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst();
}
@@ -572,9 +577,10 @@ void handleConstMemberCall(const CallExpr *CE,
return;
}
- // Cache if the const method returns a boolean type.
+ // Cache if the const method returns a boolean or pointer type.
// We may decide to cache other return types in the future.
- if (RecordLoc != nullptr && CE->getType()->isBooleanType()) {
+ if (RecordLoc != nullptr &&
+ (CE->getType()->isBooleanType() || CE->getType()->isPointerType())) {
Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc,
CE,
State.Env);
if (Val == nullptr)
@@ -597,6 +603,14 @@ void transferValue_ConstMemberCall(const CXXMemberCallExpr
*MCE,
MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result,
State);
}
+void transferValue_ConstMemberOperatorCall(
+const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
+LatticeTransferState &State) {
+ auto *RecordLoc = cast_or_null(
+ State.Env.getStorageLocation(*OCE->getArg(0)));
+ handleConstMemberCall(OCE, RecordLoc, Result, State);
+}
+
void handleNonConstMemberCall(const CallExpr *CE,
dataflow::RecordStorageLocation *RecordLoc,
const MatchFinder::MatchResult &Result,
@@ -1020,6 +1034,8 @@ auto buildTransferMatchSwitch() {
// const accessor calls
.CaseOfCFGStmt(isZeroParamConstMemberCall(),
transferValue_ConstMemberCall)
+ .CaseOfCFGStmt(isZeroParamConstMemberOperatorCall(),
+
transferValue_ConstMemberOperatorCall)
// non-const member calls that may modify the state of an object.
.CaseOfCFGStmt(isNonConstMemberCall(),
transferValue_NonConstMemberCall)
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 5b64eaca0e10d3..5890466488c3c3 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++
b/clang/unittests/Analysis/Fl
[clang] [clang][dataflow] Cache accessors returning pointers in bugprone-unchecked-optional-access (PR #113922)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/113922 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Cache accessors returning pointers in bugprone-unchecked-optional-access (PR #113922)
https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/113922
Previously, we covered returning refs, or copies of optional, and bools.
Now cover returning pointers (to any type).
This is useful for cases like operator-> of smart pointers.
Addresses more of issue llvm#58510
>From 21f15146b8a7941781b6d728cdbb0d0be50b02fc Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Mon, 28 Oct 2024 14:45:39 +
Subject: [PATCH] [clang][dataflow] Cache accessors returning pointers in
bugprone-unchecked-optional-access
Previously, we covered returning refs, or copies of optional, and bools.
Now cover returning pointers (to any type).
This is useful for cases like operator-> of smart pointers.
Addresses more of issue llvm#58510
---
.../Models/UncheckedOptionalAccessModel.h | 8 +++
.../Models/UncheckedOptionalAccessModel.cpp | 20 +-
.../UncheckedOptionalAccessModelTest.cpp | 72 ---
3 files changed, 87 insertions(+), 13 deletions(-)
diff --git
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
index 9d81cacb507351..713494178b97bd 100644
---
a/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
+++
b/clang/include/clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h
@@ -37,6 +37,14 @@ struct UncheckedOptionalAccessModelOptions {
/// can't identify when their results are used safely (across calls),
/// resulting in false positives in all such cases. Note: this option does
not
/// cover access through `operator[]`.
+ /// FIXME: we currently cache and equate the result of const accessors
+ /// returning pointers, so cover the case of operator-> followed by
+ /// operator->, which covers the common case of smart pointers. We also cover
+ /// some limited cases of returning references (if return type is an optional
+ /// type), so cover some cases of operator* followed by operator*. We don't
+ /// cover mixing operator-> and operator*. Once we are confident in this
const
+ /// accessor caching, we shouldn't need the IgnoreSmartPointerDereference
+ /// option anymore.
bool IgnoreSmartPointerDereference = false;
};
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 31ae2b94f5b617..da5dda063344f9 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -338,6 +338,11 @@ auto isZeroParamConstMemberCall() {
callee(cxxMethodDecl(parameterCountIs(0), isConst(;
}
+auto isZeroParamConstMemberOperatorCall() {
+ return cxxOperatorCallExpr(
+ callee(cxxMethodDecl(parameterCountIs(0), isConst(;
+}
+
auto isNonConstMemberCall() {
return cxxMemberCallExpr(callee(cxxMethodDecl(unless(isConst();
}
@@ -572,9 +577,10 @@ void handleConstMemberCall(const CallExpr *CE,
return;
}
- // Cache if the const method returns a boolean type.
+ // Cache if the const method returns a boolean or pointer type.
// We may decide to cache other return types in the future.
- if (RecordLoc != nullptr && CE->getType()->isBooleanType()) {
+ if (RecordLoc != nullptr &&
+ (CE->getType()->isBooleanType() || CE->getType()->isPointerType())) {
Value *Val = State.Lattice.getOrCreateConstMethodReturnValue(*RecordLoc,
CE,
State.Env);
if (Val == nullptr)
@@ -597,6 +603,14 @@ void transferValue_ConstMemberCall(const CXXMemberCallExpr
*MCE,
MCE, dataflow::getImplicitObjectLocation(*MCE, State.Env), Result,
State);
}
+void transferValue_ConstMemberOperatorCall(
+const CXXOperatorCallExpr *OCE, const MatchFinder::MatchResult &Result,
+LatticeTransferState &State) {
+ auto *RecordLoc = cast_or_null(
+ State.Env.getStorageLocation(*OCE->getArg(0)));
+ handleConstMemberCall(OCE, RecordLoc, Result, State);
+}
+
void handleNonConstMemberCall(const CallExpr *CE,
dataflow::RecordStorageLocation *RecordLoc,
const MatchFinder::MatchResult &Result,
@@ -1020,6 +1034,8 @@ auto buildTransferMatchSwitch() {
// const accessor calls
.CaseOfCFGStmt(isZeroParamConstMemberCall(),
transferValue_ConstMemberCall)
+ .CaseOfCFGStmt(isZeroParamConstMemberOperatorCall(),
+
transferValue_ConstMemberOperatorCall)
// non-const member calls that may modify the state of an object.
.CaseOfCFGStmt(isNonConstMemberCall(),
transferValue_NonConstMemberCall)
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/Fl
[clang] [clang][dataflow] Don't clear cached field state if field is const (PR #113698)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/113698 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/115051
>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/3] [clang-tidy] Filter out googletest TUs in
bugprone-unchecked-optional-access
We currently do not handle ASSERT_TRUE(opt.has_value()) macros from
googletest (and other macros), so would see lots of false positives
in test TUs.
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
.../bugprone/UncheckedOptionalAccessCheck.h | 8 ++-
.../unchecked-optional-access/gtest/gtest.h | 24 +++
...cked-optional-access-ignore-googletest.cpp | 24 +++
4 files changed, 73 insertions(+), 2 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 0a0e212f345ed8..06deea0a446809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -40,10 +40,27 @@ void
UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
this);
}
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ is_test_tu_ = false;
+}
+
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
- if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ // The googletest assertion macros are not currently recognized, so we have
+ // many false positives in tests. So, do not check functions in a test TU.
+ if (is_test_tu_ ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+return;
+
+ // Look for two (public) googletest macros; if found, we'll mark this TU as a
+ // test TU. We look for ASSERT_TRUE because it is a problematic macro that
+ // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
+ if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
+is_test_tu_ = true;
return;
+ }
const auto *FuncDecl = Result.Nodes.getNodeAs(FuncID);
if (FuncDecl->isTemplated())
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 2d1570f7df8abb..3bd7ff9867b145 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -12,7 +12,6 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include
namespace clang::tidy::bugprone {
@@ -30,6 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)}
{}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
@@ -40,6 +40,12 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // Records whether the current TU includes the googletest headers, in which
+ // case we assume it is a test TU of some sort. The googletest assertion
+ // macros are not currently recognized, so we have many false positives in
+ // tests. So, we disable checking for all test TUs.
+ bool is_test_tu_ = false;
};
} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
new file mode 100644
index 00..8a9eeac94dbb77
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+
+// Mock version of googletest macros.
+
+#define GTEST_TEST(test_suite_name, test_name)
\
+ class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {
\
+ public:
[clang] [clang][dataflow] Disambiguate a ref to "internal" in CachedConstAccessorsLattice (PR #113601)
jvoung wrote:
> AST_MATCHER_P_OVERLOAD
Ah, missed the part where the macro is generating the `internal` namespaces. I
don't think we want to move to the top of the file exactly (the instantiation
refers to some functions defined right above, so would need fwd decls?), but we
can change the namespacing inline `} ... {`.
There seem to be other recommendations in the header
https://github.com/llvm/llvm-project/blob/6854ad90e39e9d119c990043f573db7157a2b097/clang/include/clang/ASTMatchers/ASTMatchersMacros.h#L26
like putting in `clang::ast_matchers::unnamed...` (though I'm not sure if they
mean exactly `clang::ast_matchers`.
Let me try and check with the team.
https://github.com/llvm/llvm-project/pull/113601
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Don't clear cached field state if field is const (PR #113698)
https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/113698
... in the unchecked optional access model.
>From 8b955ead1e7445a7226f51078ba2d05e52f15c23 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Fri, 25 Oct 2024 14:56:49 +
Subject: [PATCH] [clang][dataflow] Don't clear cached field state if field is
const
... in the unchecked optional access model.
---
.../Models/UncheckedOptionalAccessModel.cpp | 8 +--
.../UncheckedOptionalAccessModelTest.cpp | 23 ++-
2 files changed, 28 insertions(+), 3 deletions(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index b0bd8274405d02..31ae2b94f5b617 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -601,10 +601,14 @@ void handleNonConstMemberCall(const CallExpr *CE,
dataflow::RecordStorageLocation *RecordLoc,
const MatchFinder::MatchResult &Result,
LatticeTransferState &State) {
- // When a non-const member function is called, reset some state.
if (RecordLoc != nullptr) {
+// When a non-const member function is called, clear all (non-const)
+// optional fields of the receiver. Const-qualified fields can't be
+// changed (at least, not without UB).
for (const auto &[Field, FieldLoc] : RecordLoc->children()) {
- if (isSupportedOptionalType(Field->getType())) {
+ QualType FieldType = Field->getType();
+ if (!FieldType.isConstQualified() &&
+ isSupportedOptionalType(Field->getType())) {
auto *FieldRecordLoc = cast_or_null(FieldLoc);
if (FieldRecordLoc) {
setHasValue(*FieldRecordLoc, State.Env.makeAtomicBoolValue(),
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 22fe347c425593..5b64eaca0e10d3 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2167,7 +2167,7 @@ TEST_P(UncheckedOptionalAccessTest,
OptionalReturnedFromFuntionCall) {
)");
}
-TEST_P(UncheckedOptionalAccessTest, OptionalFieldModified) {
+TEST_P(UncheckedOptionalAccessTest, NonConstMethodMayClearOptionalField) {
ExpectDiagnosticsFor(
R"(
#include "unchecked_optional_access_test.h"
@@ -2187,6 +2187,27 @@ TEST_P(UncheckedOptionalAccessTest,
OptionalFieldModified) {
)");
}
+TEST_P(UncheckedOptionalAccessTest,
+ NonConstMethodMayNotClearConstOptionalField) {
+ ExpectDiagnosticsFor(
+ R"(
+#include "unchecked_optional_access_test.h"
+
+struct Foo {
+ const $ns::$optional opt;
+ void clear();
+};
+
+void target(Foo& foo) {
+ if (foo.opt) {
+foo.opt.value();
+foo.clear();
+foo.opt.value();
+ }
+}
+ )");
+}
+
TEST_P(UncheckedOptionalAccessTest, StdSwap) {
ExpectDiagnosticsFor(
R"(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
jvoung wrote: Any other concerns? I think I've elaborated on why the ignore/exclude alternative is not better. Otherwise, eventually, it would be great to be able to understand the various macros and how they could serve as checks for later accesses, but I think this is an improvement on the status quo (we've seen lots of such googletest false positives). Thanks! https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a test demonstrating an issue in unchecked-optional-access-check (PR #110870)
https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/110870
createStorageLocation used in transferCallReturningOptional:
https://github.com/llvm/llvm-project/blob/09ba83be0ac178851e3c9c9c8fefddbdd4d8353f/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp#L515
can stop recursively creating storage locations when it hits a field of
reference type for a non-optional record:
https://github.com/llvm/llvm-project/blob/3ca5d8082a0c6bd9520544ce3bca11bf3e02a5fa/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp#L67
If an optional is reached through that field then it may not have a
storage location by the type we handle has_value in a transfer function.
>From d148d4b3187d507fb1ba1735a3111c3eac2d2157 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 2 Oct 2024 15:26:32 +
Subject: [PATCH] [clang][dataflow] Add a test demonstrating an issue in
unchecked-optional-access-check
createStorageLocation used in transferCallReturningOptional:
https://github.com/llvm/llvm-project/blob/09ba83be0ac178851e3c9c9c8fefddbdd4d8353f/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp#L515
can stop recursively creating storage locations when it hits a field of
reference type for a non-optional record:
https://github.com/llvm/llvm-project/blob/3ca5d8082a0c6bd9520544ce3bca11bf3e02a5fa/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp#L67
If an optional is reached through that field then it may not have a
storage location by the type we handle has_value in a transfer function.
---
.../UncheckedOptionalAccessModelTest.cpp | 34 +++
1 file changed, 34 insertions(+)
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 4227a6bfdeba87..1f481c0761dc33 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2873,6 +2873,40 @@ TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct)
{
)");
}
+// A case that we should handle but currently don't.
+// When there is a field of type reference to non-optional, we may
+// stop recursively creating storage locations.
+// E.g., the field `second` below in `pair` should eventually lead to
+// the optional `x` in `A`.
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalThroughNonOptionalRefField) {
+ ExpectDiagnosticsFor(R"(
+#include "unchecked_optional_access_test.h"
+
+struct A {
+ $ns::$optional x;
+};
+
+struct pair {
+ int first;
+ const A &second;
+};
+
+struct B {
+ $ns::$optional& nonConstGetRef();
+ };
+
+void target(B b) {
+ const auto& maybe_pair = b.nonConstGetRef();
+ if (!maybe_pair.has_value())
+return;
+
+ if(!maybe_pair->second.x.has_value())
+return;
+ maybe_pair->second.x.value(); // [[unsafe]]
+}
+ )");
+}
+
TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
ExpectDiagnosticsFor(
R"(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a test demonstrating an issue in unchecked-optional-access-check (PR #110870)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/110870
>From d148d4b3187d507fb1ba1735a3111c3eac2d2157 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 2 Oct 2024 15:26:32 +
Subject: [PATCH 1/2] [clang][dataflow] Add a test demonstrating an issue in
unchecked-optional-access-check
createStorageLocation used in transferCallReturningOptional:
https://github.com/llvm/llvm-project/blob/09ba83be0ac178851e3c9c9c8fefddbdd4d8353f/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp#L515
can stop recursively creating storage locations when it hits a field of
reference type for a non-optional record:
https://github.com/llvm/llvm-project/blob/3ca5d8082a0c6bd9520544ce3bca11bf3e02a5fa/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp#L67
If an optional is reached through that field then it may not have a
storage location by the type we handle has_value in a transfer function.
---
.../UncheckedOptionalAccessModelTest.cpp | 34 +++
1 file changed, 34 insertions(+)
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 4227a6bfdeba87..1f481c0761dc33 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2873,6 +2873,40 @@ TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct)
{
)");
}
+// A case that we should handle but currently don't.
+// When there is a field of type reference to non-optional, we may
+// stop recursively creating storage locations.
+// E.g., the field `second` below in `pair` should eventually lead to
+// the optional `x` in `A`.
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalThroughNonOptionalRefField) {
+ ExpectDiagnosticsFor(R"(
+#include "unchecked_optional_access_test.h"
+
+struct A {
+ $ns::$optional x;
+};
+
+struct pair {
+ int first;
+ const A &second;
+};
+
+struct B {
+ $ns::$optional& nonConstGetRef();
+ };
+
+void target(B b) {
+ const auto& maybe_pair = b.nonConstGetRef();
+ if (!maybe_pair.has_value())
+return;
+
+ if(!maybe_pair->second.x.has_value())
+return;
+ maybe_pair->second.x.value(); // [[unsafe]]
+}
+ )");
+}
+
TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
ExpectDiagnosticsFor(
R"(
>From d3a5952bd44ac5d7c6c9fadb96adcd85c9d79419 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 2 Oct 2024 15:36:45 +
Subject: [PATCH 2/2] Fix formatting
---
.../FlowSensitive/UncheckedOptionalAccessModelTest.cpp| 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 1f481c0761dc33..728f410d96a72b 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2893,7 +2893,7 @@ TEST_P(UncheckedOptionalAccessTest,
NestedOptionalThroughNonOptionalRefField) {
struct B {
$ns::$optional& nonConstGetRef();
- };
+};
void target(B b) {
const auto& maybe_pair = b.nonConstGetRef();
@@ -2902,7 +2902,7 @@ TEST_P(UncheckedOptionalAccessTest,
NestedOptionalThroughNonOptionalRefField) {
if(!maybe_pair->second.x.has_value())
return;
- maybe_pair->second.x.value(); // [[unsafe]]
+ maybe_pair->second.x.value(); // [[unsafe]]
}
)");
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a test demonstrating an issue in unchecked-optional-access-check (PR #110870)
https://github.com/jvoung ready_for_review https://github.com/llvm/llvm-project/pull/110870 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a test demonstrating an issue in unchecked-optional-access-check (PR #110870)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/110870
>From d148d4b3187d507fb1ba1735a3111c3eac2d2157 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 2 Oct 2024 15:26:32 +
Subject: [PATCH 1/3] [clang][dataflow] Add a test demonstrating an issue in
unchecked-optional-access-check
createStorageLocation used in transferCallReturningOptional:
https://github.com/llvm/llvm-project/blob/09ba83be0ac178851e3c9c9c8fefddbdd4d8353f/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp#L515
can stop recursively creating storage locations when it hits a field of
reference type for a non-optional record:
https://github.com/llvm/llvm-project/blob/3ca5d8082a0c6bd9520544ce3bca11bf3e02a5fa/clang/lib/Analysis/FlowSensitive/DataflowAnalysisContext.cpp#L67
If an optional is reached through that field then it may not have a
storage location by the type we handle has_value in a transfer function.
---
.../UncheckedOptionalAccessModelTest.cpp | 34 +++
1 file changed, 34 insertions(+)
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 4227a6bfdeba87..1f481c0761dc33 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2873,6 +2873,40 @@ TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct)
{
)");
}
+// A case that we should handle but currently don't.
+// When there is a field of type reference to non-optional, we may
+// stop recursively creating storage locations.
+// E.g., the field `second` below in `pair` should eventually lead to
+// the optional `x` in `A`.
+TEST_P(UncheckedOptionalAccessTest, NestedOptionalThroughNonOptionalRefField) {
+ ExpectDiagnosticsFor(R"(
+#include "unchecked_optional_access_test.h"
+
+struct A {
+ $ns::$optional x;
+};
+
+struct pair {
+ int first;
+ const A &second;
+};
+
+struct B {
+ $ns::$optional& nonConstGetRef();
+ };
+
+void target(B b) {
+ const auto& maybe_pair = b.nonConstGetRef();
+ if (!maybe_pair.has_value())
+return;
+
+ if(!maybe_pair->second.x.has_value())
+return;
+ maybe_pair->second.x.value(); // [[unsafe]]
+}
+ )");
+}
+
TEST_P(UncheckedOptionalAccessTest, OptionalValueInitialization) {
ExpectDiagnosticsFor(
R"(
>From d3a5952bd44ac5d7c6c9fadb96adcd85c9d79419 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 2 Oct 2024 15:36:45 +
Subject: [PATCH 2/3] Fix formatting
---
.../FlowSensitive/UncheckedOptionalAccessModelTest.cpp| 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 1f481c0761dc33..728f410d96a72b 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2893,7 +2893,7 @@ TEST_P(UncheckedOptionalAccessTest,
NestedOptionalThroughNonOptionalRefField) {
struct B {
$ns::$optional& nonConstGetRef();
- };
+};
void target(B b) {
const auto& maybe_pair = b.nonConstGetRef();
@@ -2902,7 +2902,7 @@ TEST_P(UncheckedOptionalAccessTest,
NestedOptionalThroughNonOptionalRefField) {
if(!maybe_pair->second.x.has_value())
return;
- maybe_pair->second.x.value(); // [[unsafe]]
+ maybe_pair->second.x.value(); // [[unsafe]]
}
)");
}
>From 3c5f997bb8b1b7aa411d6e70b66dc44568f8ba15 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Wed, 2 Oct 2024 15:46:27 +
Subject: [PATCH 3/3] Tag as fixme
---
.../Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 728f410d96a72b..877bfce8b27092 100644
---
a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++
b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2873,7 +2873,7 @@ TEST_P(UncheckedOptionalAccessTest, OptionalValueStruct) {
)");
}
-// A case that we should handle but currently don't.
+// FIXME: A case that we should handle but currently don't.
// When there is a field of type reference to non-optional, we may
// stop recursively creating storage locations.
// E.g., the field `second` below in `pair` should eventually lead to
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help represent cache const accessor methods (PR #111006)
https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/111006
By caching const accessor methods we can sometimes treat method call
results as stable (e.g., for issue
https://github.com/llvm/llvm-project/issues/58510).
Users can clear the cache when a non-const method is called that may
modify the state of an object.
This is represented as mixin.
It will be used in a follow on patch to change
bugprone-unchecked-optional-access's lattice from
NoopLattice to CachedConstAccessorsLattice,
along with some additional transfer functions.
>From f82e63e470f704f29f4c161579fd592c27301868 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 3 Oct 2024 15:21:32 +
Subject: [PATCH] [clang][dataflow] Add a lattice to help represent cache const
accessor methods
By caching const accessor methods we can sometimes treat method call
results as stable (e.g., for issue
https://github.com/llvm/llvm-project/issues/58510).
Users can clear the cache when a non-const method is called that may
modify the state of an object.
This is represented as mixin.
It will be used in a follow on patch to change
bugprone-unchecked-optional-access's lattice from
NoopLattice to CachedConstAccessorsLattice,
along with some additional transfer functions.
---
.../CachedConstAccessorsLattice.h | 221 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 217 +
3 files changed, 439 insertions(+)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
create mode 100644
clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
new file mode 100644
index 00..b8517fabd1f550
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -0,0 +1,221 @@
+//===-- CachedConstAccessorsLattice.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const
method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible
for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+/// const std::optional& getFoo() const;
+/// void clear();
+/// };
+//
+/// void func(Bar& s) {
+/// if (s.getFoo().has_value()) {
+/// use(s.getFoo().value()); // safe (checked earlier getFoo())
+/// s.clear();
+/// use(s.getFoo().value()); // unsafe (invalidate cache for s)
+/// }
+/// }
+template
+class CachedConstAccessorsLattice : public Base {
+public:
+ using Base::Base; // inherit all constructors
+
+ /// Creates or returns a previously created `Value` associated with a const
+ /// method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a value (not a reference or record type)
+ Value *
+ getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+const CallExpr *CE, Environment &Env);
+
+ /// Creates or returns a previously created `StorageLocation` associated a
+ /// const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a location (GLValue or a record type).
+ StorageLocation *getOrCreat
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/111006 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/111006 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/111006 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/111006
>From f82e63e470f704f29f4c161579fd592c27301868 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 3 Oct 2024 15:21:32 +
Subject: [PATCH 1/2] [clang][dataflow] Add a lattice to help represent cache
const accessor methods
By caching const accessor methods we can sometimes treat method call
results as stable (e.g., for issue
https://github.com/llvm/llvm-project/issues/58510).
Users can clear the cache when a non-const method is called that may
modify the state of an object.
This is represented as mixin.
It will be used in a follow on patch to change
bugprone-unchecked-optional-access's lattice from
NoopLattice to CachedConstAccessorsLattice,
along with some additional transfer functions.
---
.../CachedConstAccessorsLattice.h | 221 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 217 +
3 files changed, 439 insertions(+)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
create mode 100644
clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
new file mode 100644
index 00..b8517fabd1f550
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -0,0 +1,221 @@
+//===-- CachedConstAccessorsLattice.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const
method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible
for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+/// const std::optional& getFoo() const;
+/// void clear();
+/// };
+//
+/// void func(Bar& s) {
+/// if (s.getFoo().has_value()) {
+/// use(s.getFoo().value()); // safe (checked earlier getFoo())
+/// s.clear();
+/// use(s.getFoo().value()); // unsafe (invalidate cache for s)
+/// }
+/// }
+template
+class CachedConstAccessorsLattice : public Base {
+public:
+ using Base::Base; // inherit all constructors
+
+ /// Creates or returns a previously created `Value` associated with a const
+ /// method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a value (not a reference or record type)
+ Value *
+ getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+const CallExpr *CE, Environment &Env);
+
+ /// Creates or returns a previously created `StorageLocation` associated a
+ /// const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a location (GLValue or a record type).
+ StorageLocation *getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const CallExpr *CE,
+ Environment &Env,
+ llvm::function_ref Initialize);
+
+ void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnValues.erase(&RecordLoc);
+ }
+
+ void clearConstMethodReturnStorageLocations(
+ const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnStorageLocations.erase(&RecordLoc);
+ }
+
+ bool op
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
https://github.com/jvoung ready_for_review https://github.com/llvm/llvm-project/pull/111006 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/111006
>From f82e63e470f704f29f4c161579fd592c27301868 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 3 Oct 2024 15:21:32 +
Subject: [PATCH 1/4] [clang][dataflow] Add a lattice to help represent cache
const accessor methods
By caching const accessor methods we can sometimes treat method call
results as stable (e.g., for issue
https://github.com/llvm/llvm-project/issues/58510).
Users can clear the cache when a non-const method is called that may
modify the state of an object.
This is represented as mixin.
It will be used in a follow on patch to change
bugprone-unchecked-optional-access's lattice from
NoopLattice to CachedConstAccessorsLattice,
along with some additional transfer functions.
---
.../CachedConstAccessorsLattice.h | 221 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 217 +
3 files changed, 439 insertions(+)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
create mode 100644
clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
new file mode 100644
index 00..b8517fabd1f550
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -0,0 +1,221 @@
+//===-- CachedConstAccessorsLattice.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const
method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible
for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+/// const std::optional& getFoo() const;
+/// void clear();
+/// };
+//
+/// void func(Bar& s) {
+/// if (s.getFoo().has_value()) {
+/// use(s.getFoo().value()); // safe (checked earlier getFoo())
+/// s.clear();
+/// use(s.getFoo().value()); // unsafe (invalidate cache for s)
+/// }
+/// }
+template
+class CachedConstAccessorsLattice : public Base {
+public:
+ using Base::Base; // inherit all constructors
+
+ /// Creates or returns a previously created `Value` associated with a const
+ /// method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a value (not a reference or record type)
+ Value *
+ getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+const CallExpr *CE, Environment &Env);
+
+ /// Creates or returns a previously created `StorageLocation` associated a
+ /// const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a location (GLValue or a record type).
+ StorageLocation *getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const CallExpr *CE,
+ Environment &Env,
+ llvm::function_ref Initialize);
+
+ void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnValues.erase(&RecordLoc);
+ }
+
+ void clearConstMethodReturnStorageLocations(
+ const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnStorageLocations.erase(&RecordLoc);
+ }
+
+ bool op
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/111006
>From f82e63e470f704f29f4c161579fd592c27301868 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Thu, 3 Oct 2024 15:21:32 +
Subject: [PATCH 1/3] [clang][dataflow] Add a lattice to help represent cache
const accessor methods
By caching const accessor methods we can sometimes treat method call
results as stable (e.g., for issue
https://github.com/llvm/llvm-project/issues/58510).
Users can clear the cache when a non-const method is called that may
modify the state of an object.
This is represented as mixin.
It will be used in a follow on patch to change
bugprone-unchecked-optional-access's lattice from
NoopLattice to CachedConstAccessorsLattice,
along with some additional transfer functions.
---
.../CachedConstAccessorsLattice.h | 221 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 217 +
3 files changed, 439 insertions(+)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
create mode 100644
clang/unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
new file mode 100644
index 00..b8517fabd1f550
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -0,0 +1,221 @@
+//===-- CachedConstAccessorsLattice.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const
method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible
for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+/// const std::optional& getFoo() const;
+/// void clear();
+/// };
+//
+/// void func(Bar& s) {
+/// if (s.getFoo().has_value()) {
+/// use(s.getFoo().value()); // safe (checked earlier getFoo())
+/// s.clear();
+/// use(s.getFoo().value()); // unsafe (invalidate cache for s)
+/// }
+/// }
+template
+class CachedConstAccessorsLattice : public Base {
+public:
+ using Base::Base; // inherit all constructors
+
+ /// Creates or returns a previously created `Value` associated with a const
+ /// method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a value (not a reference or record type)
+ Value *
+ getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+const CallExpr *CE, Environment &Env);
+
+ /// Creates or returns a previously created `StorageLocation` associated a
+ /// const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a location (GLValue or a record type).
+ StorageLocation *getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const CallExpr *CE,
+ Environment &Env,
+ llvm::function_ref Initialize);
+
+ void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnValues.erase(&RecordLoc);
+ }
+
+ void clearConstMethodReturnStorageLocations(
+ const RecordStorageLocation &RecordLoc) {
+ConstMethodReturnStorageLocations.erase(&RecordLoc);
+ }
+
+ bool op
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
@@ -0,0 +1,217 @@
+//===- unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
+
+#include
+#include
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/NoopLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Testing/TestAST.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang::dataflow {
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasName;
+using ast_matchers::match;
+using ast_matchers::selectFirst;
+
+using dataflow::DataflowAnalysisContext;
+using dataflow::Environment;
+using dataflow::LatticeJoinEffect;
+using dataflow::RecordStorageLocation;
+using dataflow::Value;
+using dataflow::WatchedLiteralsSolver;
+
+NamedDecl *lookup(StringRef Name, const DeclContext &DC) {
+ auto Result = DC.lookup(&DC.getParentASTContext().Idents.get(Name));
+ EXPECT_TRUE(Result.isSingleResult()) << Name;
+ return Result.front();
+}
+
+class CachedConstAccessorsLatticeTest : public ::testing::Test {
+protected:
+ using LatticeT = CachedConstAccessorsLattice;
+
+ DataflowAnalysisContext DACtx{std::make_unique()};
+ Environment Env{DACtx};
+};
+
+// Basic test AST with two const methods (return a value, and return a ref).
+struct CommonTestInputs {
+ CommonTestInputs()
+ : AST(R"cpp(
+struct S {
+ int *valProperty() const;
+ int &refProperty() const;
+};
+void target() {
+ S s;
+ s.valProperty();
+ S s2;
jvoung wrote:
Thanks! Added "DifferentValsFromDifferentLocs"
https://github.com/llvm/llvm-project/pull/111006
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
@@ -0,0 +1,217 @@
+//===- unittests/Analysis/FlowSensitive/CachedConstAccessorsLatticeTest.cpp
==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h"
+
+#include
+#include
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Analysis/FlowSensitive/DataflowAnalysisContext.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/NoopLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "clang/Analysis/FlowSensitive/WatchedLiteralsSolver.h"
+#include "clang/Basic/LLVM.h"
+#include "clang/Testing/TestAST.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang::dataflow {
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasName;
+using ast_matchers::match;
+using ast_matchers::selectFirst;
+
+using dataflow::DataflowAnalysisContext;
+using dataflow::Environment;
+using dataflow::LatticeJoinEffect;
+using dataflow::RecordStorageLocation;
+using dataflow::Value;
+using dataflow::WatchedLiteralsSolver;
+
+NamedDecl *lookup(StringRef Name, const DeclContext &DC) {
+ auto Result = DC.lookup(&DC.getParentASTContext().Idents.get(Name));
+ EXPECT_TRUE(Result.isSingleResult()) << Name;
+ return Result.front();
+}
+
+class CachedConstAccessorsLatticeTest : public ::testing::Test {
+protected:
+ using LatticeT = CachedConstAccessorsLattice;
+
+ DataflowAnalysisContext DACtx{std::make_unique()};
+ Environment Env{DACtx};
+};
+
+// Basic test AST with two const methods (return a value, and return a ref).
+struct CommonTestInputs {
+ CommonTestInputs()
+ : AST(R"cpp(
+struct S {
+ int *valProperty() const;
+ int &refProperty() const;
jvoung wrote:
Thanks! Added a test "SameStructValBeforeClearOrDiffAfterClear"
https://github.com/llvm/llvm-project/pull/111006
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
@@ -0,0 +1,218 @@
+//===-- CachedConstAccessorsLattice.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const
method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible
for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+/// const std::optional& getFoo() const;
+/// void clear();
+/// };
+//
+/// void func(Bar& s) {
+/// if (s.getFoo().has_value()) {
+/// use(s.getFoo().value()); // safe (checked earlier getFoo())
+/// s.clear();
+/// use(s.getFoo().value()); // unsafe (invalidate cache for s)
+/// }
+/// }
+template class CachedConstAccessorsLattice : public Base {
+public:
+ using Base::Base; // inherit all constructors
+
+ /// Creates or returns a previously created `Value` associated with a const
+ /// method call `obj.getFoo()` where `RecordLoc` is the
jvoung wrote:
Fixed!
https://github.com/llvm/llvm-project/pull/111006
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add a lattice to help cache const accessor methods (PR #111006)
@@ -0,0 +1,218 @@
+//===-- CachedConstAccessorsLattice.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines the lattice mixin that additionally maintains a cache of
+// stable method call return values to model const accessor member functions.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+
+#include "clang/AST/Expr.h"
+#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
+#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
+#include "clang/Analysis/FlowSensitive/StorageLocation.h"
+#include "clang/Analysis/FlowSensitive/Value.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLFunctionalExtras.h"
+
+namespace clang {
+namespace dataflow {
+
+/// A mixin for a lattice that additionally maintains a cache of stable method
+/// call return values to model const accessors methods. When a non-const
method
+/// is called, the cache should be cleared causing the next call to a const
+/// method to be considered a different value. NOTE: The user is responsible
for
+/// clearing the cache.
+///
+/// For example:
+///
+/// class Bar {
+/// public:
+/// const std::optional& getFoo() const;
+/// void clear();
+/// };
+//
+/// void func(Bar& s) {
+/// if (s.getFoo().has_value()) {
+/// use(s.getFoo().value()); // safe (checked earlier getFoo())
+/// s.clear();
+/// use(s.getFoo().value()); // unsafe (invalidate cache for s)
+/// }
+/// }
+template class CachedConstAccessorsLattice : public Base {
+public:
+ using Base::Base; // inherit all constructors
+
+ /// Creates or returns a previously created `Value` associated with a const
+ /// method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`.
+ /// Returns nullptr if unable to find or create a value.
+ ///
+ /// Requirements:
+ ///
+ /// - `CE` should return a value (not a reference or record type)
+ Value *
+ getOrCreateConstMethodReturnValue(const RecordStorageLocation &RecordLoc,
+const CallExpr *CE, Environment &Env);
+
+ /// Creates or returns a previously created `StorageLocation` associated a
jvoung wrote:
Fixed!
https://github.com/llvm/llvm-project/pull/111006
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
jvoung wrote:
> > > we're not (fully) understanding the content
> >
> >
> > My thinking was that we don't even need to understand the content, we
> > simply exclude code that is contained within any of the problematic public
> > macros. This sounds like it should be possible to do? Unfortunately I don't
> > know the details on how this could be implemented, hopefully other
> > reviewers know better?
> > Otherwise ChatGPT seems to give useful ideas on how to skip a matched
> > result contained within an `ASSERT` macro (obviously untested):
> > ```
> > if (Lexer::getImmediateMacroName(Loc, SM, Result.Context->getLangOpts())
> > == "ASSERT") {
> > // The call is within ASSERT, no diagnostic needed.
> > return;
> > }
> > ```
>
> That doesn't handle some cases like:
>
> ```
> auto opt = DoSomeSetup(...)
> ASSERT_TRUE(opt.has_value())
> T x = DoMoreSetup(*opt) // warn right here, since we didn't interpret the
> above ASSERT_TRUE (or other ways to check)
>
> EXPECT_EQ(FunctionToTest(x), ...);
> ```
>
> Sometimes the `*opt` may be within a macro, but not always.
- In non-test code, it is even more likely that the `*opt` is not wrapped in a
macro, while the `ASSERT(opt.has_value())` is.
- If in non-test scenarios, the `ASSERT` macro implementation is actually
simple, and the analysis already understood `ASSERT(opt.has_value())` makes a
following `*opt` safe, then ignoring the `ASSERT` is actually worse and causes
false positives.
- We wouldn't want to accidentally ignore the wrong macros (especially in
non-test contexts)
- Coming up with a list of exactly the right macros to ignore for gtest, would
be a bigger list of public macro names than the current change.
- And it still doesn't solve the "sometimes the `*opt` may be within a macro,
but not always."
https://github.com/llvm/llvm-project/pull/115051
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
jvoung wrote:
> > we're not (fully) understanding the content
>
> My thinking was that we don't even need to understand the content, we simply
> exclude code that is contained within any of the problematic public macros.
> This sounds like it should be possible to do? Unfortunately I don't know the
> details on how this could be implemented, hopefully other reviewers know
> better?
>
> Otherwise ChatGPT seems to give useful ideas on how to skip a matched result
> contained within an `ASSERT` macro (obviously untested):
>
> ```
> if (Lexer::getImmediateMacroName(Loc, SM, Result.Context->getLangOpts()) ==
> "ASSERT") {
> // The call is within ASSERT, no diagnostic needed.
> return;
> }
> ```
That doesn't handle some cases like:
```
auto opt = DoSomeSetup(...)
ASSERT_TRUE(opt.has_value())
T x = DoMoreSetup(*opt) // warn right here, since we didn't interpret the
above ASSERT_TRUE (or other ways to check)
EXPECT_EQ(FunctionToTest(x), ...);
```
Sometimes the `*opt` may be within a macro, but not always.
https://github.com/llvm/llvm-project/pull/115051
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/115051
>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/3] [clang-tidy] Filter out googletest TUs in
bugprone-unchecked-optional-access
We currently do not handle ASSERT_TRUE(opt.has_value()) macros from
googletest (and other macros), so would see lots of false positives
in test TUs.
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
.../bugprone/UncheckedOptionalAccessCheck.h | 8 ++-
.../unchecked-optional-access/gtest/gtest.h | 24 +++
...cked-optional-access-ignore-googletest.cpp | 24 +++
4 files changed, 73 insertions(+), 2 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 0a0e212f345ed8..06deea0a446809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -40,10 +40,27 @@ void
UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
this);
}
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ is_test_tu_ = false;
+}
+
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
- if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ // The googletest assertion macros are not currently recognized, so we have
+ // many false positives in tests. So, do not check functions in a test TU.
+ if (is_test_tu_ ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+return;
+
+ // Look for two (public) googletest macros; if found, we'll mark this TU as a
+ // test TU. We look for ASSERT_TRUE because it is a problematic macro that
+ // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
+ if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
+is_test_tu_ = true;
return;
+ }
const auto *FuncDecl = Result.Nodes.getNodeAs(FuncID);
if (FuncDecl->isTemplated())
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 2d1570f7df8abb..3bd7ff9867b145 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -12,7 +12,6 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include
namespace clang::tidy::bugprone {
@@ -30,6 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)}
{}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
@@ -40,6 +40,12 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // Records whether the current TU includes the googletest headers, in which
+ // case we assume it is a test TU of some sort. The googletest assertion
+ // macros are not currently recognized, so we have many false positives in
+ // tests. So, we disable checking for all test TUs.
+ bool is_test_tu_ = false;
};
} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
new file mode 100644
index 00..8a9eeac94dbb77
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+
+// Mock version of googletest macros.
+
+#define GTEST_TEST(test_suite_name, test_name)
\
+ class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {
\
+ public:
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
jvoung wrote: > > but these are not internal implementation details - these are key elements > > of the public API > > In the unit test, you have copied internal code from here: > https://github.com/google/googletest/blob/d144031940543e15423a25ae5a8a74141044862f/googletest/include/gtest/internal/gtest-internal.h#L1477 > > This is not OK to do, since it may change at any time and make the test no > longer useful. The test may also work for some versions of googletest but not > others. > > The code that has been copied is also Copyrighted and has certain License > obligations, which we may not be allowed to bring into the LLVM repository. > Pinging @tstellar for clarification. Simplified the mock, since we're only matching on the top-level macro names. https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [LLVM][NFC] Use `used`'s element type if available (PR #116804)
https://github.com/jvoung approved this pull request. https://github.com/llvm/llvm-project/pull/116804 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
jvoung wrote:
> I don't think we can ignore a TU simply because it has the Google Test header
> included, which this will do. This would ignore real problems, such as
That is true (and we've considered that -- FWIW, ymand and our team have been
maintaining this checker).
But as is, we are currently seeing many false positives in test files, and it
becomes harder for developers to spot such true positives, given the FP noise
to filter through.
Would it help if this was instead a checker option?
> The issue with the Google Test macros seems to be, that the dataflow check
> does not understand using a helper for the cast to `bool` like this:
>
> ```c++
> ...
> if (const auto wrapped_check = Converter{v})
> ;
> else
> return;
> auto val = *v;
> }
> ```
Thanks, that's definitely part of it. If we handle `EXPECT_TRUE` as well (and
other `EXPECT_...` forms), EXPECT [does not](https://godbolt.org/z/6Tbxae9jq)
have a `return` so would need special handling (otherwise looks like both
branches go to the `*v`).
https://github.com/llvm/llvm-project/pull/115051
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
jvoung wrote: Hi @5chmidti and @HerrCai0907 any thoughts on the "Would it help if this was instead a checker option?"? Given the volume of false positives right now, I think skip by default, but can turn on if needed. I updated the description to include the point of "we are currently seeing many false positives in test files, and it becomes harder for developers to spot such true positives, given the FP noise to filter through". https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/120249
>From c526263a7accc434dbf6e93c2995ceb2f95873b8 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 17 Dec 2024 15:38:19 +
Subject: [PATCH 1/7] [clang][dataflow] Use smart pointer caching in unchecked
optional accessor
Part 2 (and final part) following
https://github.com/llvm/llvm-project/pull/120102
Allows users to do things like:
```
if (o->x.has_value()) {
((*o).x).value();
}
```
where the `->` and `*` are operator overload calls.
A user could instead extract the nested optional into a local variable
once instead of doing two accessor calls back to back, but currently
they are unsure why the code is flagged.
---
.../CachedConstAccessorsLattice.h | 41
.../SmartPointerAccessorCaching.h | 168 +++
.../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../Models/UncheckedOptionalAccessModel.cpp | 58 +-
.../SmartPointerAccessorCaching.cpp | 153 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 32 +++
.../SmartPointerAccessorCachingTest.cpp | 194 ++
.../UncheckedOptionalAccessModelTest.cpp | 50 +
9 files changed, 692 insertions(+), 6 deletions(-)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
create mode 100644
clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
create mode 100644
clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 48c5287367739a..6b5dacf9f66d2d 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -13,7 +13,9 @@
#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -71,10 +73,28 @@ template class CachedConstAccessorsLattice
: public Base {
/// Requirements:
///
/// - `CE` should return a location (GLValue or a record type).
+ ///
+ /// DEPRECATED: switch users to the below overload which takes Callee and
Type
+ /// directly.
StorageLocation *getOrCreateConstMethodReturnStorageLocation(
const RecordStorageLocation &RecordLoc, const CallExpr *CE,
Environment &Env, llvm::function_ref
Initialize);
+ /// Creates or returns a previously created `StorageLocation` associated with
+ /// a const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`,
+ /// and `Type` is the return type of `getFoo`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ ///
+ /// Requirements:
+ ///
+ /// - `Type` should return a location (GLValue or a record type).
+ StorageLocation &getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+ QualType Type, Environment &Env,
+ llvm::function_ref Initialize);
+
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
ConstMethodReturnValues.erase(&RecordLoc);
}
@@ -212,6 +232,27 @@
CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
return &Loc;
}
+template
+StorageLocation &
+CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
+const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+QualType Type, Environment &Env,
+llvm::function_ref Initialize) {
+ assert(Callee != nullptr);
+ assert(!Type.isNull());
+ assert(Type->isReferenceType() || Type->isRecordType());
+ auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
+ auto it = ObjMap.find(Callee);
+ if (it != ObjMap.end())
+return *it->second;
+
+ StorageLocation &Loc = Env.createStorageLocation(Type.getNonReferenceType());
+ Initialize(Loc);
+
+ ObjMap.insert({Callee, &Loc});
+ return Loc;
+}
+
} // namespace dataflow
} // namespace clang
diff --git
a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
new file mode 100644
index 00..e89e82c893d8cb
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -0,0 +1,168 @@
+//===-- SmartPointerAccessorCaching.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, u
[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/120249
>From c526263a7accc434dbf6e93c2995ceb2f95873b8 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 17 Dec 2024 15:38:19 +
Subject: [PATCH 1/7] [clang][dataflow] Use smart pointer caching in unchecked
optional accessor
Part 2 (and final part) following
https://github.com/llvm/llvm-project/pull/120102
Allows users to do things like:
```
if (o->x.has_value()) {
((*o).x).value();
}
```
where the `->` and `*` are operator overload calls.
A user could instead extract the nested optional into a local variable
once instead of doing two accessor calls back to back, but currently
they are unsure why the code is flagged.
---
.../CachedConstAccessorsLattice.h | 41
.../SmartPointerAccessorCaching.h | 168 +++
.../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../Models/UncheckedOptionalAccessModel.cpp | 58 +-
.../SmartPointerAccessorCaching.cpp | 153 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 32 +++
.../SmartPointerAccessorCachingTest.cpp | 194 ++
.../UncheckedOptionalAccessModelTest.cpp | 50 +
9 files changed, 692 insertions(+), 6 deletions(-)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
create mode 100644
clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
create mode 100644
clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 48c5287367739a..6b5dacf9f66d2d 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -13,7 +13,9 @@
#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -71,10 +73,28 @@ template class CachedConstAccessorsLattice
: public Base {
/// Requirements:
///
/// - `CE` should return a location (GLValue or a record type).
+ ///
+ /// DEPRECATED: switch users to the below overload which takes Callee and
Type
+ /// directly.
StorageLocation *getOrCreateConstMethodReturnStorageLocation(
const RecordStorageLocation &RecordLoc, const CallExpr *CE,
Environment &Env, llvm::function_ref
Initialize);
+ /// Creates or returns a previously created `StorageLocation` associated with
+ /// a const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`,
+ /// and `Type` is the return type of `getFoo`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ ///
+ /// Requirements:
+ ///
+ /// - `Type` should return a location (GLValue or a record type).
+ StorageLocation &getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+ QualType Type, Environment &Env,
+ llvm::function_ref Initialize);
+
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
ConstMethodReturnValues.erase(&RecordLoc);
}
@@ -212,6 +232,27 @@
CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
return &Loc;
}
+template
+StorageLocation &
+CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
+const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+QualType Type, Environment &Env,
+llvm::function_ref Initialize) {
+ assert(Callee != nullptr);
+ assert(!Type.isNull());
+ assert(Type->isReferenceType() || Type->isRecordType());
+ auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
+ auto it = ObjMap.find(Callee);
+ if (it != ObjMap.end())
+return *it->second;
+
+ StorageLocation &Loc = Env.createStorageLocation(Type.getNonReferenceType());
+ Initialize(Loc);
+
+ ObjMap.insert({Callee, &Loc});
+ return Loc;
+}
+
} // namespace dataflow
} // namespace clang
diff --git
a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
new file mode 100644
index 00..e89e82c893d8cb
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -0,0 +1,168 @@
+//===-- SmartPointerAccessorCaching.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, u
[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
@@ -58,6 +63,106 @@ ast_matchers::StatementMatcher isSmartPointerLikeOperatorArrow(); ast_matchers::StatementMatcher isSmartPointerLikeValueMethodCall(); ast_matchers::StatementMatcher isSmartPointerLikeGetMethodCall(); +// Common transfer functions. + +/// Returns the "canonical" callee for smart pointer operators (`*` and `->`) +/// as a key for caching. +/// +/// We choose `*` as the canonical one, since it needs a +/// StorageLocation anyway. +/// +/// Note: there may be multiple `operator*` (one const, one non-const). +/// We pick the const one, which the above provided matchers require to exist. +const FunctionDecl * +getCanonicalSmartPointerLikeOperatorCallee(const CallExpr *CE); + +/// A transfer function for `operator*` (and `value`) calls +/// that can be cached. jvoung wrote: Thanks -- done! https://github.com/llvm/llvm-project/pull/120249 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] [dataflow] use unqualified type for smart pointer matching (PR #125958)
https://github.com/jvoung approved this pull request. Thanks! https://github.com/llvm/llvm-project/pull/125958 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix smart pointer accessor caching to handle aliases (PR #124964)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/124964 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
https://github.com/jvoung ready_for_review https://github.com/llvm/llvm-project/pull/120102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/120102
This is part 1 of caching for smart pointer accessors, building on top
of the CachedConstAccessorsLattice, which caches "normal" accessors.
Smart pointer accessors are a bit different in that they may:
- have aliases to access the same underlying data (but potentially
returning slightly different types like `&` vs `*`). Within a
"checked" sequence users may mix uses of the different aliases and the
check should apply to any of the spellings.
- may have non-const overloads in addition to the const version, where
the non-const doesn't actually modify the container
Part 2 will follow and add transfer functions utilities. It will also
add a user UncheckedOptionalAccessModel. We'd seen false positives when
nesting StatusOr> and optional>, etc. which this
can help address.
>From f9b8cbaed4c01c2051cdcde105d6a9bca1684388 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Mon, 16 Dec 2024 16:06:43 +
Subject: [PATCH] [clang][dataflow] Add matchers for smart pointer accessors to
be cached
This is part 1 of caching for smart pointer accessors, building on top
of the CachedConstAccessorsLattice, which caches "normal" accessors.
Smart pointer accessors are a bit different in that they may:
- have aliases to access the same underlying data (but potentially
returning slightly different types like `&` vs `*`). Within a
"checked" sequence users may mix uses of the different aliases and the
check should apply to any of the spellings.
- may have non-const overloads in addition to the const version, where
the non-const doesn't actually modify the container
Part 2 will follow and add transfer functions utilities. It will also
add a user UncheckedOptionalAccessModel. We'd seen false positives when
nesting StatusOr> and optional>, etc. which this
can help address.
---
.../SmartPointerAccessorCaching.h | 63 ++
.../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../SmartPointerAccessorCaching.cpp | 134
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../SmartPointerAccessorCachingTest.cpp | 194 ++
5 files changed, 393 insertions(+)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
create mode 100644
clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
create mode 100644
clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
new file mode 100644
index 00..1adb63af4e724b
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -0,0 +1,63 @@
+//===-- SmartPointerAccessorCaching.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines utilities to help cache accessors for smart pointer
+// like objects.
+//
+// These should be combined with CachedConstAccessorsLattice.
+// Beyond basic const accessors, smart pointers may have the following two
+// additional issues:
+//
+// 1) There may be multiple accessors for the same underlying object, e.g.
+//`operator->`, `operator*`, and `get`. Users may use a mixture of these
+//accessors, so the cache should unify them.
+//
+// 2) There may be non-const overloads of accessors. They are still safe to
+//cache, as they don't modify the container object.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+
+#include
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::dataflow {
+
+/// Matchers:
+/// For now, these match on any class with an `operator*` or `operator->`
+/// where the return types have a similar shape as std::unique_ptr
+/// and std::optional.
+///
+/// - `*` returns a reference to a type `T`
+/// - `->` returns a pointer to `T`
+/// - `get` returns a pointer to `T`
+/// - `value` returns a reference `T`
+///
+/// (1) The `T` should all match across the accessors (ignoring qualifiers).
+///
+/// (2) The specific accessor used in a call isn't required to be const,
+/// but the class must have a const overload of each accessor.
+///
+/// For now, we don't have customization to ignore certain classes.
+/// For example, if writing a ClangTidy check for `std::optional`, these
+/// would also match `std::optional`. In order to have special handlin
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/120102
>From f9b8cbaed4c01c2051cdcde105d6a9bca1684388 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Mon, 16 Dec 2024 16:06:43 +
Subject: [PATCH 1/2] [clang][dataflow] Add matchers for smart pointer
accessors to be cached
This is part 1 of caching for smart pointer accessors, building on top
of the CachedConstAccessorsLattice, which caches "normal" accessors.
Smart pointer accessors are a bit different in that they may:
- have aliases to access the same underlying data (but potentially
returning slightly different types like `&` vs `*`). Within a
"checked" sequence users may mix uses of the different aliases and the
check should apply to any of the spellings.
- may have non-const overloads in addition to the const version, where
the non-const doesn't actually modify the container
Part 2 will follow and add transfer functions utilities. It will also
add a user UncheckedOptionalAccessModel. We'd seen false positives when
nesting StatusOr> and optional>, etc. which this
can help address.
---
.../SmartPointerAccessorCaching.h | 63 ++
.../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../SmartPointerAccessorCaching.cpp | 134
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../SmartPointerAccessorCachingTest.cpp | 194 ++
5 files changed, 393 insertions(+)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
create mode 100644
clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
create mode 100644
clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
new file mode 100644
index 00..1adb63af4e724b
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -0,0 +1,63 @@
+//===-- SmartPointerAccessorCaching.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines utilities to help cache accessors for smart pointer
+// like objects.
+//
+// These should be combined with CachedConstAccessorsLattice.
+// Beyond basic const accessors, smart pointers may have the following two
+// additional issues:
+//
+// 1) There may be multiple accessors for the same underlying object, e.g.
+//`operator->`, `operator*`, and `get`. Users may use a mixture of these
+//accessors, so the cache should unify them.
+//
+// 2) There may be non-const overloads of accessors. They are still safe to
+//cache, as they don't modify the container object.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+
+#include
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::dataflow {
+
+/// Matchers:
+/// For now, these match on any class with an `operator*` or `operator->`
+/// where the return types have a similar shape as std::unique_ptr
+/// and std::optional.
+///
+/// - `*` returns a reference to a type `T`
+/// - `->` returns a pointer to `T`
+/// - `get` returns a pointer to `T`
+/// - `value` returns a reference `T`
+///
+/// (1) The `T` should all match across the accessors (ignoring qualifiers).
+///
+/// (2) The specific accessor used in a call isn't required to be const,
+/// but the class must have a const overload of each accessor.
+///
+/// For now, we don't have customization to ignore certain classes.
+/// For example, if writing a ClangTidy check for `std::optional`, these
+/// would also match `std::optional`. In order to have special handling
+/// for `std::optional`, we assume the (Matcher, TransferFunction) case
+/// with custom handling is ordered early so that these generic cases
+/// do not trigger.
+ast_matchers::internal::Matcher isSmartPointerLikeOperatorStar();
+ast_matchers::internal::Matcher isSmartPointerLikeOperatorArrow();
+ast_matchers::internal::Matcher isSmartPointerLikeValueMethodCall();
+ast_matchers::internal::Matcher isSmartPointerLikeGetMethodCall();
+
+} // namespace clang::dataflow
+
+#endif // LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
diff --git a/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
b/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
index 05cdaa7e27823d..0c30df8b4b194f 100644
--- a/clang/lib/Analysis/FlowSensitive/CMakeLists.txt
+++ b/clang/lib/Analysis/
[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
https://github.com/jvoung created
https://github.com/llvm/llvm-project/pull/120249
Part 2 (and final part) following
https://github.com/llvm/llvm-project/pull/120102
Allows users to do things like:
```
if (o->x.has_value()) {
((*o).x).value();
}
```
where the `->` and `*` are operator overload calls.
A user could instead extract the nested optional into a local variable
once instead of doing two accessor calls back to back, but currently
they are unsure why the code is flagged.
>From c526263a7accc434dbf6e93c2995ceb2f95873b8 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 17 Dec 2024 15:38:19 +
Subject: [PATCH] [clang][dataflow] Use smart pointer caching in unchecked
optional accessor
Part 2 (and final part) following
https://github.com/llvm/llvm-project/pull/120102
Allows users to do things like:
```
if (o->x.has_value()) {
((*o).x).value();
}
```
where the `->` and `*` are operator overload calls.
A user could instead extract the nested optional into a local variable
once instead of doing two accessor calls back to back, but currently
they are unsure why the code is flagged.
---
.../CachedConstAccessorsLattice.h | 41
.../SmartPointerAccessorCaching.h | 168 +++
.../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../Models/UncheckedOptionalAccessModel.cpp | 58 +-
.../SmartPointerAccessorCaching.cpp | 153 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 32 +++
.../SmartPointerAccessorCachingTest.cpp | 194 ++
.../UncheckedOptionalAccessModelTest.cpp | 50 +
9 files changed, 692 insertions(+), 6 deletions(-)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
create mode 100644
clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
create mode 100644
clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 48c5287367739a..6b5dacf9f66d2d 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -13,7 +13,9 @@
#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -71,10 +73,28 @@ template class CachedConstAccessorsLattice
: public Base {
/// Requirements:
///
/// - `CE` should return a location (GLValue or a record type).
+ ///
+ /// DEPRECATED: switch users to the below overload which takes Callee and
Type
+ /// directly.
StorageLocation *getOrCreateConstMethodReturnStorageLocation(
const RecordStorageLocation &RecordLoc, const CallExpr *CE,
Environment &Env, llvm::function_ref
Initialize);
+ /// Creates or returns a previously created `StorageLocation` associated with
+ /// a const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`,
+ /// and `Type` is the return type of `getFoo`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ ///
+ /// Requirements:
+ ///
+ /// - `Type` should return a location (GLValue or a record type).
+ StorageLocation &getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+ QualType Type, Environment &Env,
+ llvm::function_ref Initialize);
+
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
ConstMethodReturnValues.erase(&RecordLoc);
}
@@ -212,6 +232,27 @@
CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
return &Loc;
}
+template
+StorageLocation &
+CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
+const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+QualType Type, Environment &Env,
+llvm::function_ref Initialize) {
+ assert(Callee != nullptr);
+ assert(!Type.isNull());
+ assert(Type->isReferenceType() || Type->isRecordType());
+ auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
+ auto it = ObjMap.find(Callee);
+ if (it != ObjMap.end())
+return *it->second;
+
+ StorageLocation &Loc = Env.createStorageLocation(Type.getNonReferenceType());
+ Initialize(Loc);
+
+ ObjMap.insert({Callee, &Loc});
+ return Loc;
+}
+
} // namespace dataflow
} // namespace clang
diff --git
a/clang/include/clang/Analysis/Flow
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
@@ -0,0 +1,134 @@
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
+
+#include "clang/AST/CanonicalType.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/OperatorKinds.h"
+
+namespace clang::dataflow {
+
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxOperatorCallExpr;
+using ast_matchers::hasName;
+using ast_matchers::hasOverloadedOperatorName;
+using ast_matchers::ofClass;
+using ast_matchers::parameterCountIs;
+using ast_matchers::pointerType;
+using ast_matchers::referenceType;
+using ast_matchers::returns;
+
+bool hasSmartPointerClassShape(const CXXRecordDecl &RD, bool &HasGet,
+ bool &HasValue) {
+ // We may want to cache this search, but in current profiles it hasn't shown
+ // up as a hot spot (possibly because there aren't many hits, relatively).
+ bool HasArrow = false;
+ bool HasStar = false;
+ CanQualType StarReturnType, ArrowReturnType, GetReturnType, ValueReturnType;
+ for (const auto *MD : RD.methods()) {
+// We only consider methods that are const and have zero parameters.
+// It may be that there is a non-const overload for the method, but
+// there should at least be a const overload as well.
+if (!MD->isConst() || MD->getNumParams() != 0) {
+ continue;
+}
+if (MD->getOverloadedOperator() == OO_Star &&
+MD->getReturnType()->isReferenceType()) {
+ HasStar = true;
+ StarReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+} else if (MD->getOverloadedOperator() == OO_Arrow &&
+ MD->getReturnType()->isPointerType()) {
+ HasArrow = true;
+ ArrowReturnType =
+ MD->getReturnType()->getPointeeType()->getCanonicalTypeUnqualified();
+} else {
+ IdentifierInfo *II = MD->getIdentifier();
+ if (II == nullptr)
+continue;
+ if (II->isStr("get") && MD->getReturnType()->isPointerType()) {
+HasGet = true;
+GetReturnType = MD->getReturnType()
+->getPointeeType()
+->getCanonicalTypeUnqualified();
+ } else if (II->isStr("value") && MD->getReturnType()->isReferenceType())
{
+HasValue = true;
+ValueReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+ }
+}
+ }
+
+ if (!HasStar || !HasArrow || StarReturnType != ArrowReturnType)
+return false;
+ HasGet = HasGet && (GetReturnType == StarReturnType);
+ HasValue = HasValue && (ValueReturnType == StarReturnType);
+ return true;
+}
+
+} // namespace
+} // namespace clang::dataflow
+
+// AST_MATCHER macros create an "internal" namespace, so we put it in
+// its own anonymous namespace instead of in clang::dataflow.
+namespace {
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGet) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasGet;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasValue;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGetOrValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && (HasGet || HasValue);
+}
+
+} // namespace
+
+namespace clang::dataflow {
+
+ast_matchers::internal::Matcher isSmartPointerLikeOperatorStar() {
jvoung wrote:
Thanks -- will keep the idea of matching once and reusing the result in mind if
performance becomes an issue later!
https://github.com/llvm/llvm-project/pull/120102
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
@@ -0,0 +1,134 @@
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
+
+#include "clang/AST/CanonicalType.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/OperatorKinds.h"
+
+namespace clang::dataflow {
+
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxOperatorCallExpr;
+using ast_matchers::hasName;
+using ast_matchers::hasOverloadedOperatorName;
+using ast_matchers::ofClass;
+using ast_matchers::parameterCountIs;
+using ast_matchers::pointerType;
+using ast_matchers::referenceType;
+using ast_matchers::returns;
+
+bool hasSmartPointerClassShape(const CXXRecordDecl &RD, bool &HasGet,
+ bool &HasValue) {
+ // We may want to cache this search, but in current profiles it hasn't shown
+ // up as a hot spot (possibly because there aren't many hits, relatively).
+ bool HasArrow = false;
+ bool HasStar = false;
+ CanQualType StarReturnType, ArrowReturnType, GetReturnType, ValueReturnType;
+ for (const auto *MD : RD.methods()) {
+// We only consider methods that are const and have zero parameters.
+// It may be that there is a non-const overload for the method, but
+// there should at least be a const overload as well.
+if (!MD->isConst() || MD->getNumParams() != 0) {
jvoung wrote:
Done
https://github.com/llvm/llvm-project/pull/120102
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
@@ -0,0 +1,194 @@
+//===- unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Testing/TestAST.h"
+#include "llvm/ADT/StringRef.h"
+#include "gtest/gtest.h"
+
+namespace clang::dataflow {
+namespace {
+
+using clang::ast_matchers::match;
+
+template
+bool matches(llvm::StringRef Decls, llvm::StringRef TestInput,
+ MatcherT Matcher) {
+ TestAST InputAST(Decls.str() + TestInput.str());
+ return !match(Matcher, InputAST.context()).empty();
+}
+
+TEST(SmartPointerAccessorCachingTest, MatchesClassWithStarArrowGet) {
+ llvm::StringRef Decls(R"cc(
+namespace std {
+template
+struct unique_ptr {
+ T* operator->() const;
+ T& operator*() const;
+ T* get() const;
+};
+} // namespace std
+
+template
+using UniquePtrAlias = std::unique_ptr;
+
+struct S { int i; };
+ )cc");
+
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr P) { return (*P).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr P) { return P.get()->i;
}",
+ isSmartPointerLikeGetMethodCall()));
+
+ EXPECT_TRUE(matches(Decls, "int target(UniquePtrAlias P) { return P->i;
}",
+ isSmartPointerLikeOperatorArrow()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfUnexpectedReturnTypes) {
+ llvm::StringRef Decls(R"cc(
+namespace std {
+// unique_ptr isn't really like this, but we aren't matching by name
+template
+struct unique_ptr {
+ U* operator->() const;
+ T& operator*() const;
+ T* get() const;
+};
+} // namespace std
+
+struct S { int i; };
+struct T { int j; };
+ )cc");
+
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr P) { return (*P).i;
}",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr P) { return P->j; }",
+ isSmartPointerLikeOperatorArrow()));
+ // The class matching arguably accidentally matches, just because the
+ // instantiation is with S, S. Hopefully doesn't happen too much in real code
+ // with such operator* and operator-> overloads.
+ EXPECT_TRUE(matches(Decls,
+ "int target(std::unique_ptr P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfBinaryStar) {
+ llvm::StringRef Decls(R"cc(
+namespace std {
+template
+struct unique_ptr {
+ T* operator->() const;
+ T& operator*(int x) const;
+ T* get() const;
+};
+} // namespace std
+
+struct S { int i; };
+ )cc");
+
+ EXPECT_FALSE(
+ matches(Decls, "int target(std::unique_ptr P) { return (P * 10).i; }",
+ isSmartPointerLikeOperatorStar()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfNoConstOverloads) {
+ llvm::StringRef Decls(R"cc(
+namespace std {
+template
+struct unique_ptr {
+ T* operator->();
+ T& operator*();
+ T* get();
+};
+} // namespace std
+
+struct S { int i; };
+ )cc");
+
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr P) { return (*P).i; }",
+ isSmartPointerLikeOperatorStar()));
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_FALSE(
+ matches(Decls, "int target(std::unique_ptr P) { return P.get()->i; }",
+ isSmartPointerLikeGetMethodCall()));
+}
+
+TEST(SmartPointerAccessorCachingTest, NoMatchIfNoStarMethod) {
+ llvm::StringRef Decls(R"cc(
+namespace std {
+template
+struct unique_ptr {
+ T* operator->();
+ T* get();
+};
+} // namespace std
+
+struct S { int i; };
+ )cc");
+
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr P) { return P->i; }",
+ isSmartPointerLikeOperatorArrow()));
+ EXPECT_FALSE(matches(Decls,
+ "int target(std::unique_ptr P) { return P->i; }",
+ isSmartPointerLikeGetMethodCall()));
+}
+
+TEST(SmartPointerAccessorCachingTest, MatchesWithValueAnd
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
@@ -0,0 +1,134 @@
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
+
+#include "clang/AST/CanonicalType.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/OperatorKinds.h"
+
+namespace clang::dataflow {
+
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxOperatorCallExpr;
+using ast_matchers::hasName;
+using ast_matchers::hasOverloadedOperatorName;
+using ast_matchers::ofClass;
+using ast_matchers::parameterCountIs;
+using ast_matchers::pointerType;
+using ast_matchers::referenceType;
+using ast_matchers::returns;
+
+bool hasSmartPointerClassShape(const CXXRecordDecl &RD, bool &HasGet,
+ bool &HasValue) {
+ // We may want to cache this search, but in current profiles it hasn't shown
+ // up as a hot spot (possibly because there aren't many hits, relatively).
+ bool HasArrow = false;
+ bool HasStar = false;
+ CanQualType StarReturnType, ArrowReturnType, GetReturnType, ValueReturnType;
+ for (const auto *MD : RD.methods()) {
+// We only consider methods that are const and have zero parameters.
+// It may be that there is a non-const overload for the method, but
+// there should at least be a const overload as well.
+if (!MD->isConst() || MD->getNumParams() != 0) {
+ continue;
+}
+if (MD->getOverloadedOperator() == OO_Star &&
+MD->getReturnType()->isReferenceType()) {
+ HasStar = true;
+ StarReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+} else if (MD->getOverloadedOperator() == OO_Arrow &&
+ MD->getReturnType()->isPointerType()) {
+ HasArrow = true;
+ ArrowReturnType =
+ MD->getReturnType()->getPointeeType()->getCanonicalTypeUnqualified();
+} else {
+ IdentifierInfo *II = MD->getIdentifier();
+ if (II == nullptr)
+continue;
+ if (II->isStr("get") && MD->getReturnType()->isPointerType()) {
+HasGet = true;
+GetReturnType = MD->getReturnType()
+->getPointeeType()
+->getCanonicalTypeUnqualified();
+ } else if (II->isStr("value") && MD->getReturnType()->isReferenceType())
{
+HasValue = true;
+ValueReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+ }
+}
+ }
+
+ if (!HasStar || !HasArrow || StarReturnType != ArrowReturnType)
+return false;
+ HasGet = HasGet && (GetReturnType == StarReturnType);
+ HasValue = HasValue && (ValueReturnType == StarReturnType);
+ return true;
+}
+
+} // namespace
+} // namespace clang::dataflow
+
+// AST_MATCHER macros create an "internal" namespace, so we put it in
+// its own anonymous namespace instead of in clang::dataflow.
+namespace {
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGet) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasGet;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasValue;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGetOrValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && (HasGet || HasValue);
+}
+
+} // namespace
+
+namespace clang::dataflow {
+
+ast_matchers::internal::Matcher isSmartPointerLikeOperatorStar() {
jvoung wrote:
Ah yes, it wasn't clear how these are used! I uploaded a draft of the user to
https://github.com/llvm/llvm-project/pull/120249/files#diff-068c9724d855a9686b7ca6573bf3e966d856f2e4200d0eba45db0738e9c9e7afR1038
in case that helps (source examples in the test there).
I'm not too sure how to match the smart pointer class once and reuse (not as
familiar with the matcher binding yet!) across different call ASTs, but I can
imagine doing a pre-pass that computed smart pointer classes and then use that
in the matcher.
So far, I hadn't seen the naive approach be slow in profiles, so I hadn't tried
optimizing. If the matchers short circuit, then maybe it doesn't try to do the
smart pointer class checks often? For example, if it has to match the 0 param
check, and match the name first? With the names being disjoint, on an example
call `ptr.get().x.value();` it would match the `hasName("get")` and then do the
smart pointer class match on the type of `ptr` but it wouldn't have passed the
`hasOv
[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/120249
>From c526263a7accc434dbf6e93c2995ceb2f95873b8 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 17 Dec 2024 15:38:19 +
Subject: [PATCH] [clang][dataflow] Use smart pointer caching in unchecked
optional accessor
Part 2 (and final part) following
https://github.com/llvm/llvm-project/pull/120102
Allows users to do things like:
```
if (o->x.has_value()) {
((*o).x).value();
}
```
where the `->` and `*` are operator overload calls.
A user could instead extract the nested optional into a local variable
once instead of doing two accessor calls back to back, but currently
they are unsure why the code is flagged.
---
.../CachedConstAccessorsLattice.h | 41
.../SmartPointerAccessorCaching.h | 168 +++
.../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../Models/UncheckedOptionalAccessModel.cpp | 58 +-
.../SmartPointerAccessorCaching.cpp | 153 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 32 +++
.../SmartPointerAccessorCachingTest.cpp | 194 ++
.../UncheckedOptionalAccessModelTest.cpp | 50 +
9 files changed, 692 insertions(+), 6 deletions(-)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
create mode 100644
clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
create mode 100644
clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 48c5287367739a..6b5dacf9f66d2d 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -13,7 +13,9 @@
#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -71,10 +73,28 @@ template class CachedConstAccessorsLattice
: public Base {
/// Requirements:
///
/// - `CE` should return a location (GLValue or a record type).
+ ///
+ /// DEPRECATED: switch users to the below overload which takes Callee and
Type
+ /// directly.
StorageLocation *getOrCreateConstMethodReturnStorageLocation(
const RecordStorageLocation &RecordLoc, const CallExpr *CE,
Environment &Env, llvm::function_ref
Initialize);
+ /// Creates or returns a previously created `StorageLocation` associated with
+ /// a const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`,
+ /// and `Type` is the return type of `getFoo`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ ///
+ /// Requirements:
+ ///
+ /// - `Type` should return a location (GLValue or a record type).
+ StorageLocation &getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+ QualType Type, Environment &Env,
+ llvm::function_ref Initialize);
+
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
ConstMethodReturnValues.erase(&RecordLoc);
}
@@ -212,6 +232,27 @@
CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
return &Loc;
}
+template
+StorageLocation &
+CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
+const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+QualType Type, Environment &Env,
+llvm::function_ref Initialize) {
+ assert(Callee != nullptr);
+ assert(!Type.isNull());
+ assert(Type->isReferenceType() || Type->isRecordType());
+ auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
+ auto it = ObjMap.find(Callee);
+ if (it != ObjMap.end())
+return *it->second;
+
+ StorageLocation &Loc = Env.createStorageLocation(Type.getNonReferenceType());
+ Initialize(Loc);
+
+ ObjMap.insert({Callee, &Loc});
+ return Loc;
+}
+
} // namespace dataflow
} // namespace clang
diff --git
a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
new file mode 100644
index 00..e89e82c893d8cb
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -0,0 +1,168 @@
+//===-- SmartPointerAccessorCaching.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under
[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/120249
>From c526263a7accc434dbf6e93c2995ceb2f95873b8 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 17 Dec 2024 15:38:19 +
Subject: [PATCH 1/2] [clang][dataflow] Use smart pointer caching in unchecked
optional accessor
Part 2 (and final part) following
https://github.com/llvm/llvm-project/pull/120102
Allows users to do things like:
```
if (o->x.has_value()) {
((*o).x).value();
}
```
where the `->` and `*` are operator overload calls.
A user could instead extract the nested optional into a local variable
once instead of doing two accessor calls back to back, but currently
they are unsure why the code is flagged.
---
.../CachedConstAccessorsLattice.h | 41
.../SmartPointerAccessorCaching.h | 168 +++
.../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../Models/UncheckedOptionalAccessModel.cpp | 58 +-
.../SmartPointerAccessorCaching.cpp | 153 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 32 +++
.../SmartPointerAccessorCachingTest.cpp | 194 ++
.../UncheckedOptionalAccessModelTest.cpp | 50 +
9 files changed, 692 insertions(+), 6 deletions(-)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
create mode 100644
clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
create mode 100644
clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 48c5287367739a..6b5dacf9f66d2d 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -13,7 +13,9 @@
#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -71,10 +73,28 @@ template class CachedConstAccessorsLattice
: public Base {
/// Requirements:
///
/// - `CE` should return a location (GLValue or a record type).
+ ///
+ /// DEPRECATED: switch users to the below overload which takes Callee and
Type
+ /// directly.
StorageLocation *getOrCreateConstMethodReturnStorageLocation(
const RecordStorageLocation &RecordLoc, const CallExpr *CE,
Environment &Env, llvm::function_ref
Initialize);
+ /// Creates or returns a previously created `StorageLocation` associated with
+ /// a const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`,
+ /// and `Type` is the return type of `getFoo`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ ///
+ /// Requirements:
+ ///
+ /// - `Type` should return a location (GLValue or a record type).
+ StorageLocation &getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+ QualType Type, Environment &Env,
+ llvm::function_ref Initialize);
+
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
ConstMethodReturnValues.erase(&RecordLoc);
}
@@ -212,6 +232,27 @@
CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
return &Loc;
}
+template
+StorageLocation &
+CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
+const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+QualType Type, Environment &Env,
+llvm::function_ref Initialize) {
+ assert(Callee != nullptr);
+ assert(!Type.isNull());
+ assert(Type->isReferenceType() || Type->isRecordType());
+ auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
+ auto it = ObjMap.find(Callee);
+ if (it != ObjMap.end())
+return *it->second;
+
+ StorageLocation &Loc = Env.createStorageLocation(Type.getNonReferenceType());
+ Initialize(Loc);
+
+ ObjMap.insert({Callee, &Loc});
+ return Loc;
+}
+
} // namespace dataflow
} // namespace clang
diff --git
a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
new file mode 100644
index 00..e89e82c893d8cb
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -0,0 +1,168 @@
+//===-- SmartPointerAccessorCaching.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, u
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
@@ -0,0 +1,134 @@
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
+
+#include "clang/AST/CanonicalType.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/OperatorKinds.h"
+
+namespace clang::dataflow {
+
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxOperatorCallExpr;
+using ast_matchers::hasName;
+using ast_matchers::hasOverloadedOperatorName;
+using ast_matchers::ofClass;
+using ast_matchers::parameterCountIs;
+using ast_matchers::pointerType;
+using ast_matchers::referenceType;
+using ast_matchers::returns;
+
+bool hasSmartPointerClassShape(const CXXRecordDecl &RD, bool &HasGet,
+ bool &HasValue) {
+ // We may want to cache this search, but in current profiles it hasn't shown
+ // up as a hot spot (possibly because there aren't many hits, relatively).
+ bool HasArrow = false;
+ bool HasStar = false;
+ CanQualType StarReturnType, ArrowReturnType, GetReturnType, ValueReturnType;
+ for (const auto *MD : RD.methods()) {
+// We only consider methods that are const and have zero parameters.
+// It may be that there is a non-const overload for the method, but
+// there should at least be a const overload as well.
+if (!MD->isConst() || MD->getNumParams() != 0) {
+ continue;
+}
+if (MD->getOverloadedOperator() == OO_Star &&
+MD->getReturnType()->isReferenceType()) {
+ HasStar = true;
+ StarReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+} else if (MD->getOverloadedOperator() == OO_Arrow &&
+ MD->getReturnType()->isPointerType()) {
+ HasArrow = true;
+ ArrowReturnType =
+ MD->getReturnType()->getPointeeType()->getCanonicalTypeUnqualified();
+} else {
+ IdentifierInfo *II = MD->getIdentifier();
+ if (II == nullptr)
+continue;
+ if (II->isStr("get") && MD->getReturnType()->isPointerType()) {
+HasGet = true;
+GetReturnType = MD->getReturnType()
+->getPointeeType()
+->getCanonicalTypeUnqualified();
+ } else if (II->isStr("value") && MD->getReturnType()->isReferenceType())
{
+HasValue = true;
+ValueReturnType = MD->getReturnType()
+ .getNonReferenceType()
+ ->getCanonicalTypeUnqualified();
+ }
+}
+ }
+
+ if (!HasStar || !HasArrow || StarReturnType != ArrowReturnType)
+return false;
+ HasGet = HasGet && (GetReturnType == StarReturnType);
+ HasValue = HasValue && (ValueReturnType == StarReturnType);
+ return true;
+}
+
+} // namespace
+} // namespace clang::dataflow
+
+// AST_MATCHER macros create an "internal" namespace, so we put it in
+// its own anonymous namespace instead of in clang::dataflow.
+namespace {
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGet) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasGet;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && HasValue;
+}
+
+AST_MATCHER(clang::CXXRecordDecl, smartPointerClassWithGetOrValue) {
+ bool HasGet = false;
+ bool HasValue = false;
+ bool HasStarAndArrow =
+ clang::dataflow::hasSmartPointerClassShape(Node, HasGet, HasValue);
+ return HasStarAndArrow && (HasGet || HasValue);
+}
+
+} // namespace
+
+namespace clang::dataflow {
+
+ast_matchers::internal::Matcher isSmartPointerLikeOperatorStar() {
jvoung wrote:
Yes in the profiles I am seeing more time in matchers generally and not much in
the `.*martPointer.*` functions.
I tried running before/after on ~20 internal files (some with ~5000 lines of
code + 100 direct `#include`s). The geomean diff is 0.01%, though for two files
the time increased by 20% (~1s wall time). For most files there is a minor
decrease (perhaps caching reduces the work later?) and for one file the checker
time decreased by 15% (~0.4s wall time). In the profile of one of the 20%
increases, it seems the solver time increased (maybe the formulas get more
complicated in some way). The `smartPointerClassWithGetOrValue` function is
only called about 300 times in that example.
I also tried removing the check filter for `HasOptionalCallDescendant` here
https://github.com/llvm/llvm-project/blob/b5d02786be31f45ca5919b3b73e99d8958330f78/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp#L36
to simulate using this infrastructure in another checker that does
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
@@ -0,0 +1,63 @@
+//===-- SmartPointerAccessorCaching.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file defines utilities to help cache accessors for smart pointer
+// like objects.
+//
+// These should be combined with CachedConstAccessorsLattice.
+// Beyond basic const accessors, smart pointers may have the following two
+// additional issues:
+//
+// 1) There may be multiple accessors for the same underlying object, e.g.
+//`operator->`, `operator*`, and `get`. Users may use a mixture of these
+//accessors, so the cache should unify them.
+//
+// 2) There may be non-const overloads of accessors. They are still safe to
+//cache, as they don't modify the container object.
+//===--===//
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_SMARTPOINTERACCESSORCACHING_H
+
+#include
+
+#include "clang/AST/Decl.h"
+#include "clang/AST/Stmt.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+
+namespace clang::dataflow {
+
+/// Matchers:
+/// For now, these match on any class with an `operator*` or `operator->`
+/// where the return types have a similar shape as std::unique_ptr
+/// and std::optional.
+///
+/// - `*` returns a reference to a type `T`
+/// - `->` returns a pointer to `T`
+/// - `get` returns a pointer to `T`
+/// - `value` returns a reference `T`
+///
+/// (1) The `T` should all match across the accessors (ignoring qualifiers).
+///
+/// (2) The specific accessor used in a call isn't required to be const,
+/// but the class must have a const overload of each accessor.
+///
+/// For now, we don't have customization to ignore certain classes.
+/// For example, if writing a ClangTidy check for `std::optional`, these
+/// would also match `std::optional`. In order to have special handling
+/// for `std::optional`, we assume the (Matcher, TransferFunction) case
+/// with custom handling is ordered early so that these generic cases
+/// do not trigger.
+ast_matchers::internal::Matcher isSmartPointerLikeOperatorStar();
jvoung wrote:
Ah, thanks! done
https://github.com/llvm/llvm-project/pull/120102
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
@@ -0,0 +1,133 @@
+#include "clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h"
+
+#include "clang/AST/CanonicalType.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/OperatorKinds.h"
+
+namespace clang::dataflow {
+
+namespace {
+
+using ast_matchers::callee;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxOperatorCallExpr;
+using ast_matchers::hasName;
+using ast_matchers::hasOverloadedOperatorName;
+using ast_matchers::ofClass;
+using ast_matchers::parameterCountIs;
+using ast_matchers::pointerType;
+using ast_matchers::referenceType;
+using ast_matchers::returns;
+
+bool hasSmartPointerClassShape(const CXXRecordDecl &RD, bool &HasGet,
+ bool &HasValue) {
+ // We may want to cache this search, but in current profiles it hasn't shown
+ // up as a hot spot (possibly because there aren't many hits, relatively).
+ bool HasArrow = false;
+ bool HasStar = false;
+ CanQualType StarReturnType, ArrowReturnType, GetReturnType, ValueReturnType;
+ for (const auto *MD : RD.methods()) {
+// We only consider methods that are const and have zero parameters.
+// It may be that there is a non-const overload for the method, but
+// there should at least be a const overload as well.
+if (!MD->isConst() || MD->getNumParams() != 0)
+ continue;
+if (MD->getOverloadedOperator() == OO_Star &&
jvoung wrote:
Good point! done
https://github.com/llvm/llvm-project/pull/120102
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Add matchers for smart pointer accessors to be cached (PR #120102)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/120102 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Fix a missing break from a switch case -Wimplicit-fallthrough (PR #120739)
jvoung wrote: > Thanks for the fix! Thanks for helping merge! And sorry again about the breakage! https://github.com/llvm/llvm-project/pull/120739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
https://github.com/jvoung ready_for_review https://github.com/llvm/llvm-project/pull/120249 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/120249
>From c526263a7accc434dbf6e93c2995ceb2f95873b8 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 17 Dec 2024 15:38:19 +
Subject: [PATCH 1/4] [clang][dataflow] Use smart pointer caching in unchecked
optional accessor
Part 2 (and final part) following
https://github.com/llvm/llvm-project/pull/120102
Allows users to do things like:
```
if (o->x.has_value()) {
((*o).x).value();
}
```
where the `->` and `*` are operator overload calls.
A user could instead extract the nested optional into a local variable
once instead of doing two accessor calls back to back, but currently
they are unsure why the code is flagged.
---
.../CachedConstAccessorsLattice.h | 41
.../SmartPointerAccessorCaching.h | 168 +++
.../lib/Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../Models/UncheckedOptionalAccessModel.cpp | 58 +-
.../SmartPointerAccessorCaching.cpp | 153 ++
.../Analysis/FlowSensitive/CMakeLists.txt | 1 +
.../CachedConstAccessorsLatticeTest.cpp | 32 +++
.../SmartPointerAccessorCachingTest.cpp | 194 ++
.../UncheckedOptionalAccessModelTest.cpp | 50 +
9 files changed, 692 insertions(+), 6 deletions(-)
create mode 100644
clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
create mode 100644
clang/lib/Analysis/FlowSensitive/SmartPointerAccessorCaching.cpp
create mode 100644
clang/unittests/Analysis/FlowSensitive/SmartPointerAccessorCachingTest.cpp
diff --git
a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
index 48c5287367739a..6b5dacf9f66d2d 100644
--- a/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
+++ b/clang/include/clang/Analysis/FlowSensitive/CachedConstAccessorsLattice.h
@@ -13,7 +13,9 @@
#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE_CACHED_CONST_ACCESSORS_LATTICE_H
+#include "clang/AST/Decl.h"
#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
#include "clang/Analysis/FlowSensitive/DataflowEnvironment.h"
#include "clang/Analysis/FlowSensitive/DataflowLattice.h"
#include "clang/Analysis/FlowSensitive/StorageLocation.h"
@@ -71,10 +73,28 @@ template class CachedConstAccessorsLattice
: public Base {
/// Requirements:
///
/// - `CE` should return a location (GLValue or a record type).
+ ///
+ /// DEPRECATED: switch users to the below overload which takes Callee and
Type
+ /// directly.
StorageLocation *getOrCreateConstMethodReturnStorageLocation(
const RecordStorageLocation &RecordLoc, const CallExpr *CE,
Environment &Env, llvm::function_ref
Initialize);
+ /// Creates or returns a previously created `StorageLocation` associated with
+ /// a const method call `obj.getFoo()` where `RecordLoc` is the
+ /// `RecordStorageLocation` of `obj`, `Callee` is the decl for `getFoo`,
+ /// and `Type` is the return type of `getFoo`.
+ ///
+ /// The callback `Initialize` runs on the storage location if newly created.
+ ///
+ /// Requirements:
+ ///
+ /// - `Type` should return a location (GLValue or a record type).
+ StorageLocation &getOrCreateConstMethodReturnStorageLocation(
+ const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+ QualType Type, Environment &Env,
+ llvm::function_ref Initialize);
+
void clearConstMethodReturnValues(const RecordStorageLocation &RecordLoc) {
ConstMethodReturnValues.erase(&RecordLoc);
}
@@ -212,6 +232,27 @@
CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
return &Loc;
}
+template
+StorageLocation &
+CachedConstAccessorsLattice::getOrCreateConstMethodReturnStorageLocation(
+const RecordStorageLocation &RecordLoc, const FunctionDecl *Callee,
+QualType Type, Environment &Env,
+llvm::function_ref Initialize) {
+ assert(Callee != nullptr);
+ assert(!Type.isNull());
+ assert(Type->isReferenceType() || Type->isRecordType());
+ auto &ObjMap = ConstMethodReturnStorageLocations[&RecordLoc];
+ auto it = ObjMap.find(Callee);
+ if (it != ObjMap.end())
+return *it->second;
+
+ StorageLocation &Loc = Env.createStorageLocation(Type.getNonReferenceType());
+ Initialize(Loc);
+
+ ObjMap.insert({Callee, &Loc});
+ return Loc;
+}
+
} // namespace dataflow
} // namespace clang
diff --git
a/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
new file mode 100644
index 00..e89e82c893d8cb
--- /dev/null
+++ b/clang/include/clang/Analysis/FlowSensitive/SmartPointerAccessorCaching.h
@@ -0,0 +1,168 @@
+//===-- SmartPointerAccessorCaching.h ---*- C++
-*-===//
+//
+// Part of the LLVM Project, u
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung edited https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/115051
>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/5] [clang-tidy] Filter out googletest TUs in
bugprone-unchecked-optional-access
We currently do not handle ASSERT_TRUE(opt.has_value()) macros from
googletest (and other macros), so would see lots of false positives
in test TUs.
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
.../bugprone/UncheckedOptionalAccessCheck.h | 8 ++-
.../unchecked-optional-access/gtest/gtest.h | 24 +++
...cked-optional-access-ignore-googletest.cpp | 24 +++
4 files changed, 73 insertions(+), 2 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 0a0e212f345ed8..06deea0a446809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -40,10 +40,27 @@ void
UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
this);
}
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ is_test_tu_ = false;
+}
+
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
- if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ // The googletest assertion macros are not currently recognized, so we have
+ // many false positives in tests. So, do not check functions in a test TU.
+ if (is_test_tu_ ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+return;
+
+ // Look for two (public) googletest macros; if found, we'll mark this TU as a
+ // test TU. We look for ASSERT_TRUE because it is a problematic macro that
+ // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
+ if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
+is_test_tu_ = true;
return;
+ }
const auto *FuncDecl = Result.Nodes.getNodeAs(FuncID);
if (FuncDecl->isTemplated())
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 2d1570f7df8abb..3bd7ff9867b145 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -12,7 +12,6 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include
namespace clang::tidy::bugprone {
@@ -30,6 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)}
{}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
@@ -40,6 +40,12 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // Records whether the current TU includes the googletest headers, in which
+ // case we assume it is a test TU of some sort. The googletest assertion
+ // macros are not currently recognized, so we have many false positives in
+ // tests. So, we disable checking for all test TUs.
+ bool is_test_tu_ = false;
};
} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
new file mode 100644
index 00..8a9eeac94dbb77
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+
+// Mock version of googletest macros.
+
+#define GTEST_TEST(test_suite_name, test_name)
\
+ class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {
\
+ public:
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
jvoung wrote: Thanks! Ok that does that look useful (and I think the our other reviewer ymand@ wrote it =) https://reviews.llvm.org/D74840). - I'll work on using those in parallel - It will need to be extended a little to handle a few more matches (right now it's the binary comparison eq/ne, etc. not unary EXPECT_TRUE that is more common for optional) - Handle enough of the cases EXPECT_TRUE, EXPECT_EQ, EXPECT_NE, EXPECT_THAT... - Do some analysis on our false positive data to see what else remains. We have some data already of other issues besides macros, like state carried over from constructors. There may be cases still where we just can't reduce the FP (developers making shortcuts not doing the check, assuming that running the test will catch the issue) In the meantime, I updated the patch to make an option that is off by default. For all other users the behavior is unchanged, but we can opt in on our side. Can we add the option to use in the meantime, while I work on the macro handling in parallel? Thanks! https://github.com/llvm/llvm-project/pull/115051 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Filter out googletest TUs in bugprone-unchecked-optional-access (PR #115051)
https://github.com/jvoung updated
https://github.com/llvm/llvm-project/pull/115051
>From 8d83ec25ccdedad5f6a48e4a23176435f71a3e72 Mon Sep 17 00:00:00 2001
From: Jan Voung
Date: Tue, 5 Nov 2024 19:20:36 +
Subject: [PATCH 1/4] [clang-tidy] Filter out googletest TUs in
bugprone-unchecked-optional-access
We currently do not handle ASSERT_TRUE(opt.has_value()) macros from
googletest (and other macros), so would see lots of false positives
in test TUs.
---
.../bugprone/UncheckedOptionalAccessCheck.cpp | 19 ++-
.../bugprone/UncheckedOptionalAccessCheck.h | 8 ++-
.../unchecked-optional-access/gtest/gtest.h | 24 +++
...cked-optional-access-ignore-googletest.cpp | 24 +++
4 files changed, 73 insertions(+), 2 deletions(-)
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
create mode 100644
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access-ignore-googletest.cpp
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
index 0a0e212f345ed8..06deea0a446809 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp
@@ -40,10 +40,27 @@ void
UncheckedOptionalAccessCheck::registerMatchers(MatchFinder *Finder) {
this);
}
+void UncheckedOptionalAccessCheck::onStartOfTranslationUnit() {
+ // Reset the flag for each TU.
+ is_test_tu_ = false;
+}
+
void UncheckedOptionalAccessCheck::check(
const MatchFinder::MatchResult &Result) {
- if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+ // The googletest assertion macros are not currently recognized, so we have
+ // many false positives in tests. So, do not check functions in a test TU.
+ if (is_test_tu_ ||
+ Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred())
+return;
+
+ // Look for two (public) googletest macros; if found, we'll mark this TU as a
+ // test TU. We look for ASSERT_TRUE because it is a problematic macro that
+ // we don't (yet) support, and GTEST_TEST to disambiguate ASSERT_TRUE.
+ if (Result.Context->Idents.get("ASSERT_TRUE").hasMacroDefinition() &&
+ Result.Context->Idents.get("GTEST_TEST").hasMacroDefinition()) {
+is_test_tu_ = true;
return;
+ }
const auto *FuncDecl = Result.Nodes.getNodeAs(FuncID);
if (FuncDecl->isTemplated())
diff --git
a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
index 2d1570f7df8abb..3bd7ff9867b145 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.h
@@ -12,7 +12,6 @@
#include "../ClangTidyCheck.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
#include "clang/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.h"
-#include
namespace clang::tidy::bugprone {
@@ -30,6 +29,7 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
Options.getLocalOrGlobal("IgnoreSmartPointerDereference", false)}
{}
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void onStartOfTranslationUnit() override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
return LangOpts.CPlusPlus;
}
@@ -40,6 +40,12 @@ class UncheckedOptionalAccessCheck : public ClangTidyCheck {
private:
dataflow::UncheckedOptionalAccessModelOptions ModelOptions;
+
+ // Records whether the current TU includes the googletest headers, in which
+ // case we assume it is a test TU of some sort. The googletest assertion
+ // macros are not currently recognized, so we have many false positives in
+ // tests. So, we disable checking for all test TUs.
+ bool is_test_tu_ = false;
};
} // namespace clang::tidy::bugprone
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
new file mode 100644
index 00..8a9eeac94dbb77
--- /dev/null
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/gtest/gtest.h
@@ -0,0 +1,24 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_GTEST_GTEST_H_
+
+// Mock version of googletest macros.
+
+#define GTEST_TEST(test_suite_name, test_name)
\
+ class GTEST_TEST_CLASS_NAME_(test_suite_name, test_name) {
\
+ public:
[clang] [clang][dataflow] Use smart pointer caching in unchecked optional accessor (PR #120249)
https://github.com/jvoung closed https://github.com/llvm/llvm-project/pull/120249 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
