[clang] Fix dangling false positives for conditional operators.

[hokein]

When analyzing a dangling gsl pointer, we currently filter out all field access MemberExpr to avoid common false positives (string_view sv = Temp().sv), However, this filter only applies to direct MemberExpr instances, leaving the conditional operator as an escaping example (GSLPointer pointer(Cond ? Owner().ptr : GSLPointer());).

This patch extends the MemberExpr logic to handle the conditional operator. The heuristic is intentionally simple, which may result in some false negatives. However, it effectively covers common cases like std::string_view sv = cond ? "123" : std::string();, which is a reasonable trade-off.

Fixes #120206

[llvmbot]

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

Fixes #120206

---

Full diff: https://github.com/llvm/llvm-project/pull/120233.diff

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+2)
• (modified) clang/lib/Sema/CheckExprLifetime.cpp (+9)
• (modified) clang/test/Sema/warn-lifetime-analysis-nocfg.cpp (+29)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index befa411e882b4c..2fb14da05bbef5 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -677,6 +677,8 @@ Improvements to Clang's diagnostics
       views.push_back(std::string("123")); // warning
     }
 
+- Fix -Wdangling false positives on conditional operators (#120206).
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/lib/Sema/CheckExprLifetime.cpp b/clang/lib/Sema/CheckExprLifetime.cpp
index 843fdb4a65cd73..e4eb7d66575bb9 100644
--- a/clang/lib/Sema/CheckExprLifetime.cpp
+++ b/clang/lib/Sema/CheckExprLifetime.cpp
@@ -582,6 +582,15 @@ static void visitFunctionCallArguments(IndirectLocalPath &Path, Expr *Call,
     //   Temp().ptr; // Here ptr might not dangle.
     if (isa<MemberExpr>(Arg->IgnoreImpCasts()))
       return;
+    // Avoid false positives when the object is constructed from a conditional
+    // operator argument. A common case is:
+    //   // 'ptr' might not be owned by the Owner object.
+    //   std::string_view s = cond() ? Owner().ptr : sv;
+    if (const auto *Cond =
+            dyn_cast<AbstractConditionalOperator>(Arg->IgnoreImpCasts());
+        Cond && isPointerLikeType(Cond->getType()))
+      return;
+
     auto ReturnType = Callee->getReturnType();
 
     // Once we initialized a value with a non gsl-owner reference, it can no
diff --git a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
index 45b4dc838f44ed..4c19367bb7f3dd 100644
--- a/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ b/clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -777,3 +777,32 @@ void test4() {
 }
 
 } // namespace LifetimeboundInterleave
+
+namespace GH120206 {
+struct S {
+  std::string_view s;
+};
+
+struct [[gsl::Owner]] Q1 {
+  const S* get() const [[clang::lifetimebound]];
+};
+std::string_view test1(int c, std::string_view sv) {
+  std::string_view k = c > 1 ? Q1().get()->s : sv;
+  if (c == 1)
+    return  c > 1 ? Q1().get()->s : sv;
+  Q1 q;
+  return c > 1 ? q.get()->s : sv;
+}
+
+struct Q2 {
+  const S* get() const [[clang::lifetimebound]];
+};
+std::string_view test2(int c, std::string_view sv) {
+  std::string_view k = c > 1 ? Q2().get()->s : sv;
+  if (c == 1)
+    return c > 1 ? Q2().get()->s : sv;
+  Q2 q;
+  return c > 1 ? q.get()->s : sv;
+}
+
+} // namespace GH120206

[hokein]

note: I keep the code simple here, and heuristic logic is not perfect and may lead to false negatives, as it filters out more cases than intended. The major case std::string_view sv = cond ? "123" : std::string(); is still covered, I think this is a right tradeoff.

[Xazax-hun]

I am a bit confused to be honest. Are there any other contexts where Owner().ptr is problematic? I'd expect our analysis to behave the same for a subexpression like that regardless the context. So I am surprised we need to insert special logic for the ternary operator.

[hokein]

I am a bit confused to be honest. Are there any other contexts where Owner().ptr is problematic? I'd expect our analysis to behave the same for a subexpression like that regardless the context. So I am surprised we need to insert special logic for the ternary operator.

Possibly, but I don’t know of any concrete examples (it’s hard to judge). We have another ad-hoc filter at the end, IsGslPtrValueFromGslTempOwner, which filters out cases where the GSL pointer doesn’t originate from a GSL owner. This works well for simple and common cases, but when combined with lifetimebound, the behavior becomes tricky.

The current fix extends the MemberExpr logic (L583) to handle cases like GSLPointer pointer(Owner().ptr);, but it doesn’t yet address cases like GSLPointer pointer(Cond ? Owner().ptr : GSLPointer());. I think this fix is a reasonable extension to address the issue.

[Xazax-hun]

but it doesn’t yet address cases like GSLPointer pointer(Cond ? Owner().ptr : GSLPointer())

I am wondering if it was possible to use the same or similar MemberExpr filter when we drill down to the branches of the conditional operator. If it is too hard to do, I am OK with the current solution. But to me expanding the MemberExpr logic to handle more expressions sounds more natural than adding logic for a new expression type.

[hokein]

I am wondering if it was possible to use the same or similar MemberExpr filter when we drill down to the branches of the conditional operator. If it is too hard to do,

This was my first approach. We could add a special MemberExpr filter inside the do-while loop in visitLocalsRetainedByReferenceBinding, but I don’t think it’s a good idea:

• the filtering logic is GSL-pointer specific, and visitLocalsRetainedByReferenceBinding shouldn’t be aware of it; (layering violation)
• adding such filtering causes new false negatives in cases like const string_view& sv = Owner().sv;.
• it would scatter the special filtering logic into another place, making the code harder to reason about.

[Xazax-hun]

Is const string_view& sv = Owner().sv; a false negative? We cannot actually know if sv is actually owned or not because we have no lifetimebound annotation and the gsl owner does not tell us anything about fields, only about conversions/ctors.

[Xazax-hun]

Ah, never mind, I missed the const ref!

[Xazax-hun]

OK, with all of these being said I am OK with the current approach.

[shafik]

Please add more details to the summary, especially a note that there theoretically could be false negatives but it fixes the overall issue and the tradeoff good.

[hokein]

Please add more details to the summary, especially a note that there theoretically could be false negatives but it fixes the overall issue and the tradeoff good.

Done.