xazax.hun added inline comments.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:190
Env.setValue(Loc, Env.takeOwnership(std::make_unique<ReferenceValue>(
SubExprVal->getPointeeLoc())));
+ break;
----------------
sgatev wrote:
> xazax.hun wrote:
> > I know this is not strictly related to this patch, but why do we actually
> > need do `getPointeeLoc` here? I'd expect `UO_Deref` to be a noop for all
> > intents and purposes.
> >
> > E.g. for a snippet like:
> > ```
> > int f(int *p) {
> > return *p;
> > }
> > ```
> >
> > The AST looks like:
> > ```
> > `-CompoundStmt 0x5565d151d038 <col:15, line:4:1>
> > `-ReturnStmt 0x5565d151d028 <line:3:5, col:13>
> > `-ImplicitCastExpr 0x5565d151d010 <col:12, col:13> 'int'
> > <LValueToRValue>
> > `-UnaryOperator 0x5565d151cff8 <col:12, col:13> 'int' lvalue prefix
> > '*' cannot overflow
> > `-ImplicitCastExpr 0x5565d151cfe0 <col:13> 'int *'
> > <LValueToRValue>
> > `-DeclRefExpr 0x5565d151cfc0 <col:13> 'int *' lvalue ParmVar
> > 0x5565d151ce00 'p' 'int *'
> > ```
> >
> > I'd expect any actual dereference to happen in the `LValueToRValue` cast.
> > The reason is, this is how references can be handled. E.g.:
> > ```
> > void f(int &p) {
> > int &q = p;
> > int r = p;
> > }
> > ```
> >
> > Has the AST:
> > ```
> > |-DeclStmt 0x55d49a1f00b0 <line:3:5, col:15>
> > | `-VarDecl 0x55d49a1effd0 <col:5, col:14> col:10 q 'int &' cinit
> > | `-DeclRefExpr 0x55d49a1f0038 <col:14> 'int' lvalue ParmVar
> > 0x55d49a1efe00 'p' 'int &'
> > `-DeclStmt 0x55d49a1f0180 <line:4:5, col:14>
> > `-VarDecl 0x55d49a1f00e0 <col:5, col:13> col:9 r 'int' cinit
> > `-ImplicitCastExpr 0x55d49a1f0168 <col:13> 'int' <LValueToRValue>
> > `-DeclRefExpr 0x55d49a1f0148 <col:13> 'int' lvalue ParmVar
> > 0x55d49a1efe00 'p' 'int &'
> > ```
> >
> > The reason why you know when should you propagate the reference or the
> > pointee of the reference from the subexpression is because of the
> > `LValueToRValue` cast. So you already need to do the "dereference" operator
> > there to correctly handle references. And if you already do that, I see no
> > reason to duplicate this logic here for pointers. Do I miss something?
> In the first example you shared the type of the `LValueToRValue` expression
> is `int *` and the type of the `UO_Deref` expression is `int` so I think we
> need to do something to translate one into the other. My understanding is
> that `LValueToRValue` performs indirection through references whereas
> `UO_Deref` performs indirection through pointers. This is why I think we
> ultimately need both. I might be wrong so please correct me and I'd be happy
> to address this in a follow up.
Indeed, the type did change at `UO_Deref`.
I think the problem with handling both as performing the indirection is that we
would end up performing two indirections for the first code snippet as both the
cast and the dereference operator are present and we would need to introduce
special code to avoid that.
When we implemented lifetime analysis, initially we also handled `UO_Deref` as
performing the dereference operator. It did not quite work, but after replacing
`UO_Deref` with noop, and relying on Clang to always put an `LValueToRValue`
cast whenever we need to do a dereference operation worked out pretty well and
we have not seen a code snippet yet that would not work with that approach.
I agree that the types are confusing in the AST. I am not even convinced about
their correctness. For us, it just worked out OK as the type of the pointee
always had the right type, so we did not even have to look at the AST.
Basically, in the current model, there is a decision one needs to make every
time we look a location up. Namely, whether we should pass
`SkipPast::Reference`. I wonder if we actually need to make that decision in
the code at all. I think it might be possible that the AST has `LValueToRValue`
casts every time we need to look past references, and we could just follow what
the AST suggests without forcing us to make a decision point at every API call.
That would make it way less error prone to work with this codebase and probably
also making it more resilient to future changes in the language or the AST.
Don't get me wrong, I think what you have currently is perfectly OK and the
tests demonstrate that this approach works, but I wonder if it is possible to
simplify it. And the main reason I think there should be a way to make this
simpler, because I implemented something similar in the past and I did not need
the equivalent of `SkipPast::Reference`. Here is most of that code:
https://github.com/mgehre/llvm-project/blob/lifetime/clang/lib/Analysis/LifetimePsetBuilder.cpp#L324
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117496/new/
https://reviews.llvm.org/D117496
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
