[PATCH] D117496: [clang][dataflow] Add transfer function for addrof

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.
This revision is now accepted and ready to land.

The address-of part looks good to me. However, just realized that I'm not convinced whether the dereference operator, or references, in general, are correctly handled.



================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:190
       Env.setValue(Loc, Env.takeOwnership(std::make_unique<ReferenceValue>(
                             SubExprVal->getPointeeLoc())));
+      break;
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117496/new/

https://reviews.llvm.org/D117496