[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D95877#2563383 , @davezarzycki 
wrote:

> The test seems to be broken on Fedora 33 (x86-64 with clang-11):
>
>   XPASS: Clang :: Analysis/reinterpret-cast-pointer-to-member.cpp (6531 of 
> 74360)
>    TEST 'Clang :: 
> Analysis/reinterpret-cast-pointer-to-member.cpp' FAILED 
>   Script:
>   --
>   : 'RUN: at line 1';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem 
> /tmp/_update_lc/r/lib/clang/13.0.0/include -nostdsysteminc -analyze 
> -analyzer-constraints=range -setup-static-analyzer 
> -analyzer-checker=core,debug.ExprInspection -verify 
> /home/dave/ro_s/lp/clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
>   --
>   Exit Code: 0

Hi there, is it happening after this commit?
https://github.com/llvm/llvm-project/commit/6f21adac6dd7082f7231ae342d40ed04f4885e79


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D95877#2563383 , @davezarzycki 
wrote:

> The test seems to be broken on Fedora 33 (x86-64 with clang-11):
>
>   XPASS: Clang :: Analysis/reinterpret-cast-pointer-to-member.cpp (6531 of 
> 74360)
>    TEST 'Clang :: 
> Analysis/reinterpret-cast-pointer-to-member.cpp' FAILED 
>   Script:
>   --
>   : 'RUN: at line 1';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem 
> /tmp/_update_lc/r/lib/clang/13.0.0/include -nostdsysteminc -analyze 
> -analyzer-constraints=range -setup-static-analyzer 
> -analyzer-checker=core,debug.ExprInspection -verify 
> /home/dave/ro_s/lp/clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
>   --
>   Exit Code: 0

Yes this has been fixed by @vsavchenko in 
https://github.com/llvm/llvm-project/commit/6f21adac6dd7082f7231ae342d40ed04f4885e79


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added a comment.

The test seems to be broken on Fedora 33 (x86-64 with clang-11):

  XPASS: Clang :: Analysis/reinterpret-cast-pointer-to-member.cpp (6531 of 
74360)
   TEST 'Clang :: 
Analysis/reinterpret-cast-pointer-to-member.cpp' FAILED 
  Script:
  --
  : 'RUN: at line 1';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem 
/tmp/_update_lc/r/lib/clang/13.0.0/include -nostdsysteminc -analyze 
-analyzer-constraints=range -setup-static-analyzer 
-analyzer-checker=core,debug.ExprInspection -verify 
/home/dave/ro_s/lp/clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
  --
  Exit Code: 0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D95877#2563220 , @RKSimon wrote:

> @RedDocMD Please can you take a look at this buildbot failure: 
> http://lab.llvm.org:8011/#/builders/124

Yes. Thanks for the ping 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

@RedDocMD Please can you take a look at this buildbot failure: 
http://lab.llvm.org:8011/#/builders/124


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG21daada95079: [analyzer] Fix static_cast on 
pointer-to-member handling (authored by RedDocMD, committed by vsavchenko).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- /dev/null
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+// XFAIL: *
+
+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 {
+  int field;
+};
+
+struct Derived : public Base {};
+
+struct DoubleDerived : public Derived {};
+
+struct Some {};
+
+void f() {
+  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}}
+}
+} // namespace testReinterpretCasting
Index: clang/test/Analysis/pointer-to-member.cpp
===
--- clang/test/Analysis/pointer-to-member.cpp
+++ clang/test/Analysis/pointer-to-member.cpp
@@ -287,3 +287,26 @@
   clang_analyzer_eval(a.*ep == 5); // expected-warning{{TRUE}}
 }
 } // namespace testAnonymousMember
+
+namespace testStaticCasting {
+// From bug #48739
+struct Grandfather {
+  int field;
+};
+
+struct Father : public Grandfather {};
+struct Son : public Father {};
+
+void test() {
+  int Son::*sf = ::field;
+  Grandfather grandpa;
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));
+  int Grandfather::*gpf3 = static_cast(static_cast(static_cast(sf)));
+  clang_analyzer_eval(grandpa.*gpf1 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf2 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf3 == 10); // expected-warning{{TRUE}}
+}
+} // namespace testStaticCasting
+
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -526,10 +526,9 @@
   case CK_ReinterpretMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
-  SVal CastedPTMSV = svalBuilder.makePointerToMember(
-  getBasicVals().accumCXXBase(
-  llvm::make_range(
-  CastE->path_begin(), CastE->path_end()), *PTMSV));
+  SVal CastedPTMSV =
+  svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
+  CastE->path(), *PTMSV, CastE->getCastKind()));
   state = state->BindExpr(CastE, LCtx, CastedPTMSV);
   Bldr.generateNode(CastE, Pred, state);
   continue;
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -21,6 +21,7 @@
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include 
 #include 
 #include 
@@ -176,28 +177,73 @@
   return D;
 }
 
+LLVM_ATTRIBUTE_UNUSED bool hasNoRepeatedElements(
+llvm::ImmutableList BaseSpecList) {
+  llvm::SmallPtrSet BaseSpecSeen;
+  for (const CXXBaseSpecifier *BaseSpec : BaseSpecList) {
+QualType BaseType = BaseSpec->getType();
+// Check whether inserted
+if (!BaseSpecSeen.insert(BaseType).second)
+  return false;
+  }
+  return true;
+}
+
 const PointerToMemberData *BasicValueFactory::accumCXXBase(
 llvm::iterator_range PathRange,
-const nonloc::PointerToMember ) {
+const nonloc::PointerToMember , const CastKind ) {
+  assert((kind == CK_DerivedToBaseMemberPointer ||
+  kind == CK_BaseToDerivedMemberPointer ||
+  kind == CK_ReinterpretMemberPointer) &&
+ "accumCXXBase called with wrong CastKind");
   nonloc::PointerToMember::PTMDataType PTMDT = PTM.getPTMData();
   const NamedDecl *ND = nullptr;
-  llvm::ImmutableList PathList;
+ 

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

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

Great job!  Thanks for dealing with all my nit-picking!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@vsavchenko, anything else to add?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D95877#2548027 , @vsavchenko wrote:

> Yes, it'd be better to extract this case into a separate file and mark it as 
> XFAIL

Done that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

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

Moved failing test of reinterpret_cast to its own file, marked to failx


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

Index: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
===
--- /dev/null
+++ clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+// XFAIL: *
+
+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 {
+  int field;
+};
+
+struct Derived : public Base {};
+
+struct DoubleDerived : public Derived {};
+
+struct Some {};
+
+void f() {
+  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}}
+}
+} // namespace testReinterpretCasting
Index: clang/test/Analysis/pointer-to-member.cpp
===
--- clang/test/Analysis/pointer-to-member.cpp
+++ clang/test/Analysis/pointer-to-member.cpp
@@ -287,3 +287,26 @@
   clang_analyzer_eval(a.*ep == 5); // expected-warning{{TRUE}}
 }
 } // namespace testAnonymousMember
+
+namespace testStaticCasting {
+// From bug #48739
+struct Grandfather {
+  int field;
+};
+
+struct Father : public Grandfather {};
+struct Son : public Father {};
+
+void test() {
+  int Son::*sf = ::field;
+  Grandfather grandpa;
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));
+  int Grandfather::*gpf3 = static_cast(static_cast(static_cast(sf)));
+  clang_analyzer_eval(grandpa.*gpf1 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf2 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf3 == 10); // expected-warning{{TRUE}}
+}
+} // namespace testStaticCasting
+
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -526,10 +526,9 @@
   case CK_ReinterpretMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
-  SVal CastedPTMSV = svalBuilder.makePointerToMember(
-  getBasicVals().accumCXXBase(
-  llvm::make_range(
-  CastE->path_begin(), CastE->path_end()), *PTMSV));
+  SVal CastedPTMSV =
+  svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
+  CastE->path(), *PTMSV, CastE->getCastKind()));
   state = state->BindExpr(CastE, LCtx, CastedPTMSV);
   Bldr.generateNode(CastE, Pred, state);
   continue;
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -21,6 +21,7 @@
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include 
 #include 
 #include 
@@ -176,28 +177,73 @@
   return D;
 }
 
+LLVM_ATTRIBUTE_UNUSED bool hasNoRepeatedElements(
+llvm::ImmutableList BaseSpecList) {
+  llvm::SmallPtrSet BaseSpecSeen;
+  for (const CXXBaseSpecifier *BaseSpec : BaseSpecList) {
+QualType BaseType = BaseSpec->getType();
+// Check whether inserted
+if (!BaseSpecSeen.insert(BaseType).second)
+  return false;
+  }
+  return true;
+}
+
 const PointerToMemberData *BasicValueFactory::accumCXXBase(
 llvm::iterator_range PathRange,
-const nonloc::PointerToMember ) {
+const nonloc::PointerToMember , const CastKind ) {
+  assert((kind == CK_DerivedToBaseMemberPointer ||
+  kind == CK_BaseToDerivedMemberPointer ||
+  kind == CK_ReinterpretMemberPointer) &&
+ "accumCXXBase called with wrong CastKind");
   nonloc::PointerToMember::PTMDataType PTMDT = PTM.getPTMData();
   const NamedDecl *ND = nullptr;
-  llvm::ImmutableList PathList;
+  llvm::ImmutableList BaseSpecList;
 
   if (PTMDT.isNull() || PTMDT.is()) {
 if (PTMDT.is())
   ND = PTMDT.get();
 

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D95877#2548021 , @RedDocMD wrote:

> In D95877#2547949 , @vsavchenko 
> wrote:
>
>> I think we had a bit of misunderstanding about the test.  It still should 
>> pass (we can't have broken tests, it will disrupt CI bots).  Try to put the 
>> test case, so it is THERE and you CHANGE the expected outcome when the 
>> analyzer behaves correctly.  Right now, as I see above, the test crashes.
>
> Actually, that is the reason why I initially had commented out the crashing 
> test. Until `reinterpret_cast` is properly handled, the test will crash due 
> to an assertion failure, irrespective of what is put as the expected outcome. 
> Is there a way to flag that a test should crash? (I see that after the tests 
> are run, there is an expectedly-failed category). The crash is caused when 
> the static analyzer tries to evaluate `base.*bf`.

Yes, it'd be better to extract this case into a separate file and mark it as 
XFAIL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

In D95877#2547949 , @vsavchenko wrote:

> I think we had a bit of misunderstanding about the test.  It still should 
> pass (we can't have broken tests, it will disrupt CI bots).  Try to put the 
> test case, so it is THERE and you CHANGE the expected outcome when the 
> analyzer behaves correctly.  Right now, as I see above, the test crashes.

Actually, that is the reason why I initially had commented out the crashing 
test. Until `reinterpret_cast` is properly handled, the test will crash due to 
an assertion failure, irrespective of what is put as the expected outcome. Is 
there a way to flag that a test should crash? (I see that after the tests are 
run, there is an expectedly-failed category). The crash is caused when the 
static analyzer tries to evaluate `base.*bf`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D95877#2547303 , @RedDocMD wrote:

> @vsavchenko are there some more changes you want done?

I think we had a bit of misunderstanding about the test.  It still should pass 
(we can't have broken tests, it will disrupt CI bots).  Try to put the test 
case, so it is THERE and you CHANGE the expected outcome when the analyzer 
behaves correctly.  Right now, as I see above, the test crashes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@vsavchenko are there some more changes you want done?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:184
+  llvm::SmallPtrSet BaseSpecSeen;
+  for (const auto  : BaseSpecList) {
+auto BaseType = BaseSpec->getType();

vsavchenko wrote:
> vsavchenko wrote:
> > LLVM Coding Style calls for very limited use of `auto`, so here and the 
> > next line would be better with actual types.
> Let's also reiterate on using functional-style predicates and reimplement it 
> in terms of `llvm::all_of` or `llvm::none_of`.
I couldn't do this since calling llvm::all_of (or std::all_of) on an 
ImmutableList causes a compile error saying that the function 
std::__iterator_category cannot be found for its iterator.



Comment at: clang/test/Analysis/pointer-to-member.cpp:314-333
+// namespace testReinterpretCasting {
+// struct Base {
+//   int field;
+// };
+//
+// struct Derived : public Base {};
+//

vsavchenko wrote:
> RedDocMD wrote:
> > vsavchenko wrote:
> > > Uncomment it, and expect the actual current result.  This is where `TODO` 
> > > will come in handy.
> > > Uncomment it, and expect the actual current result.  This is where `TODO` 
> > > will come in handy.
> > Will do it.
> > Just one clarification: the static analyzer tests only serve to check 
> > whether the Static Analyzer crashes or hits an assertion error, or is it 
> > something more?
> > 
> Mostly they check for reported results, you can see special comments like `// 
> expect-warning{...}` in almost every test.
So right now we have a failing test for this, I guess its my job in the next 
commit to make it pass. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 321816.
RedDocMD marked 10 inline comments as done.
RedDocMD added a comment.

Some more aesthetic cleanup, added a failing test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

Index: clang/test/Analysis/pointer-to-member.cpp
===
--- clang/test/Analysis/pointer-to-member.cpp
+++ clang/test/Analysis/pointer-to-member.cpp
@@ -287,3 +287,47 @@
   clang_analyzer_eval(a.*ep == 5); // expected-warning{{TRUE}}
 }
 } // namespace testAnonymousMember
+
+namespace testStaticCasting {
+// From bug #48739
+struct Grandfather {
+  int field;
+};
+
+struct Father : public Grandfather {};
+struct Son : public Father {};
+
+void test() {
+  int Son::*sf = ::field;
+  Grandfather grandpa;
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));
+  int Grandfather::*gpf3 = static_cast(static_cast(static_cast(sf)));
+  clang_analyzer_eval(grandpa.*gpf1 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf2 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf3 == 10); // expected-warning{{TRUE}}
+}
+} // namespace testStaticCasting
+
+// TODO: The following test will work properly once reinterpret_cast on pointer to member is handled properly
+namespace testReinterpretCasting {
+struct Base {
+  int field;
+};
+
+struct Derived : public Base {};
+
+struct DoubleDerived : public Derived {};
+
+struct Some {};
+
+void f() {
+  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}}
+}
+} // namespace testReinterpretCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -526,10 +526,9 @@
   case CK_ReinterpretMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
-  SVal CastedPTMSV = svalBuilder.makePointerToMember(
-  getBasicVals().accumCXXBase(
-  llvm::make_range(
-  CastE->path_begin(), CastE->path_end()), *PTMSV));
+  SVal CastedPTMSV =
+  svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
+  CastE->path(), *PTMSV, CastE->getCastKind()));
   state = state->BindExpr(CastE, LCtx, CastedPTMSV);
   Bldr.generateNode(CastE, Pred, state);
   continue;
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -21,6 +21,7 @@
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include 
 #include 
 #include 
@@ -176,28 +177,73 @@
   return D;
 }
 
+LLVM_ATTRIBUTE_UNUSED bool hasNoRepeatedElements(
+llvm::ImmutableList BaseSpecList) {
+  llvm::SmallPtrSet BaseSpecSeen;
+  for (const CXXBaseSpecifier *BaseSpec : BaseSpecList) {
+QualType BaseType = BaseSpec->getType();
+// Check whether inserted
+if (!BaseSpecSeen.insert(BaseType).second)
+  return false;
+  }
+  return true;
+}
+
 const PointerToMemberData *BasicValueFactory::accumCXXBase(
 llvm::iterator_range PathRange,
-const nonloc::PointerToMember ) {
+const nonloc::PointerToMember , const CastKind ) {
+  assert((kind == CK_DerivedToBaseMemberPointer ||
+  kind == CK_BaseToDerivedMemberPointer ||
+  kind == CK_ReinterpretMemberPointer) &&
+ "accumCXXBase called with wrong CastKind");
   nonloc::PointerToMember::PTMDataType PTMDT = PTM.getPTMData();
   const NamedDecl *ND = nullptr;
-  llvm::ImmutableList PathList;
+  llvm::ImmutableList BaseSpecList;
 
   if (PTMDT.isNull() || PTMDT.is()) {
 if (PTMDT.is())
   ND = PTMDT.get();
 
-PathList = CXXBaseListFactory.getEmptyList();
-  } else { // const PointerToMemberData *
+BaseSpecList = CXXBaseListFactory.getEmptyList();
+  } else {
 const PointerToMemberData *PTMD = PTMDT.get();
 ND = PTMD->getDeclaratorDecl();
 
-PathList = PTMD->getCXXBaseList();
+BaseSpecList = PTMD->getCXXBaseList();
   }
 
-  for (const auto  : llvm::reverse(PathRange))
-

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

I downloaded and tested this patch. On the whole it works OK. See my 
suggestions below.




Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:205
+  llvm::SmallPtrSet BaseSpecSeen;
+  for (const auto  : BaseSpecList) {
+auto BaseType = BaseSpec->getType();

I suggest to use explicit types instead of `auto` to increase readability.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:226-229
+  if (llvm::none_of(
+  PathRange, [](const auto ) -> auto {
+return BaseSpec->getType() == I->getType();
+  }))

It will look clearer if we use an intermediate variable for predicate here.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:234-238
+  } else {
+for (const auto  : llvm::reverse(PathRange))
+  BaseSpecList = prependCXXBase(I, BaseSpecList);
+return getPointerToMemberData(ND, BaseSpecList);
+  }

We can remove else branch here and move its body out unconditionally as the 
main one already has `return` at the end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:180
 
+bool noRepeatedElements(
+const llvm::ImmutableList ) {

RedDocMD wrote:
> vsavchenko wrote:
> > nit: functions represent actions, in languages verbs act the same way, so 
> > it's recommended to use verbs in functions (`hasNoRepeatedElements`)
> > 
> > + `LLVM_MAYBE_UNUSED` because in the build without assertions it would be a 
> > warning otherwise.
> > nit: functions represent actions, in languages verbs act the same way, so 
> > it's recommended to use verbs in functions (`hasNoRepeatedElements`)
> > 
> > + `LLVM_MAYBE_UNUSED` because in the build without assertions it would be a 
> > warning otherwise.
> 
> How do I put in the LLVM_MAYBE_UNUSED? Sorry for being annoying, but 
> ripgrepping or Googling didn't return anything.
Sorry, my bad, it's `LLVM_ATTRIBUTE_UNUSED`



Comment at: clang/test/Analysis/pointer-to-member.cpp:314-333
+// namespace testReinterpretCasting {
+// struct Base {
+//   int field;
+// };
+//
+// struct Derived : public Base {};
+//

RedDocMD wrote:
> vsavchenko wrote:
> > Uncomment it, and expect the actual current result.  This is where `TODO` 
> > will come in handy.
> > Uncomment it, and expect the actual current result.  This is where `TODO` 
> > will come in handy.
> Will do it.
> Just one clarification: the static analyzer tests only serve to check whether 
> the Static Analyzer crashes or hits an assertion error, or is it something 
> more?
> 
Mostly they check for reported results, you can see special comments like `// 
expect-warning{...}` in almost every test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:180
 
+bool noRepeatedElements(
+const llvm::ImmutableList ) {

vsavchenko wrote:
> nit: functions represent actions, in languages verbs act the same way, so 
> it's recommended to use verbs in functions (`hasNoRepeatedElements`)
> 
> + `LLVM_MAYBE_UNUSED` because in the build without assertions it would be a 
> warning otherwise.
> nit: functions represent actions, in languages verbs act the same way, so 
> it's recommended to use verbs in functions (`hasNoRepeatedElements`)
> 
> + `LLVM_MAYBE_UNUSED` because in the build without assertions it would be a 
> warning otherwise.

How do I put in the LLVM_MAYBE_UNUSED? Sorry for being annoying, but 
ripgrepping or Googling didn't return anything.



Comment at: clang/test/Analysis/pointer-to-member.cpp:314-333
+// namespace testReinterpretCasting {
+// struct Base {
+//   int field;
+// };
+//
+// struct Derived : public Base {};
+//

vsavchenko wrote:
> Uncomment it, and expect the actual current result.  This is where `TODO` 
> will come in handy.
> Uncomment it, and expect the actual current result.  This is where `TODO` 
> will come in handy.
Will do it.
Just one clarification: the static analyzer tests only serve to check whether 
the Static Analyzer crashes or hits an assertion error, or is it something more?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:180
 
+bool noRepeatedElements(
+const llvm::ImmutableList ) {

nit: functions represent actions, in languages verbs act the same way, so it's 
recommended to use verbs in functions (`hasNoRepeatedElements`)

+ `LLVM_MAYBE_UNUSED` because in the build without assertions it would be a 
warning otherwise.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:181
+bool noRepeatedElements(
+const llvm::ImmutableList ) {
+  // First we need to make sure that there are no-repetitions in BaseSpecList

`ImmutableList` is one of those value types that prefers copying for clarity.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:184
+  llvm::SmallPtrSet BaseSpecSeen;
+  for (const auto  : BaseSpecList) {
+auto BaseType = BaseSpec->getType();

LLVM Coding Style calls for very limited use of `auto`, so here and the next 
line would be better with actual types.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:184
+  llvm::SmallPtrSet BaseSpecSeen;
+  for (const auto  : BaseSpecList) {
+auto BaseType = BaseSpec->getType();

vsavchenko wrote:
> LLVM Coding Style calls for very limited use of `auto`, so here and the next 
> line would be better with actual types.
Let's also reiterate on using functional-style predicates and reimplement it in 
terms of `llvm::all_of` or `llvm::none_of`.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:232
+auto ReducedBaseSpecList = CXXBaseListFactory.getEmptyList();
+for (const auto  : BaseSpecList)
+  if (llvm::none_of(

The same thing about `auto`



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:234
+  if (llvm::none_of(
+  PathRange, [](const auto ) -> auto {
+return BaseSpec->getType() == I->getType();

No need



Comment at: clang/test/Analysis/pointer-to-member.cpp:314-333
+// namespace testReinterpretCasting {
+// struct Base {
+//   int field;
+// };
+//
+// struct Derived : public Base {};
+//

Uncomment it, and expect the actual current result.  This is where `TODO` will 
come in handy.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

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

@vsavchenko , does it look okay now?




Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:27
 #include 
+#include 
 #include 

vsavchenko wrote:
> We don't need this anymore :)
Wow, sharp eyes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD updated this revision to Diff 321714.
RedDocMD marked an inline comment as done.
RedDocMD added a comment.

Added TODO for reinterpret_cast, moved assertion code to its own block


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

Index: clang/test/Analysis/pointer-to-member.cpp
===
--- clang/test/Analysis/pointer-to-member.cpp
+++ clang/test/Analysis/pointer-to-member.cpp
@@ -287,3 +287,47 @@
   clang_analyzer_eval(a.*ep == 5); // expected-warning{{TRUE}}
 }
 } // namespace testAnonymousMember
+
+namespace testStaticCasting {
+// From bug #48739
+struct Grandfather {
+  int field;
+};
+
+struct Father : public Grandfather {};
+struct Son : public Father {};
+
+void test() {
+  int Son::*sf = ::field;
+  Grandfather grandpa;
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));
+  int Grandfather::*gpf3 = static_cast(static_cast(static_cast(sf)));
+  clang_analyzer_eval(grandpa.*gpf1 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf2 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf3 == 10); // expected-warning{{TRUE}}
+}
+} // namespace testStaticCasting
+
+// TODO: The following test will work properly once reinterpret_cast on pointer to member is handled properly
+// namespace testReinterpretCasting {
+// struct Base {
+//   int field;
+// };
+//
+// struct Derived : public Base {};
+//
+// struct DoubleDerived : public Derived {};
+//
+// struct Some {};
+//
+// void f() {
+//   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}}
+// }
+// } // namespace testReinterpretCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -526,10 +526,9 @@
   case CK_ReinterpretMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
-  SVal CastedPTMSV = svalBuilder.makePointerToMember(
-  getBasicVals().accumCXXBase(
-  llvm::make_range(
-  CastE->path_begin(), CastE->path_end()), *PTMSV));
+  SVal CastedPTMSV =
+  svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
+  CastE->path(), *PTMSV, CastE->getCastKind()));
   state = state->BindExpr(CastE, LCtx, CastedPTMSV);
   Bldr.generateNode(CastE, Pred, state);
   continue;
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -21,6 +21,7 @@
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include 
 #include 
 #include 
@@ -176,28 +177,74 @@
   return D;
 }
 
+bool noRepeatedElements(
+const llvm::ImmutableList ) {
+  // First we need to make sure that there are no-repetitions in BaseSpecList
+  llvm::SmallPtrSet BaseSpecSeen;
+  for (const auto  : BaseSpecList) {
+auto BaseType = BaseSpec->getType();
+auto Stat = BaseSpecSeen.insert(BaseType);
+if (!Stat.second)
+  return false;
+  }
+  return true;
+}
+
 const PointerToMemberData *BasicValueFactory::accumCXXBase(
 llvm::iterator_range PathRange,
-const nonloc::PointerToMember ) {
+const nonloc::PointerToMember , const CastKind ) {
+  assert((kind == CK_DerivedToBaseMemberPointer ||
+  kind == CK_BaseToDerivedMemberPointer ||
+  kind == CK_ReinterpretMemberPointer) &&
+ "accumCXXBase called with wrong CastKind");
   nonloc::PointerToMember::PTMDataType PTMDT = PTM.getPTMData();
   const NamedDecl *ND = nullptr;
-  llvm::ImmutableList PathList;
+  llvm::ImmutableList BaseSpecList;
 
   if (PTMDT.isNull() || PTMDT.is()) {
 if (PTMDT.is())
   ND = PTMDT.get();
 
-PathList = CXXBaseListFactory.getEmptyList();
-  } else { // const PointerToMemberData *
+BaseSpecList = CXXBaseListFactory.getEmptyList();
+  } else {
 const PointerToMemberData *PTMD = PTMDT.get();
 ND = PTMD->getDeclaratorDecl();
 
-PathList = PTMD->getCXXBaseList();
+

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:210
+   "repeated elements");
+BaseSpecSeen.insert(BaseType);
   }

Containers like `map` and `set` usually return a pair for `insert`, where the 
second element tells us whether that element was inserted or was already there. 
It is usually a bad practice to use `find` in the vicinity of `insert` for this 
very reason, you double the work.

Also, a not about this whole part of the code, if it's done only for 
assertions, none of this code should be executed in the build without 
assertions.  You'll need to extract this into a function that you call from 
assert. It means that in release builds it will be unused and should be marked 
with `LLVM_MAYBE_UNUSED`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

In D95877#2542578 , @RedDocMD wrote:

> @vsavchenko, after some experimentation, I found out that handling 
> reinterpret_cast will be a real pain. Consider the following:
>
>   struct Base {
>   int field;
>   };
>   
>   struct Derived : public Base {};
>   
>   struct DoubleDerived : public Derived {};
>   
>   struct Some {};
>   
>   void f() {
>   int DoubleDerived::* ddf = ::field;
>   int Base::* bf = reinterpret_cast(reinterpret_cast Derived::*>(reinterpret_cast(ddf)));
>   int Some::* sf = reinterpret_cast(ddf);
>   Base base;
>   base.*bf;
>   }
>
> The definition of `bf` can be handled I guess by manually discovering when to 
> insert a sub-class or when to remove. It will require a bit of code but I 
> guess is doable.
> As for the next one, also it has to be manually worked out whether it makes 
> sense to process this statement further and add a node to the Exploded Graph. 
> For the example I gave I don't think it makes a lot of sense. Multiple 
> inheritance will make the task a lot worse as far as I can tell.
> Should I try to achieve this in another commit? What are your thoughts on 
> this?

Yes, another commit is fine.  However, I'd say that it is still good to add 
test cases with TODOs with this commit.




Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:27
 #include 
+#include 
 #include 

We don't need this anymore :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@vsavchenko, after some experimentation, I found out that handling 
reinterpret_cast will be a real pain. Consider the following:

  struct Base {
int field;
  };
  
  struct Derived : public Base {};
  
  struct DoubleDerived : public Derived {};
  
  struct Some {};
  
  void f() {
int DoubleDerived::* ddf = ::field;
int Base::* bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf)));
int Some::* sf = reinterpret_cast(ddf);
Base base;
base.*bf;
  }

The definition of `bf` can be handled I guess by manually discovering when to 
insert a sub-class or when to remove. It will require a bit of code but I guess 
is doable.
As for the next one, also it has to be manually worked out whether it makes 
sense to process this statement further and add a node to the Exploded Graph. 
For the example I gave I don't think it makes a lot of sense. Multiple 
inheritance will make the task a lot worse as far as I can tell.
Should I try to achieve this in another commit? What are your thoughts on this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

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

Updated to use new function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

Index: clang/test/Analysis/pointer-to-member.cpp
===
--- clang/test/Analysis/pointer-to-member.cpp
+++ clang/test/Analysis/pointer-to-member.cpp
@@ -287,3 +287,25 @@
   clang_analyzer_eval(a.*ep == 5); // expected-warning{{TRUE}}
 }
 } // namespace testAnonymousMember
+
+namespace testStaticCasting {
+// From bug #48739
+struct Grandfather {
+  int field;
+};
+
+struct Father : public Grandfather {};
+struct Son : public Father {};
+
+void test() {
+  int Son::*sf = ::field;
+  Grandfather grandpa;
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));
+  int Grandfather::*gpf3 = static_cast(static_cast(static_cast(sf)));
+  clang_analyzer_eval(grandpa.*gpf1 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf2 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf3 == 10); // expected-warning{{TRUE}}
+}
+} // namespace testStaticCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -526,10 +526,9 @@
   case CK_ReinterpretMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
-  SVal CastedPTMSV = svalBuilder.makePointerToMember(
-  getBasicVals().accumCXXBase(
-  llvm::make_range(
-  CastE->path_begin(), CastE->path_end()), *PTMSV));
+  SVal CastedPTMSV =
+  svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
+  CastE->path(), *PTMSV, CastE->getCastKind()));
   state = state->BindExpr(CastE, LCtx, CastedPTMSV);
   Bldr.generateNode(CastE, Pred, state);
   continue;
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -21,8 +21,10 @@
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -178,26 +180,62 @@
 
 const PointerToMemberData *BasicValueFactory::accumCXXBase(
 llvm::iterator_range PathRange,
-const nonloc::PointerToMember ) {
+const nonloc::PointerToMember , const CastKind ) {
+  assert((kind == CK_DerivedToBaseMemberPointer ||
+  kind == CK_BaseToDerivedMemberPointer ||
+  kind == CK_ReinterpretMemberPointer) &&
+ "accumCXXBase called with wrong CastKind");
   nonloc::PointerToMember::PTMDataType PTMDT = PTM.getPTMData();
   const NamedDecl *ND = nullptr;
-  llvm::ImmutableList PathList;
+  llvm::ImmutableList BaseSpecList;
 
   if (PTMDT.isNull() || PTMDT.is()) {
 if (PTMDT.is())
   ND = PTMDT.get();
 
-PathList = CXXBaseListFactory.getEmptyList();
-  } else { // const PointerToMemberData *
+BaseSpecList = CXXBaseListFactory.getEmptyList();
+  } else {
 const PointerToMemberData *PTMD = PTMDT.get();
 ND = PTMD->getDeclaratorDecl();
 
-PathList = PTMD->getCXXBaseList();
+BaseSpecList = PTMD->getCXXBaseList();
+  }
+  // First we need to make sure that there are no-repetitions in BaseSpecList
+  llvm::SmallPtrSet BaseSpecSeen;
+  for (const auto  : BaseSpecList) {
+auto BaseType = BaseSpec->getType();
+assert(BaseSpecSeen.find(BaseType) == BaseSpecSeen.end() &&
+   "CXXBaseSpecifier list of PointerToMemberData must not have "
+   "repeated elements");
+BaseSpecSeen.insert(BaseType);
   }
 
-  for (const auto  : llvm::reverse(PathRange))
-PathList = prependCXXBase(I, PathList);
-  return getPointerToMemberData(ND, PathList);
+  if (kind == CK_DerivedToBaseMemberPointer) {
+// Here we pop off matching CXXBaseSpecifiers from BaseSpecList.
+// Because, CK_DerivedToBaseMemberPointer comes from a static_cast and
+// serves to remove a matching implicit cast. Note that static_cast's that
+// are no-ops do not count since they produce an empty PathRange, a nice
+// thing about Clang AST.
+
+// Now we know that there are no repetitions in BaseSpecList.
+// So, popping the first element from it 

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

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

@vsavchenko , I have used the logic that you suggested and put in a better 
assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

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

Cleaner implementation of the popping logic, using STL/LLVM algorithms


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

Index: clang/test/Analysis/pointer-to-member.cpp
===
--- clang/test/Analysis/pointer-to-member.cpp
+++ clang/test/Analysis/pointer-to-member.cpp
@@ -287,3 +287,25 @@
   clang_analyzer_eval(a.*ep == 5); // expected-warning{{TRUE}}
 }
 } // namespace testAnonymousMember
+
+namespace testStaticCasting {
+// From bug #48739
+struct Grandfather {
+  int field;
+};
+
+struct Father : public Grandfather {};
+struct Son : public Father {};
+
+void test() {
+  int Son::*sf = ::field;
+  Grandfather grandpa;
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));
+  int Grandfather::*gpf3 = static_cast(static_cast(static_cast(sf)));
+  clang_analyzer_eval(grandpa.*gpf1 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf2 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf3 == 10); // expected-warning{{TRUE}}
+}
+} // namespace testStaticCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -526,10 +526,11 @@
   case CK_ReinterpretMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
-  SVal CastedPTMSV = svalBuilder.makePointerToMember(
-  getBasicVals().accumCXXBase(
+  SVal CastedPTMSV =
+  svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
   llvm::make_range(
-  CastE->path_begin(), CastE->path_end()), *PTMSV));
+  CastE->path_begin(), CastE->path_end()),
+  *PTMSV, CastE->getCastKind()));
   state = state->BindExpr(CastE, LCtx, CastedPTMSV);
   Bldr.generateNode(CastE, Pred, state);
   continue;
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -21,8 +21,10 @@
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallPtrSet.h"
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -178,26 +180,63 @@
 
 const PointerToMemberData *BasicValueFactory::accumCXXBase(
 llvm::iterator_range PathRange,
-const nonloc::PointerToMember ) {
+const nonloc::PointerToMember , const CastKind ) {
+  assert((kind == CK_DerivedToBaseMemberPointer ||
+  kind == CK_BaseToDerivedMemberPointer ||
+  kind == CK_ReinterpretMemberPointer) &&
+ "accumCXXBase called with wrong CastKind");
   nonloc::PointerToMember::PTMDataType PTMDT = PTM.getPTMData();
   const NamedDecl *ND = nullptr;
-  llvm::ImmutableList PathList;
+  llvm::ImmutableList BaseSpecList;
 
   if (PTMDT.isNull() || PTMDT.is()) {
 if (PTMDT.is())
   ND = PTMDT.get();
 
-PathList = CXXBaseListFactory.getEmptyList();
+BaseSpecList = CXXBaseListFactory.getEmptyList();
   } else { // const PointerToMemberData *
 const PointerToMemberData *PTMD = PTMDT.get();
 ND = PTMD->getDeclaratorDecl();
 
-PathList = PTMD->getCXXBaseList();
+BaseSpecList = PTMD->getCXXBaseList();
   }
 
-  for (const auto  : llvm::reverse(PathRange))
-PathList = prependCXXBase(I, PathList);
-  return getPointerToMemberData(ND, PathList);
+  if (kind == CK_DerivedToBaseMemberPointer) {
+// Here we pop off matching CXXBaseSpecifiers from BaseSpecList.
+// Because, CK_DerivedToBaseMemberPointer comes from a static_cast and
+// serves to remove a matching implicit cast. Note that static_cast's that
+// are no-ops do not count since they produce an empty PathRange, a nice
+// thing about Clang AST.
+
+// First we need to make sure that there are no-repetitions in BaseSpecList
+llvm::SmallPtrSet BaseSpecSeen;
+for (const auto  : BaseSpecList) {
+  auto BaseType = BaseSpec->getType();
+  assert(BaseSpecSeen.find(BaseType) == BaseSpecSeen.end() &&
+ "CXXBaseSpecifier list of PointerToMemberData must not have "
+ "repeated elements");
+  BaseSpecSeen.insert(BaseType);
+}
+
+// Now we know 

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:226
+llvm::make_range(BaseList.begin(), BaseList.end()),
+[](const auto ) -> bool { return I.second; });
+

RedDocMD wrote:
> vsavchenko wrote:
> > I think it's all a bit too complicated now.
> > 
> > Right now you have two nested loops:
> >   1. over `PathRange`
> >   2. over `BaseList` (in `find_if`)
> > 
> > It won't be a problem to put them in another order, so when you make a 
> > `filter_range`, you can have `llvm::find_if` which will iterate over 
> > `PathRange` and captures the given `BaseList` element. So, yes nested 
> > lambdas.
> > 
> > Here is a sketch of the solution:
> > ```
> > auto BaseListFilteredRange = llvm::make_filter_range(
> > BaseList, // you don't need to make_range here, BaseList IS a range
> > [](const CXXBaseSpecifier *Base) {
> >   return llvm::find_if(PathRange, [Base](const auto ) {
> > return Base->getType() == It.first->getType();
> >   }) == PathRange.end();
> > });
> > ```
> > 
> > Aaaand while I was writing that, it got me thinking that we don't need 
> > `BaseList` in the first place:
> > ```
> > for (const CXXBaseSpecifier *Base : PathList)
> >   if (llvm::none_of(PathRange,
> > [Base](const auto ) { return Base->getType() == 
> > It.first->getType(); })
> >   PathList = CXXBaseListFactory.add(Base, PathList);
> > ```
> Hmm, I really should brush up on my functional programming and think more in 
> terms of STL/LLVM algorithms. Thank you for pointing out this solution, 
> @vsavchenko!
> 
> One thing I noticed though, the shortened logic that you suggested removes 
> all matching instances from PathList, while my intention was to remove only 
> the first one of every kind. If there are no repetitions, it is the same. 
> And, normally, repetitions should not be present in the PathList retrieved 
> from PTM. It is a bug elsewhere if that happens. So should I add an assert 
> for that here?
Yes, that's true.
Assertions will usually make intensions of the original author more clear for 
future readers.
Plus, I think it can be also good to add a comment (something similar to your 
summary for this change) to describe what is going on here and WHY it is done.



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp:531-532
+  svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
   llvm::make_range(
-  CastE->path_begin(), CastE->path_end()), *PTMSV));
+  CastE->path_begin(), CastE->path_end()),
+  *PTMSV, CastE->getCastKind()));

BTW, it looks like this can be replaced by `CastE->path()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.






Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:226
+llvm::make_range(BaseList.begin(), BaseList.end()),
+[](const auto ) -> bool { return I.second; });
+

vsavchenko wrote:
> I think it's all a bit too complicated now.
> 
> Right now you have two nested loops:
>   1. over `PathRange`
>   2. over `BaseList` (in `find_if`)
> 
> It won't be a problem to put them in another order, so when you make a 
> `filter_range`, you can have `llvm::find_if` which will iterate over 
> `PathRange` and captures the given `BaseList` element. So, yes nested lambdas.
> 
> Here is a sketch of the solution:
> ```
> auto BaseListFilteredRange = llvm::make_filter_range(
> BaseList, // you don't need to make_range here, BaseList IS a range
> [](const CXXBaseSpecifier *Base) {
>   return llvm::find_if(PathRange, [Base](const auto ) {
> return Base->getType() == It.first->getType();
>   }) == PathRange.end();
> });
> ```
> 
> Aaaand while I was writing that, it got me thinking that we don't need 
> `BaseList` in the first place:
> ```
> for (const CXXBaseSpecifier *Base : PathList)
>   if (llvm::none_of(PathRange,
> [Base](const auto ) { return Base->getType() == 
> It.first->getType(); })
>   PathList = CXXBaseListFactory.add(Base, PathList);
> ```
Hmm, I really should brush up on my functional programming and think more in 
terms of STL/LLVM algorithms. Thank you for pointing out this solution, 
@vsavchenko!

One thing I noticed though, the shortened logic that you suggested removes all 
matching instances from PathList, while my intention was to remove only the 
first one of every kind. If there are no repetitions, it is the same. And, 
normally, repetitions should not be present in the PathList retrieved from PTM. 
It is a bug elsewhere if that happens. So should I add an assert for that here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Also, all these `PathRange` and `PathList` don't really reflect what they stand 
for semantically and talk more about implementation.  It's not the problem of 
this patch, but it doesn't make your logic easier to comprehend.




Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:226
+llvm::make_range(BaseList.begin(), BaseList.end()),
+[](const auto ) -> bool { return I.second; });
+

I think it's all a bit too complicated now.

Right now you have two nested loops:
  1. over `PathRange`
  2. over `BaseList` (in `find_if`)

It won't be a problem to put them in another order, so when you make a 
`filter_range`, you can have `llvm::find_if` which will iterate over 
`PathRange` and captures the given `BaseList` element. So, yes nested lambdas.

Here is a sketch of the solution:
```
auto BaseListFilteredRange = llvm::make_filter_range(
BaseList, // you don't need to make_range here, BaseList IS a range
[](const CXXBaseSpecifier *Base) {
  return llvm::find_if(PathRange, [Base](const auto ) {
return Base->getType() == It.first->getType();
  }) == PathRange.end();
});
```

Aaaand while I was writing that, it got me thinking that we don't need 
`BaseList` in the first place:
```
for (const CXXBaseSpecifier *Base : PathList)
  if (llvm::none_of(PathRange,
[Base](const auto ) { return Base->getType() == 
It.first->getType(); })
  PathList = CXXBaseListFactory.add(Base, PathList);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added a comment.

@vsavchenko could you please see the current logic if it is better now?




Comment at: clang/test/Analysis/pointer-to-member.cpp:304
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));

vsavchenko wrote:
> What about other type of casts?
I am looking into this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

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

Made assertion more verbose


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

Index: clang/test/Analysis/pointer-to-member.cpp
===
--- clang/test/Analysis/pointer-to-member.cpp
+++ clang/test/Analysis/pointer-to-member.cpp
@@ -287,3 +287,25 @@
   clang_analyzer_eval(a.*ep == 5); // expected-warning{{TRUE}}
 }
 } // namespace testAnonymousMember
+
+namespace testStaticCasting {
+// From bug #48739
+struct Grandfather {
+  int field;
+};
+
+struct Father : public Grandfather {};
+struct Son : public Father {};
+
+void test() {
+  int Son::*sf = ::field;
+  Grandfather grandpa;
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));
+  int Grandfather::*gpf3 = static_cast(static_cast(static_cast(sf)));
+  clang_analyzer_eval(grandpa.*gpf1 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf2 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf3 == 10); // expected-warning{{TRUE}}
+}
+} // namespace testStaticCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -526,10 +526,11 @@
   case CK_ReinterpretMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
-  SVal CastedPTMSV = svalBuilder.makePointerToMember(
-  getBasicVals().accumCXXBase(
+  SVal CastedPTMSV =
+  svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
   llvm::make_range(
-  CastE->path_begin(), CastE->path_end()), *PTMSV));
+  CastE->path_begin(), CastE->path_end()),
+  *PTMSV, CastE->getCastKind()));
   state = state->BindExpr(CastE, LCtx, CastedPTMSV);
   Bldr.generateNode(CastE, Pred, state);
   continue;
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -21,8 +21,11 @@
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include 
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -178,7 +181,11 @@
 
 const PointerToMemberData *BasicValueFactory::accumCXXBase(
 llvm::iterator_range PathRange,
-const nonloc::PointerToMember ) {
+const nonloc::PointerToMember , const CastKind ) {
+  assert((kind == CK_DerivedToBaseMemberPointer ||
+  kind == CK_BaseToDerivedMemberPointer ||
+  kind == CK_ReinterpretMemberPointer) &&
+ "accumCXXBase called with wrong CastKind");
   nonloc::PointerToMember::PTMDataType PTMDT = PTM.getPTMData();
   const NamedDecl *ND = nullptr;
   llvm::ImmutableList PathList;
@@ -195,8 +202,39 @@
 PathList = PTMD->getCXXBaseList();
   }
 
-  for (const auto  : llvm::reverse(PathRange))
-PathList = prependCXXBase(I, PathList);
+  if (kind == CK_DerivedToBaseMemberPointer) {
+// Here we pop off matching CXXBaseSpecifiers from PathList.
+// Because, CK_DerivedToBaseMemberPointer comes from a static_cast and
+// serves to remove a matching implicit cast. Note that static_cast's that
+// are no-ops do not count since they produce an empty PathRange, a nice
+// thing about Clang AST.
+
+llvm::SmallVector, 64> BaseList;
+// Initially mark all CXXBaseSpecifier to be included
+for (const auto  : PathList)
+  BaseList.push_back({I, true});
+
+for (const auto  : PathRange) {
+  auto *it = std::find_if(
+  BaseList.begin(), BaseList.end(), [&](const auto ) -> auto {
+return I->getType() == B.first->getType();
+  });
+  assert(it != BaseList.end() &&
+ "Underlying PointerToMemberData of PTM has insufficient number of "
+ "CXXBaseSpecifiers");
+  it->second = false;
+}
+auto BaseListFilteredRange = llvm::make_filter_range(
+llvm::make_range(BaseList.begin(), BaseList.end()),
+[](const auto ) -> bool { return I.second; });
+
+PathList = CXXBaseListFactory.getEmptyList();
+for (const auto  : BaseListFilteredRange)
+  PathList = CXXBaseListFactory.add(I.first, PathList);
+  } else {

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

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

Remoevd the use of std::list with llvm::SmallVector. Removed the need to delete 
elements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

Index: clang/test/Analysis/pointer-to-member.cpp
===
--- clang/test/Analysis/pointer-to-member.cpp
+++ clang/test/Analysis/pointer-to-member.cpp
@@ -287,3 +287,25 @@
   clang_analyzer_eval(a.*ep == 5); // expected-warning{{TRUE}}
 }
 } // namespace testAnonymousMember
+
+namespace testStaticCasting {
+// From bug #48739
+struct Grandfather {
+  int field;
+};
+
+struct Father : public Grandfather {};
+struct Son : public Father {};
+
+void test() {
+  int Son::*sf = ::field;
+  Grandfather grandpa;
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));
+  int Grandfather::*gpf3 = static_cast(static_cast(static_cast(sf)));
+  clang_analyzer_eval(grandpa.*gpf1 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf2 == 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(grandpa.*gpf3 == 10); // expected-warning{{TRUE}}
+}
+} // namespace testStaticCasting
Index: clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -526,10 +526,11 @@
   case CK_ReinterpretMemberPointer: {
 SVal V = state->getSVal(Ex, LCtx);
 if (auto PTMSV = V.getAs()) {
-  SVal CastedPTMSV = svalBuilder.makePointerToMember(
-  getBasicVals().accumCXXBase(
+  SVal CastedPTMSV =
+  svalBuilder.makePointerToMember(getBasicVals().accumCXXBase(
   llvm::make_range(
-  CastE->path_begin(), CastE->path_end()), *PTMSV));
+  CastE->path_begin(), CastE->path_end()),
+  *PTMSV, CastE->getCastKind()));
   state = state->BindExpr(CastE, LCtx, CastedPTMSV);
   Bldr.generateNode(CastE, Pred, state);
   continue;
Index: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
===
--- clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
+++ clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
@@ -21,8 +21,11 @@
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/ImmutableList.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
+#include 
 #include 
 #include 
+#include 
 #include 
 
 using namespace clang;
@@ -178,7 +181,11 @@
 
 const PointerToMemberData *BasicValueFactory::accumCXXBase(
 llvm::iterator_range PathRange,
-const nonloc::PointerToMember ) {
+const nonloc::PointerToMember , const CastKind ) {
+  assert((kind == CK_DerivedToBaseMemberPointer ||
+  kind == CK_BaseToDerivedMemberPointer ||
+  kind == CK_ReinterpretMemberPointer) &&
+ "accumCXXBase called with wrong CastKind");
   nonloc::PointerToMember::PTMDataType PTMDT = PTM.getPTMData();
   const NamedDecl *ND = nullptr;
   llvm::ImmutableList PathList;
@@ -195,8 +202,36 @@
 PathList = PTMD->getCXXBaseList();
   }
 
-  for (const auto  : llvm::reverse(PathRange))
-PathList = prependCXXBase(I, PathList);
+  if (kind == CK_DerivedToBaseMemberPointer) {
+// Here we pop off matching CXXBaseSpecifiers from PathList.
+// Because, CK_DerivedToBaseMemberPointer comes from a static_cast and
+// serves to remove a matching implicit cast. Note that static_cast's that
+// are no-ops do not count since they produce an empty PathRange, a nice
+// thing about Clang AST.
+
+llvm::SmallVector, 64> BaseList;
+// Initially mark all CXXBaseSpecifier to be included
+for (const auto  : PathList)
+  BaseList.push_back({I, true});
+
+for (const auto  : PathRange) {
+  auto *it = std::find_if(
+  BaseList.begin(), BaseList.end(), [&](const auto ) -> auto {
+return I->getType() == B.first->getType();
+  });
+  it->second = false;
+}
+auto BaseListFilteredRange = llvm::make_filter_range(
+llvm::make_range(BaseList.begin(), BaseList.end()),
+[](const auto ) -> bool { return I.second; });
+
+PathList = CXXBaseListFactory.getEmptyList();
+for (const auto  : BaseListFilteredRange)
+  PathList = CXXBaseListFactory.add(I.first, PathList);
+  } else {
+for (const auto  : llvm::reverse(PathRange))
+  PathList = prependCXXBase(I, 

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Deep Majumder via Phabricator via cfe-commits]

RedDocMD added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:223
+  auto DelIt = find_first(BaseList.begin(), BaseList.end(), PathBase);
+  assert(DelIt != BaseList.end() && "PTM has insufficient base 
specifiers");
+  BaseList.erase(DelIt);

vsavchenko wrote:
> It's better to be more verbose in the assertions.
> Additionally, I'm not sure that it is clear what it is all about because 
> pointer-to-members do not have base specifiers.
Well, the `PointerToMember` contains a `PointerToMemberData`, which has a list 
of `CXXBaseSpecifier`. Should I put something like: "PointerToMemberData has 
insufficient number of base specifiers"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Thanks for taking your time and getting to the root cause of it!




Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:209
+// thing about Clang AST.
+std::list BaseList;
+for (const auto  : PathList)

`std::list` is like the slowest container, and it should be avoided in almost 
every case.
Considering the semantics of it, you can definitely use `llvm::SmallVector`.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:222
+for (const auto  : llvm::reverse(PathRange)) {
+  auto DelIt = find_first(BaseList.begin(), BaseList.end(), PathBase);
+  assert(DelIt != BaseList.end() && "PTM has insufficient base 
specifiers");

`llvm::find_if` with a lambda capturing `PathBase`?



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:223
+  auto DelIt = find_first(BaseList.begin(), BaseList.end(), PathBase);
+  assert(DelIt != BaseList.end() && "PTM has insufficient base 
specifiers");
+  BaseList.erase(DelIt);

It's better to be more verbose in the assertions.
Additionally, I'm not sure that it is clear what it is all about because 
pointer-to-members do not have base specifiers.



Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:224
+  assert(DelIt != BaseList.end() && "PTM has insufficient base 
specifiers");
+  BaseList.erase(DelIt);
+}

In order to avoid removals from the middle, you can use a more functional 
approach to collections and iterators and use `llvm::make_filter_range` and 
iterate over it in the loop below,



Comment at: clang/test/Analysis/pointer-to-member.cpp:304
+  grandpa.field = 10;
+  int Grandfather::*gpf1 = static_cast(sf);
+  int Grandfather::*gpf2 = static_cast(static_cast(sf));

What about other type of casts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95877

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