[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

@balazske , I tested these changes - again - and it all seems to work for me. I 
would have preferred we do not see the build status failures before doing this, 
but maybe we can be sure to avoid this next time (because I'll want to test 
again before our final merge). Please move ahead with your changes and let us 
know when you're ready for final review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145868

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

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Hi @balazske . Can we go back to the original proposed fix and treat the new 
issue separately? We have an internal crash open that is corrected by the 
original patch I proposed, and passes all LITs and unit tests. I think it would 
be better to separate these concerns so we can move forward. 
Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145868

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

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Hi @balazske , all LIT and unittests pass with this change. By your logic, then 
we are missing some LIT or unittest cases that support your statement. Can you 
think of a case that demonstrates this? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145868

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

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

@balazske and I discussed, he will be commandeering this patch to improve.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145868

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

[PATCH] D145479: [clang][ASTImporter] Import typedefs to distinct records as distinct nodes.

[Vince Bridgers via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145479

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

[PATCH] D145868: [clang][ASTImporter] Fix import of anonymous structures

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers created this revision.
vabridgers added reviewers: a.sidorin, shafik, donat.nagy, gamesh411, balazske.
Herald added a subscriber: martong.
Herald added a project: All.
vabridgers requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fix crash in ASTImporter for anonymous structures. There was a series of
problems exposed by https://reviews.llvm.org/D133468 (commit
69a6417406a1b0316a1fa6aeb63339d0e1d2abbd 
) in the 
ASTImporter breaking
cross-translation unit analysis. This change fixes the problem exposed
by that change for importing anonymous structures. The problem was
discovered when running clang static analysis on open source projects
using cross-translation unit analysis.

Simple test command. Produces crash without change, passes all tests
with change.

ninja ASTTests && ./tools/clang/unittests/AST/ASTTests

  --gtest_filter="*/*ImportAnonymousStruct/0"

Formatted crash stack:

ASTTests: /clang/lib/AST/ASTContext.cpp:4787:

  clang::QualType clang::ASTContext::getTypedefType(const 
clang::TypedefNameDecl*,
  clang::QualType) const: Assertion `hasSameType(Decl->getUnderlyingType(), 
Underlying)' failed.

...
 #9  clang::ASTContext::getTypedefType(clang::TypedefNameDecl const*, 
clang::QualType) const

  /clang/lib/AST/ASTContext.cpp:4789:26
  /clang/lib/AST/ASTImporter.cpp:1374:71
  /tools/clang/include/clang/AST/TypeNodes.inc:75:1
  /clang/lib/AST/ASTImporter.cpp:8663:8


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D145868

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8474,6 +8474,45 @@
   ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportAnonymousStruct) {
+  // Tests importing of anonymous structures. This is a reduction of a real
+  // issue found in the wild into unittest case form. This crashes without
+  // the fix. Just don't crash when this unittest case is run.
+  Decl *ToTU = getToTuDecl(
+  R"(  
+  typedef struct _XDisplay Display;
+  typedef struct {
+int var;
+  } *_XPrivDisplay;
+  extern Display  *xterm_dpy;
+  int function(int);
+  int internal(Display *dpy) {
+return function(((_XPrivDisplay)dpy)->var );
+  }
+  )",
+  Lang_C99);
+  auto *ToX = FirstDeclMatcher().match(
+  ToTU, functionDecl(hasName("internal"), isDefinition()));
+  ASSERT_TRUE(ToX);
+
+  Decl *FromTU = getTuDecl(
+  R"(  
+  typedef struct _XDisplay Display;
+  typedef struct {
+int var;
+  } *_XPrivDisplay;
+  extern Display  *xterm_dpy;
+  int function(int var) {
+return ((_XPrivDisplay)xterm_dpy)->var;
+  }
+  )",
+  Lang_C99, "input1.c");
+  auto *FromF = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("function"), isDefinition()));
+  auto *ToF = Import(FromF, Lang_C99);
+  EXPECT_TRUE(ToF);
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1363,12 +1363,7 @@
   Expected ToDeclOrErr = import(T->getDecl());
   if (!ToDeclOrErr)
 return ToDeclOrErr.takeError();
-  ExpectedType ToUnderlyingTypeOrErr = import(T->desugar());
-  if (!ToUnderlyingTypeOrErr)
-return ToUnderlyingTypeOrErr.takeError();
-
-  return Importer.getToContext().getTypedefType(*ToDeclOrErr,
-*ToUnderlyingTypeOrErr);
+  return Importer.getToContext().getTypeDeclType(*ToDeclOrErr);
 }
 
 ExpectedType ASTNodeImporter::VisitTypeOfExprType(const TypeOfExprType *T) {


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8474,6 +8474,45 @@
   ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, ImportAnonymousStruct) {
+  // Tests importing of anonymous structures. This is a reduction of a real
+  // issue found in the wild into unittest case form. This crashes without
+  // the fix. Just don't crash when this unittest case is run.
+  Decl *ToTU = getToTuDecl(
+  R"(  
+  typedef struct _XDisplay Display;
+  typedef struct {
+int var;
+  } *_XPrivDisplay;
+  extern Display  *xterm_dpy;
+  int function(int);
+  int internal(Display *dpy) {
+return 

[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

[Vince Bridgers via Phabricator via cfe-commits]

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

LGTM. Let's accept, merge and then watch to make sure we can keep the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144273

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

[PATCH] D144622: [clang][ASTImporter] Import TemplateName correctly

[Vince Bridgers via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144622

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

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers accepted this revision.
vabridgers added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140562

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

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Hi @donat.nagy , no problem. That's ok for me. Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140562

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

[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Patch D144622  should be integrated into this 
one when a reduced reproducer has been prepared as a unittest and/or LIT case.

I verified the patch stack D144273 , D140562 
, and D144622 
 (this one) addresses the problems we've seen 
as a result of D133468 . When the patches are 
ready, a brief history should be included in the commit header.




Comment at: clang/lib/AST/ASTContext.cpp:4627
   } else {
-Type *newType =
-  new (*this, TypeAlignment) InjectedClassNameType(Decl, TST);
+Type *newType = new (*this, TypeAlignment) InjectedClassNameType(Decl, 
TST);
 Decl->TypeForDecl = newType;

whisperity wrote:
> (Potentially unrelated change, only touching the format?)
@whisperity is correct, please correct this formatting so it's not a change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140562

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

[PATCH] D144622: [clang[[ASTImporter] Import TemplateName correctly

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

This patch needs a unit test (as @balazske mentioned). So far, the case we have 
is too large to be a suitable unittest or lit case - so requires reduction. 
@balazske , will you be adding this as a unittest or lit case?

Also, I think this patch needs to be integrated with D144273 
 IMHO, since they address similar issues.

I verified the patch stack D144273 , D140562 
, and D144622 
 (this one) addresses the problems we've seen 
as a result of D133468 . When the patches are 
ready, a brief history should be included in the commit header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144622

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

[PATCH] D144273: [clang][ASTImporter] Add VaList declaration to lookup table.

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I tested this change in the cases that previously failed for us, and this patch 
addresses our problems. It would be good to note in the commit header that this 
patch is intended to fix the problem first described by review D136886 
. Finally, this should be submitted as part 
of a patch stack intended to address the 2 AST importer problems seen and 
corrected, originating from D133468 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144273

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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers planned changes to this revision.
vabridgers added a comment.

Changes are planned. Please do not waste any more time on this for now. This 
will probably be abandoned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

@donat.nagy , regarding the namespace leaking, there was a change -> 
https://reviews.llvm.org/D116774 that modified the behavior for aarch64 and 
arm. While not incorrect, it may offer some historical view or examples of how 
to address the current cases.

@whisperity , I'll try your advice and post the results.




Comment at: clang/test/AST/ast-dump-overloaded-operators.cpp:27
 // CHECK-NEXT: | `-ParmVarDecl {{.*}}  col:19{{( imported)?}} 'E'
-// CHECK-NEXT: `-FunctionDecl {{.*}}  line:14:6{{( 
imported)?}} test 'void ()'
+// CHECK-NEXT:  -FunctionDecl {{.*}}  line:14:6{{( 
imported)?}} test 'void ()'
 // CHECK-NEXT:   `-CompoundStmt {{.*}} 

donat.nagy wrote:
> Why did a backtick disappear here and in many other locations? Is this 
> somehow related to va_list handling?
this backspace appeared in the original patch submitted by @mizvekov in review 
https://reviews.llvm.org/D136886, discussed for the test case 
clang/test/AST/ast-dump-overloaded-operators.cpp between @aaron.ballman and 
@mizvekov. 

Comments from that review from @mizvekov


What is happening here (and in all the other such instances) is that on the 
import case, this declaration stops being the last one on the TU. So the 
beginning of the line would match on |- instead of `-, but the non-import case 
this remains the last one.

So I simply relaxed the match.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

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

[PATCH] D138037: [analyzer] Remove unjustified assertion from EQClass::simplify

[Vince Bridgers via Phabricator via cfe-commits]

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

I got back to testing this through a large, internal set of builds, and do not 
see anymore problems. If everyone is ok with that, could this be merged?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138037

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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 493709.
vabridgers added a comment.

rebase, bump review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

Files:
  clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp
  clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp
  clang/docs/ReleaseNotes.rst
  clang/lib/Frontend/ASTMerge.cpp
  clang/lib/Sema/Sema.cpp
  clang/test/AST/ast-dump-file-line-json.c
  clang/test/AST/ast-dump-overloaded-operators.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/AST/ast-dump-records-json.cpp
  clang/test/AST/ast-dump-records.cpp
  clang/test/AST/ast-dump-template-decls-json.cpp
  clang/test/AST/ast-dump-traits.cpp
  clang/test/AST/fixed_point.c
  clang/test/AST/float16.cpp
  clang/test/PCH/stmt-openmp_structured_block-bit.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1248,8 +1248,8 @@
 }
 )";
   Decl *FromTU = getTuDecl(Code, Lang_CXX03);
-  auto FromNs =
-  FirstDeclMatcher().match(FromTU, namespaceDecl());
+  auto FromNs = FirstDeclMatcher().match(
+  FromTU, namespaceDecl(hasName("x")));
   auto ToNs = cast(Import(FromNs, Lang_CXX03));
   ASSERT_TRUE(ToNs);
   auto From =
@@ -7561,7 +7561,14 @@
   }
   )",
   Lang_CXX14);
-  auto *FromFD = FirstDeclMatcher().match(FromTU, fieldDecl());
+
+  auto *FromF = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("declToImport")));
+  CXXRecordDecl *FromLambda =
+  cast((cast(FromF->getBody())->body_back()))
+  ->getLambdaClass();
+
+  auto *FromFD = *FromLambda->field_begin();
   ASSERT_TRUE(FromFD);
   ASSERT_TRUE(FromFD->hasCapturedVLAType());
 
@@ -8137,6 +8144,24 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VaList) {
+  Decl *FromTU = getTuDecl(R"(typedef __builtin_va_list va_list;)", Lang_C99);
+
+  auto *FromVaList = FirstDeclMatcher().match(
+  FromTU, typedefDecl(hasName("va_list")));
+  ASSERT_TRUE(FromVaList);
+
+  auto *ToVaList = Import(FromVaList, Lang_C99);
+  ASSERT_TRUE(ToVaList);
+
+  auto *ToBuiltinVaList = FirstDeclMatcher().match(
+  ToAST->getASTContext().getTranslationUnitDecl(),
+  typedefDecl(hasName("__builtin_va_list")));
+
+  ASSERT_TRUE(ToAST->getASTContext().hasSameType(
+  ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/test/PCH/stmt-openmp_structured_block-bit.cpp
===
--- clang/test/PCH/stmt-openmp_structured_block-bit.cpp
+++ clang/test/PCH/stmt-openmp_structured_block-bit.cpp
@@ -13,7 +13,7 @@
 // expected-no-diagnostics
 
 // CHECK: TranslationUnitDecl 0x{{.*}} <> 
-// CHECK: `-FunctionDecl 0x{{.*}} <{{.*}}stmt-openmp_structured_block-bit.cpp:8:1, line:11:1> line:8:6 {{(test|imported test)}} 'void ()'
+// CHECK:  -FunctionDecl 0x{{.*}} <{{.*}}stmt-openmp_structured_block-bit.cpp:8:1, line:11:1> line:8:6 {{(test|imported test)}} 'void ()'
 // CHECK-NEXT:   `-CompoundStmt 0x{{.*}} 
 // CHECK-NEXT: `-OMPParallelDirective 0x{{.*}} 
 // CHECK-NEXT:   `-CapturedStmt 0x{{.*}} 
Index: clang/test/AST/float16.cpp
===
--- clang/test/AST/float16.cpp
+++ clang/test/AST/float16.cpp
@@ -29,7 +29,7 @@
   }
 }
 
-//CHECK:  |-NamespaceDecl
+//CHECK:  |-NamespaceDecl {{.*}} <{{.*}}:22:1,
 //CHECK-NEXT: | |-VarDecl {{.*}} f1n '_Float16'
 //CHECK-NEXT: | |-VarDecl {{.*}} f2n '_Float16' cinit
 //CHECK-NEXT: | | `-FloatingLiteral {{.*}} '_Float16' 3.30e+01
Index: clang/test/AST/fixed_point.c
===
--- clang/test/AST/fixed_point.c
+++ clang/test/AST/fixed_point.c
@@ -402,5 +402,5 @@
 
 _Accum literallast = 1.0k;// One
 
-//CHECK-NEXT: `-VarDecl {{.*}} literallast '_Accum' cinit
+//CHECK-NEXT:  -VarDecl {{.*}} literallast '_Accum' cinit
 //CHECK-NEXT:   `-FixedPointLiteral {{.*}} '_Accum' 1.0
Index: clang/test/AST/ast-dump-traits.cpp
===
--- clang/test/AST/ast-dump-traits.cpp
+++ clang/test/AST/ast-dump-traits.cpp
@@ -52,7 +52,7 @@
 // CHECK-NEXT: | `-CompoundStmt {{.*}} 
 // CHECK-NEXT: |   `-CStyleCastExpr {{.*}}  'void' 
 // CHECK-NEXT: | `-ExpressionTraitExpr {{.*}}  'bool' __is_lvalue_expr
-// CHECK-NEXT: `-FunctionDecl {{.*}}  line:30:6{{( imported)?}} 

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added subscribers: DavidSpickett, jrtc27.
vabridgers added a comment.

@DavidSpickett and/or @jrtc27 , I started with @mizvekov 's patch ( D136886 
) and attempted to address the problems with 
that patch on arm and aarch64. Is it possible for you to try testing this patch 
and/or commenting? Thank you. - Vince


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 493553.
vabridgers added a subscriber: balazske.
vabridgers added a comment.

Updates per suggestion from @balazske


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

Files:
  clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp
  clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp
  clang/docs/ReleaseNotes.rst
  clang/lib/Frontend/ASTMerge.cpp
  clang/lib/Sema/Sema.cpp
  clang/test/AST/ast-dump-file-line-json.c
  clang/test/AST/ast-dump-overloaded-operators.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/AST/ast-dump-records-json.cpp
  clang/test/AST/ast-dump-records.cpp
  clang/test/AST/ast-dump-template-decls-json.cpp
  clang/test/AST/ast-dump-traits.cpp
  clang/test/AST/fixed_point.c
  clang/test/AST/float16.cpp
  clang/test/PCH/stmt-openmp_structured_block-bit.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -1248,8 +1248,8 @@
 }
 )";
   Decl *FromTU = getTuDecl(Code, Lang_CXX03);
-  auto FromNs =
-  FirstDeclMatcher().match(FromTU, namespaceDecl());
+  auto FromNs = FirstDeclMatcher().match(
+  FromTU, namespaceDecl(hasName("x")));
   auto ToNs = cast(Import(FromNs, Lang_CXX03));
   ASSERT_TRUE(ToNs);
   auto From =
@@ -7561,7 +7561,14 @@
   }
   )",
   Lang_CXX14);
-  auto *FromFD = FirstDeclMatcher().match(FromTU, fieldDecl());
+
+  auto *FromF = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("declToImport")));
+  CXXRecordDecl *FromLambda =
+  cast((cast(FromF->getBody())->body_back()))
+  ->getLambdaClass();
+
+  auto *FromFD = *FromLambda->field_begin();
   ASSERT_TRUE(FromFD);
   ASSERT_TRUE(FromFD->hasCapturedVLAType());
 
@@ -8137,6 +8144,24 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VaList) {
+  Decl *FromTU = getTuDecl(R"(typedef __builtin_va_list va_list;)", Lang_C99);
+
+  auto *FromVaList = FirstDeclMatcher().match(
+  FromTU, typedefDecl(hasName("va_list")));
+  ASSERT_TRUE(FromVaList);
+
+  auto *ToVaList = Import(FromVaList, Lang_C99);
+  ASSERT_TRUE(ToVaList);
+
+  auto *ToBuiltinVaList = FirstDeclMatcher().match(
+  ToAST->getASTContext().getTranslationUnitDecl(),
+  typedefDecl(hasName("__builtin_va_list")));
+
+  ASSERT_TRUE(ToAST->getASTContext().hasSameType(
+  ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/test/PCH/stmt-openmp_structured_block-bit.cpp
===
--- clang/test/PCH/stmt-openmp_structured_block-bit.cpp
+++ clang/test/PCH/stmt-openmp_structured_block-bit.cpp
@@ -13,7 +13,7 @@
 // expected-no-diagnostics
 
 // CHECK: TranslationUnitDecl 0x{{.*}} <> 
-// CHECK: `-FunctionDecl 0x{{.*}} <{{.*}}stmt-openmp_structured_block-bit.cpp:8:1, line:11:1> line:8:6 {{(test|imported test)}} 'void ()'
+// CHECK:  -FunctionDecl 0x{{.*}} <{{.*}}stmt-openmp_structured_block-bit.cpp:8:1, line:11:1> line:8:6 {{(test|imported test)}} 'void ()'
 // CHECK-NEXT:   `-CompoundStmt 0x{{.*}} 
 // CHECK-NEXT: `-OMPParallelDirective 0x{{.*}} 
 // CHECK-NEXT:   `-CapturedStmt 0x{{.*}} 
Index: clang/test/AST/float16.cpp
===
--- clang/test/AST/float16.cpp
+++ clang/test/AST/float16.cpp
@@ -29,7 +29,7 @@
   }
 }
 
-//CHECK:  |-NamespaceDecl
+//CHECK:  |-NamespaceDecl {{.*}} <{{.*}}:22:1,
 //CHECK-NEXT: | |-VarDecl {{.*}} f1n '_Float16'
 //CHECK-NEXT: | |-VarDecl {{.*}} f2n '_Float16' cinit
 //CHECK-NEXT: | | `-FloatingLiteral {{.*}} '_Float16' 3.30e+01
Index: clang/test/AST/fixed_point.c
===
--- clang/test/AST/fixed_point.c
+++ clang/test/AST/fixed_point.c
@@ -402,5 +402,5 @@
 
 _Accum literallast = 1.0k;// One
 
-//CHECK-NEXT: `-VarDecl {{.*}} literallast '_Accum' cinit
+//CHECK-NEXT:  -VarDecl {{.*}} literallast '_Accum' cinit
 //CHECK-NEXT:   `-FixedPointLiteral {{.*}} '_Accum' 1.0
Index: clang/test/AST/ast-dump-traits.cpp
===
--- clang/test/AST/ast-dump-traits.cpp
+++ clang/test/AST/ast-dump-traits.cpp
@@ -52,7 +52,7 @@
 // CHECK-NEXT: | `-CompoundStmt {{.*}} 
 // CHECK-NEXT: |   `-CStyleCastExpr {{.*}}  'void' 
 // CHECK-NEXT: | `-ExpressionTraitExpr {{.*}}  'bool' __is_lvalue_expr
-// CHECK-NEXT: 

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers created this revision.
vabridgers added reviewers: aaron.ballman, mizvekov.
Herald added subscribers: carlosgalvezp, martong, kristof.beyls.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a reviewer: njames93.
Herald added a project: All.
vabridgers requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, sstefan1.
Herald added projects: clang, clang-tools-extra.

This patch was originally submitted by @mizvekov, then reverted because
it caused crashes on arm and aarch64. This has since been debugged as a
problem in the DontModifyStdNamespaceCheck.cpp tidy checker exposed by
arm and aarch64 architectures defining va_list in the std namespace. The
tidy checker was fixed by excluding the implicit cases. See D136886 
 for
original patch and notes. This patch takes changes from D136886 
, adds
the fix and LIT cases to cover the fix.

Original description from @mizvekov for the base portion of this fix.

This fixes a problem where __va_list_tag was not correctly imported,
possibly leading to multiple definitions with different types.

This adds __va_list_tag to it's proper scope, so that the ASTImporter
can find it.

Crash seen in original fix, addressed by improvements.

$ clang-tidy crash.cpp -checks="cert-dcl58-cpp" --  -target arm
clang-tidy: 
/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:178:

  clang::DiagnosticBuilder clang::tidy::ClangTidyContext::diag(llvm::StringRef,
  clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level):
  Assertion `Loc.isValid()' failed.

Stack dump:
0.Program arguments: clang-tidy crash.cpp -checks=cert-dcl58-cpp -- -target 
arm
1. parser at end of file
2.ASTMatcher: Processing 'cert-dcl58-cpp' against:
  CXXRecordDecl std::__va_list : <>

- Bound Nodes Begin --- decl - { CXXRecordDecl std::__va_list : <> } nmspc - { NamespaceDecl std : <> }
- Bound Nodes End --- #0 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
/llvm/lib/Support/Unix/Signals.inc:567:22 #1 
PrintStackTraceSignalHandler(void*) 
/llvm/lib/Support/Unix/Signals.inc:641:1 ... #9 
clang::tidy::ClangTidyContext::diag(llvm::StringRef, clang::SourceLocation, 
llvm::StringRef, clang::DiagnosticIDs::Level) 
/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:179:17 
clang::DiagnosticIDs::Level) 
/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:27:54 
clang::ast_matchers::MatchFinder::MatchResultconst&) 
/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp:121:10


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142822

Files:
  clang-tools-extra/clang-tidy/altera/StructPackAlignCheck.cpp
  clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp
  clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/cert/dcl58-cpp.cpp
  clang/docs/ReleaseNotes.rst
  clang/lib/Frontend/ASTMerge.cpp
  clang/lib/Sema/Sema.cpp
  clang/test/AST/ast-dump-file-line-json.c
  clang/test/AST/ast-dump-overloaded-operators.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/AST/ast-dump-records-json.cpp
  clang/test/AST/ast-dump-records.cpp
  clang/test/AST/ast-dump-template-decls-json.cpp
  clang/test/AST/ast-dump-traits.cpp
  clang/test/AST/fixed_point.c
  clang/test/AST/float16.cpp
  clang/test/PCH/stmt-openmp_structured_block-bit.cpp
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -7528,7 +7528,14 @@
   }
   )",
   Lang_CXX14);
-  auto *FromFD = FirstDeclMatcher().match(FromTU, fieldDecl());
+
+  auto *FromF = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("declToImport")));
+  CXXRecordDecl *FromLambda =
+  cast((cast(FromF->getBody())->body_back()))
+  ->getLambdaClass();
+
+  auto *FromFD = *FromLambda->field_begin();
   ASSERT_TRUE(FromFD);
   ASSERT_TRUE(FromFD->hasCapturedVLAType());
 
@@ -8104,6 +8111,24 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, VaList) {
+  Decl *FromTU = getTuDecl(R"(typedef __builtin_va_list va_list;)", Lang_C99);
+
+  auto *FromVaList = FirstDeclMatcher().match(
+  FromTU, typedefDecl(hasName("va_list")));
+  ASSERT_TRUE(FromVaList);
+
+  auto *ToVaList = Import(FromVaList, Lang_C99);
+  ASSERT_TRUE(ToVaList);
+
+  auto *ToBuiltinVaList = FirstDeclMatcher().match(
+  ToAST->getASTContext().getTranslationUnitDecl(),
+  typedefDecl(hasName("__builtin_va_list")));
+
+  ASSERT_TRUE(ToAST->getASTContext().hasSameType(
+  ToVaList->getUnderlyingType(), ToBuiltinVaList->getUnderlyingType()));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

@mizvekov, will you be picking this change up and finishing this, or do you 
mind if I have a go at finishing this patch? Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I've made a very simple reproducer for this crash, clang-tidy executes on an 
x86 machine. Requires aarch64 and/or arm support compiled in.

crashes - clang-tidy crash.cpp -checks="cert-dcl58-cpp" -- -target arm
crashes - clang-tidy crash.cpp -checks="cert-dcl58-cpp" -- -target aarch64
no crash - clang-tidy crash.cpp -checks="cert-dcl58-cpp" -- -target x86_64

crash.cpp ...

  namespace std {
  } // namespace std

The partial crash stack ...

  $ clang-tidy crash.cpp -checks="cert-dcl58-cpp" --  -target arm 
  clang-tidy: 
/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:178: 
clang::DiagnosticBuilder clang::tidy::ClangTidyContext::diag(llvm::StringRef, 
clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level): Assertion 
`Loc.isValid()' failed.
  
  Stack dump:
  0.Program arguments: clang-tidy crash.cpp -checks=cert-dcl58-cpp -- 
-target arm
  1. parser at end of file
  2.ASTMatcher: Processing 'cert-dcl58-cpp' against:
    CXXRecordDecl std::__va_list : <>
  --- Bound Nodes Begin ---
  decl - { CXXRecordDecl std::__va_list : <> }
  nmspc - { NamespaceDecl std : <> }
  --- Bound Nodes End ---
   #0 0x06dcfe46 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
/llvm/lib/Support/Unix/Signals.inc:567:22
   #1 0x06dd0232 PrintStackTraceSignalHandler(void*) 
/llvm/lib/Support/Unix/Signals.inc:641:1
   #2 0x06dcde75 llvm::sys::RunSignalHandlers() 
/llvm/lib/Support/Signals.cpp:104:20
   #3 0x06dcf884 SignalHandler(int) 
/llvm/lib/Support/Unix/Signals.inc:412:1
   #4 0x7fa725903630 __restore_rt sigaction.c:0:0
   #5 0x7fa722aac387 raise (/lib64/libc.so.6+0x36387)
   #6 0x7fa722aada78 abort (/lib64/libc.so.6+0x37a78)
   #7 0x7fa722aa51a6 __assert_fail_base (/lib64/libc.so.6+0x2f1a6)
   #8 0x7fa722aa5252 (/lib64/libc.so.6+0x2f252)
   #9 0x03132a80 clang::tidy::ClangTidyContext::diag(llvm::StringRef, 
clang::SourceLocation, llvm::StringRef, clang::DiagnosticIDs::Level) 
/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:179:17
  #10 0x0312ea92 
clang::tidy::ClangTidyCheck::diag(clang::SourceLocation, llvm::StringRef, 
clang::DiagnosticIDs::Level) 
/clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:27:54
  #11 0x0269e015 
clang::tidy::cert::DontModifyStdNamespaceCheck::check(clang::ast_matchers::MatchFinder::MatchResult
 const&) 
/clang-tools-extra/clang-tidy/cert/DontModifyStdNamespaceCheck.cpp:121:10
  #12 0x0312eb96 
clang::tidy::ClangTidyCheck::run(clang::ast_matchers::MatchFinder::MatchResult 
const&) /clang-tools-extra/clang-tidy/ClangTidyCheck.cpp:46:1
  #13 0x05873615 clang::ast_matchers::internal::(anonymous 
namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes
 const&) /clang/lib/ASTMatchers/ASTMatchFinder.cpp:1260:34


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

@balazske , could you please share how to repro this problem on an x86 machine? 
Thank you


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

It appears to me this change https://reviews.llvm.org/D116774 is responsible 
for the unexpected behavior. Question for @jrtc27 : do you think if we could 
make this change consistent with https://reviews.llvm.org/D116774 that the 
problem would be addressed? Looks like we could make consistent changes in 
Sema.cpp and perhaps the problem would be addressed?

I'm assuming that keeping __va_list in std for aarch6 and arm is a requirement 
for the abi and not open to change?
Best


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D142627: [analyzer] Fix crash exposed by D140059

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, hiraditya, 
xazax.hun.
Herald added a project: All.
vabridgers requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Change https://reviews.llvm.org/D140059 exposed the following crash in
Z3Solver, where bit widths were not checked consistently with that
change. This change makes the check consistent, and fixes the crash.

clang: /llvm/include/llvm/ADT/APSInt.h:99:

  int64_t llvm::APSInt::getExtValue() const: Assertion
  `isRepresentableByInt64() && "Too many bits for int64_t"' failed.

...
Stack dump:
0. Program arguments: clang -cc1 -internal-isystem /lib/clang/16/include

  -nostdsysteminc -analyze 
-analyzer-checker=core,unix.Malloc,debug.ExprInspection
  -analyzer-config crosscheck-with-z3=true -verify reproducer.c

#0 0x045b3476 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int)

  /llvm/lib/Support/Unix/Signals.inc:567:22

#1 0x045b3862 PrintStackTraceSignalHandler(void*)

  /llvm/lib/Support/Unix/Signals.inc:641:1

#2 0x045b14a5 llvm::sys::RunSignalHandlers()

  /llvm/lib/Support/Signals.cpp:104:20

#3 0x045b2eb4 SignalHandler(int)

  /llvm/lib/Support/Unix/Signals.inc:412:1

...
 #9 0x04be2eb3 llvm::APSInt::getExtValue() const

  /llvm/include/llvm/ADT/APSInt.h:99:5
  /llvm/lib/Support/Z3Solver.cpp:740:53
  clang::ASTContext&, clang::ento::SymExpr const*, llvm::APSInt const&, 
llvm::APSInt const&, bool)
  /clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:552:61


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142627

Files:
  clang/test/Analysis/z3-crosscheck.c
  llvm/lib/Support/Z3Solver.cpp


Index: llvm/lib/Support/Z3Solver.cpp
===
--- llvm/lib/Support/Z3Solver.cpp
+++ llvm/lib/Support/Z3Solver.cpp
@@ -729,7 +729,7 @@
 const Z3_sort Z3Sort = toZ3Sort(*getBitvectorSort(BitWidth)).Sort;
 
 // Slow path, when 64 bits are not enough.
-if (LLVM_UNLIKELY(Int.getBitWidth() > 64u)) {
+if (LLVM_UNLIKELY(!Int.isRepresentableByInt64())) {
   SmallString<40> Buffer;
   Int.toString(Buffer, 10);
   return newExprRef(Z3Expr(
Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -55,3 +55,15 @@
 h(2);
   }
 }
+
+// don't crash, and also produce a core.CallAndMessage finding
+void a(int);
+typedef struct {
+  int b;
+} c;
+c *d;
+void e() {
+  (void)d->b;
+  int f;
+  a(f); // expected-warning {{1st function call argument is an uninitialized 
value [core.CallAndMessage]}}
+}


Index: llvm/lib/Support/Z3Solver.cpp
===
--- llvm/lib/Support/Z3Solver.cpp
+++ llvm/lib/Support/Z3Solver.cpp
@@ -729,7 +729,7 @@
 const Z3_sort Z3Sort = toZ3Sort(*getBitvectorSort(BitWidth)).Sort;
 
 // Slow path, when 64 bits are not enough.
-if (LLVM_UNLIKELY(Int.getBitWidth() > 64u)) {
+if (LLVM_UNLIKELY(!Int.isRepresentableByInt64())) {
   SmallString<40> Buffer;
   Int.toString(Buffer, 10);
   return newExprRef(Z3Expr(
Index: clang/test/Analysis/z3-crosscheck.c
===
--- clang/test/Analysis/z3-crosscheck.c
+++ clang/test/Analysis/z3-crosscheck.c
@@ -55,3 +55,15 @@
 h(2);
   }
 }
+
+// don't crash, and also produce a core.CallAndMessage finding
+void a(int);
+typedef struct {
+  int b;
+} c;
+c *d;
+void e() {
+  (void)d->b;
+  int f;
+  a(f); // expected-warning {{1st function call argument is an uninitialized value [core.CallAndMessage]}}
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D140059: [APSInt] Fix bug in APSInt mentioned in https://github.com/llvm/llvm-project/issues/59515

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Thanks @Peter, I will try that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140059

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

[PATCH] D140059: [APSInt] Fix bug in APSInt mentioned in https://github.com/llvm/llvm-project/issues/59515

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Hello, it appears this patch causes a crash when I analyze this reproducer, 
with Z3 enabled. In the case shown here, the analyzer finds that 'f' to the 
call a(f) is uninitialized, and then is attempted to be refuted through 
SMTConv, leading to the assertion.

If I modify the below code to not use isRepresentableByIn64(), or use 
'assert(getMinSignedBits() <= 64 && "Too many bits for int64_t");' instead, I 
do not see the crash.

clang --analyze -Xclang -analyzer-config -Xclang crosscheck-with-z3=true 
--target=x86_64-redhat-linux case.c

  void a(int);
  typedef struct {
int b;
  } c;
  c *d;
  void e() {
(void)d->b;
int f;
a(f);
  }

The assert ...

  clang-16: ../include/llvm/ADT/APSInt.h:99: int64_t 
llvm::APSInt::getExtValue() const: Assertion `isRepresentableByInt64() && "Too 
many bits for int64_t"' failed.Program received signal SIGABRT, Aborted.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140059

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

[PATCH] D138037: [analyzer] Remove unjustified assertion from EQClass::simplify

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Hi @steakhal, thanks for the suggested change.
How we can help move this forward? From what I'm comprehending from the notes, 
perhaps we could try running this change through our internal systems level 
test and fuzzer. Unfortunately, I'd not be able to say more than "trust me, we 
saw no problems" if we see no problems. But if I do find additional cases I can 
make simplified reproducers and we can work on addressing those.
Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138037

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

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I think that addresses the last comments. Thanks again :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

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

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 481951.
vabridgers marked 7 inline comments as done.
vabridgers added a comment.

clean up pass per comments from @steakhal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/test/Analysis/fixed-point.c
  clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp
  clang/unittests/StaticAnalyzer/CMakeLists.txt

Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -5,6 +5,7 @@
 
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
+  APSIntTypeTest.cpp
   BugReportInterestingnessTest.cpp
   CallDescriptionTest.cpp
   CallEventTest.cpp
Index: clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp
@@ -0,0 +1,57 @@
+//===- unittest/StaticAnalyzer/APSIntTest.cpp - getAPSIntType  test --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/TargetInfo.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/APSInt.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+namespace {
+
+TEST(getAPSIntTypeTest, APSIntTypeTests) {
+  std::unique_ptr AST = clang::tooling::buildASTFromCode("");
+  clang::ASTContext  = AST->getASTContext();
+  llvm::BumpPtrAllocator Arena;
+  clang::ento::BasicValueFactory BVF{Context, Arena};
+
+  clang::ento::APSIntType Ty = BVF.getAPSIntType(Context.LongAccumTy);
+  EXPECT_TRUE(Ty.getBitWidth() == Context.getTargetInfo().getLongAccumWidth());
+  EXPECT_FALSE(Ty.isUnsigned());
+
+  Ty = BVF.getAPSIntType(Context.UnsignedLongAccumTy);
+  EXPECT_TRUE(Ty.getBitWidth() == Context.getTargetInfo().getLongAccumWidth());
+  EXPECT_TRUE(Ty.isUnsigned());
+
+  Ty = BVF.getAPSIntType(Context.LongFractTy);
+  EXPECT_TRUE(Ty.getBitWidth() == Context.getTargetInfo().getLongFractWidth());
+  EXPECT_FALSE(Ty.isUnsigned());
+
+  Ty = BVF.getAPSIntType(Context.UnsignedLongFractTy);
+  EXPECT_TRUE(Ty.getBitWidth() == Context.getTargetInfo().getLongFractWidth());
+  EXPECT_TRUE(Ty.isUnsigned());
+
+  Ty = BVF.getAPSIntType(Context.SignedCharTy);
+  EXPECT_TRUE(Ty.getBitWidth() == Context.getTargetInfo().getCharWidth());
+  EXPECT_FALSE(Ty.isUnsigned());
+
+  Ty = BVF.getAPSIntType(Context.UnsignedCharTy);
+  EXPECT_TRUE(Ty.getBitWidth() == Context.getTargetInfo().getCharWidth());
+  EXPECT_TRUE(Ty.isUnsigned());
+
+  Ty = BVF.getAPSIntType(Context.LongTy);
+  EXPECT_TRUE(Ty.getBitWidth() == Context.getTargetInfo().getLongWidth());
+  EXPECT_FALSE(Ty.isUnsigned());
+
+  Ty = BVF.getAPSIntType(Context.UnsignedLongTy);
+  EXPECT_TRUE(Ty.getBitWidth() == Context.getTargetInfo().getLongWidth());
+  EXPECT_TRUE(Ty.isUnsigned());
+}
+
+} // end namespace
Index: clang/test/Analysis/fixed-point.c
===
--- /dev/null
+++ clang/test/Analysis/fixed-point.c
@@ -0,0 +1,65 @@
+// RUN: %clang_analyze_cc1 -ffixed-point \
+// RUN:   -analyzer-checker=core,debug.ExprInspection -verify %s
+
+// expected-no-diagnostics
+
+// Check that getAPSIntType does not crash
+// when using fixed point types.
+
+enum Kind { en_0 = 1 };
+
+void _enum(int c) {
+  (void)((enum Kind) c >> 4);
+}
+
+void _inttype(int c) {
+  (void)(c >> 4);
+}
+
+void _accum(int c) {
+  (void)((_Accum) c >> 4);
+}
+
+void _fract(int c) {
+  (void)((_Fract) c >> 4);
+}
+
+void _long_fract(int c) {
+  (void)((long _Fract) c >> 4);
+}
+
+void _unsigned_accum(int c) {
+  (void)((unsigned _Accum) c >> 4);
+}
+
+void _short_unsigned_accum(int c) {
+  (void)((short unsigned _Accum) c >> 4);
+}
+
+void _unsigned_fract(int c) {
+  (void)((unsigned _Fract) c >> 4);
+}
+
+void sat_accum(int c) {
+  (void)((_Sat _Accum) c >> 4);
+}
+
+void sat_fract(int c) {
+  (void)((_Sat _Fract) c >> 4);
+}
+
+void sat_long_fract(int c) {
+  (void)((_Sat long _Fract) c >> 4);
+}
+
+void sat_unsigned_accum(int c) {
+  (void)((_Sat unsigned _Accum) c >> 4);
+}
+
+void sat_short_unsigned_accum(int c) {
+  (void)((_Sat short unsigned _Accum) c >> 4);
+}
+
+void sat_unsigned_fract(int c) {
+  (void)((_Sat unsigned _Fract) c >> 4);
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- 

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

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



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:157-158
+   Loc::isLocType(T));
 return APSIntType(Ctx.getIntWidth(T),
   !T->isSignedIntegerOrEnumerationType());
   }

vabridgers wrote:
> steakhal wrote:
> > vabridgers wrote:
> > > steakhal wrote:
> > > > I don't think you are supposed to call 
> > > > `isSignedIntegerOrEnumerationType()` if you have a //fixed-point// type.
> > > > ```lang=C++
> > > > inline bool Type::isSignedFixedPointType() const {
> > > >   if (const auto *BT = dyn_cast(CanonicalType)) {
> > > > return ((BT->getKind() >= BuiltinType::ShortAccum &&
> > > >  BT->getKind() <= BuiltinType::LongAccum) ||
> > > > (BT->getKind() >= BuiltinType::ShortFract &&
> > > >  BT->getKind() <= BuiltinType::LongFract) ||
> > > > (BT->getKind() >= BuiltinType::SatShortAccum &&
> > > >  BT->getKind() <= BuiltinType::SatLongAccum) ||
> > > > (BT->getKind() >= BuiltinType::SatShortFract &&
> > > >  BT->getKind() <= BuiltinType::SatLongFract));
> > > >   }
> > > >   return false;
> > > > }
> > > > ```
> > > > By looking at the implementation of this, I don't think you could 
> > > > substitute that with `isSignedIntegerOrEnumerationType()`.
> > > > Am I wrong about this?
> > > > 
> > > > Please demonstrate this by tests.
> > > I tried using isSignedIntegerOrEnumerationType() instead of 
> > > (T->isIntegralOrEnumerationType() || T->isFixedPointType() ... ), but got 
> > > the same assert :/  
> > > 
> > > I corrected the formatting and expanded the test cases. 
> > Is hould have clarified, sorry.
> > 
> > My point is that for constructing the APSIntType, we calculate the bitwidth 
> > and the signedness.
> > 
> > My problem is that the calculation is wrong for the signedness in case we 
> > have a signed fixedpointtype.
> > It is wrong because we reach `isSignedIntegerOrEnumerationType()` with a 
> > fixedpoint type. For that even though its signed, it will return false!
> > 
> > And in the end we will have an APSIntType with the wrong signednss.
> > 
> > So my point is that we should probably handle fixedpoint types separately 
> > to have a distict return statement for it.
> > But im jumping to the solution, what I originally wanted to highlight was 
> > this.
> > That was why I requested changes.
> > And this is what I wanted to see some how intests, but I wont insist if its 
> > too difficult to craft.
> In retrospect, your original comment was clear. I did not fully comprehend 
> the comment. 
> 
> I'll explore a few options. I checked the unittests for a "reference", and it 
> seems our static analysis unittest coverage is sparse. Not sure I need to go 
> that far, but I'll explore the possibility of crafting a unittest for this 
> case that demonstrates the problem and solution. 
> 
> Thanks again - best!
Thanks for your insightful comments, in retrospect I should have dug into this 
more from the very beginning. Turns out there is an API get the fixed point 
sign I should be using, and creating a simple unittest had value in working 
this out (imagine that ! :) ). 

So, I updated with the correction and a simple unittest. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

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

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 481940.
vabridgers added a comment.

correct handling of sign type per @steakhal comments. Thank you, sir :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/test/Analysis/fixed-point.c
  clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp
  clang/unittests/StaticAnalyzer/CMakeLists.txt

Index: clang/unittests/StaticAnalyzer/CMakeLists.txt
===
--- clang/unittests/StaticAnalyzer/CMakeLists.txt
+++ clang/unittests/StaticAnalyzer/CMakeLists.txt
@@ -5,6 +5,7 @@
 
 add_clang_unittest(StaticAnalysisTests
   AnalyzerOptionsTest.cpp
+  APSIntTypeTest.cpp
   BugReportInterestingnessTest.cpp
   CallDescriptionTest.cpp
   CallEventTest.cpp
Index: clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp
===
--- /dev/null
+++ clang/unittests/StaticAnalyzer/APSIntTypeTest.cpp
@@ -0,0 +1,63 @@
+//===- unittest/StaticAnalyzer/AnalyzerOptionsTest.cpp - SA Options test --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Builtins.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/APSInt.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace ento {
+
+TEST(getAPSIntTypeTest, foo) {
+  std::unique_ptr AST = tooling::buildASTFromCode("");
+  ASTContext  = AST->getASTContext();
+  llvm::BumpPtrAllocator Arena;
+  BasicValueFactory BVF{Context, Arena};
+
+  APSIntType ty = BVF.getAPSIntType(Context.LongAccumTy);
+  EXPECT_TRUE(ty.getBitWidth() == Context.getTargetInfo().getLongAccumWidth());
+  EXPECT_FALSE(ty.isUnsigned());
+
+  ty = BVF.getAPSIntType(Context.UnsignedLongAccumTy);
+  EXPECT_TRUE(ty.getBitWidth() == Context.getTargetInfo().getLongAccumWidth());
+  EXPECT_TRUE(ty.isUnsigned());
+
+  ty = BVF.getAPSIntType(Context.LongFractTy);
+  EXPECT_TRUE(ty.getBitWidth() == Context.getTargetInfo().getLongFractWidth());
+  EXPECT_FALSE(ty.isUnsigned());
+
+  ty = BVF.getAPSIntType(Context.UnsignedLongFractTy);
+  EXPECT_TRUE(ty.getBitWidth() == Context.getTargetInfo().getLongFractWidth());
+  EXPECT_TRUE(ty.isUnsigned());
+
+  ty = BVF.getAPSIntType(Context.SignedCharTy);
+  EXPECT_TRUE(ty.getBitWidth() == Context.getTargetInfo().getCharWidth());
+  EXPECT_FALSE(ty.isUnsigned());
+
+  ty = BVF.getAPSIntType(Context.UnsignedCharTy);
+  EXPECT_TRUE(ty.getBitWidth() == Context.getTargetInfo().getCharWidth());
+  EXPECT_TRUE(ty.isUnsigned());
+
+  ty = BVF.getAPSIntType(Context.LongTy);
+  EXPECT_TRUE(ty.getBitWidth() == Context.getTargetInfo().getLongWidth());
+  EXPECT_FALSE(ty.isUnsigned());
+
+  ty = BVF.getAPSIntType(Context.UnsignedLongTy);
+  EXPECT_TRUE(ty.getBitWidth() == Context.getTargetInfo().getLongWidth());
+  EXPECT_TRUE(ty.isUnsigned());
+}
+
+} // end namespace ento
+} // end namespace clang
Index: clang/test/Analysis/fixed-point.c
===
--- /dev/null
+++ clang/test/Analysis/fixed-point.c
@@ -0,0 +1,65 @@
+// RUN: %clang_analyze_cc1 -ffixed-point \
+// RUN:   -analyzer-checker=core,debug.ExprInspection -verify %s
+
+// expected-no-diagnostics
+
+// Check that getAPSIntType does not crash
+// when using fixed point types.
+
+enum en_t { en_0 = 1 };
+
+void _enum(int c) {
+  (void)((enum en_t) c >> 4);
+}
+
+void _inttype(int c) {
+  (void)(c >> 4);
+}
+
+void _accum(int c) {
+  (void)((_Accum) c >> 4);
+}
+
+void _fract(int c) {
+  (void)((_Fract) c >> 4);
+}
+
+void _long_fract(int c) {
+  (void)((long _Fract) c >> 4);
+}
+
+void _unsigned_accum(int c) {
+  (void)((unsigned _Accum) c >> 4);
+}
+
+void _short_unsigned_accum(int c) {
+  (void)((short unsigned _Accum) c >> 4);
+}
+
+void _unsigned_fract(int c) {
+  (void)((unsigned _Fract) c >> 4);
+}
+
+void sat_accum(int c) {
+  (void)((_Sat _Accum) c >> 4);
+}
+
+void sat_fract(int c) {
+  (void)((_Sat _Fract) c >> 4);
+}
+
+void sat_long_fract(int c) {
+  (void)((_Sat long _Fract) c >> 4);
+}
+
+void sat_unsigned_accum(int c) {
+  (void)((_Sat unsigned _Accum) c >> 4);
+}
+
+void sat_short_unsigned_accum(int c) {
+  (void)((_Sat short unsigned _Accum) c >> 4);
+}
+
+void sat_unsigned_fract(int c) {
+  (void)((_Sat unsigned _Fract) c >> 4);
+}
Index: 

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

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



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:157-158
+   Loc::isLocType(T));
 return APSIntType(Ctx.getIntWidth(T),
   !T->isSignedIntegerOrEnumerationType());
   }

steakhal wrote:
> vabridgers wrote:
> > steakhal wrote:
> > > I don't think you are supposed to call 
> > > `isSignedIntegerOrEnumerationType()` if you have a //fixed-point// type.
> > > ```lang=C++
> > > inline bool Type::isSignedFixedPointType() const {
> > >   if (const auto *BT = dyn_cast(CanonicalType)) {
> > > return ((BT->getKind() >= BuiltinType::ShortAccum &&
> > >  BT->getKind() <= BuiltinType::LongAccum) ||
> > > (BT->getKind() >= BuiltinType::ShortFract &&
> > >  BT->getKind() <= BuiltinType::LongFract) ||
> > > (BT->getKind() >= BuiltinType::SatShortAccum &&
> > >  BT->getKind() <= BuiltinType::SatLongAccum) ||
> > > (BT->getKind() >= BuiltinType::SatShortFract &&
> > >  BT->getKind() <= BuiltinType::SatLongFract));
> > >   }
> > >   return false;
> > > }
> > > ```
> > > By looking at the implementation of this, I don't think you could 
> > > substitute that with `isSignedIntegerOrEnumerationType()`.
> > > Am I wrong about this?
> > > 
> > > Please demonstrate this by tests.
> > I tried using isSignedIntegerOrEnumerationType() instead of 
> > (T->isIntegralOrEnumerationType() || T->isFixedPointType() ... ), but got 
> > the same assert :/  
> > 
> > I corrected the formatting and expanded the test cases. 
> Is hould have clarified, sorry.
> 
> My point is that for constructing the APSIntType, we calculate the bitwidth 
> and the signedness.
> 
> My problem is that the calculation is wrong for the signedness in case we 
> have a signed fixedpointtype.
> It is wrong because we reach `isSignedIntegerOrEnumerationType()` with a 
> fixedpoint type. For that even though its signed, it will return false!
> 
> And in the end we will have an APSIntType with the wrong signednss.
> 
> So my point is that we should probably handle fixedpoint types separately to 
> have a distict return statement for it.
> But im jumping to the solution, what I originally wanted to highlight was 
> this.
> That was why I requested changes.
> And this is what I wanted to see some how intests, but I wont insist if its 
> too difficult to craft.
In retrospect, your original comment was clear. I did not fully comprehend the 
comment. 

I'll explore a few options. I checked the unittests for a "reference", and it 
seems our static analysis unittest coverage is sparse. Not sure I need to go 
that far, but I'll explore the possibility of crafting a unittest for this case 
that demonstrates the problem and solution. 

Thanks again - best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

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

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

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

Thanks Balazs, I think the comments have been addressed. Let me know if there's 
anything else to do, or if this is ready to land.

Best!




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:157-158
+   Loc::isLocType(T));
 return APSIntType(Ctx.getIntWidth(T),
   !T->isSignedIntegerOrEnumerationType());
   }

steakhal wrote:
> I don't think you are supposed to call `isSignedIntegerOrEnumerationType()` 
> if you have a //fixed-point// type.
> ```lang=C++
> inline bool Type::isSignedFixedPointType() const {
>   if (const auto *BT = dyn_cast(CanonicalType)) {
> return ((BT->getKind() >= BuiltinType::ShortAccum &&
>  BT->getKind() <= BuiltinType::LongAccum) ||
> (BT->getKind() >= BuiltinType::ShortFract &&
>  BT->getKind() <= BuiltinType::LongFract) ||
> (BT->getKind() >= BuiltinType::SatShortAccum &&
>  BT->getKind() <= BuiltinType::SatLongAccum) ||
> (BT->getKind() >= BuiltinType::SatShortFract &&
>  BT->getKind() <= BuiltinType::SatLongFract));
>   }
>   return false;
> }
> ```
> By looking at the implementation of this, I don't think you could substitute 
> that with `isSignedIntegerOrEnumerationType()`.
> Am I wrong about this?
> 
> Please demonstrate this by tests.
I tried using isSignedIntegerOrEnumerationType() instead of 
(T->isIntegralOrEnumerationType() || T->isFixedPointType() ... ), but got the 
same assert :/  

I corrected the formatting and expanded the test cases. 



Comment at: clang/test/Analysis/fixed-point.c:6-9
+long a(int c) {
   
+  (long _Accum) c >> 4;
+  return c;
+}  

steakhal wrote:
> It had a few extra spaces. And by casting it to void we could eliminate 
> `-Wno-unused` as well.
Dang, I used git-clang-format to try cleaning the patch thinking it would clean 
up formatting, but I guess it didn't :/ 

I cleaned up the test case and removed the -Wno-unused option. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

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

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 481882.
vabridgers edited the summary of this revision.
vabridgers added a comment.

correct formatting of test case and expand test cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/test/Analysis/fixed-point.c


Index: clang/test/Analysis/fixed-point.c
===
--- /dev/null
+++ clang/test/Analysis/fixed-point.c
@@ -0,0 +1,65 @@
+// RUN: %clang_analyze_cc1 -ffixed-point \
+// RUN:   -analyzer-checker=core,debug.ExprInspection -verify %s
+
+// expected-no-diagnostics
+
+// Check that getAPSIntType does not crash
+// when using fixed point types.
+
+enum en_t { en_0 = 1 };
+
+void _enum(int c) {
+  (void)((enum en_t) c >> 4);
+}
+
+void _inttype(int c) {
+  (void)(c >> 4);
+}
+
+void _accum(int c) {
+  (void)((_Accum) c >> 4);
+}
+
+void _fract(int c) {
+  (void)((_Fract) c >> 4);
+}
+
+void _long_fract(int c) {
+  (void)((long _Fract) c >> 4);
+}
+
+void _unsigned_accum(int c) {
+  (void)((unsigned _Accum) c >> 4);
+}
+
+void _short_unsigned_accum(int c) {
+  (void)((short unsigned _Accum) c >> 4);
+}
+
+void _unsigned_fract(int c) {
+  (void)((unsigned _Fract) c >> 4);
+}
+
+void sat_accum(int c) {
+  (void)((_Sat _Accum) c >> 4);
+}
+
+void sat_fract(int c) {
+  (void)((_Sat _Fract) c >> 4);
+}
+
+void sat_long_fract(int c) {
+  (void)((_Sat long _Fract) c >> 4);
+}
+
+void sat_unsigned_accum(int c) {
+  (void)((_Sat unsigned _Accum) c >> 4);
+}
+
+void sat_short_unsigned_accum(int c) {
+  (void)((_Sat short unsigned _Accum) c >> 4);
+}
+
+void sat_unsigned_fract(int c) {
+  (void)((_Sat unsigned _Fract) c >> 4);
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -152,7 +152,8 @@
   T = AT->getValueType();
 }
 
-assert(T->isIntegralOrEnumerationType() || Loc::isLocType(T));
+assert(T->isIntegralOrEnumerationType() || T->isFixedPointType() ||
+   Loc::isLocType(T));
 return APSIntType(Ctx.getIntWidth(T),
   !T->isSignedIntegerOrEnumerationType());
   }


Index: clang/test/Analysis/fixed-point.c
===
--- /dev/null
+++ clang/test/Analysis/fixed-point.c
@@ -0,0 +1,65 @@
+// RUN: %clang_analyze_cc1 -ffixed-point \
+// RUN:   -analyzer-checker=core,debug.ExprInspection -verify %s
+
+// expected-no-diagnostics
+
+// Check that getAPSIntType does not crash
+// when using fixed point types.
+
+enum en_t { en_0 = 1 };
+
+void _enum(int c) {
+  (void)((enum en_t) c >> 4);
+}
+
+void _inttype(int c) {
+  (void)(c >> 4);
+}
+
+void _accum(int c) {
+  (void)((_Accum) c >> 4);
+}
+
+void _fract(int c) {
+  (void)((_Fract) c >> 4);
+}
+
+void _long_fract(int c) {
+  (void)((long _Fract) c >> 4);
+}
+
+void _unsigned_accum(int c) {
+  (void)((unsigned _Accum) c >> 4);
+}
+
+void _short_unsigned_accum(int c) {
+  (void)((short unsigned _Accum) c >> 4);
+}
+
+void _unsigned_fract(int c) {
+  (void)((unsigned _Fract) c >> 4);
+}
+
+void sat_accum(int c) {
+  (void)((_Sat _Accum) c >> 4);
+}
+
+void sat_fract(int c) {
+  (void)((_Sat _Fract) c >> 4);
+}
+
+void sat_long_fract(int c) {
+  (void)((_Sat long _Fract) c >> 4);
+}
+
+void sat_unsigned_accum(int c) {
+  (void)((_Sat unsigned _Accum) c >> 4);
+}
+
+void sat_short_unsigned_accum(int c) {
+  (void)((_Sat short unsigned _Accum) c >> 4);
+}
+
+void sat_unsigned_fract(int c) {
+  (void)((_Sat unsigned _Fract) c >> 4);
+}
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -152,7 +152,8 @@
   T = AT->getValueType();
 }
 
-assert(T->isIntegralOrEnumerationType() || Loc::isLocType(T));
+assert(T->isIntegralOrEnumerationType() || T->isFixedPointType() ||
+   Loc::isLocType(T));
 return APSIntType(Ctx.getIntWidth(T),
   !T->isSignedIntegerOrEnumerationType());
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Thanks for the comments. I assumed git-clang-format cleaned up the cruft, but 
it didn't - that's disappointing. I'll try these things and update the review. 
Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

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

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Some debug context ...

  (gdb) frame 4
  #4  0x06f55b73 in clang::ento::BasicValueFactory::getAPSIntType 
(this=0x1088e470, T=...)
  at 
/clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:155
  155   assert(T->isIntegralOrEnumerationType() || Loc::isLocType(T));
  (gdb) p T
  $1 = {Value = {Value = 276884496}}
  (gdb) p T.dump()
  BuiltinType 0x1080ec10 'long _Accum'
  $2 = void
  (gdb) p T->isIntegralOrEnumerationType()
  $3 = false
  (gdb) p Loc::isLocType(T)
  $4 = false
  (gdb) p Ctx.getIntWidth(T)
  $5 = 64
  (gdb) p !T->isSignedIntegerOrEnumerationType()
  $6 = true


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

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

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 481809.
vabridgers added a comment.

update commit header


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139759

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/test/Analysis/fixed-point.c


Index: clang/test/Analysis/fixed-point.c
===
--- /dev/null
+++ clang/test/Analysis/fixed-point.c
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 -ffixed-point \
+// RUN:   -analyzer-checker=core,debug.ExprInspection -Wno-unused -verify %s
+
+// expected-no-diagnostics
+
+long a(int c) {
   
+  (long _Accum) c >> 4;
+  return c;
+}  
+
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -152,7 +152,8 @@
   T = AT->getValueType();
 }
 
-assert(T->isIntegralOrEnumerationType() || Loc::isLocType(T));
+assert(T->isIntegralOrEnumerationType() || T->isFixedPointType() ||
+   Loc::isLocType(T));
 return APSIntType(Ctx.getIntWidth(T),
   !T->isSignedIntegerOrEnumerationType());
   }


Index: clang/test/Analysis/fixed-point.c
===
--- /dev/null
+++ clang/test/Analysis/fixed-point.c
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 -ffixed-point \
+// RUN:   -analyzer-checker=core,debug.ExprInspection -Wno-unused -verify %s
+
+// expected-no-diagnostics
+
+long a(int c) {   
+  (long _Accum) c >> 4;
+  return c;
+}  
+
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -152,7 +152,8 @@
   T = AT->getValueType();
 }
 
-assert(T->isIntegralOrEnumerationType() || Loc::isLocType(T));
+assert(T->isIntegralOrEnumerationType() || T->isFixedPointType() ||
+   Loc::isLocType(T));
 return APSIntType(Ctx.getIntWidth(T),
   !T->isSignedIntegerOrEnumerationType());
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139759: [analyzer] Fix assertion in getAPSIntType

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers created this revision.
Herald added subscribers: steakhal, manas, ASDenysPetrov, dkrupp, donat.nagy, 
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
vabridgers requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

getAPSIntType crashes when analzying a simple case that uses a fixed
point type. Just add a check for isFixedPointType() to the assert to
address the problem.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139759

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  clang/test/Analysis/fixed-point.c


Index: clang/test/Analysis/fixed-point.c
===
--- /dev/null
+++ clang/test/Analysis/fixed-point.c
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 -ffixed-point \
+// RUN:   -analyzer-checker=core,debug.ExprInspection -Wno-unused -verify %s
+
+// expected-no-diagnostics
+
+long a(int c) {
   
+  (long _Accum) c >> 4;
+  return c;
+}  
+
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -152,7 +152,8 @@
   T = AT->getValueType();
 }
 
-assert(T->isIntegralOrEnumerationType() || Loc::isLocType(T));
+assert(T->isIntegralOrEnumerationType() || T->isFixedPointType() ||
+   Loc::isLocType(T));
 return APSIntType(Ctx.getIntWidth(T),
   !T->isSignedIntegerOrEnumerationType());
   }


Index: clang/test/Analysis/fixed-point.c
===
--- /dev/null
+++ clang/test/Analysis/fixed-point.c
@@ -0,0 +1,10 @@
+// RUN: %clang_analyze_cc1 -ffixed-point \
+// RUN:   -analyzer-checker=core,debug.ExprInspection -Wno-unused -verify %s
+
+// expected-no-diagnostics
+
+long a(int c) {   
+  (long _Accum) c >> 4;
+  return c;
+}  
+
Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -152,7 +152,8 @@
   T = AT->getValueType();
 }
 
-assert(T->isIntegralOrEnumerationType() || Loc::isLocType(T));
+assert(T->isIntegralOrEnumerationType() || T->isFixedPointType() ||
+   Loc::isLocType(T));
 return APSIntType(Ctx.getIntWidth(T),
   !T->isSignedIntegerOrEnumerationType());
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136886: [clang] ASTImporter: Fix importing of va_list types and declarations

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

@mizvekov, thanks for posting a fix. LGTM, but someone else must approve. Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D136886: [clang] [ASTImporter] RFC: Correct importer to not duplicate sugared types

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a subscriber: gamesh411.
vabridgers added a comment.

Adding more information, seems this patch's "hack" returns the following

QualType(T, 0).getDesugaredType(T->getDecl()->getASTContext()); ->

  ConstantArrayType 0x11413090 'struct __va_list_tag[1]' imported 1 
  `-RecordType 0x11412ed0 'struct __va_list_tag' imported
`-Record 0x11412e38 '__va_list_tag'

The recent change from @mizvekov returns 
T->desugar(); ->

  ElaboratedType 0x11442870 '__builtin_va_list' sugar imported
  `-TypedefType 0x11442840 '__builtin_va_list' sugar imported
|-Typedef 0x114130e8 '__builtin_va_list'
`-ConstantArrayType 0x11413090 'struct __va_list_tag[1]' imported 1 
  `-RecordType 0x11412ed0 'struct __va_list_tag' imported
`-Record 0x11412e38 '__va_list_tag'

And the code prior to @mizvekov code returns the following...
T->getDecl()->getASTContext().getTypeDeclType(*ToDeclOrErr); ->

  TypedefType 0x11442bf0 'va_list' sugar
  |-Typedef 0x113d4ad0 'va_list'
  `-ElaboratedType 0x1136dbc0 '__builtin_va_list' sugar
`-TypedefType 0x1136db90 '__builtin_va_list' sugar
  |-Typedef 0x1136db38 '__builtin_va_list'
  `-ConstantArrayType 0x1136dae0 'struct __va_list_tag[1]' 1 
`-RecordType 0x1136d920 'struct __va_list_tag'
  `-Record 0x1136d898 '__va_list_tag'

Should import desugar types to a canonical underlying type free of syntactic 
sugar? That's not clear to me at least. @gamesh411 pointed out in our debug 
session that there are sections of code that iteratively strip sugar until 
there's no more sugar left wrapping the type. But there's at least one instance 
where only a single level of sugar is removed.

OTOH, I did find a section of code where va_list is not desugared - looks like 
for diagnostic purposes - maybe this is a reason to use a solution following 
@balazske's finding.

Any suggestions on how to proceed with this?

Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D136886: [clang] [ASTImporter] RFC: Correct importer to not duplicate sugared types

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

In D136886#3892261 , @balazske wrote:

> `ASTImporterLookupTable` do not contain an entry for `__va_list_tag`, I do 
> not know why it is missing. If it is added "manually" the crash disappears 
> (without fix in `VisitTypedefType`). Following code was used to add 
> VaListTagDecl:
>
>   ASTImporterLookupTable::ASTImporterLookupTable(TranslationUnitDecl ) {
> Builder B(*this);
> B.TraverseDecl();
> // Add __va_list_tag to the table, it is not visited by the builder.
> if (NamedDecl *D = 
> dyn_cast_or_null(TU.getASTContext().getVaListTagDecl()))
>   add(, D);
>   }
>
> The problem probably existed before but did not have visible effect until the 
> new assertion was added.

Nice find @balazske. Is this fix correct, and would you like to commandeer this 
crash for a correct fix? If so, we can wait for your patch and I'll just 
abandon this one. Thank you! - Vince


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

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

[PATCH] D136886: [clang] [ASTImporter] RFC: Correct importer to not duplicate sugared types

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 471328.
vabridgers added a comment.

remove commented code from test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8075,6 +8075,34 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isSugaredImport) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...)
+  {
+va_list va;
+  } 
+  )",
+  Lang_C99);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...);
+  void maindbgout(char* str)
+  {
+dbgout((char*)str);
+  }
+  )",
+  Lang_C99);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("dbgout")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_C99);
+  ASSERT_TRUE(ToOther);
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1358,7 +1358,8 @@
   Expected ToDeclOrErr = import(T->getDecl());
   if (!ToDeclOrErr)
 return ToDeclOrErr.takeError();
-  ExpectedType ToUnderlyingTypeOrErr = import(T->desugar());
+  ExpectedType ToUnderlyingTypeOrErr =
+  import(QualType(T, 0).getDesugaredType(T->getDecl()->getASTContext()));
   if (!ToUnderlyingTypeOrErr)
 return ToUnderlyingTypeOrErr.takeError();
 


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8075,6 +8075,34 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isSugaredImport) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...)
+  {
+va_list va;
+  } 
+  )",
+  Lang_C99);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...);
+  void maindbgout(char* str)
+  {
+dbgout((char*)str);
+  }
+  )",
+  Lang_C99);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("dbgout")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_C99);
+  ASSERT_TRUE(ToOther);
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1358,7 +1358,8 @@
   Expected ToDeclOrErr = import(T->getDecl());
   if (!ToDeclOrErr)
 return ToDeclOrErr.takeError();
-  ExpectedType ToUnderlyingTypeOrErr = import(T->desugar());
+  ExpectedType ToUnderlyingTypeOrErr =
+  import(QualType(T, 0).getDesugaredType(T->getDecl()->getASTContext()));
   if (!ToUnderlyingTypeOrErr)
 return ToUnderlyingTypeOrErr.takeError();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136886: [ASTImporter] RFC: Correct importer to not duplicate sugared types

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 471303.
vabridgers added a comment.

remove commented line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136886

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8075,6 +8075,36 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isSugaredImport) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...)
+  {
+va_list va;
+  } 
+  )",
+  Lang_C99);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...);
+  void maindbgout(char* str)
+  {
+dbgout((char*)str);
+  }
+  )",
+  Lang_C99);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("dbgout")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_C99);
+  ASSERT_TRUE(ToOther);
+
+  // EXPECT_TRUE(SharedStatePtr->isSugaredImport(ToOther));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1358,7 +1358,8 @@
   Expected ToDeclOrErr = import(T->getDecl());
   if (!ToDeclOrErr)
 return ToDeclOrErr.takeError();
-  ExpectedType ToUnderlyingTypeOrErr = import(T->desugar());
+  ExpectedType ToUnderlyingTypeOrErr =
+  import(QualType(T, 0).getDesugaredType(T->getDecl()->getASTContext()));
   if (!ToUnderlyingTypeOrErr)
 return ToUnderlyingTypeOrErr.takeError();
 


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8075,6 +8075,36 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isSugaredImport) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...)
+  {
+va_list va;
+  } 
+  )",
+  Lang_C99);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...);
+  void maindbgout(char* str)
+  {
+dbgout((char*)str);
+  }
+  )",
+  Lang_C99);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("dbgout")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_C99);
+  ASSERT_TRUE(ToOther);
+
+  // EXPECT_TRUE(SharedStatePtr->isSugaredImport(ToOther));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1358,7 +1358,8 @@
   Expected ToDeclOrErr = import(T->getDecl());
   if (!ToDeclOrErr)
 return ToDeclOrErr.takeError();
-  ExpectedType ToUnderlyingTypeOrErr = import(T->desugar());
+  ExpectedType ToUnderlyingTypeOrErr =
+  import(QualType(T, 0).getDesugaredType(T->getDecl()->getASTContext()));
   if (!ToUnderlyingTypeOrErr)
 return ToUnderlyingTypeOrErr.takeError();
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D136886: [ASTImporter] RFC: Correct importer to not duplicate sugared types

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers created this revision.
vabridgers added a reviewer: mizvekov.
Herald added a reviewer: a.sidorin.
Herald added a reviewer: shafik.
Herald added a project: All.
vabridgers requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

I'm not sure if this is a "correct" fix, but this change fixes the added
Unit test case in ASTImporterTest.cpp.

The change 69a6417406a1 
 - 
"[clang] Implement divergence for TypedefType
and UsingType", introduces a problem we've seen when the AST Importer is
used to import a sugared type with more than one level of syntactic
sugar.

The type va_list is imported like so

TypedefType 0x4a89410 'va_list' sugar

| -Typedef 0x4a891d0 'va_list' |
|

`-ElaboratedType 0x4a2d420 '__builtin_va_list' sugar

  `-TypedefType 0x4a2d3f0 '__builtin_va_list' sugar
|-Typedef 0x4a2d398 '__builtin_va_list'
`-ConstantArrayType 0x4a2d340 'struct __va_list_tag[1]' 1
  `-RecordType 0x4a2d180 'struct __va_list_tag'
`-Record 0x4a2d0f8 '__va_list_tag'

and when "desugar" is used, comes back as follows.

VisitTypedefType T - desugared
ElaboratedType 0x4a2d420 '__builtin_va_list' sugar
`-TypedefType 0x4a2d3f0 '__builtin_va_list' sugar

  |-Typedef 0x4a2d398 '__builtin_va_list'
  `-ConstantArrayType 0x4a2d340 'struct __va_list_tag[1]' 1
`-RecordType 0x4a2d180 'struct __va_list_tag'
  `-Record 0x4a2d0f8 '__va_list_tag'

So it appears the type must be desugared past the TypedefType to the
ElaboratedType, subsequently to the ConstantArrayType.

@mizvekov, could you please review this change and suggest different
approaches if this was not your intent?

This change seems to pass current Unit tests and LITs.

@mizvekov, if you wish to pick this up and finish, the UnitTest case
should be all you need for what we see.

Thanks


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136886

Files:
  clang/lib/AST/ASTImporter.cpp
  clang/unittests/AST/ASTImporterTest.cpp


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8075,6 +8075,36 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isSugaredImport) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...)
+  {
+va_list va;
+  } 
+  )",
+  Lang_C99);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...);
+  void maindbgout(char* str)
+  {
+dbgout((char*)str);
+  }
+  )",
+  Lang_C99);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("dbgout")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_C99);
+  ASSERT_TRUE(ToOther);
+
+  // EXPECT_TRUE(SharedStatePtr->isSugaredImport(ToOther));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  DefaultTestValuesForRunOptions);
 
Index: clang/lib/AST/ASTImporter.cpp
===
--- clang/lib/AST/ASTImporter.cpp
+++ clang/lib/AST/ASTImporter.cpp
@@ -1358,7 +1358,9 @@
   Expected ToDeclOrErr = import(T->getDecl());
   if (!ToDeclOrErr)
 return ToDeclOrErr.takeError();
-  ExpectedType ToUnderlyingTypeOrErr = import(T->desugar());
+  // ExpectedType ToUnderlyingTypeOrErr = import(T->desugar());
+  ExpectedType ToUnderlyingTypeOrErr =
+  import(QualType(T, 0).getDesugaredType(T->getDecl()->getASTContext()));
   if (!ToUnderlyingTypeOrErr)
 return ToUnderlyingTypeOrErr.takeError();
 


Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -8075,6 +8075,36 @@
   EXPECT_FALSE(SharedStatePtr->isNewDecl(ToBar));
 }
 
+TEST_P(ASTImporterOptionSpecificTestBase, isSugaredImport) {
+  Decl *FromTU = getTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...)
+  {
+va_list va;
+  } 
+  )",
+  Lang_C99);
+  Decl *ToTU = getToTuDecl(
+  R"(
+  typedef __builtin_va_list va_list;
+  void dbgout(char* fmt, ...);
+  void maindbgout(char* str)
+  {
+dbgout((char*)str);
+  }
+  )",
+  Lang_C99);
+  auto *FromOther = FirstDeclMatcher().match(
+  FromTU, functionDecl(hasName("dbgout")));
+  ASSERT_TRUE(FromOther);
+
+  auto *ToOther = Import(FromOther, Lang_C99);
+  ASSERT_TRUE(ToOther);
+
+  // EXPECT_TRUE(SharedStatePtr->isSugaredImport(ToOther));
+}
+
 INSTANTIATE_TEST_SUITE_P(ParameterizedTests, ASTImporterLookupTableTest,
  

[PATCH] D129269: [analyzer] Fix use of length in CStringChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 443658.
vabridgers edited the summary of this revision.
vabridgers added a comment.

update per comments from @martong


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129269

Files:
  clang/docs/analyzer/checkers.rst
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/string.c


Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -1652,3 +1652,18 @@
   __builtin___memset_chk(, 0, sizeof(x), __builtin_object_size(, 0));
   clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
 }
+
+#ifndef SUPPRESS_OUT_OF_BOUND
+void strcpy_no_overflow_2(char *y) {
+  char x[3];
+  // FIXME: string literal modeling does not account for embedded NULLs.
+  //This case should not elicit a warning, but does.
+  //See discussion at https://reviews.llvm.org/D129269
+  strcpy(x, "12\0"); // expected-warning{{String copy function overflows the 
destination buffer}}
+}
+#else
+void strcpy_no_overflow_2(char *y) {
+  char x[3];
+  strcpy(x, "12\0");
+}
+#endif
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -848,7 +848,7 @@
 SValBuilder  = C.getSValBuilder();
 QualType sizeTy = svalBuilder.getContext().getSizeType();
 const StringLiteral *strLit = cast(MR)->getStringLiteral();
-return svalBuilder.makeIntVal(strLit->getByteLength(), sizeTy);
+return svalBuilder.makeIntVal(strLit->getLength(), sizeTy);
   }
   case MemRegion::SymbolicRegionKind:
   case MemRegion::AllocaRegionKind:
Index: clang/docs/analyzer/checkers.rst
===
--- clang/docs/analyzer/checkers.rst
+++ clang/docs/analyzer/checkers.rst
@@ -2726,6 +2726,9 @@
 ""
 Check for out-of-bounds access in string functions; applies to:`` strncopy, 
strncat``.
 
+This check also applies to string literals, except there is a known bug in that
+the analyzer cannot detect embedded NULL characters.
+
 .. code-block:: c
 
  void test() {


Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -1652,3 +1652,18 @@
   __builtin___memset_chk(, 0, sizeof(x), __builtin_object_size(, 0));
   clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
 }
+
+#ifndef SUPPRESS_OUT_OF_BOUND
+void strcpy_no_overflow_2(char *y) {
+  char x[3];
+  // FIXME: string literal modeling does not account for embedded NULLs.
+  //This case should not elicit a warning, but does.
+  //See discussion at https://reviews.llvm.org/D129269
+  strcpy(x, "12\0"); // expected-warning{{String copy function overflows the destination buffer}}
+}
+#else
+void strcpy_no_overflow_2(char *y) {
+  char x[3];
+  strcpy(x, "12\0");
+}
+#endif
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -848,7 +848,7 @@
 SValBuilder  = C.getSValBuilder();
 QualType sizeTy = svalBuilder.getContext().getSizeType();
 const StringLiteral *strLit = cast(MR)->getStringLiteral();
-return svalBuilder.makeIntVal(strLit->getByteLength(), sizeTy);
+return svalBuilder.makeIntVal(strLit->getLength(), sizeTy);
   }
   case MemRegion::SymbolicRegionKind:
   case MemRegion::AllocaRegionKind:
Index: clang/docs/analyzer/checkers.rst
===
--- clang/docs/analyzer/checkers.rst
+++ clang/docs/analyzer/checkers.rst
@@ -2726,6 +2726,9 @@
 ""
 Check for out-of-bounds access in string functions; applies to:`` strncopy, strncat``.
 
+This check also applies to string literals, except there is a known bug in that
+the analyzer cannot detect embedded NULL characters.
+
 .. code-block:: c
 
  void test() {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129269: [analyzer] Fix use of length in CStringChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Thanks @martong , I understand. I'll make the changes and resubmit. Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129269

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

[PATCH] D129269: [analyzer] Fix use of length in CStringChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 443101.
vabridgers added a comment.

a proposal to handle embedded null case caught by @steakhal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129269

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/test/Analysis/string.c


Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -1652,3 +1652,8 @@
   __builtin___memset_chk(, 0, sizeof(x), __builtin_object_size(, 0));
   clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
 }
+
+void strcpy_no_overflow_2(char *y) {
+  char x[3];
+  strcpy(x, "12\0");
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -848,7 +848,15 @@
 SValBuilder  = C.getSValBuilder();
 QualType sizeTy = svalBuilder.getContext().getSizeType();
 const StringLiteral *strLit = cast(MR)->getStringLiteral();
-return svalBuilder.makeIntVal(strLit->getByteLength(), sizeTy);
+unsigned countx = 0;
+// get the number of string literal characters by the target's "code unit"
+// size, checking for an embedded literal of 0 up to the string literal's
+// length.
+for (countx = 0;
+ countx < strLit->getLength() && (strLit->getCodeUnit(countx) != 0);
+ countx++)
+  ;
+return svalBuilder.makeIntVal(countx, sizeTy);
   }
   case MemRegion::SymbolicRegionKind:
   case MemRegion::AllocaRegionKind:


Index: clang/test/Analysis/string.c
===
--- clang/test/Analysis/string.c
+++ clang/test/Analysis/string.c
@@ -1652,3 +1652,8 @@
   __builtin___memset_chk(, 0, sizeof(x), __builtin_object_size(, 0));
   clang_analyzer_eval(x == 0); // expected-warning{{TRUE}}
 }
+
+void strcpy_no_overflow_2(char *y) {
+  char x[3];
+  strcpy(x, "12\0");
+}
Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -848,7 +848,15 @@
 SValBuilder  = C.getSValBuilder();
 QualType sizeTy = svalBuilder.getContext().getSizeType();
 const StringLiteral *strLit = cast(MR)->getStringLiteral();
-return svalBuilder.makeIntVal(strLit->getByteLength(), sizeTy);
+unsigned countx = 0;
+// get the number of string literal characters by the target's "code unit"
+// size, checking for an embedded literal of 0 up to the string literal's
+// length.
+for (countx = 0;
+ countx < strLit->getLength() && (strLit->getCodeUnit(countx) != 0);
+ countx++)
+  ;
+return svalBuilder.makeIntVal(countx, sizeTy);
   }
   case MemRegion::SymbolicRegionKind:
   case MemRegion::AllocaRegionKind:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129269: [analyzer] Fix use of length in CStringChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Thanks Balazs, you mean something like this correct?

void strcpy_no_overflow_2(char *y) {

  char x[3];
  strcpy(x, "12\0"); // this produces a warning, but should not. 

}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129269

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

[PATCH] D129269: [analyzer] Fix use of length in CStringChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers created this revision.
vabridgers added reviewers: martong, steakhal.
Herald added subscribers: manas, ASDenysPetrov, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
Herald added a reviewer: NoQ.
Herald added a project: All.
vabridgers requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

CStringChecker is using getByteLength to get the length of a string
literal. For targets where a "char" is 8-bits, getByteLength() and
getLength() will be equal for a C string, but for targets where a "char"
is 16-bits getByteLength() returns the size in octets.

This is verified in our downstream target, but we have no way to add a
test case for this case since there is no target supporting 16-bit
"char" upstream. Since this cannot have a test case, I'm asserted this
change is "correct by construction", and visually inspected to be
correct by way of the following example where this was found.

The case that shows this fails using a target with 16-bit chars is here.
getByteLength() for the string literal returns 4, which fails when
checked against "char x[4]". With the change, the string literal is
evaluated to a size of 2 which is a correct number of "char"'s for a
16-bit target.

void strcpy_no_overflow_2(char *y) {

  char x[4];
  strcpy(x, "12"); // with getByteLength(), returns 4 using 16-bit chars

}


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D129269

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -848,7 +848,7 @@
 SValBuilder  = C.getSValBuilder();
 QualType sizeTy = svalBuilder.getContext().getSizeType();
 const StringLiteral *strLit = cast(MR)->getStringLiteral();
-return svalBuilder.makeIntVal(strLit->getByteLength(), sizeTy);
+return svalBuilder.makeIntVal(strLit->getLength(), sizeTy);
   }
   case MemRegion::SymbolicRegionKind:
   case MemRegion::AllocaRegionKind:


Index: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -848,7 +848,7 @@
 SValBuilder  = C.getSValBuilder();
 QualType sizeTy = svalBuilder.getContext().getSizeType();
 const StringLiteral *strLit = cast(MR)->getStringLiteral();
-return svalBuilder.makeIntVal(strLit->getByteLength(), sizeTy);
+return svalBuilder.makeIntVal(strLit->getLength(), sizeTy);
   }
   case MemRegion::SymbolicRegionKind:
   case MemRegion::AllocaRegionKind:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127105: [analyzer] Fix null pointer deref in CastValueChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked 8 inline comments as done.
vabridgers added a comment.

I think all comments have been addressed, please let me know if I missed some 
detail :)  Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127105

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

[PATCH] D127105: [analyzer] Fix null pointer deref in CastValueChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 434821.
vabridgers added a comment.

address @steakhal comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127105

Files:
  clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
  clang/test/Analysis/cast-value-notes.cpp
  clang/test/Analysis/cast-value-state-dump.cpp

Index: clang/test/Analysis/cast-value-state-dump.cpp
===
--- clang/test/Analysis/cast-value-state-dump.cpp
+++ clang/test/Analysis/cast-value-state-dump.cpp
@@ -18,12 +18,12 @@
 
 void evalNonNullParamNonNullReturn(const Shape *S) {
   const auto *C = dyn_cast_or_null(S);
-  // expected-note@-1 {{Assuming 'S' is a 'Circle'}}
+  // expected-note@-1 {{Assuming 'S' is a 'const class clang::Circle *'}}
   // expected-note@-2 {{'C' initialized here}}
 
   // FIXME: We assumed that 'S' is a 'Circle' therefore it is not a 'Square'.
   if (dyn_cast_or_null(S)) {
-// expected-note@-1 {{Assuming 'S' is not a 'Square'}}
+// expected-note@-1 {{Assuming 'S' is not a 'const class clang::Square *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -73,7 +73,7 @@
 #if defined(X86)
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
-  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-1 {{Assuming 'S' is not a 'const class clang::Circle &'}}
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
@@ -93,7 +93,7 @@
 #if defined(NOT_SUPPRESSED)
 void evalReferences_addrspace(const Shape ) {
   const auto  = dyn_cast(S);
-  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-1 {{Assuming 'S' is not a 'const __attribute__((address_space(3))) class clang::Circle &'}}
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
@@ -105,7 +105,7 @@
 #elif defined(MIPS)
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
-  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-1 {{Assuming 'S' is not a 'const class clang::Circle &'}}
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
 }
@@ -122,25 +122,25 @@
   // expected-note@-1 {{'C' initialized here}}
 
   if (!dyn_cast_or_null(C)) {
-// expected-note@-1 {{'C' is a 'Circle'}}
+// expected-note@-1 {{Assuming 'C' is a 'const class clang::Circle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Triangle'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Triangle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Rectangle'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Rectangle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Hexagon'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Hexagon *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
@@ -176,29 +176,29 @@
 
 void evalNonNullParamNonNullReturn(const Shape *S) {
   const auto *C = cast(S);
-  // expected-note@-1 {{'S' is a 'Circle'}}
+  // expected-note@-1 {{'S' is a 'const class clang::Circle *'}}
   // expected-note@-2 {{'C' initialized here}}
 
   if (!dyn_cast_or_null(C)) {
-// expected-note@-1 {{'C' is a 'Circle'}}
+// expected-note@-1 {{Assuming 'C' is a 'const class clang::Circle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Triangle'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Triangle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Rectangle'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Rectangle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Hexagon'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Hexagon *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
@@ -234,10 +234,10 @@
 
 void evalNonNullParamNullReturn(const Shape *S) {
   const auto *C = dyn_cast_or_null(S);
-  // 

[PATCH] D127105: [analyzer] Fix null pointer deref in CastValueChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:168
-CastToTy->getAsCXXRecordDecl()->getNameAsString() :
-CastToTy->getPointeeCXXRecordDecl()->getNameAsString();
   Out << ' ' << ((CastToTyVec.size() == 1) ? "not" :

steakhal wrote:
> So this was null, right?  Which caused the crash.
Yes, the call to "CastToTy->getPointeeCXXRecordDecl()" returned nullptr, which 
was then used to dereference getNameAsString(), then boom :) 



Comment at: clang/test/Analysis/cast-value-notes.cpp:306
+
+// don't crash
+namespace llvm {

steakhal wrote:
> It's good to know which line exactly caused the crash. Put this note right 
> there.
Will address, thank you



Comment at: clang/test/Analysis/cast-value-notes.cpp:311
+public:
+  template  void b() { isa(*this); }
+};

steakhal wrote:
> This gotta be the `getAs`. Please try to reconstruct the 'feel' of it; 
> like return a `T*` instead of `void` etc.
I'll attempt a further simplification. This was the product of a very long and 
tedious manual and creduce reduction process from a 12M preprocessed file :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127105

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

[PATCH] D127105: [analyzer] Fix null pointer deref in CastValueChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 434746.
vabridgers added a comment.

Use QualType::getAsString() per suggestion from martong


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127105

Files:
  clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
  clang/test/Analysis/cast-value-notes.cpp
  clang/test/Analysis/cast-value-state-dump.cpp

Index: clang/test/Analysis/cast-value-state-dump.cpp
===
--- clang/test/Analysis/cast-value-state-dump.cpp
+++ clang/test/Analysis/cast-value-state-dump.cpp
@@ -18,12 +18,12 @@
 
 void evalNonNullParamNonNullReturn(const Shape *S) {
   const auto *C = dyn_cast_or_null(S);
-  // expected-note@-1 {{Assuming 'S' is a 'Circle'}}
+  // expected-note@-1 {{Assuming 'S' is a 'const class clang::Circle *'}}
   // expected-note@-2 {{'C' initialized here}}
 
   // FIXME: We assumed that 'S' is a 'Circle' therefore it is not a 'Square'.
   if (dyn_cast_or_null(S)) {
-// expected-note@-1 {{Assuming 'S' is not a 'Square'}}
+// expected-note@-1 {{Assuming 'S' is not a 'const class clang::Square *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -73,7 +73,7 @@
 #if defined(X86)
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
-  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-1 {{Assuming 'S' is not a 'const class clang::Circle &'}}
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
@@ -93,7 +93,7 @@
 #if defined(NOT_SUPPRESSED)
 void evalReferences_addrspace(const Shape ) {
   const auto  = dyn_cast(S);
-  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-1 {{Assuming 'S' is not a 'const __attribute__((address_space(3))) class clang::Circle &'}}
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
@@ -105,7 +105,7 @@
 #elif defined(MIPS)
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
-  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-1 {{Assuming 'S' is not a 'const class clang::Circle &'}}
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
 }
@@ -122,25 +122,25 @@
   // expected-note@-1 {{'C' initialized here}}
 
   if (!dyn_cast_or_null(C)) {
-// expected-note@-1 {{'C' is a 'Circle'}}
+// expected-note@-1 {{Assuming 'C' is a 'const class clang::Circle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Triangle'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Triangle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Rectangle'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Rectangle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Hexagon'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Hexagon *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
@@ -176,29 +176,29 @@
 
 void evalNonNullParamNonNullReturn(const Shape *S) {
   const auto *C = cast(S);
-  // expected-note@-1 {{'S' is a 'Circle'}}
+  // expected-note@-1 {{'S' is a 'const class clang::Circle *'}}
   // expected-note@-2 {{'C' initialized here}}
 
   if (!dyn_cast_or_null(C)) {
-// expected-note@-1 {{'C' is a 'Circle'}}
+// expected-note@-1 {{Assuming 'C' is a 'const class clang::Circle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Triangle'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Triangle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Rectangle'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Rectangle *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
 
   if (dyn_cast_or_null(C)) {
-// expected-note@-1 {{Assuming 'C' is not a 'Hexagon'}}
+// expected-note@-1 {{Assuming 'C' is not a 'const class clang::Hexagon *'}}
 // expected-note@-2 {{Taking false branch}}
 return;
   }
@@ -234,10 +234,10 @@
 
 void evalNonNullParamNullReturn(const Shape *S) {
   const auto *C = 

[PATCH] D127105: [analyzer] Fix null pointer deref in CastValueChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I was able to find and add a covering test case. I understand the fix may not 
be "correct", but it does avoid the crash demonstrated by the test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127105

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

[PATCH] D127105: [analyzer] Fix null pointer deref in CastValueChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 434628.
vabridgers added a comment.

add test case


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127105

Files:
  clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
  clang/test/Analysis/cast-value-notes.cpp


Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -302,3 +302,17 @@
   // expected-note@-1 {{Division by zero}}
   // expected-warning@-2 {{Division by zero}}
 }
+
+// don't crash
+namespace llvm {
+template  void isa(a &);
+template  class PointerUnion {
+public:
+  template  void b() { isa(*this); }
+};
+class LLVMContext {
+  PointerUnion c;
+  void d() { c.b(); }
+};
+} // namespace llvm
+
Index: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
@@ -108,7 +108,9 @@
  bool CastSucceeds, bool IsKnownCast) {
   std::string CastToName =
   CastInfo ? CastInfo->to()->getAsCXXRecordDecl()->getNameAsString()
-   : CastToTy->getPointeeCXXRecordDecl()->getNameAsString();
+  : (CastToTy->getPointeeCXXRecordDecl() != nullptr)
+  ? CastToTy->getPointeeCXXRecordDecl()->getNameAsString()
+  : "(nil)";
   Object = Object->IgnoreParenImpCasts();
 
   return C.getNoteTag(
@@ -163,9 +165,11 @@
 bool First = true;
 for (QualType CastToTy: CastToTyVec) {
   std::string CastToName =
-CastToTy->getAsCXXRecordDecl() ?
-CastToTy->getAsCXXRecordDecl()->getNameAsString() :
-CastToTy->getPointeeCXXRecordDecl()->getNameAsString();
+  CastToTy->getAsCXXRecordDecl()
+  ? CastToTy->getAsCXXRecordDecl()->getNameAsString()
+  : (CastToTy->getPointeeCXXRecordDecl() != nullptr)
+  ? CastToTy->getPointeeCXXRecordDecl()->getNameAsString()
+  : "(nil)";
   Out << ' ' << ((CastToTyVec.size() == 1) ? "not" :
  (First ? "neither" : "nor")) << " a '" << CastToName
   << '\'';


Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -302,3 +302,17 @@
   // expected-note@-1 {{Division by zero}}
   // expected-warning@-2 {{Division by zero}}
 }
+
+// don't crash
+namespace llvm {
+template  void isa(a &);
+template  class PointerUnion {
+public:
+  template  void b() { isa(*this); }
+};
+class LLVMContext {
+  PointerUnion c;
+  void d() { c.b(); }
+};
+} // namespace llvm
+
Index: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
@@ -108,7 +108,9 @@
  bool CastSucceeds, bool IsKnownCast) {
   std::string CastToName =
   CastInfo ? CastInfo->to()->getAsCXXRecordDecl()->getNameAsString()
-   : CastToTy->getPointeeCXXRecordDecl()->getNameAsString();
+  : (CastToTy->getPointeeCXXRecordDecl() != nullptr)
+  ? CastToTy->getPointeeCXXRecordDecl()->getNameAsString()
+  : "(nil)";
   Object = Object->IgnoreParenImpCasts();
 
   return C.getNoteTag(
@@ -163,9 +165,11 @@
 bool First = true;
 for (QualType CastToTy: CastToTyVec) {
   std::string CastToName =
-CastToTy->getAsCXXRecordDecl() ?
-CastToTy->getAsCXXRecordDecl()->getNameAsString() :
-CastToTy->getPointeeCXXRecordDecl()->getNameAsString();
+  CastToTy->getAsCXXRecordDecl()
+  ? CastToTy->getAsCXXRecordDecl()->getNameAsString()
+  : (CastToTy->getPointeeCXXRecordDecl() != nullptr)
+  ? CastToTy->getPointeeCXXRecordDecl()->getNameAsString()
+  : "(nil)";
   Out << ' ' << ((CastToTyVec.size() == 1) ? "not" :
  (First ? "neither" : "nor")) << " a '" << CastToName
   << '\'';
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127105: [analyzer] Fix null pointer deref in CastValueChecker

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers planned changes to this revision.
vabridgers added a comment.

I know this will need a reproducer, and I'm working on that. That work is still 
in progress.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127105

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

[PATCH] D127105: [analyzer] Fix null pointer deref in CastValueChecker

[Vince Bridgers via Phabricator via cfe-commits]

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

A crash was seen in CastValueChecker due to a null pointer dereference
when analyzing proprietary source code. A suitably small and obfuscated
reproducer cannot be provided at this time, but a description of the
problem seen can be provided. The crash seen is as follows, with debug
information dump following.

The fix seeks to simply avoid the null pointer dereference, thus
preventing the crash.

Program received signal SIGSEGV, Segmentation fault.
0x09c2d380 in clang::DeclarationName::getAsString[abi:cxx11]()
const (this=0x28)

  at ../../clang/lib/AST/DeclarationName.cpp:238

238   OS << *this;
(gdb) bt
clang::DeclarationName::getAsString[abi:cxx11]() const (this=0x28)

  at ../../clang/lib/AST/DeclarationName.cpp:238

const (this=0x0)

  at ../../clang/include/clang/AST/Decl.h:290

Object=0x1c11af18,

  CastSucceeds=true, IsKnownCast=false)
  at ../../clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:111

State=..., C=..., IsInstanceOf=true)

  at ../../clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:319

namespace)::CastValueChecker::evalIsa (this=0x10cdf140, Call=...,

  DV=..., C=...) at

../../clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:437
...

(gdb) frame 2
Object=0x1c11af18,

  CastSucceeds=true, IsKnownCast=false)
  at ../../clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:111

111:
CastToTy->getPointeeCXXRecordDecl()->getNameAsString();

(gdb) p CastToTy
$1 = {Value = {Value = 312725088}}

(gdb) p CastToTy->getPointeeCXXRecordDecl()
$2 = (const clang::CXXRecordDecl *) 0x0

(gdb) p CastToTy.dump()
LValueReferenceType 0x12a3ce60 'class llvm::ReplaceableMetadataImpl *&'
`-PointerType 0x129971b0 'class llvm::ReplaceableMetadataImpl *'

  `-RecordType 0x128afce0 'class llvm::ReplaceableMetadataImpl'
`-CXXRecord 0x128ff160 'ReplaceableMetadataImpl'

(gdb) frame 16
(this=0x7fff9208, currStmt=0x1f026978, Pred=0x22cf76f0)

  at ../../clang/lib/StaticAnalyzer/Core/ExprEngine.cpp:783

783Visit(currStmt, I, DstI);

(gdb) p currStmt
$1 = (const clang::Stmt *) 0x1f026978

(gdb) p currStmt->dump()
CallExpr 0x1f026978 '_Bool'

| -ImplicitCastExpr 0x1f026960 '_Bool (*)(const class |
|

llvm::PointerUnion &)' 

| `-DeclRefExpr 0x1f026908 '_Bool (const class llvm::PointerUnion &)' lvalue Function
0x1f007e58 'isa' '_Bool (const class llvm::PointerUnion &)' (FunctionTemplate
0x11c5df38 'isa')
`-UnaryOperator 0x1f026738 'const class llvm::PointerUnion' lvalue prefix '*' cannot
overflow

  `-CXXThisExpr 0x1f026728 'const class llvm::PointerUnion *' this
$2 = void


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127105

Files:
  clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
@@ -108,7 +108,9 @@
  bool CastSucceeds, bool IsKnownCast) {
   std::string CastToName =
   CastInfo ? CastInfo->to()->getAsCXXRecordDecl()->getNameAsString()
-   : CastToTy->getPointeeCXXRecordDecl()->getNameAsString();
+  : (CastToTy->getPointeeCXXRecordDecl() != nullptr)
+  ? CastToTy->getPointeeCXXRecordDecl()->getNameAsString()
+  : "(nil)";
   Object = Object->IgnoreParenImpCasts();
 
   return C.getNoteTag(
@@ -163,9 +165,11 @@
 bool First = true;
 for (QualType CastToTy: CastToTyVec) {
   std::string CastToName =
-CastToTy->getAsCXXRecordDecl() ?
-CastToTy->getAsCXXRecordDecl()->getNameAsString() :
-CastToTy->getPointeeCXXRecordDecl()->getNameAsString();
+  CastToTy->getAsCXXRecordDecl()
+  ? CastToTy->getAsCXXRecordDecl()->getNameAsString()
+  : (CastToTy->getPointeeCXXRecordDecl() != nullptr)
+  ? CastToTy->getPointeeCXXRecordDecl()->getNameAsString()
+  : "(nil)";
   Out << ' ' << ((CastToTyVec.size() == 1) ? "not" :
  (First ? "neither" : "nor")) << " a '" << CastToName
   << '\'';


Index: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp
@@ -108,7 +108,9 @@
  bool CastSucceeds, bool IsKnownCast) {
   std::string CastToName =
   CastInfo ? 

[PATCH] D124349: [analyzer] Get direct binding for specific punned case

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked an inline comment as done.
vabridgers added a comment.

Thanks @martong. I fixed the "typo" :/, landing. Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124349

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

[PATCH] D124349: [analyzer] Get direct binding for specific punned case

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 427246.
vabridgers added a comment.

fix a stupid leftover cut/paste mistake in the test case :/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124349

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/array-punned-region.c


Index: clang/test/Analysis/array-punned-region.c
===
--- /dev/null
+++ clang/test/Analysis/array-punned-region.c
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config 
eagerly-assume=false -triple x86_64-pc-linux-gnu %s
+
+// RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config 
eagerly-assume=false -triple i386-pc-linux-gnu  %s
+
+int clang_analyzer_eval(int);
+
+typedef struct {
+  int a : 1;
+  int b[2];
+} BITFIELD_CAST;
+
+void array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or 
undefined [core.uninitialized.Assign]}}
+  return a;
+}
+
+typedef struct {
+  unsigned int a : 1;
+  unsigned int x : 31;
+  unsigned int c : 1;
+  int b[2];
+} mystruct;
+
+void array_struct_bitfield_3() {
+  mystruct ff;
+  mystruct *pff = 
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 2) == 3); // expected-warning{{TRUE}}
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2147,8 +2147,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.


Index: clang/test/Analysis/array-punned-region.c
===
--- /dev/null
+++ clang/test/Analysis/array-punned-region.c
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config eagerly-assume=false -triple x86_64-pc-linux-gnu %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config eagerly-assume=false -triple i386-pc-linux-gnu  %s
+
+int clang_analyzer_eval(int);
+
+typedef struct {
+  int a : 1;
+  int b[2];
+} BITFIELD_CAST;
+
+void array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return a;
+}
+
+typedef struct {
+  unsigned int a : 1;
+  unsigned int x : 31;
+  unsigned int c : 1;
+  int b[2];
+} mystruct;
+
+void array_struct_bitfield_3() {
+  mystruct ff;
+  mystruct *pff = 
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 2) == 3); // expected-warning{{TRUE}}
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2147,8 +2147,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124349: [analyzer] Get direct binding for specific punned case

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 427106.
vabridgers added a comment.

Address comments from @martong


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124349

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/array-punned-region.c


Index: clang/test/Analysis/array-punned-region.c
===
--- /dev/null
+++ clang/test/Analysis/array-punned-region.c
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config 
eagerly-assume=false -triple x86_64-pc-linux-gnu %s
+
+// RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config 
eagerly-assume=false -triple i386-pc-linux-gnu  %s
+
+int clang_analyzer_eval(int);
+
+typedef struct {
+  int a : 1;
+  int b[2];
+} BITFIELD_CAST;
+
+void array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or 
undefined [core.uninitialized.Assign]}}
+  return a;
+}
+
+typedef struct {
+  unsigned int a : 1;
+  unsigned int x : 31;
+  unsigned int c : 1;
+  int b[2];
+} mystruct;
+
+void array_struct_bitfield_3() {
+  mystruct ff;
+  mystruct *pff = 
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 2) == 3); // expected-warning{{TRUE}}  // 
Or should this be `pff + 3` ???
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2147,8 +2147,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.


Index: clang/test/Analysis/array-punned-region.c
===
--- /dev/null
+++ clang/test/Analysis/array-punned-region.c
@@ -0,0 +1,39 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config eagerly-assume=false -triple x86_64-pc-linux-gnu %s
+
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,debug.ExprInspection -verify -analyzer-config eagerly-assume=false -triple i386-pc-linux-gnu  %s
+
+int clang_analyzer_eval(int);
+
+typedef struct {
+  int a : 1;
+  int b[2];
+} BITFIELD_CAST;
+
+void array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return a;
+}
+
+typedef struct {
+  unsigned int a : 1;
+  unsigned int x : 31;
+  unsigned int c : 1;
+  int b[2];
+} mystruct;
+
+void array_struct_bitfield_3() {
+  mystruct ff;
+  mystruct *pff = 
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 2) == 3); // expected-warning{{TRUE}}  // Or should this be `pff + 3` ???
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2147,8 +2147,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124349: [analyzer] Get direct binding for specific punned case

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Thanks for the comments! Per our discussion, I'll create a different test case 
with a pinned target and add the additional test case to be sure we see 
expected behavior. Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124349

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

[PATCH] D124349: [analyzer] Get direct binding for specific punned case

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

polite ping to @NoQ and/or @martong. @steakhal is ok with this change, are you 
ok if I land this? Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124349

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

[PATCH] D124349: [analyzer] Get direct binding for specific punned case

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 424844.
vabridgers added a comment.

update LIT per comments from @steakhal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124349

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/array-struct-region.c


Index: clang/test/Analysis/array-struct-region.c
===
--- clang/test/Analysis/array-struct-region.c
+++ clang/test/Analysis/array-struct-region.c
@@ -353,3 +353,23 @@
   // FIXME: Should be TRUE.
   clang_analyzer_eval(vals[index].a[0].x == 42); // expected-warning{{UNKNOWN}}
 }
+
+typedef struct {
+  int a : 1;
+  int b[2];
+} BITFIELD_CAST;
+
+void array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or 
undefined [core.uninitialized.Assign]}}
+  return a;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2153,8 +2153,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.


Index: clang/test/Analysis/array-struct-region.c
===
--- clang/test/Analysis/array-struct-region.c
+++ clang/test/Analysis/array-struct-region.c
@@ -353,3 +353,23 @@
   // FIXME: Should be TRUE.
   clang_analyzer_eval(vals[index].a[0].x == 42); // expected-warning{{UNKNOWN}}
 }
+
+typedef struct {
+  int a : 1;
+  int b[2];
+} BITFIELD_CAST;
+
+void array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
+  ff.b[0] = 3;
+  clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return a;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2153,8 +2153,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124349: [analyzer] Get direct binding for specific punned case

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added inline comments.



Comment at: clang/test/Analysis/array-struct-region.c:362-366
+int array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  return *((int *)pff + 1);
+}

steakhal wrote:
> ```lang=c
> void array_struct_bitfield_1() {
>   BITFIELD_CAST ff = {0};
>   BITFIELD_CAST *pff = 
>   clang_analyzer_eval(*((int *)pff + 1) == 0); // expected-warning{{TRUE}}
>   ff.b[0] = 3;
>   clang_analyzer_eval(*((int *)pff + 1) == 3); // expected-warning{{TRUE}}
> }
> ```
Good suggestion, thanks. I'll add this and wait for another accept.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124349

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

[PATCH] D124349: [analyzer] Get direct binding for specific punned case

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 424796.
vabridgers added a comment.

clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124349

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/array-struct-region.c


Index: clang/test/Analysis/array-struct-region.c
===
--- clang/test/Analysis/array-struct-region.c
+++ clang/test/Analysis/array-struct-region.c
@@ -353,3 +353,21 @@
   // FIXME: Should be TRUE.
   clang_analyzer_eval(vals[index].a[0].x == 42); // expected-warning{{UNKNOWN}}
 }
+
+typedef struct {
+  int a : 1;
+  int b[2];
+} BITFIELD_CAST;
+
+int array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  return *((int *)pff + 1);
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or 
undefined [core.uninitialized.Assign]}}
+  return a;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2153,8 +2153,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.


Index: clang/test/Analysis/array-struct-region.c
===
--- clang/test/Analysis/array-struct-region.c
+++ clang/test/Analysis/array-struct-region.c
@@ -353,3 +353,21 @@
   // FIXME: Should be TRUE.
   clang_analyzer_eval(vals[index].a[0].x == 42); // expected-warning{{UNKNOWN}}
 }
+
+typedef struct {
+  int a : 1;
+  int b[2];
+} BITFIELD_CAST;
+
+int array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  return *((int *)pff + 1);
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST *pff = 
+  int a = *((int *)pff + 2); // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return a;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2153,8 +2153,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124349: [analyzer] Get direct binding for specific punned case

[Vince Bridgers via Phabricator via cfe-commits]

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

Region store was not able to see through this case to the actual
initialized value of STRUCT ff. This change addresses this case by
getting the direct binding. This was found and debugged in a downstream
compiler, with debug guidance from @steakhal. A positive and negative
test case is added.

The specific case where this issue was exposed.

  typedef struct {
int a:1;
int b[2];
  } STRUCT;
  
  int main() {
STRUCT ff = {0};
STRUCT* pff = 
int a = ((int)pff + 1);
return a;
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124349

Files:
  clang/lib/StaticAnalyzer/Core/RegionStore.cpp
  clang/test/Analysis/array-struct-region.c


Index: clang/test/Analysis/array-struct-region.c
===
--- clang/test/Analysis/array-struct-region.c
+++ clang/test/Analysis/array-struct-region.c
@@ -353,3 +353,21 @@
   // FIXME: Should be TRUE.
   clang_analyzer_eval(vals[index].a[0].x == 42); // expected-warning{{UNKNOWN}}
 }
+
+typedef struct {
+  int a:1;
+  int b[2];
+} BITFIELD_CAST;
+
+int array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST* pff = 
+  return *((int*)pff + 1);
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST* pff = 
+  int a = *((int*)pff + 2); // expected-warning{{Assigned value is garbage or 
undefined [core.uninitialized.Assign]}}
+  return a;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2153,8 +2153,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.


Index: clang/test/Analysis/array-struct-region.c
===
--- clang/test/Analysis/array-struct-region.c
+++ clang/test/Analysis/array-struct-region.c
@@ -353,3 +353,21 @@
   // FIXME: Should be TRUE.
   clang_analyzer_eval(vals[index].a[0].x == 42); // expected-warning{{UNKNOWN}}
 }
+
+typedef struct {
+  int a:1;
+  int b[2];
+} BITFIELD_CAST;
+
+int array_struct_bitfield_1() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST* pff = 
+  return *((int*)pff + 1);
+}
+
+int array_struct_bitfield_2() {
+  BITFIELD_CAST ff = {0};
+  BITFIELD_CAST* pff = 
+  int a = *((int*)pff + 2); // expected-warning{{Assigned value is garbage or undefined [core.uninitialized.Assign]}}
+  return a;
+}
Index: clang/lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- clang/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ clang/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2153,8 +2153,13 @@
   return UnknownVal();
 
 // Additionally allow introspection of a block's internal layout.
-if (!hasPartialLazyBinding && !isa(R->getBaseRegion()))
+// Try to get direct binding if all other attempts failed thus far.
+// Else, return UndefinedVal()
+if (!hasPartialLazyBinding && !isa(R->getBaseRegion())) {
+  if (const Optional  = B.getDefaultBinding(R))
+return *V;
   return UndefinedVal();
+}
   }
 
   // All other values are symbolic.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 424752.
vabridgers added a comment.

add back amdgcn cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cast-value-notes.cpp

Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -1,10 +1,44 @@
-// RUN: %clang_analyze_cc1 -std=c++14 \
-// RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
-// RUN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
+// RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-output=text -verify -DX86 -DSUPPRESSED %s 2>&1 | FileCheck %s -check-prefix=X86-CHECK
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN:  -analyzer-config core.NullDereference:SuppressAddressSpaces=false\
+// RUN:  -analyzer-output=text -verify -DX86 -DNOT_SUPPRESSED %s 2>&1 | FileCheck %s -check-prefix=X86-CHECK
 //
 // RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
 // RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
-// RUN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
+// RUN:  -analyzer-config core.NullDereference:SuppressAddressSpaces=true\
+// RUN:  -analyzer-output=text -verify -DX86 -DSUPPRESSED %s 2>&1 | FileCheck %s -check-prefix=X86-CHECK
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-unknown-unknown \
+// RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN:  -analyzer-output=text -verify -DX86 -DSUPPRESSED %s 2>&1 | FileCheck %s --check-prefix=X86-CHECK
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-unknown-unknown \
+// RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN:  -analyzer-config core.NullDereference:SuppressAddressSpaces=true\
+// RUN:  -analyzer-output=text -verify -DX86 -DSUPPRESSED %s 2>&1 | FileCheck %s --check-prefix=X86-CHECK-SUPPRESSED
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-unknown-unknown \
+// RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN:  -analyzer-config core.NullDereference:SuppressAddressSpaces=false\
+// RUN:  -analyzer-output=text -verify -DX86 -DNOT_SUPPRESSED %s 2>&1 | FileCheck %s --check-prefix=X86-CHECK
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple mips-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-output=text -verify -DMIPS %s 2>&1
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple mips-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-config core.NullDereference:SuppressAddressSpaces=false\
+// RUN: -analyzer-output=text -verify -DMIPS %s 2>&1
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple mips-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-config core.NullDereference:SuppressAddressSpaces=true\
+// RUN: -analyzer-output=text -verify -DMIPS_SUPPRESSED %s
 
 #include "Inputs/llvm.h"
 
@@ -36,27 +70,51 @@
 
 void clang_analyzer_printState();
 
-#if defined(DEFAULT_TRIPLE)
+#if defined(X86)
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
   // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
-  // DEFAULT-CHECK: "dynamic_types": [
-  // DEFAULT-CHECK-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const class clang::Circle &", "sub_classable": true }
+  // XX86-CHECK:  "dynamic_types": [
+  // XX86-CHECK-NEXT:   { "region": "SymRegion{reg_$0}", "dyn_type": "const class clang::Circle &", "sub_classable": true }
   (void)C;
 }
-#elif defined(AMDGCN_TRIPLE)
-void evalReferences(const Shape ) {
+#if defined(SUPPRESSED)
+void evalReferences_addrspace(const Shape ) {
   const auto  = dyn_cast(S);
   clang_analyzer_printState();
-  // AMDGCN-CHECK: "dynamic_types": [
-  // AMDGCN-CHECK-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  // X86-CHECK-SUPPRESSED: "dynamic_types": [
+  // X86-CHECK-SUPPRESSED-NEXT: { "region": "SymRegion{reg_$0}", 

[PATCH] D123464: [analyzer] Clean checker options from bool to DefaultBool (NFC)

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Hi @steakhal, I believe I've addressed the comments. Please have a look and let 
me know if this is what you were after. Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123464

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

[PATCH] D123464: [analyzer] Clean checker options from bool to DefaultBool (NFC)

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 424720.
vabridgers added a comment.

refactor patch to remove DefaultBool, replace with bool as suggested


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123464

Files:
  clang/include/clang/StaticAnalyzer/Core/Checker.h
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CheckSecuritySyntaxOnly.cpp
  clang/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  clang/lib/StaticAnalyzer/Checkers/IvarInvalidationChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -46,7 +46,7 @@
 CK_NumCheckKinds
   };
 
-  DefaultBool ChecksEnabled[CK_NumCheckKinds];
+  bool ChecksEnabled[CK_NumCheckKinds] = {false};
   CheckerNameRef CheckNames[CK_NumCheckKinds];
 
   void checkPreStmt(const VAArgExpr *VAA, CheckerContext ) const;
Index: clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/UnixAPIChecker.cpp
@@ -43,7 +43,7 @@
   mutable Optional Val_O_CREAT;
 
 public:
-  DefaultBool CheckMisuse, CheckPortability;
+  bool CheckMisuse = false, CheckPortability = false;
 
   void checkPreStmt(const CallExpr *CE, CheckerContext ) const;
 
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -563,7 +563,7 @@
 CK_StdCLibraryFunctionsTesterChecker,
 CK_NumCheckKinds
   };
-  DefaultBool ChecksEnabled[CK_NumCheckKinds];
+  bool ChecksEnabled[CK_NumCheckKinds] = {false};
   CheckerNameRef CheckNames[CK_NumCheckKinds];
 
   bool DisplayLoadedSummaries = false;
Index: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
@@ -42,7 +42,7 @@
 CK_NumCheckKinds
   };
 
-  DefaultBool ChecksEnabled[CK_NumCheckKinds];
+  bool ChecksEnabled[CK_NumCheckKinds] = {false};
   CheckerNameRef CheckNames[CK_NumCheckKinds];
 
   void checkPreCall(const CallEvent , CheckerContext ) const;
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -48,7 +48,7 @@
 
 public:
   // Whether the checker should model for null dereferences of smart pointers.
-  DefaultBool ModelSmartPtrDereference;
+  bool ModelSmartPtrDereference = false;
   bool evalCall(const CallEvent , CheckerContext ) const;
   void checkPreCall(const CallEvent , CheckerContext ) const;
   void checkDeadSymbols(SymbolReaper , CheckerContext ) const;
Index: clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
@@ -78,7 +78,7 @@
 CK_C11LockChecker,
 CK_NumCheckKinds
   };
-  DefaultBool ChecksEnabled[CK_NumCheckKinds];
+  bool ChecksEnabled[CK_NumCheckKinds] = {false};
   CheckerNameRef CheckNames[CK_NumCheckKinds];
 
 private:
Index: clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -90,7 +90,7 @@
   // find warnings about nullability annotations that they have explicitly
   // added themselves higher priority to fix than warnings on calls to system
   // libraries.
-  DefaultBool NoDiagnoseCallsToSystemHeaders;
+  bool NoDiagnoseCallsToSystemHeaders = 

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked an inline comment as done.
vabridgers added a comment.

I believe all comments have been addressed. After discussion with @steakhal, 
we're leaning towards removing the DefaultBool class and refactoring in favor 
of the language native bool type - mainly because DefaultBool doesn't add 
anything. Minimal yet effective is usually better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

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

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 424711.
vabridgers added a comment.

Update test cases, address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cast-value-notes.cpp

Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -1,10 +1,30 @@
-// RUN: %clang_analyze_cc1 -std=c++14 \
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-unknown-unknown \
 // RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
-// RUN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
+// RUN:  -analyzer-output=text -verify -DX86 -DSUPPRESSED %s 2>&1 | FileCheck %s --check-prefix=X86-CHECK
 //
-// RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-unknown-unknown \
+// RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN:  -analyzer-config core.NullDereference:SuppressAddressSpaces=true\
+// RUN:  -analyzer-output=text -verify -DX86 -DSUPPRESSED %s 2>&1 | FileCheck %s --check-prefix=X86-CHECK-SUPPRESSED
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-unknown-unknown \
+// RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN:  -analyzer-config core.NullDereference:SuppressAddressSpaces=false\
+// RUN:  -analyzer-output=text -verify -DX86 -DNOT_SUPPRESSED %s 2>&1 | FileCheck %s --check-prefix=X86-CHECK
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple mips-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-output=text -verify -DMIPS %s 2>&1
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple mips-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-config core.NullDereference:SuppressAddressSpaces=false\
+// RUN: -analyzer-output=text -verify -DMIPS %s 2>&1
+//
+// RUN: %clang_analyze_cc1 -std=c++14 -triple mips-unknown-unknown \
 // RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
-// RUN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
+// RUN: -analyzer-config core.NullDereference:SuppressAddressSpaces=true\
+// RUN: -analyzer-output=text -verify -DMIPS_SUPPRESSED %s
 
 #include "Inputs/llvm.h"
 
@@ -36,27 +56,51 @@
 
 void clang_analyzer_printState();
 
-#if defined(DEFAULT_TRIPLE)
+#if defined(X86)
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
   // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
   // expected-note@-2 {{Dereference of null pointer}}
   // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
-  // DEFAULT-CHECK: "dynamic_types": [
-  // DEFAULT-CHECK-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const class clang::Circle &", "sub_classable": true }
+  // XX86-CHECK:  "dynamic_types": [
+  // XX86-CHECK-NEXT:   { "region": "SymRegion{reg_$0}", "dyn_type": "const class clang::Circle &", "sub_classable": true }
   (void)C;
 }
-#elif defined(AMDGCN_TRIPLE)
-void evalReferences(const Shape ) {
+#if defined(SUPPRESSED)
+void evalReferences_addrspace(const Shape ) {
+  const auto  = dyn_cast(S);
+  clang_analyzer_printState();
+  // X86-CHECK-SUPPRESSED: "dynamic_types": [
+  // X86-CHECK-SUPPRESSED-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  (void)C;
+}
+#endif
+#if defined(NOT_SUPPRESSED)
+void evalReferences_addrspace(const Shape ) {
   const auto  = dyn_cast(S);
+  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-2 {{Dereference of null pointer}}
+  // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
-  // AMDGCN-CHECK: "dynamic_types": [
-  // AMDGCN-CHECK-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  // X86-CHECK: "dynamic_types": [
+  // X86-CHECK-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  (void)C;
+}
+#endif
+#elif defined(MIPS)
+void evalReferences(const Shape ) {
+  const auto  = dyn_cast(S);
+  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-2 {{Dereference of null pointer}}
+  // expected-warning@-3 {{Dereference of null pointer}}
+}
+#if defined(MIPS_SUPPRESSED)

[PATCH] D123464: [analyzer] Clean checker options from bool to DefaultBool (NFC)

[Vince Bridgers via Phabricator via cfe-commits]

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

A recent review emphasized the preference to use DefaultBool instead of
bool for checker options. This change is a NFC and cleans up some of the
instances where bool was used, and could be changed to DefaultBool.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123464

Files:
  clang/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
  clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp


Index: clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
@@ -44,7 +44,7 @@
 public:
   // These are going to be null if the respective check is disabled.
   mutable std::unique_ptr BT_Pure, BT_Impure;
-  bool ShowFixIts = false;
+  DefaultBool ShowFixIts;
 
   void checkBeginFunction(CheckerContext ) const;
   void checkEndFunction(const ReturnStmt *RS, CheckerContext ) const;
Index: 
clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
===
--- clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
+++ clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
@@ -71,11 +71,11 @@
 namespace ento {
 
 struct UninitObjCheckerOptions {
-  bool IsPedantic = false;
-  bool ShouldConvertNotesToWarnings = false;
-  bool CheckPointeeInitialization = false;
+  DefaultBool IsPedantic;
+  DefaultBool ShouldConvertNotesToWarnings;
+  DefaultBool CheckPointeeInitialization;
   std::string IgnoredRecordsWithFieldPattern;
-  bool IgnoreGuardedFields = false;
+  DefaultBool IgnoreGuardedFields;
 };
 
 /// A lightweight polymorphic wrapper around FieldRegion *. We'll use this
Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -566,9 +566,9 @@
   DefaultBool ChecksEnabled[CK_NumCheckKinds];
   CheckerNameRef CheckNames[CK_NumCheckKinds];
 
-  bool DisplayLoadedSummaries = false;
-  bool ModelPOSIX = false;
-  bool ShouldAssumeControlledEnvironment = false;
+  DefaultBool DisplayLoadedSummaries;
+  DefaultBool ModelPOSIX;
+  DefaultBool ShouldAssumeControlledEnvironment;
 
 private:
   Optional findFunctionSummary(const FunctionDecl *FD,
Index: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
===
--- clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
+++ clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
@@ -267,13 +267,13 @@
   static std::unique_ptr CastFailTag;
 
   /// Track Objective-C and CoreFoundation objects.
-  bool TrackObjCAndCFObjects = false;
+  DefaultBool TrackObjCAndCFObjects;
 
   /// Track sublcasses of OSObject.
-  bool TrackOSObjects = false;
+  DefaultBool TrackOSObjects;
 
   /// Track initial parameters (for the entry point) for NS/CF objects.
-  bool TrackNSCFStartParam = false;
+  DefaultBool TrackNSCFStartParam;
 
   RetainCountChecker() {};
 
Index: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp
@@ -120,7 +120,7 @@
 namespace {
 class DeadStoresChecker : public Checker {
 public:
-  bool ShowFixIts = false;
+  DefaultBool ShowFixIts;
   bool WarnForDeadNestedAssignments = true;
 
   void checkASTCodeBody(const Decl *D, AnalysisManager ,
Index: clang/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/CloneChecker.cpp
@@ -30,7 +30,7 @@
 public:
   // Checker options.
   int MinComplexity;
-  bool ReportNormalClones;
+  DefaultBool ReportNormalClones;
   StringRef IgnoredFilesPattern;
 
 private:


Index: clang/lib/StaticAnalyzer/Checkers/VirtualCallChecker.cpp
===
--- 

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked 3 inline comments as done.
vabridgers added a comment.

The two small issues are addressed, I think the only outstanding issue is the 
question about the test case. Please let me know a preference and I'll 
implement accordingly. Best!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

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

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 421759.
vabridgers added a comment.
This revision is now accepted and ready to land.

update per @steakhal's latest comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cast-value-notes.cpp

Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -4,8 +4,21 @@
 //
 // RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
 // RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -DSUPPRESS_ADDRESS_SPACES\
 // RUN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
 
+// RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-config core.NullDereference:SuppressAddressSpaces=false -analyzer-output=text\
+// RUN: -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s\
+// RUN: -check-prefix=AMDGCN-CHECK-WARNADDRSPACE
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-unknown-unknown\
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-config core.NullDereference:SuppressAddressSpaces=false -analyzer-output=text\
+// RUN: -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s\
+// RUN: -check-prefix=DEFAULT-CHECK
+
 #include "Inputs/llvm.h"
 
 // The amggcn triple case uses an intentionally different address space.
@@ -48,6 +61,7 @@
   (void)C;
 }
 #elif defined(AMDGCN_TRIPLE)
+#if defined(SUPPRESS_ADDRESS_SPACES)
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
   clang_analyzer_printState();
@@ -56,6 +70,18 @@
   (void)C;
 }
 #else
+void evalReferences(const Shape ) {
+  const auto  = dyn_cast(S);
+  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-2 {{Dereference of null pointer}}
+  // expected-warning@-3 {{Dereference of null pointer}}
+  clang_analyzer_printState();
+  // AMDGCN-CHECK-WARNADDRSPACE: "dynamic_types": [
+  // AMDGCN-CHECK-WARNADDRSPACE-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  (void)C;
+}
+#endif
+#else
 #error Target must be specified, and must be pinned
 #endif
 
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -41,6 +41,7 @@
 // CHECK-NEXT: core.CallAndMessage:NilReceiver = true
 // CHECK-NEXT: core.CallAndMessage:ParameterCount = true
 // CHECK-NEXT: core.CallAndMessage:UndefReceiver = true
+// CHECK-NEXT: core.NullDereference:SuppressAddressSpaces = true
 // CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: cplusplus.SmartPtrModeling:ModelSmartPtrDereference = false
 // CHECK-NEXT: crosscheck-with-z3 = false
Index: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -11,9 +11,10 @@
 //
 //===--===//
 
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
@@ -39,6 +40,8 @@
   void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S,
  CheckerContext ) const;
 
+  bool suppressReport(CheckerContext , const Expr *E) const;
+
 public:
   void checkLocation(SVal location, bool isLoad, const Stmt* S,
  CheckerContext ) const;
@@ -49,6 +52,8 @@
  const Expr *Ex, const ProgramState *state,
  const LocationContext *LCtx,
  bool loadedFrom = false);
+
+  DefaultBool SuppressAddressSpaces;
 };
 } // end anonymous namespace
 
@@ -109,9 +114,35 @@
   return E;
 }
 
-static bool suppressReport(const Expr *E) {
-  // Do not report dereferences on memory in non-default address spaces.
-  return E->getType().hasAddressSpace();
+bool DereferenceChecker::suppressReport(CheckerContext ,
+ 

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers planned changes to this revision.
vabridgers added a comment.

Thanks for the the comments. I'll make the changes and resubmit. Please let me 
know your preferences for the test case (default vs pinned x86 or same).

Best!




Comment at: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp:331
+  auto *Chk = mgr.registerChecker();
+  // auto *Chk = mgr.getChecker();
+  Chk->DetectAllNullDereferences =

steakhal wrote:
> steakhal wrote:
> > dead-code?
> This still looks dead.
arg, missed that. I'll clean this up.



Comment at: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp:56
+
+  bool SuppressAddressSpaces = false;
 };

steakhal wrote:
> If I'm correct, we tend to use the 'DefaultBool' instead of plain old bools 
> for checker options. Could you please check if this is the case and 
> consolidate this?
I'll address. FYI, looks like I chose the red pill when I found an example to 
go by :/  Looks like there's opportunity for some cleanup NFC type of patches. 
I'll pick these up in a separate patch. 


```
120 namespace {
121 class DeadStoresChecker : public Checker {
122 public:
123   bool ShowFixIts = false;
124   bool WarnForDeadNestedAssignments = true;
125 
126   void checkASTCodeBody(const Decl *D, AnalysisManager ,
127 BugReporter ) const;
128 };

```



Comment at: clang/test/Analysis/cast-value-notes.cpp:20
+// RUN: -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s\
+// RUN: -check-prefix=DEFAULT-CHECK
+

steakhal wrote:
> Nit: the default triple is the host/native triple, but you did actually pin 
> it to x86. Hence it might not be the default on other platforms.
Yeah, the result is the same (as far as what the test code would look like). 
Not sure what your preferences are here. 

Would you prefer to see separate cases (default and pinned x86), or perhaps the 
macro names renamed to something that means both (instead of implying default)? 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

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

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers marked 3 inline comments as done.
vabridgers added a comment.

I believe all points have been addressed. Thanks for the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

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

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 421706.
vabridgers added a comment.

update per @steakhal's comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cast-value-notes.cpp

Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -4,8 +4,21 @@
 //
 // RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
 // RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -DSUPPRESS_ADDRESS_SPACES\
 // RUN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
 
+// RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-config core.NullDereference:SuppressAddressSpaces=false -analyzer-output=text\
+// RUN: -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s\
+// RUN: -check-prefix=AMDGCN-CHECK-WARNADDRSPACE
+
+// RUN: %clang_analyze_cc1 -std=c++14 -triple x86_64-unknown-unknown\
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-config core.NullDereference:SuppressAddressSpaces=false -analyzer-output=text\
+// RUN: -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s\
+// RUN: -check-prefix=DEFAULT-CHECK
+
 #include "Inputs/llvm.h"
 
 // The amggcn triple case uses an intentionally different address space.
@@ -48,6 +61,7 @@
   (void)C;
 }
 #elif defined(AMDGCN_TRIPLE)
+#if defined(SUPPRESS_ADDRESS_SPACES)
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
   clang_analyzer_printState();
@@ -56,6 +70,18 @@
   (void)C;
 }
 #else
+void evalReferences(const Shape ) {
+  const auto  = dyn_cast(S);
+  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-2 {{Dereference of null pointer}}
+  // expected-warning@-3 {{Dereference of null pointer}}
+  clang_analyzer_printState();
+  // AMDGCN-CHECK-WARNADDRSPACE: "dynamic_types": [
+  // AMDGCN-CHECK-WARNADDRSPACE-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  (void)C;
+}
+#endif
+#else
 #error Target must be specified, and must be pinned
 #endif
 
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -41,6 +41,7 @@
 // CHECK-NEXT: core.CallAndMessage:NilReceiver = true
 // CHECK-NEXT: core.CallAndMessage:ParameterCount = true
 // CHECK-NEXT: core.CallAndMessage:UndefReceiver = true
+// CHECK-NEXT: core.NullDereference:SuppressAddressSpaces = true
 // CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: cplusplus.SmartPtrModeling:ModelSmartPtrDereference = false
 // CHECK-NEXT: crosscheck-with-z3 = false
Index: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -11,9 +11,10 @@
 //
 //===--===//
 
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/AST/ExprOpenMP.h"
+#include "clang/Basic/TargetInfo.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
@@ -39,6 +40,8 @@
   void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S,
  CheckerContext ) const;
 
+  bool suppressReport(CheckerContext , const Expr *E) const;
+
 public:
   void checkLocation(SVal location, bool isLoad, const Stmt* S,
  CheckerContext ) const;
@@ -49,6 +52,8 @@
  const Expr *Ex, const ProgramState *state,
  const LocationContext *LCtx,
  bool loadedFrom = false);
+
+  bool SuppressAddressSpaces = false;
 };
 } // end anonymous namespace
 
@@ -109,9 +114,35 @@
   return E;
 }
 
-static bool suppressReport(const Expr *E) {
-  // Do not report dereferences on memory in non-default address spaces.
-  return E->getType().hasAddressSpace();
+bool DereferenceChecker::suppressReport(CheckerContext ,
+const Expr *E) const {
+  // Do not report 

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I'll address, thanks for the comments.




Comment at: clang/docs/analyzer/checkers.rst:69-74
+Check for dereferences of null pointers. This checker specifically does
+not report null pointer dereferences for x86 and x86-64 targets when the
+address space is 256 (x86 GS Segment), 257 (x86 FS Segment), or 258 (x86 SS
+segment). See `X86/X86-64 Language Extensions
+`__
+for reference.

steakhal wrote:
> IMO this should be in a separate paragraph. It's such a niche use-case.
I'll address.



Comment at: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp:119
+  // and #258. Those address spaces are used when dereferencing address spaces
+  // relative to the GS, FS, and SS segments on x86/x86-64 targets.
+  // Dereferencing a null pointer in these address spaces is not defined

steakhal wrote:
> Shouldn't we check for this target?
> Add a test if we have `x86` and a non-`x86` target.
I wasn't sure it was needed, but easy enough to add I think. I'll address both. 



Comment at: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp:125-133
+  if (isTargetAddressSpace(E->getType().getAddressSpace())) {
+switch (toTargetAddressSpace(E->getType().getAddressSpace())) {
+case 256:
+case 257:
+case 258:
+  return true;
+}

steakhal wrote:
> Even though `DetectAllNullDereferences=true`, `address_space(256)` would be 
> suppressed, which seems to be a contradiction to me.
> 
> We should either pick a different name or implement a different semantic.
> I would vote for both, personally.
> 
> Then name should encode that it's purely about address-space attributes.
> The implementation should check if we have attributes or not, and bail out in 
> the generic case. I would do something like this:
> 
> ```
> QualType Ty = E->getType();
> if (!Ty.hasAddressSpace())
>   return false;
> if (SuppressAddressSpaces)
>   return true;
> // On X86 addr spaces , thus ignored.
> return target == x86 && addressspace in {256,257,258};
> ```
I'll clean up the semantics. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

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

[PATCH] D122841: [analyzer] Add option for AddrSpace in core.NullDereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 421521.
vabridgers edited the summary of this revision.
vabridgers added a comment.

clean up a few debug edits


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cast-value-notes.cpp

Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -6,6 +6,12 @@
 // RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
 // RUN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
 
+// RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
+// RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// RUN: -analyzer-config core.NullDereference:DetectAllNullDereferences=true -analyzer-output=text\
+// RUN: -verify -DDETECT_ALL_NULL_DEREFERENCES  -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s\
+// RUN: -check-prefix=AMDGCN-CHECK-WARNADDRSPACE
+
 #include "Inputs/llvm.h"
 
 // The amggcn triple case uses an intentionally different address space.
@@ -48,6 +54,18 @@
   (void)C;
 }
 #elif defined(AMDGCN_TRIPLE)
+#if defined(DETECT_ALL_NULL_DEREFERENCES)
+void evalReferences(const Shape ) {
+  const auto  = dyn_cast(S);
+  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-2 {{Dereference of null pointer}}
+  // expected-warning@-3 {{Dereference of null pointer}}
+  clang_analyzer_printState();
+  // AMDGCN-CHECK-WARNADDRSPACE: "dynamic_types": [
+  // AMDGCN-CHECK-WARNADDRSPACE-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  (void)C;
+}
+#else
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
   clang_analyzer_printState();
@@ -55,6 +73,7 @@
   // AMDGCN-CHECK-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
   (void)C;
 }
+#endif
 #else
 #error Target must be specified, and must be pinned
 #endif
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -41,6 +41,7 @@
 // CHECK-NEXT: core.CallAndMessage:NilReceiver = true
 // CHECK-NEXT: core.CallAndMessage:ParameterCount = true
 // CHECK-NEXT: core.CallAndMessage:UndefReceiver = true
+// CHECK-NEXT: core.NullDereference:DetectAllNullDereferences = false
 // CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: cplusplus.SmartPtrModeling:ModelSmartPtrDereference = false
 // CHECK-NEXT: crosscheck-with-z3 = false
Index: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -39,6 +39,8 @@
   void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S,
  CheckerContext ) const;
 
+  bool suppressReport(const Expr *E) const;
+
 public:
   void checkLocation(SVal location, bool isLoad, const Stmt* S,
  CheckerContext ) const;
@@ -49,6 +51,8 @@
  const Expr *Ex, const ProgramState *state,
  const LocationContext *LCtx,
  bool loadedFrom = false);
+
+  bool DetectAllNullDereferences = false;
 };
 } // end anonymous namespace
 
@@ -109,9 +113,24 @@
   return E;
 }
 
-static bool suppressReport(const Expr *E) {
-  // Do not report dereferences on memory in non-default address spaces.
-  return E->getType().hasAddressSpace();
+bool DereferenceChecker::suppressReport(const Expr *E) const {
+  // Do not report dereferences on memory that use address space #256, #257,
+  // and #258. Those address spaces are used when dereferencing address spaces
+  // relative to the GS, FS, and SS segments on x86/x86-64 targets.
+  // Dereferencing a null pointer in these address spaces is not defined
+  // as an error. All other null dereferences in other address spaces
+  // are defined as an error unless explicitly defined.
+  // See https://clang.llvm.org/docs/LanguageExtensions.html, the section
+  // "X86/X86-64 Language Extensions"
+  if (isTargetAddressSpace(E->getType().getAddressSpace())) {
+switch (toTargetAddressSpace(E->getType().getAddressSpace())) {
+case 256:
+case 257:
+case 258:
+  return true;
+}
+  }
+  return (E->getType().hasAddressSpace() && !DetectAllNullDereferences);
 

[PATCH] D122841: [analyzer] Consider all addrspaces in null dereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 421519.
vabridgers added a comment.

add option, retain legacy behavior per comments. update rst


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/cast-value-notes.cpp

Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -1,10 +1,16 @@
-// RUN: %clang_analyze_cc1 -std=c++14 \
-// RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
-// RUN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
+// R UN: %clang_analyze_cc1 -std=c++14 \
+// R UN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// R UN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
 //
+// R UN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
+// R UN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// R UN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
+
 // RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
 // RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
-// RUN: -analyzer-output=text -verify -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=AMDGCN-CHECK
+// RUN: -analyzer-config core.NullDereference:DetectAllNullDereferences=true -analyzer-output=text\
+// RUN: -verify -DDETECT_ALL_NULL_DEREFERENCES  -DAMDGCN_TRIPLE %s 2>&1 | FileCheck %s\
+// RUN: -check-prefix=AMDGCN-CHECK-WARNADDRSPACE
 
 #include "Inputs/llvm.h"
 
@@ -48,6 +54,18 @@
   (void)C;
 }
 #elif defined(AMDGCN_TRIPLE)
+#if defined(DETECT_ALL_NULL_DEREFERENCES)
+void evalReferences(const Shape ) {
+  const auto  = dyn_cast(S);
+  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-2 {{Dereference of null pointer}}
+  // expected-warning@-3 {{Dereference of null pointer}}
+  clang_analyzer_printState();
+  // AMDGCN-CHECK-WARNADDRSPACE: "dynamic_types": [
+  // AMDGCN-CHECK-WARNADDRSPACE-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
+  (void)C;
+}
+#else
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
   clang_analyzer_printState();
@@ -55,6 +73,7 @@
   // AMDGCN-CHECK-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
   (void)C;
 }
+#endif
 #else
 #error Target must be specified, and must be pinned
 #endif
Index: clang/test/Analysis/analyzer-config.c
===
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -41,6 +41,7 @@
 // CHECK-NEXT: core.CallAndMessage:NilReceiver = true
 // CHECK-NEXT: core.CallAndMessage:ParameterCount = true
 // CHECK-NEXT: core.CallAndMessage:UndefReceiver = true
+// CHECK-NEXT: core.NullDereference:DetectAllNullDereferences = false
 // CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
 // CHECK-NEXT: cplusplus.SmartPtrModeling:ModelSmartPtrDereference = false
 // CHECK-NEXT: crosscheck-with-z3 = false
Index: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -39,6 +39,8 @@
   void reportBug(DerefKind K, ProgramStateRef State, const Stmt *S,
  CheckerContext ) const;
 
+  bool suppressReport(const Expr *E) const;
+
 public:
   void checkLocation(SVal location, bool isLoad, const Stmt* S,
  CheckerContext ) const;
@@ -49,6 +51,8 @@
  const Expr *Ex, const ProgramState *state,
  const LocationContext *LCtx,
  bool loadedFrom = false);
+
+  bool DetectAllNullDereferences = false;
 };
 } // end anonymous namespace
 
@@ -109,9 +113,24 @@
   return E;
 }
 
-static bool suppressReport(const Expr *E) {
-  // Do not report dereferences on memory in non-default address spaces.
-  return E->getType().hasAddressSpace();
+bool DereferenceChecker::suppressReport(const Expr *E) const {
+  // Do not report dereferences on memory that use address space #256, #257,
+  // and #258. Those address spaces are used when dereferencing address spaces
+  // relative to the GS, FS, and SS segments on x86/x86-64 targets.
+  // Dereferencing a null pointer in 

[PATCH] D122841: [analyzer] Consider all addrspaces in null dereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Ahhh, I see - thanks for the references and discussion. I'll craft a patch to 
comprehend the use case and user-facing option, and post for review. We'll see 
how it goes :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

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

[PATCH] D122841: [analyzer] Consider all addrspaces in null dereference check

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

@steakhal, good question. Digging into the change archaeology, I see this 
change was made by Anna Zaks in Jan 2016 - see commit header below.

There's no reference to a review with comments, and I don't know how to find 
those if they exist, perhaps @NoQ could help with that?

In any case, judging by the changes I had to make to the test cases, there were 
no regressions in the current test set, and at least our build tests ran ok - 
seems like this is no longer necessary. But I understand the need to approach 
this carefully.

Thanks

  commit c9f16fe48c40e7683d0029b65090d0007414d7af
  Author: Anna Zaks 
  Date:   Wed Jan 6 00:32:49 2016 +
  
  [analyzer] Don't report null dereferences on address_space annotated 
memory
  
  llvm-svn: 256885


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122841

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

[PATCH] D122841: [analyzer] Consider all addrspaces in null dereference check

[Vince Bridgers via Phabricator via cfe-commits]

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

This change enables the null dereference checker to consider all null
dereferences, even those with address space qualifiers. The original
checker did not check null dereferences with address space qualifiers
for some reason. There's no reason to not consider all null
dereferences.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122841

Files:
  clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
  clang/test/Analysis/cast-value-notes.cpp
  clang/test/Analysis/null-deref-ps.c
  clang/test/Analysis/nullptr.cpp

Index: clang/test/Analysis/nullptr.cpp
===
--- clang/test/Analysis/nullptr.cpp
+++ clang/test/Analysis/nullptr.cpp
@@ -160,16 +160,19 @@
 public:
   int x;
   ~AS1() {
-int AS_ATTRIBUTE *x = 0;
-*x = 3; // no-warning
+int AS_ATTRIBUTE *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+*x = 3;  // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
   }
 };
 void test_address_space_field_access() {
-  AS1 AS_ATTRIBUTE *pa = 0;
-  pa->x = 0; // no-warning
+  AS1 AS_ATTRIBUTE *pa = 0; // expected-note{{'pa' initialized to a null pointer value}}
+  pa->x = 0;// expected-warning{{Access to field 'x' results in a dereference of a null pointer (loaded from variable 'pa')}}
+// expected-note@-1{{Access to field 'x' results in a dereference of a null pointer (loaded from variable 'pa')}}
 }
 void test_address_space_bind() {
-  AS1 AS_ATTRIBUTE *pa = 0;
-  AS1 AS_ATTRIBUTE  = *pa;
+  AS1 AS_ATTRIBUTE *pa = 0;  // expected-note{{'pa' initialized to a null pointer value}}
+  AS1 AS_ATTRIBUTE  = *pa; // expected-warning{{Dereference of null pointer}}
+ // expected-note@-1{{Dereference of null pointer}}
   r.x = 0; // no-warning
 }
Index: clang/test/Analysis/null-deref-ps.c
===
--- clang/test/Analysis/null-deref-ps.c
+++ clang/test/Analysis/null-deref-ps.c
@@ -315,17 +315,17 @@
 #define AS_ATTRIBUTE volatile __attribute__((address_space(256)))
 #define _get_base() ((void * AS_ATTRIBUTE *)0)
 void* test_address_space_array(unsigned long slot) {
-  return _get_base()[slot]; // no-warning
+  return _get_base()[slot]; // expected-warning{{Array access results in a null pointer dereference}}
 }
 void test_address_space_condition(int AS_ATTRIBUTE *cpu_data) {
if (cpu_data == 0) {
-*cpu_data = 3; // no-warning
+ *cpu_data = 3; // expected-warning{{Dereference of null pointer}}
   }
 }
 struct X { int member; };
 int test_address_space_member(void) {
   struct X AS_ATTRIBUTE *data = (struct X AS_ATTRIBUTE *)0UL;
   int ret;
-  ret = data->member; // no-warning
+  ret = data->member; // expected-warning{{Access to field 'member' results in a dereference of a null pointer}}
   return ret;
 }
Index: clang/test/Analysis/cast-value-notes.cpp
===
--- clang/test/Analysis/cast-value-notes.cpp
+++ clang/test/Analysis/cast-value-notes.cpp
@@ -1,6 +1,6 @@
-// RUN: %clang_analyze_cc1 -std=c++14 \
-// RUN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
-// RUN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
+// R UN: %clang_analyze_cc1 -std=c++14 \
+// R UN:  -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
+// R UN:  -analyzer-output=text -verify -DDEFAULT_TRIPLE %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-CHECK
 //
 // RUN: %clang_analyze_cc1 -std=c++14 -triple amdgcn-unknown-unknown \
 // RUN: -analyzer-checker=core,apiModeling.llvm.CastValue,debug.ExprInspection\
@@ -50,6 +50,9 @@
 #elif defined(AMDGCN_TRIPLE)
 void evalReferences(const Shape ) {
   const auto  = dyn_cast(S);
+  // expected-note@-1 {{Assuming 'S' is not a 'Circle'}}
+  // expected-note@-2 {{Dereference of null pointer}}
+  // expected-warning@-3 {{Dereference of null pointer}}
   clang_analyzer_printState();
   // AMDGCN-CHECK: "dynamic_types": [
   // AMDGCN-CHECK-NEXT: { "region": "SymRegion{reg_$0}", "dyn_type": "const __attribute__((address_space(3))) class clang::Circle &", "sub_classable": true }
Index: clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp
@@ -109,11 

[PATCH] D118050: [analyzer] Different address spaces cannot overlap

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 419462.
vabridgers added a comment.
This revision is now accepted and ready to land.

make assert function void, update summary


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118050

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/cstring-addrspace.c

Index: clang/test/Analysis/cstring-addrspace.c
===
--- /dev/null
+++ clang/test/Analysis/cstring-addrspace.c
@@ -0,0 +1,64 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core,alpha.unix.cstring \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config crosscheck-with-z3=true -verify %s \
+// RUN: -Wno-incompatible-library-redeclaration
+// REQUIRES: z3
+
+void clang_analyzer_warnIfReached();
+
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+#define DEVICE __attribute__((address_space(3)))
+_Static_assert(sizeof(int *) == 8, "");
+_Static_assert(sizeof(DEVICE int *) == 4, "");
+_Static_assert(sizeof(void *) == 8, "");
+_Static_assert(sizeof(DEVICE void *) == 4, "");
+
+// Copy from host to device memory. Note this is specialized
+// since one of the parameters is assigned an address space such
+// that the sizeof the the pointer is different than the other.
+//
+// Some downstream implementations may have specialized memcpy
+// routines that copy from one address space to another. In cases
+// like that, the address spaces are assumed to not overlap, so the
+// cstring overlap check is not needed. When a static analysis report
+// is generated in as case like this, SMTConv may attempt to create
+// a refutation to Z3 with different bitwidth pointers which lead to
+// a crash. This is not common in directly used upstream compiler builds,
+// but can be seen in specialized downstrean implementations. This case
+// covers those specific instances found and debugged.
+//
+// Since memcpy is a builtin, a specialized builtin instance named like
+// 'memcpy_special' will hit in cstring, triggering this behavior. The
+// best we can do for an upstream test is use the same memcpy function name.
+DEVICE void *memcpy(DEVICE void *dst, const void *src, unsigned long len);
+
+void top1(DEVICE void *dst, void *src, int len) {
+  memcpy(dst, src, len);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void top2(DEVICE int *dst, void *src, int len) {
+  memcpy(dst, src, len);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void top3(DEVICE int *dst, int *src, int len) {
+  memcpy(dst, src, len);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void top4() {
+  memcpy((DEVICE void *)1, (const void *)1, 1);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -715,11 +715,41 @@
   llvm_unreachable("Fields not found in parent record's definition");
 }
 
+// This is used in debug builds only for now because some downstream users
+// may hit this assert in their subsequent merges.
+// There are still places in the analyzer where equal bitwidth Locs
+// are compared, and need to be found and corrected. Recent previous fixes have
+// addressed the known problems of making NULLs with specific bitwidths
+// for Loc comparisons along with deprecation of APIs for the same purpose.
+//
+static void assertEqualBitWidths(ProgramStateRef State, Loc RhsLoc,
+ Loc LhsLoc) {
+  // Implements a "best effort" check for RhsLoc and LhsLoc bit widths
+  ASTContext  = State->getStateManager().getContext();
+  uint64_t RhsBitwidth =
+  RhsLoc.getType(Ctx).isNull() ? 0 : Ctx.getTypeSize(RhsLoc.getType(Ctx));
+  uint64_t LhsBitwidth =
+  LhsLoc.getType(Ctx).isNull() ? 0 : Ctx.getTypeSize(LhsLoc.getType(Ctx));
+  if (RhsBitwidth && LhsBitwidth &&
+  (LhsLoc.getSubKind() == RhsLoc.getSubKind())) {
+assert(RhsBitwidth == LhsBitwidth &&
+   "RhsLoc and LhsLoc bitwidth must be same!");
+  }
+}
+
 // FIXME: all this logic will change if/when we have MemRegion::getLocation().
 SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state,
   BinaryOperator::Opcode op,
   Loc 

[PATCH] D118050: [analyzer] Different address spaces cannot overlap

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers planned changes to this revision.
vabridgers added a comment.

I need to make a few changes. Please hold off any comments until the next 
patch, coming soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118050

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

[PATCH] D118050: [analyzer] Different address spaces cannot overlap

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I added #ifndef NDEBUG around the assert since I'm not sure if downstream 
consumers that use different pointer sizes by address space will hit this 
assert. Our downstream target causes this assert to fire, so I will be 
continuing to find and fix these issues, submitting test cases as I can. Heck, 
we may even have to submit a mock target to properly suss these out :)

Thanks for the support for sorting these out. Best


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118050

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

[PATCH] D118050: [analyzer] Different address spaces cannot overlap

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 419438.
vabridgers added a comment.

add NDEBUG around assert


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118050

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/cstring-addrspace.c

Index: clang/test/Analysis/cstring-addrspace.c
===
--- /dev/null
+++ clang/test/Analysis/cstring-addrspace.c
@@ -0,0 +1,64 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core,alpha.unix.cstring \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config crosscheck-with-z3=true -verify %s \
+// RUN: -Wno-incompatible-library-redeclaration
+// REQUIRES: z3
+
+void clang_analyzer_warnIfReached();
+
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+#define DEVICE __attribute__((address_space(3)))
+_Static_assert(sizeof(int *) == 8, "");
+_Static_assert(sizeof(DEVICE int *) == 4, "");
+_Static_assert(sizeof(void *) == 8, "");
+_Static_assert(sizeof(DEVICE void *) == 4, "");
+
+// Copy from host to device memory. Note this is specialized
+// since one of the parameters is assigned an address space such
+// that the sizeof the the pointer is different than the other.
+//
+// Some downstream implementations may have specialized memcpy
+// routines that copy from one address space to another. In cases
+// like that, the address spaces are assumed to not overlap, so the
+// cstring overlap check is not needed. When a static analysis report
+// is generated in as case like this, SMTConv may attempt to create
+// a refutation to Z3 with different bitwidth pointers which lead to
+// a crash. This is not common in directly used upstream compiler builds,
+// but can be seen in specialized downstrean implementations. This case
+// covers those specific instances found and debugged.
+//
+// Since memcpy is a builtin, a specialized builtin instance named like
+// 'memcpy_special' will hit in cstring, triggering this behavior. The
+// best we can do for an upstream test is use the same memcpy function name.
+DEVICE void *memcpy(DEVICE void *dst, const void *src, unsigned long len);
+
+void top1(DEVICE void *dst, void *src, int len) {
+  memcpy(dst, src, len);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void top2(DEVICE int *dst, void *src, int len) {
+  memcpy(dst, src, len);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void top3(DEVICE int *dst, int *src, int len) {
+  memcpy(dst, src, len);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void top4() {
+  memcpy((DEVICE void *)1, (const void *)1, 1);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -715,11 +715,47 @@
   llvm_unreachable("Fields not found in parent record's definition");
 }
 
+#ifndef NDEBUG
+// This is used in debug builds only for now because some downstream users
+// may hit this assert in their subsequent merges.
+// There are still places in the analyzer where equal bitwidth Locs
+// are compared, and need to be found and corrected. Recent previous fixes have
+// addressed the known problems of making NULLs with specific bitwidths
+// for Loc comparisons along with deprecation of APIs for the same purpose.
+//
+static bool assertEqualBitWidths(ProgramStateRef State, Loc RhsLoc,
+ Loc LhsLoc) {
+  // Implements a "best effort" check for RhsLoc and LhsLoc bit widths
+  ASTContext  = State->getStateManager().getContext();
+  uint64_t RhsBitwidth =
+  RhsLoc.getType(Ctx).isNull() ? 0 : Ctx.getTypeSize(RhsLoc.getType(Ctx));
+  uint64_t LhsBitwidth =
+  LhsLoc.getType(Ctx).isNull() ? 0 : Ctx.getTypeSize(LhsLoc.getType(Ctx));
+  if (RhsBitwidth && LhsBitwidth &&
+  (LhsLoc.getSubKind() == RhsLoc.getSubKind())) {
+assert(RhsBitwidth == LhsBitwidth &&
+   "RhsLoc and LhsLoc bitwidth must be same!");
+return RhsBitwidth == LhsBitwidth;
+  }
+  return false;
+}
+#endif
+
 // FIXME: all this logic will change if/when we have MemRegion::getLocation().
 SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state,
   BinaryOperator::Opcode op,
  

[PATCH] D118050: [analyzer] Different address spaces cannot overlap

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 419013.
vabridgers added a comment.
Herald added a project: All.

rebase, check ci


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118050

Files:
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/cstring-addrspace.c

Index: clang/test/Analysis/cstring-addrspace.c
===
--- /dev/null
+++ clang/test/Analysis/cstring-addrspace.c
@@ -0,0 +1,64 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core,alpha.unix.cstring \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -analyzer-config crosscheck-with-z3=true -verify %s \
+// RUN: -Wno-incompatible-library-redeclaration
+// REQUIRES: z3
+
+void clang_analyzer_warnIfReached();
+
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+#define DEVICE __attribute__((address_space(3)))
+_Static_assert(sizeof(int *) == 8, "");
+_Static_assert(sizeof(DEVICE int *) == 4, "");
+_Static_assert(sizeof(void *) == 8, "");
+_Static_assert(sizeof(DEVICE void *) == 4, "");
+
+// Copy from host to device memory. Note this is specialized
+// since one of the parameters is assigned an address space such
+// that the sizeof the the pointer is different than the other.
+//
+// Some downstream implementations may have specialized memcpy
+// routines that copy from one address space to another. In cases
+// like that, the address spaces are assumed to not overlap, so the
+// cstring overlap check is not needed. When a static analysis report
+// is generated in as case like this, SMTConv may attempt to create
+// a refutation to Z3 with different bitwidth pointers which lead to
+// a crash. This is not common in directly used upstream compiler builds,
+// but can be seen in specialized downstrean implementations. This case
+// covers those specific instances found and debugged.
+//
+// Since memcpy is a builtin, a specialized builtin instance named like
+// 'memcpy_special' will hit in cstring, triggering this behavior. The
+// best we can do for an upstream test is use the same memcpy function name.
+DEVICE void *memcpy(DEVICE void *dst, const void *src, unsigned long len);
+
+void top1(DEVICE void *dst, void *src, int len) {
+  memcpy(dst, src, len);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void top2(DEVICE int *dst, void *src, int len) {
+  memcpy(dst, src, len);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void top3(DEVICE int *dst, int *src, int len) {
+  memcpy(dst, src, len);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void top4() {
+  memcpy((DEVICE void *)1, (const void *)1, 1);
+
+  // Create a bugreport for triggering Z3 refutation.
+  clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -715,11 +715,35 @@
   llvm_unreachable("Fields not found in parent record's definition");
 }
 
+static bool assertEqualBitWidths(ProgramStateRef State, Loc RhsLoc,
+ Loc LhsLoc) {
+  // Implements a "best effort" check for RhsLoc and LhsLoc bit widths
+  ASTContext  = State->getStateManager().getContext();
+  uint64_t RhsBitwidth =
+  RhsLoc.getType(Ctx).isNull() ? 0 : Ctx.getTypeSize(RhsLoc.getType(Ctx));
+  uint64_t LhsBitwidth =
+  LhsLoc.getType(Ctx).isNull() ? 0 : Ctx.getTypeSize(LhsLoc.getType(Ctx));
+  if (RhsBitwidth && LhsBitwidth &&
+  (LhsLoc.getSubKind() == RhsLoc.getSubKind())) {
+assert(RhsBitwidth == LhsBitwidth &&
+   "RhsLoc and LhsLoc bitwidth must be same!");
+return RhsBitwidth == LhsBitwidth;
+  }
+  return false;
+}
+
 // FIXME: all this logic will change if/when we have MemRegion::getLocation().
 SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state,
   BinaryOperator::Opcode op,
   Loc lhs, Loc rhs,
   QualType resultTy) {
+
+  // Assert that bitwidth of lhs and rhs are the same.
+  // This can happen if two different address spaces are used,
+  // and the bitwidths of the address spaces are different.
+  // See LIT case clang/test/Analysis/cstring-checker-addressspace.c
+  assertEqualBitWidths(state, rhs, lhs);
+
   // Only comparisons and subtractions are valid 

[PATCH] D122513: [analyzer] Fix "RhsLoc and LhsLoc bitwidth must be same"

[Vince Bridgers via Phabricator via cfe-commits]

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



Comment at: clang/test/Analysis/addrspace-null.c:19
+
+#if defined(AMDGCN)
+// this crashes

NoQ wrote:
> Shouldn't this be `AMDGCN_TRIPLE`?
fixed, thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122513

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

[PATCH] D122513: [analyzer] Fix "RhsLoc and LhsLoc bitwidth must be same"

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 418996.
vabridgers added a comment.

fix typo in test - ready to land


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122513

Files:
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/addrspace-null.c


Index: clang/test/Analysis/addrspace-null.c
===
--- /dev/null
+++ clang/test/Analysis/addrspace-null.c
@@ -0,0 +1,47 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DAMDGCN_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+//
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DDEFAULT_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+
+// expected-no-diagnostics
+
+#define DEVICE __attribute__((address_space(3)))
+
+#if defined(AMDGCN_TRIPLE)
+// this crashes
+int fn1() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (void *)0;
+}
+
+// does not crash
+int fn2() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+
+// this crashes
+int fn3() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+#endif
+
+// does not crash
+int fn4() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (void *)0;
+}
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -682,8 +682,11 @@
   }
 
   // Pointer to any pointer.
-  if (Loc::isLocType(CastTy))
-return V;
+  if (Loc::isLocType(CastTy)) {
+llvm::APSInt Value = V.getValue();
+BasicVals.getAPSIntType(CastTy).apply(Value);
+return loc::ConcreteInt(BasicVals.getValue(Value));
+  }
 
   // Pointer to whatever else.
   return UnknownVal();


Index: clang/test/Analysis/addrspace-null.c
===
--- /dev/null
+++ clang/test/Analysis/addrspace-null.c
@@ -0,0 +1,47 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DAMDGCN_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+//
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DDEFAULT_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+
+// expected-no-diagnostics
+
+#define DEVICE __attribute__((address_space(3)))
+
+#if defined(AMDGCN_TRIPLE)
+// this crashes
+int fn1() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (void *)0;
+}
+
+// does not crash
+int fn2() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+
+// this crashes
+int fn3() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+#endif
+
+// does not crash
+int fn4() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (void *)0;
+}
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -682,8 +682,11 @@
   }
 
   // Pointer to any pointer.
-  if (Loc::isLocType(CastTy))
-return V;
+  if (Loc::isLocType(CastTy)) {
+llvm::APSInt Value = V.getValue();
+BasicVals.getAPSIntType(CastTy).apply(Value);
+return loc::ConcreteInt(BasicVals.getValue(Value));
+  }
 
   // Pointer to whatever else.
   return UnknownVal();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122513: [analyzer] Fix "RhsLoc and LhsLoc bitwidth must be same"

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 418749.
vabridgers added a comment.

Come up with a more principaled fix, thanks @NoQ :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122513

Files:
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/addrspace-null.c


Index: clang/test/Analysis/addrspace-null.c
===
--- /dev/null
+++ clang/test/Analysis/addrspace-null.c
@@ -0,0 +1,47 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DAMDGCN_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+//
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DDEFAULT_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+
+// expected-no-diagnostics
+
+#define DEVICE __attribute__((address_space(3)))
+
+#if defined(AMDGCN)
+// this crashes
+int fn1() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (void *)0;
+}
+
+// does not crash
+int fn2() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+
+// this crashes
+int fn3() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+#endif
+
+// does not crash
+int fn4() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (void *)0;
+}
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -682,8 +682,11 @@
   }
 
   // Pointer to any pointer.
-  if (Loc::isLocType(CastTy))
-return V;
+  if (Loc::isLocType(CastTy)) {
+llvm::APSInt Value = V.getValue();
+BasicVals.getAPSIntType(CastTy).apply(Value);
+return loc::ConcreteInt(BasicVals.getValue(Value));
+  }
 
   // Pointer to whatever else.
   return UnknownVal();


Index: clang/test/Analysis/addrspace-null.c
===
--- /dev/null
+++ clang/test/Analysis/addrspace-null.c
@@ -0,0 +1,47 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DAMDGCN_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+//
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DDEFAULT_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+
+// expected-no-diagnostics
+
+#define DEVICE __attribute__((address_space(3)))
+
+#if defined(AMDGCN)
+// this crashes
+int fn1() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (void *)0;
+}
+
+// does not crash
+int fn2() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+
+// this crashes
+int fn3() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+#endif
+
+// does not crash
+int fn4() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (void *)0;
+}
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -682,8 +682,11 @@
   }
 
   // Pointer to any pointer.
-  if (Loc::isLocType(CastTy))
-return V;
+  if (Loc::isLocType(CastTy)) {
+llvm::APSInt Value = V.getValue();
+BasicVals.getAPSIntType(CastTy).apply(Value);
+return loc::ConcreteInt(BasicVals.getValue(Value));
+  }
 
   // Pointer to whatever else.
   return UnknownVal();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122513: [analyzer] Fix "RhsLoc and LhsLoc bitwidth must be same"

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

I think I got it, looks like we're losing the width information at line 685, 
686 below.

Looks like I need to adjust the bitwidth of V before returning.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

  671 SVal SValBuilder::evalCastSubKind(loc::ConcreteInt V, QualType CastTy,
  672   QualType OriginalTy) {
  673   // Pointer to bool.
  674   if (CastTy->isBooleanType())
  675 return makeTruthVal(V.getValue().getBoolValue(), CastTy);
  676 
  677   // Pointer to integer.
  678   if (CastTy->isIntegralOrEnumerationType()) {
  679 llvm::APSInt Value = V.getValue();
  680 BasicVals.getAPSIntType(CastTy).apply(Value);
  681 return makeIntVal(Value);
  682   }
  683 
  684   // Pointer to any pointer.
  685   if (Loc::isLocType(CastTy))
  686 return V;
  687 
  688   // Pointer to whatever else.
  689   return UnknownVal();
  690 }



  Breakpoint 8, clang::ento::SValBuilder::evalCastSubKind (this=0xff34500, 
V=..., CastTy=..., OriginalTy=...)
  at  /clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:674
  674 if (CastTy->isBooleanType())
  (gdb) next
  678 if (CastTy->isIntegralOrEnumerationType()) {
  (gdb) 
  685 if (Loc::isLocType(CastTy))
  (gdb) p V
  $25 = { = { = 
{ = { = {Data = 
0xff37cf0, 
Kind = 2}, }, }, }, 
}
  (gdb) p V.dump()
  0 (Loc)$26 = void
  (gdb) p CastTy.dump()
  PointerType 0xfede460 '__attribute__((address_space(3))) int *'
  `-QualType 0xfede418 '__attribute__((address_space(3))) int' 
__attribute__((address_space(3)))
`-BuiltinType 0xfedd640 'int'
  $27 = void


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122513

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

[PATCH] D122513: [analyzer] Fix "RhsLoc and LhsLoc bitwidth must be same"

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Ahhh, gotcha. I'll run that down and report back. Thanks !


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122513

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

[PATCH] D122513: [analyzer] Fix "RhsLoc and LhsLoc bitwidth must be same"

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Apparently, it's up to an implementation to determine the specific relations 
that can be computed between different address spaces. Perhaps a better way to 
deal with this for now, to avoid crashes, is follow the DereferenceChecker 
model. That checker punts on checking relations with any pointers that have a 
address space. If a downstream compiler wants to compute relations between 
pointers with address spaces, they are certainly free to do that - but I get 
the instinct this might not be best to push upstream :/

Ideas? Best!

Here's the snippet, and comment, from 
clang/lib/StaticAnalyzer/Checkers/DereferenceChecker.cpp

  ...
  112 static bool suppressReport(const Expr *E) {
  113   // Do not report dereferences on memory in non-default address spaces.
  114   return E->getType().hasAddressSpace();
  115 }
  ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122513

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

[PATCH] D122513: [analyzer] Fix "RhsLoc and LhsLoc bitwidth must be same"

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers added a comment.

Hi @NoQ, good question :) When I looked into the existing SimpleSValBuilder.cpp 
code, I found a few places that did width modifications so I concluded this was 
the correct and accepted way to handle this. A few notes to help decide if my 
suggestion is correct or is there's a different concrete way to move forward. 
Looks like we get address space information in the AST, and it's up to the AST 
client to do the needful (whatever that is). We do get warnings and/or errors 
in some cases, but it seems this is a case where it's implied that the "void *" 
cast is same address space as the LHS of the comparison. I'll check the 
Embedded C Spec to see what it says about this.

The Sample function looks like this:

  int fn1() {
int val = 0;
__attribute__((address_space(3))) int *dptr = val;
return dptr == (void *)0;
  }

The AST for the return statement "return dptr == (void *)0;" looks like this:

  `-ReturnStmt 0x118f2078 
`-BinaryOperator 0x118f2058  'int' '=='
  |-ImplicitCastExpr 0x118f2028  '__attribute__((address_space(3))) 
int *' 
  | `-DeclRefExpr 0x118f1fa8  '__attribute__((address_space(3))) 
int *' lvalue Var 0x118f1af0 'dptr' '__attribute__((address_space(3))) int *'
  `-ImplicitCastExpr 0x118f2040  
'__attribute__((address_space(3))) int *' 
`-CStyleCastExpr 0x118f2000  'void *' 
  `-IntegerLiteral 0x118f1fc8  'int' 0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122513

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

[PATCH] D122513: [analyzer] Fix "RhsLoc and LhsLoc bitwidth must be same"

[Vince Bridgers via Phabricator via cfe-commits]

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

clang: /clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:727:
void assertEqualBitWidths(clang::ento::ProgramStateRef,

  clang::ento::Loc, clang::ento::Loc): Assertion `RhsBitwidth ==
  LhsBitwidth && "RhsLoc and LhsLoc bitwidth must be same!"'

This change adjusts the bitwidth of the smaller operand for an evalBinOp
as a result of a comparison operation. This can occur in the specific
case represented by the test cases for a target with different pointer
sizes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122513

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/addrspace-null.c


Index: clang/test/Analysis/addrspace-null.c
===
--- /dev/null
+++ clang/test/Analysis/addrspace-null.c
@@ -0,0 +1,47 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DAMDGCN_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+//
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DDEFAULT_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+
+// expected-no-diagnostics
+
+#define DEVICE __attribute__((address_space(3)))
+
+#if defined(AMDGCN)
+// this crashes
+int fn1() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (void *)0;
+}
+
+// does not crash
+int fn2() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+
+// this crashes
+int fn3() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+#endif
+
+// does not crash
+int fn4() {
+  int val = 0;
+  int *dptr = val;
+  return dptr == (void *)0;
+}
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -792,8 +792,27 @@
 
 // If both operands are constants, just perform the operation.
 if (Optional rInt = rhs.getAs()) {
-  SVal ResultVal =
-  lhs.castAs().evalBinOp(BasicVals, op, *rInt);
+  // Need a big enough type to compare both values.
+  //
+  // In the case of something like this for targets with different
+  // pointer sizes based on address space, need to find the largest
+  // bitwidth to use for the evalBinOp
+  //
+  // int fn1() {
+  //   int val = 0;
+  //   __attribute__((address_space(3))) int *dptr = val;
+  //   return dptr == (void *)0;
+  // }
+  llvm::APSInt LHSValue = lhs.castAs().getValue();
+  llvm::APSInt RHSValue = rhs.castAs().getValue();
+  APSIntType OpType = std::max(APSIntType(LHSValue), APSIntType(RHSValue));
+
+  OpType.apply(LHSValue);
+  OpType.apply(RHSValue);
+  loc::ConcreteInt ciLHS = loc::ConcreteInt(LHSValue);
+  loc::ConcreteInt ciRHS = loc::ConcreteInt(RHSValue);
+  SVal ResultVal = ciLHS.evalBinOp(BasicVals, op, ciRHS);
+
   if (Optional Result = ResultVal.getAs())
 return evalCast(*Result, resultTy, QualType{});
 


Index: clang/test/Analysis/addrspace-null.c
===
--- /dev/null
+++ clang/test/Analysis/addrspace-null.c
@@ -0,0 +1,47 @@
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DAMDGCN_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+//
+// RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown \
+// RUN: -analyze -analyzer-checker=core -DDEFAULT_TRIPLE \
+// RUN: -analyze -analyzer-checker=debug.ExprInspection \
+// RUN: -Wno-implicit-int -Wno-int-conversion -verify %s
+
+// From https://llvm.org/docs/AMDGPUUsage.html#address-spaces,
+// select address space 3 (local), since the pointer size is
+// different than Generic.
+
+// expected-no-diagnostics
+
+#define DEVICE __attribute__((address_space(3)))
+
+#if defined(AMDGCN)
+// this crashes
+int fn1() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (void *)0;
+}
+
+// does not crash
+int fn2() {
+  int val = 0;
+  DEVICE int *dptr = val;
+  return dptr == (DEVICE void *)0;
+}
+
+// this crashes
+int 

[PATCH] D122277: [analyzer] Fix crash in RangedConstraintManager.cpp

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 417525.
vabridgers added a comment.

update comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122277

Files:
  clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
  clang/test/Analysis/symbol-simplification-bo-div.c

Index: clang/test/Analysis/symbol-simplification-bo-div.c
===
--- /dev/null
+++ clang/test/Analysis/symbol-simplification-bo-div.c
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core %s \
+// RUN:-triple x86_64-pc-linux-gnu -verify
+
+// don't crash
+// expected-no-diagnostics
+
+int a, b;
+int c(void) {
+  unsigned d = a;
+  --d;
+  short e = b / b - a;
+  ++e;
+  return d <= 0 && e && e;
+}
Index: clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
@@ -48,47 +48,48 @@
 
   if (const auto *SSE = dyn_cast(Sym)) {
 BinaryOperator::Opcode Op = SSE->getOpcode();
-assert(BinaryOperator::isComparisonOp(Op));
-
-// We convert equality operations for pointers only.
-if (Loc::isLocType(SSE->getLHS()->getType()) &&
-Loc::isLocType(SSE->getRHS()->getType())) {
-  // Translate "a != b" to "(b - a) != 0".
-  // We invert the order of the operands as a heuristic for how loop
-  // conditions are usually written ("begin != end") as compared to length
-  // calculations ("end - begin"). The more correct thing to do would be to
-  // canonicalize "a - b" and "b - a", which would allow us to treat
-  // "a != b" and "b != a" the same.
-
-  SymbolManager  = getSymbolManager();
-  QualType DiffTy = SymMgr.getContext().getPointerDiffType();
-  SymbolRef Subtraction =
-  SymMgr.getSymSymExpr(SSE->getRHS(), BO_Sub, SSE->getLHS(), DiffTy);
-
-  const llvm::APSInt  = getBasicVals().getValue(0, DiffTy);
-  Op = BinaryOperator::reverseComparisonOp(Op);
-  if (!Assumption)
-Op = BinaryOperator::negateComparisonOp(Op);
-  return assumeSymRel(State, Subtraction, Op, Zero);
-}
+if (BinaryOperator::isComparisonOp(Op)) {
+
+  // We convert equality operations for pointers only.
+  if (Loc::isLocType(SSE->getLHS()->getType()) &&
+  Loc::isLocType(SSE->getRHS()->getType())) {
+// Translate "a != b" to "(b - a) != 0".
+// We invert the order of the operands as a heuristic for how loop
+// conditions are usually written ("begin != end") as compared to length
+// calculations ("end - begin"). The more correct thing to do would be
+// to canonicalize "a - b" and "b - a", which would allow us to treat
+// "a != b" and "b != a" the same.
+
+SymbolManager  = getSymbolManager();
+QualType DiffTy = SymMgr.getContext().getPointerDiffType();
+SymbolRef Subtraction =
+SymMgr.getSymSymExpr(SSE->getRHS(), BO_Sub, SSE->getLHS(), DiffTy);
+
+const llvm::APSInt  = getBasicVals().getValue(0, DiffTy);
+Op = BinaryOperator::reverseComparisonOp(Op);
+if (!Assumption)
+  Op = BinaryOperator::negateComparisonOp(Op);
+return assumeSymRel(State, Subtraction, Op, Zero);
+  }
 
-if (BinaryOperator::isEqualityOp(Op)) {
-  SymbolManager  = getSymbolManager();
+  if (BinaryOperator::isEqualityOp(Op)) {
+SymbolManager  = getSymbolManager();
 
-  QualType ExprType = SSE->getType();
-  SymbolRef CanonicalEquality =
-  SymMgr.getSymSymExpr(SSE->getLHS(), BO_EQ, SSE->getRHS(), ExprType);
+QualType ExprType = SSE->getType();
+SymbolRef CanonicalEquality =
+SymMgr.getSymSymExpr(SSE->getLHS(), BO_EQ, SSE->getRHS(), ExprType);
 
-  bool WasEqual = SSE->getOpcode() == BO_EQ;
-  bool IsExpectedEqual = WasEqual == Assumption;
+bool WasEqual = SSE->getOpcode() == BO_EQ;
+bool IsExpectedEqual = WasEqual == Assumption;
 
-  const llvm::APSInt  = getBasicVals().getValue(0, ExprType);
+const llvm::APSInt  = getBasicVals().getValue(0, ExprType);
 
-  if (IsExpectedEqual) {
-return assumeSymNE(State, CanonicalEquality, Zero, Zero);
-  }
+if (IsExpectedEqual) {
+  return assumeSymNE(State, CanonicalEquality, Zero, Zero);
+}
 
-  return assumeSymEQ(State, CanonicalEquality, Zero, Zero);
+return assumeSymEQ(State, CanonicalEquality, Zero, Zero);
+  }
 }
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122277: [analyzer] Fix crash in RangedConstraintManager.cpp

[Vince Bridgers via Phabricator via cfe-commits]

vabridgers updated this revision to Diff 417450.
vabridgers added a comment.

remove assert that was commented out


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122277

Files:
  clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
  clang/test/Analysis/symbol-simplification-bo-div.c

Index: clang/test/Analysis/symbol-simplification-bo-div.c
===
--- /dev/null
+++ clang/test/Analysis/symbol-simplification-bo-div.c
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core %s \
+// RUN:-triple x86_64-pc-linux-gnu -verify
+
+// don't crash
+// expected-no-diagnostics
+
+int a, b;
+int c(void) {
+  unsigned d = a;
+  --d;
+  short e = b / b - a;
+  ++e;
+  return d <= 0 && e && e;
+}
Index: clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
@@ -48,47 +48,48 @@
 
   if (const auto *SSE = dyn_cast(Sym)) {
 BinaryOperator::Opcode Op = SSE->getOpcode();
-assert(BinaryOperator::isComparisonOp(Op));
-
-// We convert equality operations for pointers only.
-if (Loc::isLocType(SSE->getLHS()->getType()) &&
-Loc::isLocType(SSE->getRHS()->getType())) {
-  // Translate "a != b" to "(b - a) != 0".
-  // We invert the order of the operands as a heuristic for how loop
-  // conditions are usually written ("begin != end") as compared to length
-  // calculations ("end - begin"). The more correct thing to do would be to
-  // canonicalize "a - b" and "b - a", which would allow us to treat
-  // "a != b" and "b != a" the same.
-
-  SymbolManager  = getSymbolManager();
-  QualType DiffTy = SymMgr.getContext().getPointerDiffType();
-  SymbolRef Subtraction =
-  SymMgr.getSymSymExpr(SSE->getRHS(), BO_Sub, SSE->getLHS(), DiffTy);
-
-  const llvm::APSInt  = getBasicVals().getValue(0, DiffTy);
-  Op = BinaryOperator::reverseComparisonOp(Op);
-  if (!Assumption)
-Op = BinaryOperator::negateComparisonOp(Op);
-  return assumeSymRel(State, Subtraction, Op, Zero);
-}
+if (BinaryOperator::isComparisonOp(Op)) {
+
+  // We convert equality operations for pointers only.
+  if (Loc::isLocType(SSE->getLHS()->getType()) &&
+  Loc::isLocType(SSE->getRHS()->getType())) {
+// Translate "a != b" to "(b - a) != 0".
+// We invert the order of the operands as a heuristic for how loop
+// conditions are usually written ("begin != end") as compared to length
+// calculations ("end - begin"). The more correct thing to do would be
+// to canonicalize "a - b" and "b - a", which would allow us to treat "a
+// != b" and "b != a" the same.
+
+SymbolManager  = getSymbolManager();
+QualType DiffTy = SymMgr.getContext().getPointerDiffType();
+SymbolRef Subtraction =
+SymMgr.getSymSymExpr(SSE->getRHS(), BO_Sub, SSE->getLHS(), DiffTy);
+
+const llvm::APSInt  = getBasicVals().getValue(0, DiffTy);
+Op = BinaryOperator::reverseComparisonOp(Op);
+if (!Assumption)
+  Op = BinaryOperator::negateComparisonOp(Op);
+return assumeSymRel(State, Subtraction, Op, Zero);
+  }
 
-if (BinaryOperator::isEqualityOp(Op)) {
-  SymbolManager  = getSymbolManager();
+  if (BinaryOperator::isEqualityOp(Op)) {
+SymbolManager  = getSymbolManager();
 
-  QualType ExprType = SSE->getType();
-  SymbolRef CanonicalEquality =
-  SymMgr.getSymSymExpr(SSE->getLHS(), BO_EQ, SSE->getRHS(), ExprType);
+QualType ExprType = SSE->getType();
+SymbolRef CanonicalEquality =
+SymMgr.getSymSymExpr(SSE->getLHS(), BO_EQ, SSE->getRHS(), ExprType);
 
-  bool WasEqual = SSE->getOpcode() == BO_EQ;
-  bool IsExpectedEqual = WasEqual == Assumption;
+bool WasEqual = SSE->getOpcode() == BO_EQ;
+bool IsExpectedEqual = WasEqual == Assumption;
 
-  const llvm::APSInt  = getBasicVals().getValue(0, ExprType);
+const llvm::APSInt  = getBasicVals().getValue(0, ExprType);
 
-  if (IsExpectedEqual) {
-return assumeSymNE(State, CanonicalEquality, Zero, Zero);
-  }
+if (IsExpectedEqual) {
+  return assumeSymNE(State, CanonicalEquality, Zero, Zero);
+}
 
-  return assumeSymEQ(State, CanonicalEquality, Zero, Zero);
+return assumeSymEQ(State, CanonicalEquality, Zero, Zero);
+  }
 }
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D122277: [analyzer] Fix crash in RangedConstraintManager.cpp

[Vince Bridgers via Phabricator via cfe-commits]

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

This change fixes a crash in RangedConstraintManager.cpp:assumeSym due to an
unhandled BO_Div case.

clang: clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp:51:

  virtual clang::ento::ProgramStateRef
  clang::ento::RangedConstraintManager::assumeSym(clang::ento::ProgramStateRef,
clang::ento::SymbolRef, bool):
  Assertion `BinaryOperator::isComparisonOp(Op)' failed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122277

Files:
  clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
  clang/test/Analysis/symbol-simplification-bo-div.c

Index: clang/test/Analysis/symbol-simplification-bo-div.c
===
--- /dev/null
+++ clang/test/Analysis/symbol-simplification-bo-div.c
@@ -0,0 +1,14 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core %s \
+// RUN:-triple x86_64-pc-linux-gnu -verify
+
+// don't crash
+// expected-no-diagnostics
+
+int a, b;
+int c(void) {
+  unsigned d = a;
+  --d;
+  short e = b / b - a;
+  ++e;
+  return d <= 0 && e && e;
+}
Index: clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
+++ clang/lib/StaticAnalyzer/Core/RangedConstraintManager.cpp
@@ -48,47 +48,49 @@
 
   if (const auto *SSE = dyn_cast(Sym)) {
 BinaryOperator::Opcode Op = SSE->getOpcode();
-assert(BinaryOperator::isComparisonOp(Op));
-
-// We convert equality operations for pointers only.
-if (Loc::isLocType(SSE->getLHS()->getType()) &&
-Loc::isLocType(SSE->getRHS()->getType())) {
-  // Translate "a != b" to "(b - a) != 0".
-  // We invert the order of the operands as a heuristic for how loop
-  // conditions are usually written ("begin != end") as compared to length
-  // calculations ("end - begin"). The more correct thing to do would be to
-  // canonicalize "a - b" and "b - a", which would allow us to treat
-  // "a != b" and "b != a" the same.
-
-  SymbolManager  = getSymbolManager();
-  QualType DiffTy = SymMgr.getContext().getPointerDiffType();
-  SymbolRef Subtraction =
-  SymMgr.getSymSymExpr(SSE->getRHS(), BO_Sub, SSE->getLHS(), DiffTy);
-
-  const llvm::APSInt  = getBasicVals().getValue(0, DiffTy);
-  Op = BinaryOperator::reverseComparisonOp(Op);
-  if (!Assumption)
-Op = BinaryOperator::negateComparisonOp(Op);
-  return assumeSymRel(State, Subtraction, Op, Zero);
-}
+// assert(BinaryOperator::isComparisonOp(Op));
+if (BinaryOperator::isComparisonOp(Op)) {
+
+  // We convert equality operations for pointers only.
+  if (Loc::isLocType(SSE->getLHS()->getType()) &&
+  Loc::isLocType(SSE->getRHS()->getType())) {
+// Translate "a != b" to "(b - a) != 0".
+// We invert the order of the operands as a heuristic for how loop
+// conditions are usually written ("begin != end") as compared to length
+// calculations ("end - begin"). The more correct thing to do would be
+// to canonicalize "a - b" and "b - a", which would allow us to treat "a
+// != b" and "b != a" the same.
+
+SymbolManager  = getSymbolManager();
+QualType DiffTy = SymMgr.getContext().getPointerDiffType();
+SymbolRef Subtraction =
+SymMgr.getSymSymExpr(SSE->getRHS(), BO_Sub, SSE->getLHS(), DiffTy);
+
+const llvm::APSInt  = getBasicVals().getValue(0, DiffTy);
+Op = BinaryOperator::reverseComparisonOp(Op);
+if (!Assumption)
+  Op = BinaryOperator::negateComparisonOp(Op);
+return assumeSymRel(State, Subtraction, Op, Zero);
+  }
 
-if (BinaryOperator::isEqualityOp(Op)) {
-  SymbolManager  = getSymbolManager();
+  if (BinaryOperator::isEqualityOp(Op)) {
+SymbolManager  = getSymbolManager();
 
-  QualType ExprType = SSE->getType();
-  SymbolRef CanonicalEquality =
-  SymMgr.getSymSymExpr(SSE->getLHS(), BO_EQ, SSE->getRHS(), ExprType);
+QualType ExprType = SSE->getType();
+SymbolRef CanonicalEquality =
+SymMgr.getSymSymExpr(SSE->getLHS(), BO_EQ, SSE->getRHS(), ExprType);
 
-  bool WasEqual = SSE->getOpcode() == BO_EQ;
-  bool IsExpectedEqual = WasEqual == Assumption;
+bool WasEqual = SSE->getOpcode() == BO_EQ;
+bool IsExpectedEqual = WasEqual == Assumption;
 
-  const llvm::APSInt  = getBasicVals().getValue(0, ExprType);
+const llvm::APSInt  = getBasicVals().getValue(0, ExprType);
 
-  if