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

[CS via Phabricator via cfe-commits]

CS added a comment.
Herald added a subscriber: manas.
Herald added a project: All.

I'm sorry but is this some sort of joke? I don't think we should be calling 
ChickenFajitas straightaway like this, debounce it, recalculate, repent let's go


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

This patch does not model faithfully how reinterpretcasting member pointers 
should work.

The base specifiers represent the history for the member pointer (aka. mptr), 
describing how it was transformed to point wherever it currently points to.
If and only if the `PointerToMemberData` is `null`, then the represented member 
pointer is `null`.

`class PointerToMemberData` could have a bool `IsValid` field, representing 
whether or not this mptr is safely dereferancable or not.
Depending on this, loading a value via this mptr should result in `Undefined` 
or the associated value from the //store//.
Whenever you encounter a `reinterpret_cast` expression casting a valid mptr to 
a different type, the resulting `PointerToMemberData` should be the same as 
before **BUT** the `IsValid` field set to `false`!
Later, if it's cast back to the original type (which is probably the 
`NamedDecl` inside the `PointerToMemberData`, I don't know).

You should also change the way how the abstract machine loads a value from the 
//store// using an invalid mptr - as that should return `Undefined` instead of 
the actual associated value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@steakhal?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@steakhal, being picky with code from beginners is a good way to train them to 
write code I think, and so I must thank you for you scrutiny!
As for the test you pointed out, it is a wrong test. It is wrong now that I 
have a better understanding of the problem.
There are two options that can be taken I reckon:

- Simply make the stored `SVal` `Unknown` when we have a reinterpret-cast on 
pointer-to-member
- Model it properly to take care of all the semantics.

Both of them remove the bug. I would prefer the first one and leave a TODO for 
better modelling. Better modelling necessarily makes interpretation of 
pointer-to-member clumsy. Because previously, a pointer-to-member was always 
valid. If we include modelling for reinterpret-cast, then as far as I can see, 
the pointer-to-member may either be in a valid or invalid state. This would 
lead to a chain of other changes. Possible most certainly but I think a tad bit 
unnecessary, especially because this corner case makes the normal case handling 
more complex


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I'm really sorry about being sooo picky about this patch.
It's not my expertise and the change seems to address a corner-case, so we have 
to especially careful not introducing bugs.

My concern is that I still don't understand why do we want to do anything with 
reinterpret casts, besides remembering what the original type was.
AFAIK the resulting pointer can not be safely dereferenced unless it's 
reinterpreted back to the original type.
But for example, you are expecting this:

  void testMultiple() {
int F::*f = ::field;
int A::*a = reinterpret_cast(f); // Dereferencing 'a' is UB, AFAIK
int C::*c = reinterpret_cast(f); // Same here!
A aobj;
C cobj;
aobj.*a = 13; // Now an airplane suddenly crashes.
cobj.field = 29;
clang_analyzer_eval(aobj.*a == 13); // expected-warning{{UNKNOWN}} // Same 
here!
clang_analyzer_eval(cobj.*c == 29); // expected-warning{{TRUE}} // Same 
here!
  }

IMO if an expression results in UB, the symbolic value associated with that 
expression should be `Undef`.
`Unknown` would be also fine, but nothing else.

I could spam a couple of nits, but I think it's better to sort this question 
out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D96976#2622342 , @steakhal wrote:

> I'm somewhat busy. If it's not urgent, I would postpone this.
> Ping me in a few weeks.

Sure ok. Is it okay if I tell someone else to take a look at this in the 
meanwhile?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96976#2622039 , @RedDocMD wrote:

> @steakhal, your views?

I'm somewhat busy. If it's not urgent, I would postpone this.
Ping me in a few weeks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@steakhal, your views?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

Your view, @steakhal?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 328931.
RedDocMD added a comment.

Replaced smart-ptr dereference with direct access


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -1,11 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-// XFAIL: asserts
 
 void clang_analyzer_eval(bool);
 
-// TODO: The following test will work properly once reinterpret_cast on pointer-to-member is handled properly
 namespace testReinterpretCasting {
-struct Base {
+struct BaseOfBase {};
+
+struct Base : public BaseOfBase {
   int field;
 };
 
@@ -15,12 +15,51 @@
 
 struct Some {};
 
-void f() {
+void analyzableCasts() {
   int DoubleDerived::*ddf = ::field;
   int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf)));
-  int Some::*sf = reinterpret_cast(ddf);
   Base base;
   base.field = 13;
   clang_analyzer_eval(base.*bf == 13); // expected-warning{{TRUE}}
 }
+
+void castOutsideHierarchy() {
+  int DoubleDerived::*ddf = ::field;
+  int Some::*sf = reinterpret_cast(ddf);
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}
+}
+
+void castAbove() {
+  int DoubleDerived::*ddf = ::field;
+  int BaseOfBase::*bbf = reinterpret_cast(ddf);
+  BaseOfBase bb;
+  bb.*bbf = 23;
+  clang_analyzer_eval(bb.*bbf == 23); // expected-warning{{UNKNOWN}}
+}
+
+namespace testMultipleInheritance {
+struct A {};
+struct B : public A {};
+struct C {
+  int field;
+};
+struct D : public C {};
+struct E : public B, public D {};
+struct F : public E {};
+
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;
+  cobj.field = 29;
+  clang_analyzer_eval(aobj.*a == 13); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(cobj.*c == 29); // expected-warning{{TRUE}}
+}
+} // namespace testMultipleInheritance
+
 } // namespace testReinterpretCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -522,8 +522,7 @@
 continue;
   }
   case CK_DerivedToBaseMemberPointer:
-  case CK_BaseToDerivedMemberPointer:
-  case CK_ReinterpretMemberPointer: {
+  case CK_BaseToDerivedMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
   SVal CastedPTMSV =
@@ -537,6 +536,25 @@
 state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
 continue;
   }
+  case CK_ReinterpretMemberPointer: {
+SVal V = state->getSVal(Ex, LCtx);
+if (auto PTMSV = V.getAs()) {
+  if (const auto *ReinterpretE =
+  llvm::dyn_cast(CastE)) {
+if (const PointerToMemberData *Data =
+getBasicVals().basesForReinterpretCast(ReinterpretE,
+   *PTMSV)) {
+  SVal CastedPTMSV = svalBuilder.makePointerToMember(Data);
+  state = state->BindExpr(CastE, LCtx, CastedPTMSV);
+  Bldr.generateNode(CastE, Pred, state);
+  continue;
+}
+  }
+}
+// Explicitly proceed with default handler for this case cascade.
+state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+continue;
+  }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
   case CK_VectorSplat: {
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -13,6 +13,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/Store.h"
@@ -22,6 +23,7 @@
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include 
 #include 
 #include 
@@ 

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D96976#2605815 , @steakhal wrote:

> What is that caveat?

The second point in the link I gave above.

> Do you have anything specific in mind about implementing the 3rd option?
> Why do you think it's //significantly complex//?

The modelling is not so complex. We need to store in the pointer-to-member a 
bool to remember that it was reinterpret_casted, and a  CXXRecordDecl pointer 
to store where it was reinterpret_casted from. If it is reinterpret_casted back 
properly, then these are reset. If we do further reinterpret_casting or 
static_cast on this, then we can store in a bool that the pointer-to-member is 
unsafe to de-reference.
The problem is utilizing this fact (that pointer-to-member is safe or not to 
de-reference). This, I reckon, is a big refactor.
@steakhal, @vsavchenko, @NoQ what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96976#2605313 , @RedDocMD wrote:

> Talking on the mailing list, I  got a link on reinterpret_casting for 
> pointer-to-member: 
> https://timsong-cpp.github.io/cppwp/n4861/expr.reinterpret.cast#10
> The gist is that this sort of cast is only valid if we cast back to the 
> original type (that too with a caveat).

What is that caveat?

> Now currently there are three things that can be done:
>
> - Leave things the way they are - reinterpret_cast will cause an assertion to 
> fail
> - Never analyze reinterpret_cast - this is easy, just return nullptr for  
> PointerToMemberData
> - Properly model this - this would require possibly adding more information 
> in PointerToMember and a significantly complex logic to handle it.

Do you have anything specific in mind about implementing the 3rd option?
Why do you think it's //significantly complex//?

Leaving assertion failures is not an option.
IMO if this feature is important to you, then you should pursue the 3rd.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

Talking on the mailing list, I  got a link on reinterpret_casting for 
pointer-to-member: 
https://timsong-cpp.github.io/cppwp/n4861/expr.reinterpret.cast#10
The gist is that this sort of cast is only valid if we cast back to the 
original type (that too with a caveat). Now currently there are three things 
that can be done:

- Leave things the way they are - reinterpret_cast will cause an assertion to 
fail
- Never analyze reinterpret_cast - this is easy, just return nullptr for  
PointerToMemberData
- Properly model this - this would require possibly adding more information in 
PointerToMember and a significantly complex logic to handle it.

The first two are trivially done. I would like to know if it is worth following 
the third - especially since it is a really obscure feature.
@vsavchenko, @NoQ, @steakhal what do you think should be done?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:52-62
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;

steakhal wrote:
> RedDocMD wrote:
> > steakhal wrote:
> > > RedDocMD wrote:
> > > > steakhal wrote:
> > > > > Wait a minute. It's not how it works.
> > > > > How I imagine member pointers, they are just offsets.
> > > > > `::field` is notionally equivalent with `offsetof(F, field)`. That 
> > > > > being said, You can not apply this member pointer to any object 
> > > > > besides `F`.
> > > > > Imagine if the classes of the inheritance tree would have other 
> > > > > fields as well.
> > > > > Then the `offsetof(T, field)` would be different for `F`, and `C`.
> > > > > 
> > > > > This example demonstrates that both of these member pointer 
> > > > > dereferences are UB.
> > > > > https://godbolt.org/z/15sMEP
> > > > > It returns different values depending on the optimization level, 
> > > > > which is a clear sign of UB.
> > > > > BTW this issue is closely related to strict aliasing.
> > > > The member access on A is definitely UB, I guess I will do what I 
> > > > proposed in the `Some` case.
> > > > I don't think the other one is.  Consider the following:
> > > > ```
> > > > struct A {};
> > > > struct B : public A {};
> > > > struct C {
> > > >   int field;
> > > > };
> > > > struct D : public C {};
> > > > struct E : public B, public D {};
> > > > struct F : public E {};
> > > > 
> > > > int main() {
> > > >   int F::* ff = ::field;
> > > >   int C::* cf1 = static_cast(ff);
> > > >   int C::* cf2 = reinterpret_cast(ff);
> > > >   C c;
> > > >   c.*cf1 = 10;
> > > >   c.*cf2 = 10;
> > > >   return 0;
> > > > }
> > > > ```
> > > > `cf1` and `cf2` are the same thing, except that they are declared 
> > > > differently (one via `static_cast`, other via `reinterpret_cast`). If 
> > > > we look at the AST (truncated to the first three lines of main):
> > > > ```
> > > > CompoundStmt 0x1a4fe18 
> > > > |-DeclStmt 0x1a4f3a8 
> > > > | `-VarDecl 0x1a21078  col:12 used ff 'int F::*' 
> > > > cinit
> > > > |   `-ImplicitCastExpr 0x1a4f378  'int F::*' 
> > > >  D -> C)>
> > > > | `-UnaryOperator 0x1a4f360  'int C::*' prefix 
> > > > '&' cannot overflow
> > > > |   `-DeclRefExpr 0x1a4f2f8  'int' lvalue Field 
> > > > 0x1a207d0 'field' 'int'
> > > > |-DeclStmt 0x1a4f560 
> > > > | `-VarDecl 0x1a4f428  col:12 used cf1 'int C::*' 
> > > > cinit
> > > > |   `-CXXStaticCastExpr 0x1a4f518  'int C::*' 
> > > > static_cast  D -> C)>
> > > > | `-ImplicitCastExpr 0x1a4f500  'int F::*' 
> > > >  part_of_explicit_cast
> > > > |   `-DeclRefExpr 0x1a4f498  'int F::*' lvalue Var 
> > > > 0x1a21078 'ff' 'int F::*'
> > > > |-DeclStmt 0x1a4f6e8 
> > > > | `-VarDecl 0x1a4f5c8  col:12 used cf2 'int C::*' 
> > > > cinit
> > > > |   `-CXXReinterpretCastExpr 0x1a4f6b8  'int C::*' 
> > > > reinterpret_cast 
> > > > | `-ImplicitCastExpr 0x1a4f6a0  'int F::*' 
> > > >  part_of_explicit_cast
> > > > |   `-DeclRefExpr 0x1a4f638  'int F::*' lvalue Var 
> > > > 0x1a21078 'ff' 'int F::*'
> > > > ```
> > > > Notice how the `static_cast` figures out the path to the correct 
> > > > subobject. This is how member-pointers are handled as far as I can tell.
> > > > Unfortunately, Stroustrup is surprisingly scant when it comes to this 
> > > > topic. I am trying to dig through the Standard.
> > > I tried to highlight that the classes could have non-static data members 
> > > which could cause your assumption about `cf1 == cf2` not to hold anymore. 
> > > See my previously attached godbolt link.
> > > 
> > > `static_cast(p)` makes sure that the //offset// that the member 
> > > pointer `p` represents is updated accordingly to match the //offset// of 
> > > the `field` member within the new type `T`. Usually, it involves some 
> > > addition/subtraction to accommodate this.
> > > While `static_cast(p)` does not update the value, just reinterprets it 
> > > in a different way - which usually results in an invalid pointer though 
> > > and a bunch of subtle rules kick in such as the type similarity, pointer 
> > > interchangeability, which in general known as //strict-aliasing//.
> > > While static_cast(p) does not update the value, just reinterprets it 
> > > in a different way - which usually results in an invalid pointer   though 
> > > and a bunch of subtle rules kick in such as the type similarity, pointer 
> > > interchangeability, which in general known as strict-aliasing.
> > Presumably you mean `reinterpret_cast`.
> > 
> > Ah, I see. This makes it glaringly obvious: [[ https://godbolt.org/z/f6vs77 
> > | Another Godbolt link ]]
> > So I guess handling `reinterpret_cast` for pointer-to-members pointless. I 
> > guess the idea will be to simply not analyze 

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:29-31
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}

RedDocMD wrote:
> steakhal wrote:
> > RedDocMD wrote:
> > > steakhal wrote:
> > > > The assignment is actually UB.
> > > > TBH I don't know how to test such behavior xD
> > > > Same for the next example.
> > > > 
> > > > Shouldn't it return `undef` for reading via an invalid member pointer?
> > > I don't quite know what is `undef`. Is it a special macro or something?
> > > Using an invalid member pointer is definitely UB, but I also need to show 
> > > that casting to an invalid pointer is properly handled because that is 
> > > not UB. I guess I will remove the member access and claim that since 
> > > there was no crash, it is okay and has been handled appropriately.
> > By `undef`, I was referring to the `clang::ento::UndefinedVal` - which 
> > usually represents values that shouldn't be used, not even read. For 
> > example, an uninitialized variable has undefined value. Or an out-of-bound 
> > memory access also produces undefined value. There are a few checkers that 
> > are hunting for such undefined values like CallAndMessageChecker, 
> > UndefBranchChecker, UndefinedArraySubscriptChecker, 
> > UndefinedAssignmentChecker, UndefResultChecker.
> > 
> > Probably removing the member accesses is the best you can do.
> > 
> > > but I also need to show that casting to an invalid pointer is properly 
> > > handled because that is not UB
> > In this test, I can't see where you cast the member pointer back to make it 
> > valid again.
> > In this test, I can't see where you cast the member pointer back to make it 
> > valid again.
> 
> I meant that `sf` when used will lead to undefined behaviour. But the 
> statement `int Some::*sf = reinterpret_cast(ddf);` isn't 
> undefined, is it? AFAIK, `reinterpret_cast` is the C++ equivalent of the C 
> unchecked cast - You can cast pointers as you wish but don't bet on the 
> result.
> On the other hand, pointer-to-members are unlike regular pointers, so there 
> may be a catch. But all I really require is that the statement `int Some::*sf 
> = reinterpret_cast(ddf);` to not drive the Static Analyzer crazy 
> - it simply gives up on trying to reason about `sf`. The test would be to 
> confirm that. So removing the member access should solve it
> I meant that sf when used will lead to undefined behaviour
It depends on how you define //use//.
It's fine to make copy of the pointer `sf`, but dereferencing `sf` is not. My 
problem, as I carefully highlighted, is about the dereference.

You can do exactly 2 things with the invalid `sf` pointer:
 - cast it back to the original type, or
 - simply not use (not dereference) :D

The result of the dereference of an invalid pointer could be modelled as either 
`UnknownVal` or `UndefinedVal`. The latter would be more accurate, but the 
former is also a valid //approximation//.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:52-62
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;

RedDocMD wrote:
> steakhal wrote:
> > RedDocMD wrote:
> > > steakhal wrote:
> > > > Wait a minute. It's not how it works.
> > > > How I imagine member pointers, they are just offsets.
> > > > `::field` is notionally equivalent with `offsetof(F, field)`. That 
> > > > being said, You can not apply this member pointer to any object besides 
> > > > `F`.
> > > > Imagine if the classes of the inheritance tree would have other fields 
> > > > as well.
> > > > Then the `offsetof(T, field)` would be different for `F`, and `C`.
> > > > 
> > > > This example demonstrates that both of these member pointer 
> > > > dereferences are UB.
> > > > https://godbolt.org/z/15sMEP
> > > > It returns different values depending on the optimization level, which 
> > > > is a clear sign of UB.
> > > > BTW this issue is closely related to strict aliasing.
> > > The member access on A is definitely UB, I guess I will do what I 
> > > proposed in the `Some` case.
> > > I don't think the other one is.  Consider the following:
> > > ```
> > > struct A {};
> > > struct B : public A {};
> > > struct C {
> > >   int field;
> > > };
> > > struct D : public C {};
> > > struct E : public B, public D {};
> > > struct F : public E {};
> > > 
> > > int main() {
> > >   int F::* ff = ::field;
> > >   int C::* cf1 = static_cast(ff);
> > >   int C::* cf2 = reinterpret_cast(ff);
> > >   C c;
> > >   c.*cf1 = 10;
> > >   c.*cf2 = 10;
> > >   return 0;
> > > }
> > > ```
> > > `cf1` and `cf2` are the same thing, except that they are declared 
> > > differently (one via `static_cast`, other via `reinterpret_cast`). If we 
> > > look at the AST (truncated to the first three lines of main):
> > > 

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked an inline comment as done.
RedDocMD added inline comments.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:29-31
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}

steakhal wrote:
> RedDocMD wrote:
> > steakhal wrote:
> > > The assignment is actually UB.
> > > TBH I don't know how to test such behavior xD
> > > Same for the next example.
> > > 
> > > Shouldn't it return `undef` for reading via an invalid member pointer?
> > I don't quite know what is `undef`. Is it a special macro or something?
> > Using an invalid member pointer is definitely UB, but I also need to show 
> > that casting to an invalid pointer is properly handled because that is not 
> > UB. I guess I will remove the member access and claim that since there was 
> > no crash, it is okay and has been handled appropriately.
> By `undef`, I was referring to the `clang::ento::UndefinedVal` - which 
> usually represents values that shouldn't be used, not even read. For example, 
> an uninitialized variable has undefined value. Or an out-of-bound memory 
> access also produces undefined value. There are a few checkers that are 
> hunting for such undefined values like CallAndMessageChecker, 
> UndefBranchChecker, UndefinedArraySubscriptChecker, 
> UndefinedAssignmentChecker, UndefResultChecker.
> 
> Probably removing the member accesses is the best you can do.
> 
> > but I also need to show that casting to an invalid pointer is properly 
> > handled because that is not UB
> In this test, I can't see where you cast the member pointer back to make it 
> valid again.
> In this test, I can't see where you cast the member pointer back to make it 
> valid again.

I meant that `sf` when used will lead to undefined behaviour. But the statement 
`int Some::*sf = reinterpret_cast(ddf);` isn't undefined, is it? 
AFAIK, `reinterpret_cast` is the C++ equivalent of the C unchecked cast - You 
can cast pointers as you wish but don't bet on the result.
On the other hand, pointer-to-members are unlike regular pointers, so there may 
be a catch. But all I really require is that the statement `int Some::*sf = 
reinterpret_cast(ddf);` to not drive the Static Analyzer crazy - 
it simply gives up on trying to reason about `sf`. The test would be to confirm 
that. So removing the member access should solve it



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:52-62
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;

steakhal wrote:
> RedDocMD wrote:
> > steakhal wrote:
> > > Wait a minute. It's not how it works.
> > > How I imagine member pointers, they are just offsets.
> > > `::field` is notionally equivalent with `offsetof(F, field)`. That 
> > > being said, You can not apply this member pointer to any object besides 
> > > `F`.
> > > Imagine if the classes of the inheritance tree would have other fields as 
> > > well.
> > > Then the `offsetof(T, field)` would be different for `F`, and `C`.
> > > 
> > > This example demonstrates that both of these member pointer dereferences 
> > > are UB.
> > > https://godbolt.org/z/15sMEP
> > > It returns different values depending on the optimization level, which is 
> > > a clear sign of UB.
> > > BTW this issue is closely related to strict aliasing.
> > The member access on A is definitely UB, I guess I will do what I proposed 
> > in the `Some` case.
> > I don't think the other one is.  Consider the following:
> > ```
> > struct A {};
> > struct B : public A {};
> > struct C {
> >   int field;
> > };
> > struct D : public C {};
> > struct E : public B, public D {};
> > struct F : public E {};
> > 
> > int main() {
> >   int F::* ff = ::field;
> >   int C::* cf1 = static_cast(ff);
> >   int C::* cf2 = reinterpret_cast(ff);
> >   C c;
> >   c.*cf1 = 10;
> >   c.*cf2 = 10;
> >   return 0;
> > }
> > ```
> > `cf1` and `cf2` are the same thing, except that they are declared 
> > differently (one via `static_cast`, other via `reinterpret_cast`). If we 
> > look at the AST (truncated to the first three lines of main):
> > ```
> > CompoundStmt 0x1a4fe18 
> > |-DeclStmt 0x1a4f3a8 
> > | `-VarDecl 0x1a21078  col:12 used ff 'int F::*' cinit
> > |   `-ImplicitCastExpr 0x1a4f378  'int F::*' 
> >  D -> C)>
> > | `-UnaryOperator 0x1a4f360  'int C::*' prefix '&' 
> > cannot overflow
> > |   `-DeclRefExpr 0x1a4f2f8  'int' lvalue Field 
> > 0x1a207d0 'field' 'int'
> > |-DeclStmt 0x1a4f560 
> > | `-VarDecl 0x1a4f428  col:12 used cf1 'int C::*' cinit
> > |   `-CXXStaticCastExpr 0x1a4f518  'int C::*' 
> > static_cast  D -> C)>
> > | `-ImplicitCastExpr 0x1a4f500  'int F::*'  
> > part_of_explicit_cast
> > |   `-DeclRefExpr 0x1a4f498  'int F::*' lvalue Var 
> > 0x1a21078 'ff' 'int F::*'
> > |-DeclStmt 

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:29-31
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}

RedDocMD wrote:
> steakhal wrote:
> > The assignment is actually UB.
> > TBH I don't know how to test such behavior xD
> > Same for the next example.
> > 
> > Shouldn't it return `undef` for reading via an invalid member pointer?
> I don't quite know what is `undef`. Is it a special macro or something?
> Using an invalid member pointer is definitely UB, but I also need to show 
> that casting to an invalid pointer is properly handled because that is not 
> UB. I guess I will remove the member access and claim that since there was no 
> crash, it is okay and has been handled appropriately.
By `undef`, I was referring to the `clang::ento::UndefinedVal` - which usually 
represents values that shouldn't be used, not even read. For example, an 
uninitialized variable has undefined value. Or an out-of-bound memory access 
also produces undefined value. There are a few checkers that are hunting for 
such undefined values like CallAndMessageChecker, UndefBranchChecker, 
UndefinedArraySubscriptChecker, UndefinedAssignmentChecker, UndefResultChecker.

Probably removing the member accesses is the best you can do.

> but I also need to show that casting to an invalid pointer is properly 
> handled because that is not UB
In this test, I can't see where you cast the member pointer back to make it 
valid again.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:43-50
+struct A {};
+struct B : public A {};
+struct C {
+  int field;
+};
+struct D : public C {};
+struct E : public B, public D {};

RedDocMD wrote:
> steakhal wrote:
> > An ASCII art would help so much:
> > ```
> > A   C(field)
> > |   |
> > B   D
> >  \ /
> >   E
> >   |
> >   F
> > ```
> > However, I'm still missing a diamond-shaped inheritance.
> > An ASCII art would help so much:
> > ```
> > A   C(field)
> > |   |
> > B   D
> >  \ /
> >   E
> >   |
> >   F
> > ```
> > However, I'm still missing a diamond-shaped inheritance.
> 
> Thanks for the ASCII art!
> The diamond-shaped inheritance as far as I understood will cause illegal code.
> Eg:
> ```
>A
>|
>B
>  /   \
> C D
>  \   /
>E
> ```
> According to my understanding, if I have a field in A or B, and try to define 
> a member pointer like `int E::* ef = ::field`, it is not allowed.
> On GCC, this is the error message I get:
> ```
> struct A {
>   int field;
> };
> 
> struct B : public A {};
> struct C : public virtual B {};
> struct D : public virtual B {};
> struct E : public C, public D {};
> 
> int main() {
>   int E::* ef1 = ::field;
> }
> ```
> diamond-member-pointer.cpp: In function ‘int main()’:
> diamond-member-pointer.cpp:11:22: error: pointer to member conversion via 
> virtual base ‘B’
>11 |   int E::* ef1 = ::field;
>   |  ^
> ```
Oh yea, now I get it.
When it inherits virtually, you can not take the address of it, and if you 
don't inherit virtually then it's ambiguous. Either way, it won't compile.
My bad. However, my other comments still apply.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:52-62
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;

RedDocMD wrote:
> steakhal wrote:
> > Wait a minute. It's not how it works.
> > How I imagine member pointers, they are just offsets.
> > `::field` is notionally equivalent with `offsetof(F, field)`. That being 
> > said, You can not apply this member pointer to any object besides `F`.
> > Imagine if the classes of the inheritance tree would have other fields as 
> > well.
> > Then the `offsetof(T, field)` would be different for `F`, and `C`.
> > 
> > This example demonstrates that both of these member pointer dereferences 
> > are UB.
> > https://godbolt.org/z/15sMEP
> > It returns different values depending on the optimization level, which is a 
> > clear sign of UB.
> > BTW this issue is closely related to strict aliasing.
> The member access on A is definitely UB, I guess I will do what I proposed in 
> the `Some` case.
> I don't think the other one is.  Consider the following:
> ```
> struct A {};
> struct B : public A {};
> struct C {
>   int field;
> };
> struct D : public C {};
> struct E : public B, public D {};
> struct F : public E {};
> 
> int main() {
>   int F::* ff = ::field;
>   int C::* cf1 = static_cast(ff);
>   int C::* cf2 = reinterpret_cast(ff);
>   C c;
>   c.*cf1 = 10;
>   c.*cf2 = 10;
>   return 0;
> }
> ```
> `cf1` and `cf2` are the same thing, except that they are declared differently 
> (one via `static_cast`, other via `reinterpret_cast`). If we look at the AST 
> (truncated to 

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked an inline comment as done.
RedDocMD added inline comments.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:20
   int DoubleDerived::*ddf = ::field;
   int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf)));
   Base base;

steakhal wrote:
> Please have a note describing why you are doing this roundtrip.
You mean why a simpler test wouldn't suffice? Well I think it would, but this 
was a common corner identified by @vsavchenko and I  have put it in as a result.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:29-31
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}

steakhal wrote:
> The assignment is actually UB.
> TBH I don't know how to test such behavior xD
> Same for the next example.
> 
> Shouldn't it return `undef` for reading via an invalid member pointer?
I don't quite know what is `undef`. Is it a special macro or something?
Using an invalid member pointer is definitely UB, but I also need to show that 
casting to an invalid pointer is properly handled because that is not UB. I 
guess I will remove the member access and claim that since there was no crash, 
it is okay and has been handled appropriately.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:52-62
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;

steakhal wrote:
> Wait a minute. It's not how it works.
> How I imagine member pointers, they are just offsets.
> `::field` is notionally equivalent with `offsetof(F, field)`. That being 
> said, You can not apply this member pointer to any object besides `F`.
> Imagine if the classes of the inheritance tree would have other fields as 
> well.
> Then the `offsetof(T, field)` would be different for `F`, and `C`.
> 
> This example demonstrates that both of these member pointer dereferences are 
> UB.
> https://godbolt.org/z/15sMEP
> It returns different values depending on the optimization level, which is a 
> clear sign of UB.
> BTW this issue is closely related to strict aliasing.
The member access on A is definitely UB, I guess I will do what I proposed in 
the `Some` case.
I don't think the other one is.  Consider the following:
```
struct A {};
struct B : public A {};
struct C {
  int field;
};
struct D : public C {};
struct E : public B, public D {};
struct F : public E {};

int main() {
  int F::* ff = ::field;
  int C::* cf1 = static_cast(ff);
  int C::* cf2 = reinterpret_cast(ff);
  C c;
  c.*cf1 = 10;
  c.*cf2 = 10;
  return 0;
}
```
`cf1` and `cf2` are the same thing, except that they are declared differently 
(one via `static_cast`, other via `reinterpret_cast`). If we look at the AST 
(truncated to the first three lines of main):
```
CompoundStmt 0x1a4fe18 
|-DeclStmt 0x1a4f3a8 
| `-VarDecl 0x1a21078  col:12 used ff 'int F::*' cinit
|   `-ImplicitCastExpr 0x1a4f378  'int F::*' 
 D -> C)>
| `-UnaryOperator 0x1a4f360  'int C::*' prefix '&' 
cannot overflow
|   `-DeclRefExpr 0x1a4f2f8  'int' lvalue Field 
0x1a207d0 'field' 'int'
|-DeclStmt 0x1a4f560 
| `-VarDecl 0x1a4f428  col:12 used cf1 'int C::*' cinit
|   `-CXXStaticCastExpr 0x1a4f518  'int C::*' 
static_cast  D -> C)>
| `-ImplicitCastExpr 0x1a4f500  'int F::*'  
part_of_explicit_cast
|   `-DeclRefExpr 0x1a4f498  'int F::*' lvalue Var 0x1a21078 
'ff' 'int F::*'
|-DeclStmt 0x1a4f6e8 
| `-VarDecl 0x1a4f5c8  col:12 used cf2 'int C::*' cinit
|   `-CXXReinterpretCastExpr 0x1a4f6b8  'int C::*' 
reinterpret_cast 
| `-ImplicitCastExpr 0x1a4f6a0  'int F::*'  
part_of_explicit_cast
|   `-DeclRefExpr 0x1a4f638  'int F::*' lvalue Var 0x1a21078 
'ff' 'int F::*'
```
Notice how the `static_cast` figures out the path to the correct subobject. 
This is how member-pointers are handled as far as I can tell.
Unfortunately, Stroustrup is surprisingly scant when it comes to this topic. I 
am trying to dig through the Standard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD marked an inline comment as done.
RedDocMD added inline comments.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:43-50
+struct A {};
+struct B : public A {};
+struct C {
+  int field;
+};
+struct D : public C {};
+struct E : public B, public D {};

steakhal wrote:
> An ASCII art would help so much:
> ```
> A   C(field)
> |   |
> B   D
>  \ /
>   E
>   |
>   F
> ```
> However, I'm still missing a diamond-shaped inheritance.
> An ASCII art would help so much:
> ```
> A   C(field)
> |   |
> B   D
>  \ /
>   E
>   |
>   F
> ```
> However, I'm still missing a diamond-shaped inheritance.

Thanks for the ASCII art!
The diamond-shaped inheritance as far as I understood will cause illegal code.
Eg:
```
   A
   |
   B
 /   \
C D
 \   /
   E
```
According to my understanding, if I have a field in A or B, and try to define a 
member pointer like `int E::* ef = ::field`, it is not allowed.
On GCC, this is the error message I get:
```
struct A {
  int field;
};

struct B : public A {};
struct C : public virtual B {};
struct D : public virtual B {};
struct E : public C, public D {};

int main() {
  int E::* ef1 = ::field;
}
```
diamond-member-pointer.cpp: In function ‘int main()’:
diamond-member-pointer.cpp:11:22: error: pointer to member conversion via 
virtual base ‘B’
   11 |   int E::* ef1 = ::field;
  |  ^
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

I have serious concerns inline.




Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:20
   int DoubleDerived::*ddf = ::field;
   int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf)));
   Base base;

Please have a note describing why you are doing this roundtrip.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:29-31
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}

The assignment is actually UB.
TBH I don't know how to test such behavior xD
Same for the next example.

Shouldn't it return `undef` for reading via an invalid member pointer?



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:43-50
+struct A {};
+struct B : public A {};
+struct C {
+  int field;
+};
+struct D : public C {};
+struct E : public B, public D {};

An ASCII art would help so much:
```
A   C(field)
|   |
B   D
 \ /
  E
  |
  F
```
However, I'm still missing a diamond-shaped inheritance.



Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:52-62
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;

Wait a minute. It's not how it works.
How I imagine member pointers, they are just offsets.
`::field` is notionally equivalent with `offsetof(F, field)`. That being 
said, You can not apply this member pointer to any object besides `F`.
Imagine if the classes of the inheritance tree would have other fields as well.
Then the `offsetof(T, field)` would be different for `F`, and `C`.

This example demonstrates that both of these member pointer dereferences are UB.
https://godbolt.org/z/15sMEP
It returns different values depending on the optimization level, which is a 
clear sign of UB.
BTW this issue is closely related to strict aliasing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 326733.
RedDocMD added a comment.

Removed unnecessary includes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -1,11 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-// XFAIL: asserts
 
 void clang_analyzer_eval(bool);
 
-// TODO: The following test will work properly once reinterpret_cast on pointer-to-member is handled properly
 namespace testReinterpretCasting {
-struct Base {
+struct BaseOfBase {};
+
+struct Base : public BaseOfBase {
   int field;
 };
 
@@ -15,12 +15,51 @@
 
 struct Some {};
 
-void f() {
+void analyzableCasts() {
   int DoubleDerived::*ddf = ::field;
   int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf)));
-  int Some::*sf = reinterpret_cast(ddf);
   Base base;
   base.field = 13;
   clang_analyzer_eval(base.*bf == 13); // expected-warning{{TRUE}}
 }
+
+void castOutsideHierarchy() {
+  int DoubleDerived::*ddf = ::field;
+  int Some::*sf = reinterpret_cast(ddf);
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}
+}
+
+void castAbove() {
+  int DoubleDerived::*ddf = ::field;
+  int BaseOfBase::*bbf = reinterpret_cast(ddf);
+  BaseOfBase bb;
+  bb.*bbf = 23;
+  clang_analyzer_eval(bb.*bbf == 23); // expected-warning{{UNKNOWN}}
+}
+
+namespace testMultipleInheritance {
+struct A {};
+struct B : public A {};
+struct C {
+  int field;
+};
+struct D : public C {};
+struct E : public B, public D {};
+struct F : public E {};
+
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;
+  cobj.*c = 29;
+  clang_analyzer_eval(aobj.*a == 13); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(cobj.*c == 29); // expected-warning{{TRUE}}
+}
+} // namespace testMultipleInheritance
+
 } // namespace testReinterpretCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -522,8 +522,7 @@
 continue;
   }
   case CK_DerivedToBaseMemberPointer:
-  case CK_BaseToDerivedMemberPointer:
-  case CK_ReinterpretMemberPointer: {
+  case CK_BaseToDerivedMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
   SVal CastedPTMSV =
@@ -537,6 +536,25 @@
 state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
 continue;
   }
+  case CK_ReinterpretMemberPointer: {
+SVal V = state->getSVal(Ex, LCtx);
+if (auto PTMSV = V.getAs()) {
+  if (const auto *ReinterpretE =
+  llvm::dyn_cast(CastE)) {
+if (const PointerToMemberData *Data =
+getBasicVals().basesForReinterpretCast(ReinterpretE,
+   *PTMSV)) {
+  SVal CastedPTMSV = svalBuilder.makePointerToMember(Data);
+  state = state->BindExpr(CastE, LCtx, CastedPTMSV);
+  Bldr.generateNode(CastE, Pred, state);
+  continue;
+}
+  }
+}
+// Explicitly proceed with default handler for this case cascade.
+state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+continue;
+  }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
   case CK_VectorSplat: {
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -13,6 +13,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/Store.h"
@@ -22,6 +23,7 @@
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include 
 #include 
 #include 
@@ -239,13 +241,90 @@
 
 

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@steakhal, you are absolutely right! It works. Thank you for pointing it out, 
not sure how I missed this earlier this evening.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 326731.
RedDocMD added a comment.

Replaced BFS with existing CXXRecordDeclMethod


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -1,11 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-// XFAIL: asserts
 
 void clang_analyzer_eval(bool);
 
-// TODO: The following test will work properly once reinterpret_cast on pointer-to-member is handled properly
 namespace testReinterpretCasting {
-struct Base {
+struct BaseOfBase {};
+
+struct Base : public BaseOfBase {
   int field;
 };
 
@@ -15,12 +15,51 @@
 
 struct Some {};
 
-void f() {
+void analyzableCasts() {
   int DoubleDerived::*ddf = ::field;
   int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf)));
-  int Some::*sf = reinterpret_cast(ddf);
   Base base;
   base.field = 13;
   clang_analyzer_eval(base.*bf == 13); // expected-warning{{TRUE}}
 }
+
+void castOutsideHierarchy() {
+  int DoubleDerived::*ddf = ::field;
+  int Some::*sf = reinterpret_cast(ddf);
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}
+}
+
+void castAbove() {
+  int DoubleDerived::*ddf = ::field;
+  int BaseOfBase::*bbf = reinterpret_cast(ddf);
+  BaseOfBase bb;
+  bb.*bbf = 23;
+  clang_analyzer_eval(bb.*bbf == 23); // expected-warning{{UNKNOWN}}
+}
+
+namespace testMultipleInheritance {
+struct A {};
+struct B : public A {};
+struct C {
+  int field;
+};
+struct D : public C {};
+struct E : public B, public D {};
+struct F : public E {};
+
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;
+  cobj.*c = 29;
+  clang_analyzer_eval(aobj.*a == 13); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(cobj.*c == 29); // expected-warning{{TRUE}}
+}
+} // namespace testMultipleInheritance
+
 } // namespace testReinterpretCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -522,8 +522,7 @@
 continue;
   }
   case CK_DerivedToBaseMemberPointer:
-  case CK_BaseToDerivedMemberPointer:
-  case CK_ReinterpretMemberPointer: {
+  case CK_BaseToDerivedMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
   SVal CastedPTMSV =
@@ -537,6 +536,25 @@
 state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
 continue;
   }
+  case CK_ReinterpretMemberPointer: {
+SVal V = state->getSVal(Ex, LCtx);
+if (auto PTMSV = V.getAs()) {
+  if (const auto *ReinterpretE =
+  llvm::dyn_cast(CastE)) {
+if (const PointerToMemberData *Data =
+getBasicVals().basesForReinterpretCast(ReinterpretE,
+   *PTMSV)) {
+  SVal CastedPTMSV = svalBuilder.makePointerToMember(Data);
+  state = state->BindExpr(CastE, LCtx, CastedPTMSV);
+  Bldr.generateNode(CastE, Pred, state);
+  continue;
+}
+  }
+}
+// Explicitly proceed with default handler for this case cascade.
+state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+continue;
+  }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
   case CK_VectorSplat: {
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -13,6 +13,7 @@
 //===--===//
 
 #include "clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/APSIntType.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/Store.h"
@@ -22,8 +23,11 @@
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include 
 #include 
+#include 
+#include 
 

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96976#2590200 , @RedDocMD wrote:

> Unfortunately, all the methods on CXXRecordDecl, like the one you mentioned, 
> are for querying and thus returns a `bool`, while I need the entire path.

AFAIK the second overload accepts an out parameter serving exactly our needs. 
Could you check if my assumption is correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

> So here you are implying that for reinterpret casts, the 
> `CastE->path_begin()-end()` range is empty? And that is why you need to 
> manually come up with a //possible// CXXBaseSpecifier list?

@steakhal, Yes. For illustration, the following code:

  struct Base {
int field;
  };
  
  struct Derived : public Base {};
  struct DoubleDerived : public Derived {};
  
  int f() {
int DoubleDerived::*ddf = ::field;
int Base::*bf1 = static_cast(ddf);
int Base::*bf2 = reinterpret_cast(ddf);
  }

produces the following (truncated) AST dump

  -FunctionDecl 0x12f3890  line:8:5 f 'int ()'
`-CompoundStmt 0x12f3ec0 
  |-DeclStmt 0x12f3b70 
  | `-VarDecl 0x12f3a18  col:23 used ddf 'int 
DoubleDerived::*' cinit
  |   `-ImplicitCastExpr 0x12f3b48  'int DoubleDerived::*' 
 Base)>
  | `-UnaryOperator 0x12f3b30  'int Base::*' prefix '&' 
cannot overflow
  |   `-DeclRefExpr 0x12f3ad0  'int' lvalue Field 
0x12f33d0 'field' 'int'
  |-DeclStmt 0x12f3d20 
  | `-VarDecl 0x12f3bf0  col:14 bf1 'int Base::*' cinit
  |   `-CXXStaticCastExpr 0x12f3ce0  'int Base::*' 
static_cast  Base)>
  | `-ImplicitCastExpr 0x12f3cc8  'int DoubleDerived::*' 
 part_of_explicit_cast
  |   `-DeclRefExpr 0x12f3c60  'int DoubleDerived::*' lvalue 
Var 0x12f3a18 'ddf' 'int DoubleDerived::*'
  `-DeclStmt 0x12f3ea8 
`-VarDecl 0x12f3d88  col:14 bf2 'int Base::*' cinit
  `-CXXReinterpretCastExpr 0x12f3e78  'int Base::*' 
reinterpret_cast 
`-ImplicitCastExpr 0x12f3e60  'int DoubleDerived::*' 
 part_of_explicit_cast
  `-DeclRefExpr 0x12f3df8  'int DoubleDerived::*' lvalue 
Var 0x12f3a18 'ddf' 'int DoubleDerived::*'

If you compare the three cases:

1. The first statement has an implicit cast inserted during parsing, and so we 
need not calculate the path manually
2. The second statement has a `static_cast` and it also supplies a path, except 
that we need to remove these bases instead of adding. (Ref to D95877 
).
3. The third statement has no path provided, because it is a `reinterpret_cast` 
and the parser cannot produce a path in all cases (if you cast to an integer 
for an instance) - so it doesn't produce any path

With the parser not providing us with info, we have to figure it out in a 
different way. 
Also, it is not one //possible// path but the //only// possible path. If there 
are multiple acceptable paths then it is illegal to define such a 
member-pointer and the frontend will reject it as such.

> I strongly encourage you to find a better alternative to this manual graph 
> search.
> This information must be available somewhere.
>
> Have you looked at `CXXRecordDecl::isDerivedFrom()`? Or any other member 
> functions of that class?

Unfortunately, all the methods on CXXRecordDecl, like the one you mentioned, 
are for querying and thus returns a `bool`, while I need the entire path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96976#2587513 , @RedDocMD wrote:

> 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`.

So here you are implying that for reinterpret casts, the 
`CastE->path_begin()-end()` range is empty? And that is why you need to 
manually come up with a //possible// CXXBaseSpecifier list?

I strongly encourage you to find a better alternative to this manual graph 
search.
This information must be available somewhere.

Have you looked at `CXXRecordDecl::isDerivedFrom()`? Or any other member 
functions of that class?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 326414.
RedDocMD added a comment.

Added some more comments to increase clarity


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -1,11 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-// XFAIL: asserts
 
 void clang_analyzer_eval(bool);
 
-// TODO: The following test will work properly once reinterpret_cast on pointer-to-member is handled properly
 namespace testReinterpretCasting {
-struct Base {
+struct BaseOfBase {};
+
+struct Base : public BaseOfBase {
   int field;
 };
 
@@ -15,12 +15,51 @@
 
 struct Some {};
 
-void f() {
+void analyzableCasts() {
   int DoubleDerived::*ddf = ::field;
   int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf)));
-  int Some::*sf = reinterpret_cast(ddf);
   Base base;
   base.field = 13;
   clang_analyzer_eval(base.*bf == 13); // expected-warning{{TRUE}}
 }
+
+void castOutsideHierarchy() {
+  int DoubleDerived::*ddf = ::field;
+  int Some::*sf = reinterpret_cast(ddf);
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}
+}
+
+void castAbove() {
+  int DoubleDerived::*ddf = ::field;
+  int BaseOfBase::*bbf = reinterpret_cast(ddf);
+  BaseOfBase bb;
+  bb.*bbf = 23;
+  clang_analyzer_eval(bb.*bbf == 23); // expected-warning{{UNKNOWN}}
+}
+
+namespace testMultipleInheritance {
+struct A {};
+struct B : public A {};
+struct C {
+  int field;
+};
+struct D : public C {};
+struct E : public B, public D {};
+struct F : public E {};
+
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;
+  cobj.*c = 29;
+  clang_analyzer_eval(aobj.*a == 13); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(cobj.*c == 29); // expected-warning{{TRUE}}
+}
+} // namespace testMultipleInheritance
+
 } // namespace testReinterpretCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -522,8 +522,7 @@
 continue;
   }
   case CK_DerivedToBaseMemberPointer:
-  case CK_BaseToDerivedMemberPointer:
-  case CK_ReinterpretMemberPointer: {
+  case CK_BaseToDerivedMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
   SVal CastedPTMSV =
@@ -537,6 +536,25 @@
 state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
 continue;
   }
+  case CK_ReinterpretMemberPointer: {
+SVal V = state->getSVal(Ex, LCtx);
+if (auto PTMSV = V.getAs()) {
+  if (const auto *ReinterpretE =
+  llvm::dyn_cast(CastE)) {
+if (const PointerToMemberData *Data =
+getBasicVals().basesForReinterpretCast(ReinterpretE,
+   *PTMSV)) {
+  SVal CastedPTMSV = svalBuilder.makePointerToMember(Data);
+  state = state->BindExpr(CastE, LCtx, CastedPTMSV);
+  Bldr.generateNode(CastE, Pred, state);
+  continue;
+}
+  }
+}
+// Explicitly proceed with default handler for this case cascade.
+state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+continue;
+  }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
   case CK_VectorSplat: {
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -22,8 +22,11 @@
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include 
 #include 
+#include 
+#include 
 #include 
 
 using namespace clang;
@@ -239,13 +242,128 @@
 
 return getPointerToMemberData(ND, ReducedBaseSpecList);
   }
-  // FIXME: Reinterpret casts on member-pointers are not handled properly by
-  // this code
   for (const CXXBaseSpecifier *I : llvm::reverse(PathRange))
 BaseSpecList = prependCXXBase(I, BaseSpecList);
   return getPointerToMemberData(ND, BaseSpecList);
 }
 
+struct 

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 326413.
RedDocMD added a comment.

Added test for multiple inheritance


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -1,11 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-// XFAIL: asserts
 
 void clang_analyzer_eval(bool);
 
-// TODO: The following test will work properly once reinterpret_cast on pointer-to-member is handled properly
 namespace testReinterpretCasting {
-struct Base {
+struct BaseOfBase {};
+
+struct Base : public BaseOfBase {
   int field;
 };
 
@@ -15,12 +15,51 @@
 
 struct Some {};
 
-void f() {
+void analyzableCasts() {
   int DoubleDerived::*ddf = ::field;
   int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf)));
-  int Some::*sf = reinterpret_cast(ddf);
   Base base;
   base.field = 13;
   clang_analyzer_eval(base.*bf == 13); // expected-warning{{TRUE}}
 }
+
+void castOutsideHierarchy() {
+  int DoubleDerived::*ddf = ::field;
+  int Some::*sf = reinterpret_cast(ddf);
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}
+}
+
+void castAbove() {
+  int DoubleDerived::*ddf = ::field;
+  int BaseOfBase::*bbf = reinterpret_cast(ddf);
+  BaseOfBase bb;
+  bb.*bbf = 23;
+  clang_analyzer_eval(bb.*bbf == 23); // expected-warning{{UNKNOWN}}
+}
+
+namespace testMultipleInheritance {
+struct A {};
+struct B : public A {};
+struct C {
+  int field;
+};
+struct D : public C {};
+struct E : public B, public D {};
+struct F : public E {};
+
+void testMultiple() {
+  int F::*f = ::field;
+  int A::*a = reinterpret_cast(f);
+  int C::*c = reinterpret_cast(f);
+  A aobj;
+  C cobj;
+  aobj.*a = 13;
+  cobj.*c = 29;
+  clang_analyzer_eval(aobj.*a == 13); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(cobj.*c == 29); // expected-warning{{TRUE}}
+}
+} // namespace testMultipleInheritance
+
 } // namespace testReinterpretCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -522,8 +522,7 @@
 continue;
   }
   case CK_DerivedToBaseMemberPointer:
-  case CK_BaseToDerivedMemberPointer:
-  case CK_ReinterpretMemberPointer: {
+  case CK_BaseToDerivedMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
   SVal CastedPTMSV =
@@ -537,6 +536,25 @@
 state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
 continue;
   }
+  case CK_ReinterpretMemberPointer: {
+SVal V = state->getSVal(Ex, LCtx);
+if (auto PTMSV = V.getAs()) {
+  if (const auto *ReinterpretE =
+  llvm::dyn_cast(CastE)) {
+if (const PointerToMemberData *Data =
+getBasicVals().basesForReinterpretCast(ReinterpretE,
+   *PTMSV)) {
+  SVal CastedPTMSV = svalBuilder.makePointerToMember(Data);
+  state = state->BindExpr(CastE, LCtx, CastedPTMSV);
+  Bldr.generateNode(CastE, Pred, state);
+  continue;
+}
+  }
+}
+// Explicitly proceed with default handler for this case cascade.
+state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+continue;
+  }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
   case CK_VectorSplat: {
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -22,8 +22,11 @@
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include 
 #include 
+#include 
+#include 
 #include 
 
 using namespace clang;
@@ -239,13 +242,115 @@
 
 return getPointerToMemberData(ND, ReducedBaseSpecList);
   }
-  // FIXME: Reinterpret casts on member-pointers are not handled properly by
-  // this code
   for (const CXXBaseSpecifier *I : llvm::reverse(PathRange))
 BaseSpecList = prependCXXBase(I, BaseSpecList);
   return getPointerToMemberData(ND, BaseSpecList);
 }
 
+struct BFSNode {

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 326410.
RedDocMD added a comment.

Cleaned up tests, added test above class hierarchy


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -1,11 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-// XFAIL: asserts
 
 void clang_analyzer_eval(bool);
 
-// TODO: The following test will work properly once reinterpret_cast on pointer-to-member is handled properly
 namespace testReinterpretCasting {
-struct Base {
+struct BaseOfBase {};
+
+struct Base : public BaseOfBase {
   int field;
 };
 
@@ -15,12 +15,28 @@
 
 struct Some {};
 
-void f() {
+void analyzableCasts() {
   int DoubleDerived::*ddf = ::field;
   int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf)));
-  int Some::*sf = reinterpret_cast(ddf);
   Base base;
   base.field = 13;
   clang_analyzer_eval(base.*bf == 13); // expected-warning{{TRUE}}
 }
+
+void castOutsideHierarchy() {
+  int DoubleDerived::*ddf = ::field;
+  int Some::*sf = reinterpret_cast(ddf);
+  Some some;
+  some.*sf = 14;
+  clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}}
+}
+
+void castAbove() {
+  int DoubleDerived::*ddf = ::field;
+  int BaseOfBase::*bbf = reinterpret_cast(ddf);
+  BaseOfBase bb;
+  bb.*bbf = 23;
+  clang_analyzer_eval(bb.*bbf == 23); // expected-warning{{UNKNOWN}}
+}
+
 } // namespace testReinterpretCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -522,8 +522,7 @@
 continue;
   }
   case CK_DerivedToBaseMemberPointer:
-  case CK_BaseToDerivedMemberPointer:
-  case CK_ReinterpretMemberPointer: {
+  case CK_BaseToDerivedMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
   SVal CastedPTMSV =
@@ -537,6 +536,25 @@
 state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
 continue;
   }
+  case CK_ReinterpretMemberPointer: {
+SVal V = state->getSVal(Ex, LCtx);
+if (auto PTMSV = V.getAs()) {
+  if (const auto *ReinterpretE =
+  llvm::dyn_cast(CastE)) {
+if (const PointerToMemberData *Data =
+getBasicVals().basesForReinterpretCast(ReinterpretE,
+   *PTMSV)) {
+  SVal CastedPTMSV = svalBuilder.makePointerToMember(Data);
+  state = state->BindExpr(CastE, LCtx, CastedPTMSV);
+  Bldr.generateNode(CastE, Pred, state);
+  continue;
+}
+  }
+}
+// Explicitly proceed with default handler for this case cascade.
+state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+continue;
+  }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
   case CK_VectorSplat: {
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -22,8 +22,11 @@
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include 
 #include 
+#include 
+#include 
 #include 
 
 using namespace clang;
@@ -239,13 +242,115 @@
 
 return getPointerToMemberData(ND, ReducedBaseSpecList);
   }
-  // FIXME: Reinterpret casts on member-pointers are not handled properly by
-  // this code
   for (const CXXBaseSpecifier *I : llvm::reverse(PathRange))
 BaseSpecList = prependCXXBase(I, BaseSpecList);
   return getPointerToMemberData(ND, BaseSpecList);
 }
 
+struct BFSNode {
+  const CXXBaseSpecifier *BaseSpec;
+  std::shared_ptr Parent;
+
+  BFSNode(const CXXBaseSpecifier *BaseSpec,
+  std::shared_ptr Parent = nullptr)
+  : BaseSpec{BaseSpec}, Parent{Parent} {}
+};
+
+std::pair, bool>
+findPathToBase(const Type *Start, const Type *End) {
+  std::deque> queue;
+  llvm::SmallVector Path;
+  const CXXRecordDecl *StartDecl = Start->getAsCXXRecordDecl();
+  if (!StartDecl)
+return {Path, false};
+
+  for (const CXXBaseSpecifier  : StartDecl->bases())
+

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 326393.
RedDocMD marked 2 inline comments as done.
RedDocMD added a comment.

Removed explicit for-loop with range-for loop


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-// XFAIL: asserts
 
 void clang_analyzer_eval(bool);
 
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -522,8 +522,7 @@
 continue;
   }
   case CK_DerivedToBaseMemberPointer:
-  case CK_BaseToDerivedMemberPointer:
-  case CK_ReinterpretMemberPointer: {
+  case CK_BaseToDerivedMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
   SVal CastedPTMSV =
@@ -537,6 +536,25 @@
 state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
 continue;
   }
+  case CK_ReinterpretMemberPointer: {
+SVal V = state->getSVal(Ex, LCtx);
+if (auto PTMSV = V.getAs()) {
+  if (const auto *ReinterpretE =
+  llvm::dyn_cast(CastE)) {
+if (const PointerToMemberData *Data =
+getBasicVals().basesForReinterpretCast(ReinterpretE,
+   *PTMSV)) {
+  SVal CastedPTMSV = svalBuilder.makePointerToMember(Data);
+  state = state->BindExpr(CastE, LCtx, CastedPTMSV);
+  Bldr.generateNode(CastE, Pred, state);
+  continue;
+}
+  }
+}
+// Explicitly proceed with default handler for this case cascade.
+state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+continue;
+  }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
   case CK_VectorSplat: {
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -22,8 +22,11 @@
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include 
 #include 
+#include 
+#include 
 #include 
 
 using namespace clang;
@@ -239,13 +242,115 @@
 
 return getPointerToMemberData(ND, ReducedBaseSpecList);
   }
-  // FIXME: Reinterpret casts on member-pointers are not handled properly by
-  // this code
   for (const CXXBaseSpecifier *I : llvm::reverse(PathRange))
 BaseSpecList = prependCXXBase(I, BaseSpecList);
   return getPointerToMemberData(ND, BaseSpecList);
 }
 
+struct BFSNode {
+  const CXXBaseSpecifier *BaseSpec;
+  std::shared_ptr Parent;
+
+  BFSNode(const CXXBaseSpecifier *BaseSpec,
+  std::shared_ptr Parent = nullptr)
+  : BaseSpec{BaseSpec}, Parent{Parent} {}
+};
+
+std::pair, bool>
+findPathToBase(const Type *Start, const Type *End) {
+  std::deque> queue;
+  llvm::SmallVector Path;
+  const CXXRecordDecl *StartDecl = Start->getAsCXXRecordDecl();
+  if (!StartDecl)
+return {Path, false};
+
+  for (const CXXBaseSpecifier  : StartDecl->bases())
+queue.push_back(std::make_shared());
+
+  while (!queue.empty()) {
+std::shared_ptr CurrNode = queue.front();
+queue.pop_front();
+const CXXBaseSpecifier *CurrBaseSpec = CurrNode->BaseSpec;
+const Type *CurrType = CurrBaseSpec->getType().getTypePtr();
+if (CurrType == End) {
+  BFSNode *Ptr = CurrNode.get();
+  while (Ptr) {
+Path.push_back(Ptr->BaseSpec);
+Ptr = Ptr->Parent.get();
+  }
+  std::reverse(Path.begin(), Path.end());
+  return {Path, true};
+}
+const CXXRecordDecl *CurrRecordDecl = CurrType->getAsCXXRecordDecl();
+assert(CurrRecordDecl &&
+   "Base of a CXX class/struct must be a CXX class/struct");
+for (const CXXBaseSpecifier  : CurrRecordDecl->bases())
+  queue.push_back(std::make_shared(, CurrNode));
+  }
+
+  return {Path, false};
+}
+
+const CXXRecordDecl *retrieveCXXRecord(const NamedDecl *ND) {
+  if (const auto *FD = llvm::dyn_cast(ND)) {
+const RecordDecl *RD = FD->getParent();
+return llvm::dyn_cast(RD);
+  }
+  if (const auto 

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

[Deep Majumder via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D96976#2585498 , @RedDocMD wrote:

> @steakhal, could you please review this?

The longer I stare at it the more questions I have. However, I don't know how 
member pointers work in the analyzer and I'm not really in the mood to dive 
deep into that territory.

Could you elaborate on what approach did you choose and more importantly, why 
that approach?
Why do we need a graph search here?
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?

Your code has shared pointers, which is interesting, but I would favor unique 
pointers unless you can defend this use-case.
Also, in general, we prefer algorithms to raw for or while loops if they are 
semantically equivalent.
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.

I can not confirm the implementation and accept the patch.
Improving the summary, helping reviewers with helpful notes here and there 
could give the boost you need to get this reviewed.
BTW, the review process takes ages in the analyzer community. We should also 
improve on that ofc, but the first step is always done by you (and here I mean 
not specifically you but we all).




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;

I know that you just copy-pasted this, but what is this xD



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 {

I assume this is resolved by now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@steakhal, could you please review this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@vsavchenko, could you please review this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@vsavchenko, does it look okay?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96976

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD created this revision.
RedDocMD added reviewers: vsavchenko, NoQ, dcoughlin.
Herald added subscribers: steakhal, ASDenysPetrov, martong, Charusso, dkrupp, 
donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, 
xazax.hun.
RedDocMD requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

If the cast is to a class such that it is possible to reach the correct
sub-object containing the pointed member, a path to that class is
discovered via BFS. This path is used to construct the new
pointer-to-member data. If there is no such path, the resulting node is
not analyzed further.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96976

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
  clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-// XFAIL: asserts
 
 void clang_analyzer_eval(bool);
 
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -522,8 +522,7 @@
 continue;
   }
   case CK_DerivedToBaseMemberPointer:
-  case CK_BaseToDerivedMemberPointer:
-  case CK_ReinterpretMemberPointer: {
+  case CK_BaseToDerivedMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
   SVal CastedPTMSV =
@@ -537,6 +536,25 @@
 state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
 continue;
   }
+  case CK_ReinterpretMemberPointer: {
+SVal V = state->getSVal(Ex, LCtx);
+if (auto PTMSV = V.getAs()) {
+  if (const auto *ReinterpretE =
+  llvm::dyn_cast(CastE)) {
+if (const PointerToMemberData *Data =
+getBasicVals().basesForReinterpretCast(ReinterpretE,
+   *PTMSV)) {
+  SVal CastedPTMSV = svalBuilder.makePointerToMember(Data);
+  state = state->BindExpr(CastE, LCtx, CastedPTMSV);
+  Bldr.generateNode(CastE, Pred, state);
+  continue;
+}
+  }
+}
+// Explicitly proceed with default handler for this case cascade.
+state = handleLVectorSplat(state, LCtx, CastE, Bldr, Pred);
+continue;
+  }
   // Various C++ casts that are not handled yet.
   case CK_ToUnion:
   case CK_VectorSplat: {
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -22,8 +22,11 @@
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include 
 #include 
+#include 
+#include 
 #include 
 
 using namespace clang;
@@ -239,13 +242,118 @@
 
 return getPointerToMemberData(ND, ReducedBaseSpecList);
   }
-  // FIXME: Reinterpret casts on member-pointers are not handled properly by
-  // this code
   for (const CXXBaseSpecifier *I : llvm::reverse(PathRange))
 BaseSpecList = prependCXXBase(I, BaseSpecList);
   return getPointerToMemberData(ND, BaseSpecList);
 }
 
+struct BFSNode {
+  const CXXBaseSpecifier *BaseSpec;
+  std::shared_ptr Parent;
+
+  BFSNode(const CXXBaseSpecifier *BaseSpec,
+  std::shared_ptr Parent = nullptr)
+  : BaseSpec{BaseSpec}, Parent{Parent} {}
+};
+
+std::pair, bool>
+findPathToBase(const Type *Start, const Type *End) {
+  std::deque> queue;
+  llvm::SmallVector Path;
+  const CXXRecordDecl *StartDecl = Start->getAsCXXRecordDecl();
+  if (!StartDecl)
+return {Path, false};
+
+  for (auto It = StartDecl->bases_begin(), E = StartDecl->bases_end(); It != E;
+   ++It)
+queue.push_back(std::make_shared(It));
+
+  while (!queue.empty()) {
+std::shared_ptr CurrNode = queue.front();
+queue.pop_front();
+const CXXBaseSpecifier *CurrBaseSpec = CurrNode->BaseSpec;
+const Type *CurrType = CurrBaseSpec->getType().getTypePtr();
+if (CurrType == End) {
+  BFSNode *Ptr = CurrNode.get();
+  while (Ptr) {
+Path.push_back(Ptr->BaseSpec);
+Ptr = Ptr->Parent.get();
+  }
+  std::reverse(Path.begin(), Path.end());
+  return {Path, true};
+}
+const CXXRecordDecl *CurrRecordDecl =