[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

Thu Feb 25 06:07:42 PST 2021

RedDocMD marked 2 inline comments as done.
RedDocMD added a comment.

Many thanks for you comments, @steakhal!
I will address the issues you have pointed out in this comment. To clean things up I should perhaps add some more clarification to the summary.

> Could you elaborate on what approach did you choose and more importantly, why that approach?
> Why do we need a graph search here?

Pointer-to-members contain two things - a pointer to a NamedDecl to store the field/method being pointed to and a list of CXXBaseSpeciifier. This list is used to determine which sub-object the member lies in. This path needs to be determined and unfortunately with `reinterpret_cast`, the AST provides no structural assistance (unlike `static_cast`). Hence, it needs to be searched by searching through the BaseSpecifiers of the `CXXRecordDecl`.
As for the need of graph search, it is due to multiple-inheritance. For a given class, it may have two or more bases and we need to follow into both of them to find the path to the required sub-object.
Why do I use BFS? Because one branch of the (inverted) inheritance tree may be very deep yet not contain the required class. But DFS will first exhaust it and then go the other branch. BFS on the other hand will find the shortest path to the correct base optimally, and this shortest path is all we need.

> What cases do you try to resolve? I mean, there is a single test-case, which has reinterpret casts back and forth (literally), and it's not immediately clear to me why are you doing those gymnastics.
> Maybe worth creating other test-cases as well. Probably covering multiple inheritance or virtual inheritance?

I will add a test case for multiple inheritance and perhaps replace existing tests with clearer tests. Virtual inheritance will not help since pointer-to-member to virtual base field/method is not allowed.
There are three cases which I have handled:

1. The required class is the class we are casting to
2. The required class is higher in the class hierarchy
3. The required class is lower in the class hierarchy
4. The required class is not related to the class being cast to.

For Case 1 and 2, a valid path can be calculated and so a pointer-to-member is created.
For Case 3 and 4, no valid path can be calculated and so it doesn't make sense to further pursue static analysis down this path.
Note that the required class is determined by the actual field/member we are pointing to and is not explicit in the `reinterpret_cast`.

> Your code has shared pointers, which is interesting, but I would favor unique pointers unless you can defend this use-case.

The reason for not using `unique_ptr` is as follows:- each `BFSNode` holds a pointer to its parent. This cannot be a `unique_ptr` since there may be multiple children to a `BFSNode`. This must be a `shared_ptr` as a consequence.

> Also, in general, we prefer algorithms to raw for or while loops if they are semantically equivalent.

I am not aware of any std/LLVM algorithm that perform BFS. If there are any, I would be glad to learn more about them.
I am going to replace the for loops with a range-for loop. Sorry about that one!

> Besides all of these, I can see a few copy-pasted lines here and there. I'm not saying that it's bad, I'm just highlighting this fact.

Yes this was a conscious choice. The other alternative perhaps is to use fall-through, which I think would be confusing in this case since it will be obscured by the intermediate code.

================
Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:554-555
+        }
+        // Explicitly proceed with default handler for this case cascade.
+        state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+        continue;
----------------
steakhal wrote:
> I know that you just copy-pasted this, but what is this xD
> I know that you just copy-pasted this, but what is this xD

Not entirely sure, but I think it performs tasks required to handle the pointer-to-member created or do the requisite default in the absence of a pointer-to-member.

================
Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:5

 // TODO: The following test will work properly once reinterpret_cast on pointer-to-member is handled properly
 namespace testReinterpretCasting {
----------------
steakhal wrote:
> I assume this is resolved by now.
> I assume this is resolved by now.

Sorry, that was a lame mistake on my part.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976