steakhal wrote:
I've vibe coded two commits - they might not be correct!
`reset --hard` to drop any of these if you disagree.
## Follow-up: scope the destructor-elision to return values only
I dug into the copy-elision semantics and verified behavior
against a locally-rebuilt clang. The return-value half is sound, but the
argument half isn't, so I pushed two follow-up commits that tighten it.
### Why
The skip assumes the smart-pointer temporary "never invokes delete." That's true
for a **returned prvalue**: under C++17 guaranteed copy elision it is
constructed
directly into the caller's return slot, so the temporary is destructed by the
*caller*, not in the analyzed function.
It is **not** true for an **argument**: an argument temporary's lifetime ends at
the full-expression in *this* function (clang emits the
`ExprWithCleanups`/`CXXBindTemporaryExpr` at the call site; the Itanium C++ ABI
also has the caller destroy by-value arguments), so its destructor runs here and
can `delete` unless the callee takes ownership — which the checker doesn't
prove.
The sharpest symptom: a discarded `make();` is flagged, while `sink(make())`
(with `Ref&&`) and `byValue(make())` are silently accepted, even though all
three
destruct the same `Ref` temporary at the same point. For a `nodelete` annotation
that's a false negative.
> Incidentally, `const Ref&` arguments were only being spared by accident — the
> materialized temporary is `const`-qualified, so the exact `QualType ==` in the
> helper failed. That fragility is the second commit.
## Verified against `-O2` codegen
To confirm this is real (not just an AST artifact), I compiled representative
cases with out-of-line special members at `-O2` (arm64-apple-darwin, Itanium
ABI):
```asm
; Data retPrvalue() { return makeData(); } -- returned prvalue
__Z10retPrvaluev:
b __Z8makeDatav ; tail call, sret forwarded; NO ctor, NO ~Data
; void passByValue() { byValue(makeData()); } -- argument temporary
__Z11passByValuev:
sub x8, x29, #4 ; &temp (sret slot, in THIS frame)
bl __Z8makeDatav
bl __Z7byValue4Data
bl __ZN4DataD1Ev ; ~Data() runs HERE, in the caller
ret
```
The returned prvalue emits no destructor at all (the temporary is the caller's
return slot), whereas every argument form — by value, `&&`, and `const&` — plus
a
discarded temporary emit a `~T()` call in the analyzed function. By-value
included,
matching the Itanium rule that the caller destroys arguments. So the analyzer is
flagging exactly the functions that physically run the destructor.
### Commits
1. **only elide returned-prvalue temporaries** — `checkArguments()` and
`VisitCXXConstructExpr()` go back to the normal `Visit()`; the helper is kept
for `VisitReturnStmt` only, renamed `visitReturnValueElidingTemp()`, with a
comment on the rationale. Fully sound, no ownership heuristic.
2. **compare canonical types** — replace exact `QualType ==` with a canonical,
unqualified comparison so the elision recognizes the returned object
regardless of typedef/qualifier spelling. Defensive hardening.
### Test impact
`nodelete-lazy-initialize.cpp`'s `foo()` now (correctly) warns — its
`RefObj::create()` is an argument temporary, so it is no longer elided. I added
an `argument_temporaries_are_not_elided` block to `nodelete-annotation.cpp`
pinning the boundary (returned prvalue → ok; by-value / `&&` / `const&` argument
temporaries and a discarded temporary → flagged), plus a
`returned_prvalue_typedef` case for the canonical comparison. The whole WebKit
suite stays green.
https://github.com/llvm/llvm-project/pull/200912
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
