checkLocation for ImplicitCastExpr of kind CK_NoOp

Hello everyone, I hope this is the proper platform for my questions.

I am currently working with the Clang Static Analyser and have a question regarding the checkLocation callback.
This callback function is generally triggered for LValueToRValue casts.
The following code example triggers the checkBind (for the LHS) callback but not the checkLocation (for the RHS):

template <typename T> class MyClass {
private:
  T Value;

public:
  explicit MyClass(T &&Val = T{}) : Value{Val} {}
  T &getValue() { return Value; }
};

MyClass<int> IntIn{3};
MyClass<int> IntOut;

int test_main_cpp() {

  IntOut = IntIn;

  return 0;
}

When looking at the AST, this also makes sense, as the cast involved here is a NoOp cast:

`-FunctionDecl 0x55a07fade998 <line:40:1, line:45:1> line:40:5 test_main_cpp 'int ()'
  `-CompoundStmt 0x55a07fadfdc0 <col:21, line:45:1>
    |-CXXOperatorCallExpr 0x55a07fadfd58 <line:42:3, col:12> 'MyClass<int>' lvalue '='
    | |-ImplicitCastExpr 0x55a07fadfd40 <col:10> 'MyClass<int> &(*)(const MyClass<int> &) noexcept' <FunctionToPointerDecay>
    | | `-DeclRefExpr 0x55a07fadfb18 <col:10> 'MyClass<int> &(const MyClass<int> &) noexcept' lvalue CXXMethod 0x55a07fadf7b0 'operator=' 'MyClass<int> &(const MyClass<int> &) noexcept'
    | |-DeclRefExpr 0x55a07fadea40 <col:3> 'MyClass<int>' lvalue Var 0x55a07fade700 'IntOut' 'MyClass<int>'
    | `-ImplicitCastExpr 0x55a07fadfb00 <col:12> 'const MyClass<int>' lvalue <NoOp>
    |   `-DeclRefExpr 0x55a07fadea60 <col:12> 'MyClass<int>' lvalue Var 0x55a07fabb6c0 'IntIn' 'MyClass<int>'
    `-ReturnStmt 0x55a07fadfdb0 <line:44:3, col:10>
      `-IntegerLiteral 0x55a07fadfd90 <col:10> 'int' 0

As can be seen in the ExprEgnine, evalLocation (and subsequently checkLcoation) is only triggered for LValueToRValue cast.

void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
                           ExplodedNode *Pred, ExplodedNodeSet &Dst) {

  ExplodedNodeSet dstPreStmt;
  getCheckerManager().runCheckersForPreStmt(dstPreStmt, Pred, CastE, *this);

  if (CastE->getCastKind() == CK_LValueToRValue ||
      CastE->getCastKind() == CK_LValueToRValueBitCast) {
    for (ExplodedNode *subExprNode : dstPreStmt) {
      ProgramStateRef state = subExprNode->getState();
      const LocationContext *LCtx = subExprNode->getLocationContext();
      evalLoad(Dst, CastE, CastE, subExprNode, state, state->getSVal(Ex, LCtx));
    }
    return;
  }
...

Should corresponding casts like the NoOp shown also trigger checkLocation (At least in some cases)? Or am I misunderstanding something here and the implied behaviour is correct?

I would be grateful for any suggestions and answers!

Hey!

This is an area that definitely needs some cleanup. We were not able to figure out yet what would be the most intuitive way to restructure this. There were some previous discussions about this topic here: `checkLocation` vs `checkBind` when `isLoad=false` - #7 by steakhal

Feel free to add your opinion what do you think the best solution here would be.

To be honest, I’m having difficulties finding more or detailed information about when exactly a NoOp ImplicitCastExpr is generated in the AST.
However, I personally have the opinion that it would be beneficial to include the ImplicitCastExpr in the list of casts that trigger the checkLocation. Like this:

void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
                           ExplodedNode *Pred, ExplodedNodeSet &Dst) {

  ExplodedNodeSet dstPreStmt;
  getCheckerManager().runCheckersForPreStmt(dstPreStmt, Pred, CastE, *this);

  if (CastE->getCastKind() == CK_LValueToRValue ||
      CastE->getCastKind() == CK_LValueToRValueBitCast ||
      CastE->getCastKind() == CK_NoOp) {

Another example that supports this suggestion is this one:

int main() {
  int Int = 1;
  const int &IntRef = Int;
}

… and the AST:

`-DeclStmt 0x561615d9ed90 <line:46:3, col:26>
  `-VarDecl 0x561615d9ec98 <col:3, col:23> col:14 IntRef 'const int &' cinit
    `-ImplicitCastExpr 0x561615d9ed20 <col:23> 'const int' lvalue <NoOp>
      `-DeclRefExpr 0x561615d9ed00 <col:23> 'int' lvalue Var 0x561615d9ebb0 'Int' 'int'

Here, too, checkBind is triggered for the LHS but not checkLocation for the RHS.

On the other hand, this second code example does not lead to any ImplicitCasts:

int main() {
  int Int = 1;
  int &IntRef = Int;
}

AST:

`-DeclStmt 0x55f161c18a48 <line:46:3, col:20>
  `-VarDecl 0x55f161c18968 <col:3, col:17> col:8 IntRef 'int &' cinit
    `-DeclRefExpr 0x55f161c189d0 <col:17> 'int' lvalue Var 0x55f161c18880 'Int' 'int'

So it would not really be consistent that the first example triggers a checkLocation via the ImplicitCastExpr (NoOp) and the second example does not.

It’s unfortunate that we have two overlapping APIs. And they don’t even behave the same way for the supposed overlap: for checking stores.

Yep, thet’s a bug. Harmonizing these two for stores would be the first step of getting rid of checkLocation in favor of having a dedicated callback: check::Load

Thank you for your answers!
Do you think it is worth implementing a temporary solution for checkLocation that reacts to NoOp implicit casts or does it make more sense to tackle this in a larger refactoring?

It makes sense to harmonize the two APIs that are used for the same thing.
Since dropping an AST node visit is not an option (because that would be a semantic breaking change).
So the only option is to get checkLocation behave the same as checkBind (for binds).

This is a prequisite to any further refactor.

1 Like

Okay, thank you very much. Then this will be improved as part of a larger refactoring and does not need a temporary solution.

No, the opposite. They should be done separately, because refactors should not have semantic changes. This means, we should fix checkLocation first, before even trying the refactoring (turning) checkLocation into checkLoad while adding the necessary checkBind hooks to the places where checkLocation was used for checking stores.

Okay, I understand that point. However, I’m not sure how to solve my problem generically and in a good way.
As I said, my first idea was to trigger corresponding callbacks (checkLocation) for NoOp implicit casts. That makes sense for the example above:

MyClass<int> IntIn{3};
MyClass<int> IntOut;

int test_main_cpp() {

  IntOut = IntIn; // NoOp cast for RHS

  return 0;
}

But in this example that would mean that a checkLocation is only triggered for the RHS in the first case. This discrepancy seems inconsistent to me.

const int &IntRef = Int; // NoOp cast for RHS
int &IntRef = Int;           // No ImplCast at all for RHS

The only AST node that all cases have in common is the DeclRefExpr. So if a checkLocation is to be triggered for all 3, this would be the only approach in my opinion. Or what do you think?

@T-Gruber Okay, I finally took my time to actually read your post. My bad.
For binding references it’s the desired behavior to have a bind, but no read of the right hand side.
It’s because there is no read happening there. Reads only happen when an Lval2Rval cast is present. Consequently, the checkLocation (load) should only ever happen for such casts. Noop casts shouldn’t trigger a read, thus the checkLocation shouldn’t be triggered.

Sorry again for the confusion, I was tunnel visioned on the connection between the checkLocation and checkBind FOR the bind scenario. That is still a problem, but not your problem.

1 Like

Thank you very much for your answer! In the case of the reference, I can fully understand that. However, what is still not clear to me is that in the example with the MyClass assignment, no read accesses are recognised. As I understand it, the default copy assignment operator should result in read access for the RHS and write access for the LHS. However, this is not the case (no LValueToRValue cast). I would expect it to be recognisable in some way that IntIn is read and IntOut is written based on this data.

You are correct. The assignment operator calls are handled by ExprEngine::performTrivialCopy and that only calls evalBind().
You could probably patch it to also call evalLocation, like it’s already done in some other places.
Making them to work consistent for stores makes sense.

Perfect, then I’ll tackle that in the course of this week!