[clang] [clang] Fix the post-filtering heuristic for GSLPointer. (PR #114044)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 4 00:21:42 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Haojian Wu (hokein)
<details>
<summary>Changes</summary>
The lifetime analyzer processes GSL pointers:
- when encountering a constructor for a `gsl::pointer`, the analyzer continues traversing the constructor argument, regardless of whether the parameter has a `lifetimebound` annotation. This aims to catch cases where a GSL pointer is constructed from a GSL owner, either directly (e.g., `FooPointer(FooOwner)`) or through a chain of GSL pointers (e.g., `FooPointer(FooPointer(FooOwner))`);
- When a temporary object is reported in the callback, the analyzer has heuristics to exclude non-owner types, aiming to avoid false positives (like `FooPointer(FooPointer())`).
In the problematic case of `return foo.get();`:
- When the analyzer reports the local object `foo`, the `Path` is `[GslPointerInit, Lifetimebound]`.
- The `Path` goes through [`pathOnlyHandlesGslPointer`](https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/CheckExprLifetime.cpp#L1136) and isn’t filtered out by the [[heuristics]](because `foo` is an owner type), the analyzer treats it as the `FooPointer(FooOwner())` scenario, thus triggering a diagnostic.
Filtering out base on the object 'foo' is wrong, because the GSLPointer is constructed from the return result of the `foo.get()`. The patch fixes this by teaching the heuristic to use the return result (only `const GSLOwner&` is considered) of the lifetimebound annotated function.
---
Full diff: https://github.com/llvm/llvm-project/pull/114044.diff
3 Files Affected:
- (modified) clang/docs/ReleaseNotes.rst (+2)
- (modified) clang/lib/Sema/CheckExprLifetime.cpp (+92-21)
- (modified) clang/test/Sema/warn-lifetime-analysis-nocfg.cpp (+45-3)
``````````diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1372e49dfac03c..d3f346eb71e951 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -464,6 +464,8 @@ Improvements to Clang's diagnostics
- Clang now diagnoses ``[[deprecated]]`` attribute usage on local variables (#GH90073).
+- Fix false positives when `[[gsl::Owner/Pointer]]` and `[[clang::lifetimebound]]` are used together.
+
Improvements to Clang's time-trace
----------------------------------
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index a1a402b4a2b530..d1e8cc9f9b075c 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -1093,6 +1093,87 @@ static bool pathOnlyHandlesGslPointer(const IndirectLocalPath &Path) {
}
return false;
}
+// Result of analyzing the Path for GSLPointer.
+enum AnalysisResult {
+ // Path does not correspond to a GSLPointer.
+ NotGSLPointer,
+
+ // A relevant case was identified.
+ Report,
+ // Stop the entire traversal.
+ Abandon,
+ // Skip this step and continue traversing inner AST nodes.
+ Skip,
+};
+// Analyze cases where a GSLPointer is initialized or assigned from a
+// temporary owner object.
+static AnalysisResult analyzePathForGSLPointer(const IndirectLocalPath &Path,
+ Local L) {
+ if (!pathOnlyHandlesGslPointer(Path))
+ return NotGSLPointer;
+
+ // At this point, Path represents a series of operations involving a
+ // GSLPointer, either in the process of initialization or assignment.
+
+ // Note: A LifetimeBoundCall can appear interleaved in this sequence.
+ // For example:
+ // const std::string& Ref(const std::string& a [[clang::lifetimebound]]);
+ // string_view abc = Ref(std::string());
+ // The "Path" is [GSLPointerInit, LifetimeboundCall], where "L" is the
+ // temporary "std::string()" object. We need to check if the function with the
+ // lifetimebound attribute returns a "owner" type.
+ if (Path.back().Kind == IndirectLocalPathEntry::LifetimeBoundCall) {
+ // The lifetimebound applies to the implicit object parameter of a method.
+ if (const auto *Method = llvm::dyn_cast<CXXMethodDecl>(Path.back().D)) {
+ if (Method->getReturnType()->isReferenceType() &&
+ isRecordWithAttr<OwnerAttr>(
+ Method->getReturnType()->getPointeeType()))
+ return Report;
+ return Abandon;
+ }
+ // The lifetimebound applies to a function parameter.
+ const auto *PD = llvm::dyn_cast<ParmVarDecl>(Path.back().D);
+ if (const auto *FD = llvm::dyn_cast<FunctionDecl>(PD->getDeclContext())) {
+ if (isa<CXXConstructorDecl>(FD)) {
+ // Constructor case: the parameter is annotated with lifetimebound
+ // e.g., GSLPointer(const S& s [[clang::lifetimebound]])
+ // We still respect this case even the type S is not an owner.
+ return Report;
+ }
+ // For regular functions, check if the return type has an Owner attribute.
+ // e.g., const GSLOwner& func(const Foo& foo [[clang::lifetimebound]])
+ if (FD->getReturnType()->isReferenceType() &&
+ isRecordWithAttr<OwnerAttr>(FD->getReturnType()->getPointeeType()))
+ return Report;
+ }
+ return Abandon;
+ }
+
+ if (isa<DeclRefExpr>(L)) {
+ // We do not want to follow the references when returning a pointer
+ // originating from a local owner to avoid the following false positive:
+ // int &p = *localUniquePtr;
+ // someContainer.add(std::move(localUniquePtr));
+ // return p;
+ if (!pathContainsInit(Path) && isRecordWithAttr<OwnerAttr>(L->getType()))
+ return Report;
+ return Abandon;
+ }
+
+ // The GSLPointer is from a temporary object.
+ auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L);
+
+ bool IsGslPtrValueFromGslTempOwner =
+ MTE && !MTE->getExtendingDecl() &&
+ isRecordWithAttr<OwnerAttr>(MTE->getType());
+ // Skipping a chain of initializing gsl::Pointer annotated objects.
+ // We are looking only for the final source to find out if it was
+ // a local or temporary owner or the address of a local
+ // variable/param.
+ if (!IsGslPtrValueFromGslTempOwner)
+ return Skip;
+ return Report;
+}
static bool isAssignmentOperatorLifetimeBound(CXXMethodDecl *CMD) {
if (!CMD)
@@ -1131,27 +1212,17 @@ static void checkExprLifetimeImpl(Sema &SemaRef,
auto *MTE = dyn_cast<MaterializeTemporaryExpr>(L);
- bool IsGslPtrValueFromGslTempOwner = false;
- if (pathOnlyHandlesGslPointer(Path)) {
- if (isa<DeclRefExpr>(L)) {
- // We do not want to follow the references when returning a pointer
- // originating from a local owner to avoid the following false positive:
- // int &p = *localUniquePtr;
- // someContainer.add(std::move(localUniquePtr));
- // return p;
- if (pathContainsInit(Path) ||
- !isRecordWithAttr<OwnerAttr>(L->getType()))
- return false;
- } else {
- IsGslPtrValueFromGslTempOwner =
- MTE && !MTE->getExtendingDecl() &&
- isRecordWithAttr<OwnerAttr>(MTE->getType());
- // Skipping a chain of initializing gsl::Pointer annotated objects.
- // We are looking only for the final source to find out if it was
- // a local or temporary owner or the address of a local variable/param.
- if (!IsGslPtrValueFromGslTempOwner)
- return true;
- }
+ bool IsGslPtrValueFromGslTempOwner = true;
+ switch (analyzePathForGSLPointer(Path, L)) {
+ case Abandon:
+ return false;
+ case Skip:
+ return true;
+ case NotGSLPointer:
+ IsGslPtrValueFromGslTempOwner = false;
+ LLVM_FALLTHROUGH;
+ case Report:
+ break;
}
switch (LK) {
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 6a2af01ea5116c..3b237e99dd3b33 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -727,8 +727,9 @@ struct [[gsl::Pointer]] Span {
// Pointer from Owner<Pointer>
std::string_view test5() {
- std::string_view a = StatusOr<std::string_view>().valueLB(); // expected-warning {{object backing the pointer will be dest}}
- return StatusOr<std::string_view>().valueLB(); // expected-warning {{returning address of local temporary}}
+ // The Owner<Pointer> doesn't own the object which its inner pointer points to.
+ std::string_view a = StatusOr<std::string_view>().valueLB(); // OK
+ return StatusOr<std::string_view>().valueLB(); // OK
// No dangling diagnostics on non-lifetimebound methods.
std::string_view b = StatusOr<std::string_view>().valueNoLB();
@@ -775,7 +776,7 @@ Span<std::string> test10(StatusOr<std::vector<std::string>> aa) {
// Pointer<Owner>> from Owner<Pointer<Owner>>
Span<std::string> test11(StatusOr<Span<std::string>> aa) {
- return aa.valueLB(); // expected-warning {{address of stack memory}}
+ return aa.valueLB(); // OK
return aa.valueNoLB(); // OK.
}
@@ -793,3 +794,44 @@ void test13() {
}
} // namespace GH100526
+
+namespace LifetimeboundInterleave {
+
+const std::string& Ref(const std::string& abc [[clang::lifetimebound]]);
+std::string_view test1() {
+ std::string_view t1 = Ref(std::string()); // expected-warning {{object backing}}
+ t1 = Ref(std::string()); // expected-warning {{object backing}}
+ return Ref(std::string()); // expected-warning {{returning address}}
+}
+
+template <typename T>
+struct Foo {
+ const T& get() const [[clang::lifetimebound]];
+ const T& getNoLB() const;
+};
+std::string_view test2(Foo<std::string> r1, Foo<std::string_view> r2) {
+ std::string_view t1 = Foo<std::string>().get(); // expected-warning {{object backing}}
+ t1 = Foo<std::string>().get(); // expected-warning {{object backing}}
+ return r1.get(); // expected-warning {{address of stack}}
+
+ std::string_view t2 = Foo<std::string_view>().get();
+ t2 = Foo<std::string_view>().get();
+ return r2.get();
+
+ // no warning on no-LB-annotated method.
+ std::string_view t3 = Foo<std::string>().getNoLB();
+ t3 = Foo<std::string>().getNoLB();
+ return r1.getNoLB();
+}
+
+struct Bar {};
+struct [[gsl::Pointer]] Pointer {
+ Pointer(const Bar & bar [[clang::lifetimebound]]);
+};
+Pointer test3(Bar bar) {
+ Pointer p = Pointer(Bar()); // expected-warning {{temporary}}
+ p = Pointer(Bar()); // expected-warning {{object backing}}
+ return bar; // expected-warning {{address of stack}}
+}
+
+} // namespace LifetimeboundInterleave
``````````
</details>
https://github.com/llvm/llvm-project/pull/114044
More information about the cfe-commits
mailing list