melver wrote:
This is grossly incomplete (did you build & test?), and the AI-assisted PR
description is too noisy vs. the underdocumented code being added. Neither do
you consider the Thread Safety Analysis case, which is actually what triggered
all this in https://github.com/llvm/llvm-project/pull/194457. Problems:
1. `EXPR_C_THIS` opcode in `ASTBitCodes.h` is needed -- PCH round-trips will
silently fail.
2. The new `StmtClass` enum value needs entries in `ExprClassification.cpp`,
`ExprConstant.cpp` (`CheckICE`), `Expr.cpp` (`HasSideEffects`,
`isSameComparisonOperand`), `ItaniumMangle.cpp`, `ExprEngine.cpp`, and
`CXCursor.cpp`.
3. `computeDependence` should propagate from the type -- `setDependence(None)`
will produce wrong dependence on class templates.
4. `TransformCThisExpr` needs to `TransformType` the enclosing record type for
the same reason.
5. The intercept in `handleCountedByAttrField` doesn't catch
`IndirectFieldDecl` (when the count is inside an anonymous struct/union, you'll
still hit the original crash in `BuildCountAttributedArrayOrPointerType`).
Existing test
`clang/test/AST/attr-counted-by-struct-ptrs.c` (`on_pointer_anon_count_ty_pos`
case) exercises this.
6. As already mentioned by other reviewers, pretty-printing currently emits
`__counted_by(this->size)`, which is misleading.
7. The same `DeclRefExpr(FieldDecl)` problem motivates a workaround in
`ThreadSafetyCommon.cpp` (commit dba9696cce009ca661b5505b2e75fc47bb401731, see
FIXME). If you move the rewrite into `Sema::ActOnIdExpression` rather than
`handleCountedByAttrField`,
every attribute (`counted_by`, `sized_by`, `guarded_by`, all the TSA attrs)
gets the right AST in one place.
- [Note, for the TSA fix, parameter attributes on member functions still
produce raw `DeclRefExpr` even in C++ because `CXXThisType` isn't set correctly
in paths; those are independent of CThisExpr introduction but worth knowing
about if you want to remove the TSA workaround entirely.]
I have a more complete prototype implementing the same idea called
`ImplicitThisExpr` (which I'd rename to `CThisExpr` per your suggestion --
that's shorter and clearer IMHO):
https://github.com/melver/llvm-project/commits/tsa-ImplicitThisExpr/
I haven't sent it because I still wanted to clean it up more and test more, and
don't have time for that right now (also AI-assisted, but designed against the
existing TSA case first -- which is what surfaced most of the corner cases
above). Feel free to copy things over if useful.
https://github.com/llvm/llvm-project/pull/199241
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
