[clang] Extend lifetime analysis to support assignments for pointer-like objects. #99032
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…en pointer-like objects.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Haojian Wu (hokein) ChangesThis is a follow-up patch to #96475 to detect dangling assignments for C++ pointer-like objects (classes annotated with the Similar to the behavior for built-in pointer types, if a temporary owner ( This new warning is I have initially tested this patch on our internal codebase, and it has identified many use-after-free bugs, primarily related to Full diff: https://github.com/llvm/llvm-project/pull/99032.diff 8 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 969856a8f978c..26dcd54d65547 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -713,6 +713,9 @@ Improvements to Clang's diagnostics
- Clang now diagnoses integer constant expressions that are folded to a constant value as an extension in more circumstances. Fixes #GH59863
+- Clang now diagnoses dangling assignments for pointer-like objects (annotated with `[[gsl::Pointer]]`) under `-Wdangling-assignment-gsl` (off by default)
+ Fixes #GH63310.
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 2241f8481484e..e0d97c6ce99db 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -449,6 +449,7 @@ def LogicalNotParentheses: DiagGroup<"logical-not-parentheses">;
def ShiftOpParentheses: DiagGroup<"shift-op-parentheses">;
def OverloadedShiftOpParentheses: DiagGroup<"overloaded-shift-op-parentheses">;
def DanglingAssignment: DiagGroup<"dangling-assignment">;
+def DanglingAssignmentGsl : DiagGroup<"dangling-assignment-gsl">;
def DanglingElse: DiagGroup<"dangling-else">;
def DanglingField : DiagGroup<"dangling-field">;
def DanglingInitializerList : DiagGroup<"dangling-initializer-list">;
@@ -457,6 +458,7 @@ def ReturnStackAddress : DiagGroup<"return-stack-address">;
// Name of this warning in GCC
def : DiagGroup<"return-local-addr", [ReturnStackAddress]>;
def Dangling : DiagGroup<"dangling", [DanglingAssignment,
+ DanglingAssignmentGsl,
DanglingField,
DanglingInitializerList,
DanglingGsl,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 52ff4b026a60e..94e6d5c1322ce 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10121,6 +10121,9 @@ def warn_dangling_lifetime_pointer : Warning<
"object backing the pointer "
"will be destroyed at the end of the full-expression">,
InGroup<DanglingGsl>;
+def warn_dangling_lifetime_pointer_assignment : Warning<"object backing the "
+ "pointer %0 will be destroyed at the end of the full-expression">,
+ InGroup<DanglingAssignmentGsl>, DefaultIgnore;
def warn_new_dangling_initializer_list : Warning<
"array backing "
"%select{initializer list subobject of the allocated object|"
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 995e4cbadacfe..ff32ef8c4a23c 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -191,7 +191,8 @@ struct IndirectLocalPathEntry {
TemporaryCopy,
LambdaCaptureInit,
GslReferenceInit,
- GslPointerInit
+ GslPointerInit,
+ GslPointerAssignment,
} Kind;
Expr *E;
union {
@@ -337,7 +338,8 @@ static void handleGslAnnotatedTypes(IndirectLocalPath &Path, Expr *Call,
for (const IndirectLocalPathEntry &PE : llvm::reverse(Path)) {
if (PE.Kind == IndirectLocalPathEntry::GslReferenceInit)
continue;
- if (PE.Kind == IndirectLocalPathEntry::GslPointerInit)
+ if (PE.Kind == IndirectLocalPathEntry::GslPointerInit ||
+ PE.Kind == IndirectLocalPathEntry::GslPointerAssignment)
return;
break;
}
@@ -937,6 +939,7 @@ static SourceRange nextPathEntryRange(const IndirectLocalPath &Path, unsigned I,
case IndirectLocalPathEntry::TemporaryCopy:
case IndirectLocalPathEntry::GslReferenceInit:
case IndirectLocalPathEntry::GslPointerInit:
+ case IndirectLocalPathEntry::GslPointerAssignment:
// These exist primarily to mark the path as not permitting or
// supporting lifetime extension.
break;
@@ -966,7 +969,8 @@ static bool pathOnlyInitializesGslPointer(IndirectLocalPath &Path) {
if (It.Kind == IndirectLocalPathEntry::LifetimeBoundCall)
continue;
return It.Kind == IndirectLocalPathEntry::GslPointerInit ||
- It.Kind == IndirectLocalPathEntry::GslReferenceInit;
+ It.Kind == IndirectLocalPathEntry::GslReferenceInit ||
+ It.Kind == IndirectLocalPathEntry::GslPointerAssignment;
}
return false;
}
@@ -975,7 +979,8 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
const InitializedEntity *InitEntity,
const InitializedEntity *ExtendingEntity,
LifetimeKind LK,
- const AssignedEntity *AEntity, Expr *Init) {
+ const AssignedEntity *AEntity, Expr *Init,
+ bool EnableLifetimeWarnings) {
assert((AEntity && LK == LK_Assignment) ||
(InitEntity && LK != LK_Assignment));
// If this entity doesn't have an interesting lifetime, don't bother looking
@@ -1073,14 +1078,16 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
}
case LK_Assignment: {
- if (!MTE)
+ if (!MTE || pathContainsInit(Path))
return false;
assert(shouldLifetimeExtendThroughPath(Path) ==
PathLifetimeKind::NoExtend &&
"No lifetime extension for assignments");
- if (!pathContainsInit(Path))
- SemaRef.Diag(DiagLoc, diag::warn_dangling_pointer_assignment)
- << AEntity->LHS << DiagRange;
+ SemaRef.Diag(DiagLoc,
+ IsGslPtrInitWithGslTempOwner
+ ? diag::warn_dangling_lifetime_pointer_assignment
+ : diag::warn_dangling_pointer_assignment)
+ << AEntity->LHS << DiagRange;
return false;
}
case LK_MemInitializer: {
@@ -1226,6 +1233,7 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
case IndirectLocalPathEntry::TemporaryCopy:
case IndirectLocalPathEntry::GslPointerInit:
case IndirectLocalPathEntry::GslReferenceInit:
+ case IndirectLocalPathEntry::GslPointerAssignment:
// FIXME: Consider adding a note for these.
break;
@@ -1265,9 +1273,11 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
return false;
};
- bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored(
- diag::warn_dangling_lifetime_pointer, SourceLocation());
llvm::SmallVector<IndirectLocalPathEntry, 8> Path;
+ if (EnableLifetimeWarnings && LK == LK_Assignment &&
+ isRecordWithAttr<PointerAttr>(AEntity->LHS->getType()))
+ Path.push_back({IndirectLocalPathEntry::GslPointerAssignment, Init});
+
if (Init->isGLValue())
visitLocalsRetainedByReferenceBinding(Path, Init, RK_ReferenceBinding,
TemporaryVisitor,
@@ -1284,16 +1294,26 @@ void checkExprLifetime(Sema &SemaRef, const InitializedEntity &Entity,
auto LTResult = getEntityLifetime(&Entity);
LifetimeKind LK = LTResult.getInt();
const InitializedEntity *ExtendingEntity = LTResult.getPointer();
- checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init);
+ bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored(
+ diag::warn_dangling_lifetime_pointer, SourceLocation());
+ checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init,
+ EnableLifetimeWarnings);
}
void checkExprLifetime(Sema &SemaRef, const AssignedEntity &Entity,
Expr *Init) {
- if (!Entity.LHS->getType()->isPointerType()) // builtin pointer type
+ bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored(
+ diag::warn_dangling_lifetime_pointer, SourceLocation());
+ bool RunAnalysis = Entity.LHS->getType()->isPointerType() ||
+ (EnableLifetimeWarnings &&
+ isRecordWithAttr<PointerAttr>(Entity.LHS->getType()));
+
+ if (!RunAnalysis)
return;
+
checkExprLifetimeImpl(SemaRef, /*InitEntity=*/nullptr,
/*ExtendingEntity=*/nullptr, LK_Assignment, &Entity,
- Init);
+ Init, EnableLifetimeWarnings);
}
} // namespace clang::sema
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 074062ebbb594..1db06ac05ddea 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -10,6 +10,7 @@
//
//===----------------------------------------------------------------------===//
+#include "CheckExprLifetime.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/ASTLambda.h"
#include "clang/AST/CXXInheritance.h"
@@ -14714,10 +14715,12 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation OpLoc,
FnDecl))
return ExprError();
- // Check for a self move.
- if (Op == OO_Equal)
+ if (Op == OO_Equal) {
+ // Check for a self move.
DiagnoseSelfMove(Args[0], Args[1], OpLoc);
-
+ // lifetime check.
+ checkExprLifetime(*this, AssignedEntity{Args[0]}, Args[1]);
+ }
if (ImplicitThis) {
QualType ThisType = Context.getPointerType(ImplicitThis->getType());
QualType ThisTypeFromDecl = Context.getPointerType(
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp
index 60b8f3ddedcd1..d1266027bdd34 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg-disabled.cpp
@@ -21,3 +21,7 @@ MyIntPointer g() {
MyIntOwner o;
return o; // No warning, it is disabled.
}
+
+void h(MyIntPointer p) {
+ p = MyIntOwner(); // No warning, it is disabled.
+}
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index b3ca173c1fdbc..09dfb2b5d96a8 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -120,11 +120,13 @@ MyLongPointerFromConversion global2;
void initLocalGslPtrWithTempOwner() {
MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
- p = MyIntOwner{}; // TODO ?
- global = MyIntOwner{}; // TODO ?
+ MyIntPointer pp = p = MyIntOwner{}; // expected-warning {{object backing the pointer p will be}}
+ p = MyIntOwner{}; // expected-warning {{object backing the pointer p }}
+ pp = p; // no warning
+ global = MyIntOwner{}; // expected-warning {{object backing the pointer global }}
MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
- p2 = MyLongOwnerWithConversion{}; // TODO ?
- global2 = MyLongOwnerWithConversion{}; // TODO ?
+ p2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer p2 }}
+ global2 = MyLongOwnerWithConversion{}; // expected-warning {{object backing the pointer global2 }}
}
namespace __gnu_cxx {
@@ -170,6 +172,7 @@ struct basic_string_view {
basic_string_view(const T *);
const T *begin() const;
};
+using string_view = basic_string_view<char>;
template<class _Mystr> struct iter {
iter& operator-=(int);
@@ -188,7 +191,7 @@ struct basic_string {
operator basic_string_view<T> () const;
using const_iterator = iter<T>;
};
-
+using string = basic_string<char>;
template<typename T>
struct unique_ptr {
@@ -346,6 +349,12 @@ void handleTernaryOperator(bool cond) {
std::basic_string_view<char> v = cond ? def : ""; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
}
+std::string operator+(std::string_view s1, std::string_view s2);
+void danglingStringviewAssignment(std::string_view a1, std::string_view a2) {
+ a1 = std::string(); // expected-warning {{object backing}}
+ a2 = a1 + a1; // expected-warning {{object backing}}
+}
+
std::reference_wrapper<int> danglingPtrFromNonOwnerLocal() {
int i = 5;
return i; // TODO
diff --git a/clang/test/SemaCXX/warn-dangling-local.cpp b/clang/test/SemaCXX/warn-dangling-local.cpp
index 2808a4c01f88d..5ad5013b6f025 100644
--- a/clang/test/SemaCXX/warn-dangling-local.cpp
+++ b/clang/test/SemaCXX/warn-dangling-local.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -std=c++11 %s
+// RUN: %clang_cc1 -verify -std=c++11 -Wdangling-assignment-gsl %s
using T = int[];
@@ -34,6 +34,6 @@ struct basic_string {
};
} // namespace std
void test(const char* a) {
- // verify we're emitting the `-Wdangling-assignment` warning.
+ // verify we're emitting the `-Wdangling-assignment-gsl` warning.
a = std::basic_string().c_str(); // expected-warning {{object backing the pointer a will be destroyed at the end of the full-expression}}
}
|
|
Any reason why it is not enabled by default for clang-19? Are there any known false positives or just precautions? |
Xazax-hun
approved these changes
Jul 16, 2024
clang/lib/Sema/CheckExprLifetime.cpp
Outdated
| SemaRef.Diag(DiagLoc, diag::warn_dangling_pointer_assignment) | ||
| << AEntity->LHS << DiagRange; | ||
| SemaRef.Diag(DiagLoc, | ||
| IsGslPtrInitWithGslTempOwner |
There was a problem hiding this comment.
IsGslPtrInitWithGslTempOwner might no longer be an accurate name as it is also used for assignments. Naming is hard, I am ok leaving this as is. Alternatively, it could be something like IsGslPtrValueFromGslTempOwner.
There was a problem hiding this comment.
Done, renamed to IsGslPtrValueFromGslTempOwner.
It's primarily for caution. We're close to the clang 19 release cut, so I want to be conservative here. I haven't found any false positives with the current implementation, but I'd prefer to ensure stability before making it the default. |
Xazax-hun
reviewed
Jul 16, 2024
clang/lib/Sema/CheckExprLifetime.cpp
Outdated
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init); | ||
| bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored( | ||
| diag::warn_dangling_lifetime_pointer, SourceLocation()); | ||
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init, |
There was a problem hiding this comment.
Not really a problem since I do not expect people to turn off the default dangling warnings, but I wonder if we actually run this code when all dangling related diagnostics are off.
There was a problem hiding this comment.
We still need to run the code (not the gsl-specific part) even if we disable all dangling diagnostics. Because this code has the functionality that performs the lifetime extension when needed (see https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/CheckExprLifetime.cpp#L1043-L1045).
I totally understand that. That being said, I think it should be really easy to cherry-pick turning a warning off to the release branch, so I'd be also OK with the alternative approach of releasing a release candidate with the warning on and only disabling it in a cherry-pick if there are some users experiencing some issues. |
|
To clarify, I don't have strong feelings about whether the warning is on or off, I just wanted to point out that I would not be concerned about the short period before the feature work cutoff date. There is still some time to stabilize after the branching. |
usx95
reviewed
Jul 16, 2024
shafik
reviewed
Jul 17, 2024
clang/lib/Sema/CheckExprLifetime.cpp
Outdated
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init); | ||
| bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored( | ||
| diag::warn_dangling_lifetime_pointer, SourceLocation()); | ||
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init, |
There was a problem hiding this comment.
Suggested change
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init, | |
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, /*AEntity=*/nullptr, Init, |
nitpick
hokein
commented
Jul 17, 2024
clang/lib/Sema/CheckExprLifetime.cpp
Outdated
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init); | ||
| bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored( | ||
| diag::warn_dangling_lifetime_pointer, SourceLocation()); | ||
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init, |
There was a problem hiding this comment.
We still need to run the code (not the gsl-specific part) even if we disable all dangling diagnostics. Because this code has the functionality that performs the lifetime extension when needed (see https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/CheckExprLifetime.cpp#L1043-L1045).
clang/lib/Sema/CheckExprLifetime.cpp
Outdated
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init); | ||
| bool EnableLifetimeWarnings = !SemaRef.getDiagnostics().isIgnored( | ||
| diag::warn_dangling_lifetime_pointer, SourceLocation()); | ||
| checkExprLifetimeImpl(SemaRef, &Entity, ExtendingEntity, LK, nullptr, Init, |
clang/lib/Sema/CheckExprLifetime.cpp
Outdated
| SemaRef.Diag(DiagLoc, diag::warn_dangling_pointer_assignment) | ||
| << AEntity->LHS << DiagRange; | ||
| SemaRef.Diag(DiagLoc, | ||
| IsGslPtrInitWithGslTempOwner |
There was a problem hiding this comment.
Done, renamed to IsGslPtrValueFromGslTempOwner.
ilya-biryukov
approved these changes
Jul 17, 2024
| @@ -34,6 +34,6 @@ struct basic_string { | |||
| }; | |||
| } // namespace std | |||
| void test(const char* a) { | |||
| // verify we're emitting the `-Wdangling-assignment` warning. | |||
| // verify we're emitting the `-Wdangling-assignment-gsl` warning. | |||
| a = std::basic_string().c_str(); // expected-warning {{object backing the pointer a will be destroyed at the end of the full-expression}} | |||
There was a problem hiding this comment.
I'm a little confused here, since a is of type const char*, why do we mention the -gsl version of the warning here?
There was a problem hiding this comment.
The warning is triggered by the implicit gsl::Owner attribute on std::basic_string. Without this attribute, the warning would not be issued.
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
…ike objects. (#99032) Summary: This is a follow-up patch to #96475 to detect dangling assignments for C++ pointer-like objects (classes annotated with the `[[gsl::Pointer]]`). Fixes #63310. Similar to the behavior for built-in pointer types, if a temporary owner (`[[gsl::Owner]]`) object is assigned to a pointer-like class object, and this temporary object is destroyed at the end of the full assignment expression, the assignee pointer is considered dangling. In such cases, clang will emit a warning: ``` /tmp/t.cpp:7:20: warning: object backing the pointer my_string_view will be destroyed at the end of the full-expression [-Wdangling-assignment-gsl] 7 | my_string_view = CreateString(); | ^~~~~~~~~~~~~~ 1 warning generated. ``` This new warning is `-Wdangling-assignment-gsl`. It is initially disabled, but I intend to enable it by default in clang 20. I have initially tested this patch on our internal codebase, and it has identified many use-after-free bugs, primarily related to `string_view`. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251757
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up patch to #96475 to detect dangling assignments for C++ pointer-like objects (classes annotated with the
[[gsl::Pointer]]). Fixes #63310.Similar to the behavior for built-in pointer types, if a temporary owner (
[[gsl::Owner]]) object is assigned to a pointer-like class object, and this temporary object is destroyed at the end of the full assignment expression, the assignee pointer is considered dangling. In such cases, clang will emit a warning:This new warning is
-Wdangling-assignment-gsl. It is initially disabled, but I intend to enable it by default in clang 20.I have initially tested this patch on our internal codebase, and it has identified many use-after-free bugs, primarily related to
string_view.