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

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.
Herald added a reviewer: njames93.

@mizvekov , I'm willing/trying to assist in addressing the buildbot failures 
that caused you to revert this change.  We are eager to see this fix land 
stably upstream as the crash it addresses is breaking a number of our internal 
tests.  Main thing I lack is any reasonable way to test code changes on 
aarch64, where it's breaking in buildbot.  Your last comment suggests to me 
that you believe this is a namespace issue; however, I don't see an obvious 
connection to namespace from the location where the crash occurs in the failing 
tests (at clang/lib/Serialization/ASTWriter.cpp:5455 in all the failed tests I 
reviewed).  Can you give a bit of context?


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] D126534: [analyzer] Deadstore static analysis: Fix false positive on C++17 assignments

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

This change affected analyzer findings for C language source as well.  Mostly 
true positives, but there is one false positive we know about.  I filed 
https://github.com/llvm/llvm-project/issues/57434 to track that new false 
positive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126534

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

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9e1a4ce0b55d: [clang-tidy] Fix for 
bugprone-sizeof-expression PR57167 (authored by chrish_ericsson_atx).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -147,8 +147,8 @@
   const auto StructAddrOfExpr = unaryOperator(
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
-  const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  const auto PointerToStructType =
+  hasUnqualifiedDesugaredType(pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 453442.
chrish_ericsson_atx added a comment.

Removed `// FIXME` comments, per @aaron.ballman's feedback.


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

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -147,8 +147,8 @@
   const auto StructAddrOfExpr = unaryOperator(
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
-  const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  const auto PointerToStructType =
+  hasUnqualifiedDesugaredType(pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

aaron.ballman wrote:
> mizvekov wrote:
> > aaron.ballman wrote:
> > > mizvekov wrote:
> > > > chrish_ericsson_atx wrote:
> > > > > mizvekov wrote:
> > > > > > chrish_ericsson_atx wrote:
> > > > > > > njames93 wrote:
> > > > > > > > mizvekov wrote:
> > > > > > > > > Is this change really desirable, or should we put a FIXME 
> > > > > > > > > here?
> > > > > > > > Not warning on these cases seems like a pretty big red flag, 
> > > > > > > > especially the `MyStruct *`.
> > > > > > > Thank you for your comment!  I'm not sure I fully agree that this 
> > > > > > > is a red flag.  I'm inclined to think that whether or not there 
> > > > > > > should be a warning on `MyStruct *` or `PMyStruct` is a pretty 
> > > > > > > subjective.  These are both very common idioms, and are 
> > > > > > > meaningful.  I do appreciate the perspective that `MyStruct *` is 
> > > > > > > just one character different from `MyStruct`, and as such, it 
> > > > > > > might be a typo to ask for sizeof the pointer, when you really 
> > > > > > > wanted sizeof the struct.  But the counter-argument (accidentally 
> > > > > > > asking for sizeof the struct when you really wanted the pointer 
> > > > > > > size) is just as valid-- and the checker obviously cannot warn in 
> > > > > > > that case.   
> > > > > > > 
> > > > > > > I am perfectly fine with kicking the can down the road a bit by 
> > > > > > > replacing the discarded `// CHECK-MESSAGES` directive with a `// 
> > > > > > > FIXME`, as @mizvekov suggested.  I expect that when someone 
> > > > > > > circles back to really deeply reconsider the semantics of this 
> > > > > > > checker, there will be a number of changes (other existing 
> > > > > > > warnings dropped, warnings for new and missed cases added, much 
> > > > > > > better sync between C and C++, as well as intentional 
> > > > > > > consideration of things like __typeof__ (in it's various forms) 
> > > > > > > and decltype, rather than the haphazard coarse-grain matching 
> > > > > > > that seems to be going on now.
> > > > > > I agree with this patch only in so far that:
> > > > > > 
> > > > > > * This is effectively a partial revert of the changes made in 
> > > > > > https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
> > > > > > * The checker was pretty much buggy to begin with.
> > > > > > * That change significantly increased the amount of patterns we 
> > > > > > would accept, but at the same time the existing tests did not pick 
> > > > > > that up and this was not carefully considered.
> > > > > > * It seems unreasonable that there is effectively no way to shut 
> > > > > > this warning up per site, except by a NOLINT directive.
> > > > > > 
> > > > > > If the amount of false positives is so high now that this check is 
> > > > > > unusable, then this is just a question of reverting to a less 
> > > > > > broken state temporarily.
> > > > > > 
> > > > > > But otherwise we can't leave it in the reverted state either. 
> > > > > > Without that change to use the canonical type or something similar, 
> > > > > > there is no reason to suppose any of these test cases would work at 
> > > > > > all as clang evolves and we improve the quality of implementation 
> > > > > > wrt type sugar retention.
> > > > > @mizvekov, I agree with your reasoning for saying "we can't leave it 
> > > > > in the reverted state either".  But I'm not sure how to ensure that 
> > > > > this gets the needed attention.  Should we just file a separate PR on 
> > > > > github to track the needed refactoring?  I do not believe I'll have 
> > > > > the bandwidth to look at this in the next few months.
> > > > > 
> > > > > In the meantime, assuming the bots are happy with patchset 2, I'll 
> > > > > land this as-is later today.
> > > > > 
> > > > > Thank you very much for your feedback!
> > > > Oh please wait for others to review, I don't think landing today is 
> > > > reasonable!
> > > > 
> > > > I am not really a stakeholder for this checker except for that original 
> > > > change.
> > > > 
> > > > I would advise for you to wait for another more responsible reviewer to 
> > > > accept as well before merging, unless this gets stale for a long time 
> > > > and no one else seems to be interested.
> > > > Thank you for your comment! I'm not sure I fully agree that this is a 
> > > > red flag. I'm inclined to think that whether or not there should be a 
> > > > warning on MyStruct * or PMyStruct is a pretty subjective. These are 
> > > > both very common idioms, and are meaningful. I do appreciate the 
> > > > perspective that 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 453286.
chrish_ericsson_atx added a comment.

Corrected git-clang-format issue.


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

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -232,10 +232,8 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(MyStruct*);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
+  sum += sizeof(PMyStruct);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -147,8 +147,8 @@
   const auto StructAddrOfExpr = unaryOperator(
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
-  const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  const auto PointerToStructType =
+  hasUnqualifiedDesugaredType(pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -232,10 +232,8 @@
   // CHECK-MESSAGES: 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Sounds fair.  I had taken your acceptance of the change as a green light.  :)  
TBH, the acceptance came much faster than I'd expected-- even though this is a 
trivial and low-risk change, I expected it to sit for at least several days.  
I'll plan to wait a few more days before landing it.


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

https://reviews.llvm.org/D131926

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

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

mizvekov wrote:
> chrish_ericsson_atx wrote:
> > njames93 wrote:
> > > mizvekov wrote:
> > > > Is this change really desirable, or should we put a FIXME here?
> > > Not warning on these cases seems like a pretty big red flag, especially 
> > > the `MyStruct *`.
> > Thank you for your comment!  I'm not sure I fully agree that this is a red 
> > flag.  I'm inclined to think that whether or not there should be a warning 
> > on `MyStruct *` or `PMyStruct` is a pretty subjective.  These are both very 
> > common idioms, and are meaningful.  I do appreciate the perspective that 
> > `MyStruct *` is just one character different from `MyStruct`, and as such, 
> > it might be a typo to ask for sizeof the pointer, when you really wanted 
> > sizeof the struct.  But the counter-argument (accidentally asking for 
> > sizeof the struct when you really wanted the pointer size) is just as 
> > valid-- and the checker obviously cannot warn in that case.   
> > 
> > I am perfectly fine with kicking the can down the road a bit by replacing 
> > the discarded `// CHECK-MESSAGES` directive with a `// FIXME`, as @mizvekov 
> > suggested.  I expect that when someone circles back to really deeply 
> > reconsider the semantics of this checker, there will be a number of changes 
> > (other existing warnings dropped, warnings for new and missed cases added, 
> > much better sync between C and C++, as well as intentional consideration of 
> > things like __typeof__ (in it's various forms) and decltype, rather than 
> > the haphazard coarse-grain matching that seems to be going on now.
> I agree with this patch only in so far that:
> 
> * This is effectively a partial revert of the changes made in 
> https://reviews.llvm.org/rG15f3cd6bfc670ba6106184a903eb04be059e5977
> * The checker was pretty much buggy to begin with.
> * That change significantly increased the amount of patterns we would accept, 
> but at the same time the existing tests did not pick that up and this was not 
> carefully considered.
> * It seems unreasonable that there is effectively no way to shut this warning 
> up per site, except by a NOLINT directive.
> 
> If the amount of false positives is so high now that this check is unusable, 
> then this is just a question of reverting to a less broken state temporarily.
> 
> But otherwise we can't leave it in the reverted state either. Without that 
> change to use the canonical type or something similar, there is no reason to 
> suppose any of these test cases would work at all as clang evolves and we 
> improve the quality of implementation wrt type sugar retention.
@mizvekov, I agree with your reasoning for saying "we can't leave it in the 
reverted state either".  But I'm not sure how to ensure that this gets the 
needed attention.  Should we just file a separate PR on github to track the 
needed refactoring?  I do not believe I'll have the bandwidth to look at this 
in the next few months.

In the meantime, assuming the bots are happy with patchset 2, I'll land this 
as-is later today.

Thank you very much for your feedback!


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

https://reviews.llvm.org/D131926

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

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 453022.
chrish_ericsson_atx added a comment.

Added missing newline and added // FIXME comments, per reviewer comments.


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

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -232,10 +232,8 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
-  sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(MyStruct*);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
+  sum += sizeof(PMyStruct);  // FIXME: Warning here prior to 15f3cd6bfc6, 
consider whether to add it back.
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -148,7 +148,7 @@
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
   const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -232,10 +232,8 @@
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 'sizeof(A*)'; pointer to 

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp:236-238
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate

njames93 wrote:
> mizvekov wrote:
> > Is this change really desirable, or should we put a FIXME here?
> Not warning on these cases seems like a pretty big red flag, especially the 
> `MyStruct *`.
Thank you for your comment!  I'm not sure I fully agree that this is a red 
flag.  I'm inclined to think that whether or not there should be a warning on 
`MyStruct *` or `PMyStruct` is a pretty subjective.  These are both very common 
idioms, and are meaningful.  I do appreciate the perspective that `MyStruct *` 
is just one character different from `MyStruct`, and as such, it might be a 
typo to ask for sizeof the pointer, when you really wanted sizeof the struct.  
But the counter-argument (accidentally asking for sizeof the struct when you 
really wanted the pointer size) is just as valid-- and the checker obviously 
cannot warn in that case.   

I am perfectly fine with kicking the can down the road a bit by replacing the 
discarded `// CHECK-MESSAGES` directive with a `// FIXME`, as @mizvekov 
suggested.  I expect that when someone circles back to really deeply reconsider 
the semantics of this checker, there will be a number of changes (other 
existing warnings dropped, warnings for new and missed cases added, much better 
sync between C and C++, as well as intentional consideration of things like 
__typeof__ (in it's various forms) and decltype, rather than the haphazard 
coarse-grain matching that seems to be going on now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131926

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

[PATCH] D131926: [clang-tidy] Fix for bugprone-sizeof-expression PR57167

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx created this revision.
chrish_ericsson_atx added reviewers: mizvekov, baloghadamsoftware, njames93, 
cfe-commits.
Herald added subscribers: carlosgalvezp, rnkovacs, xazax.hun.
Herald added a project: All.
chrish_ericsson_atx requested review of this revision.
Herald added a project: clang-tools-extra.

This addresses a change in behavior of the bugprone-sizeof-expression
checker after upstream commit 15f3cd6bfc6 
, which 
cleaned up
ElaboratedType sugaring in the AST.  This restores (mostly) the
beahvior of the checker prior to that commit, which may or may not have
been consistent with the intent of the checker, but at least gave a
tolerable level of what users would consider false positives.

  

Bug: https://github.com/llvm/llvm-project/issues/57167


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D131926

Files:
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
  clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression.cpp
@@ -233,9 +233,7 @@
   sum += sizeof();
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(MyStruct*);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PMyStruct);
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS);
   // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
   sum += sizeof(PS2);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/sizeof-expression-2.c
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- --
+// RUN: %check_clang_tidy %s bugprone-sizeof-expression %t -- -- -x c++
+
+#ifdef __cplusplus
+#define STRKWD
+#else
+#define STRKWD struct
+#endif
+
+int Test5() {
+  typedef int Array10[10];
+
+  struct MyStruct {
+Array10 arr;
+Array10* ptr;
+  };
+
+  typedef struct TypedefStruct {
+Array10 arr;
+Array10* ptr;
+  } TypedefStruct;
+
+  typedef const STRKWD MyStruct TMyStruct;
+  typedef const STRKWD MyStruct *PMyStruct;
+  typedef TMyStruct *PMyStruct2;
+  typedef const TypedefStruct *PTTStruct;
+
+  STRKWD MyStruct S;
+  TypedefStruct TS;
+  PMyStruct PS;
+  PMyStruct2 PS2;
+  Array10 A10;
+  PTTStruct PTTS;
+
+  int sum = 0;
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(__typeof());
+  sum += sizeof(STRKWD MyStruct*);
+  sum += sizeof(__typeof(STRKWD MyStruct*));
+  sum += sizeof(TypedefStruct*);
+  sum += sizeof(__typeof(TypedefStruct*));
+  sum += sizeof(PTTS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PMyStruct);
+  sum += sizeof(PS);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof(PS2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+  sum += sizeof();
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: suspicious usage of 
'sizeof(A*)'; pointer to aggregate
+
+#ifdef __cplusplus
+  MyStruct  = S;
+  sum += sizeof(rS); // same as sizeof(S), not a pointer.  So should not warn.
+#endif
+
+  return sum;
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
@@ -148,7 +148,7 @@
   hasOperatorName("&"), hasUnaryOperand(ignoringParenImpCasts(
 hasType(hasCanonicalType(recordType());
   const auto PointerToStructType = hasUnqualifiedDesugaredType(
-  pointerType(pointee(hasCanonicalType(recordType();
+  pointerType(pointee(recordType(;
   const auto PointerToStructExpr = ignoringParenImpCasts(expr(
   hasType(hasCanonicalType(PointerToStructType)), unless(cxxThisExpr(;
 


Index: 

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

I've added a few suggestions for more rigorous testing, and raised a concern 
about a bug in this patch that is hiding expected -Warray-bounds warnings in 
certain cases.




Comment at: clang/lib/CodeGen/CGExpr.cpp:888-889
   if (const auto *CAT = dyn_cast(AT)) {
 // FIXME: Sema doesn't treat [1] as a flexible array member if the bound
 // was produced by macro expansion.
+if (StrictFlexArraysLevel >= 2 && CAT->getSize().ugt(0))

I was exploring this difference and during my testing, I ran across what I 
think is a bug in mode 1 behavior of this patch:

https://godbolt.org/z/hj9T4MY61 -fsfa=0, no-warnings: f[0] f[ZERO] f[1] 
warnings: f[ONE] f[5]
https://godbolt.org/z/Ea9MY9jjh -fsfa=1, no-warnings: f[0] f[ZERO] f[5] 
warnings: f[1] f[ONE]  <-- Incorrect behavior?
https://godbolt.org/z/aerMvxs6q -fsfa=2, no-warnings: f[0] f[ZERO] warnings: 
f[1] f[ONE] f[5]

I would think that -Warray-bounds should give a warning for accesses beyond the 
declared size of an array of 5 elements no matter what the 
-fstrict-flex-arrays= mode is.  Have I misunderstood?

Testing with compiler prior to this patch (using 14.0.0, e.g., and not giving 
-fsfa option) gives behavior consistent with -fsfa=0 on trunk.  So it seems 
-Warray-bounds has always treated size>1 trailing arrays as concrete/fixed 
arrays, just as it does with non-trailing arrays.  But I may be missing 
something, of course



Comment at: clang/lib/Sema/SemaChecking.cpp:15801-15803
+  // FIXME: While the default -fstrict-flex-arrays=0 permits Size>1 trailing
+  // arrays to be treated as flexible-array-members, we still emit diagnostics
+  // as if they are not. Pending further discussion...

Under what circumstances (what compiler options, what example code) would 
size>1 trailing arrays be treated as flexible array members?  I'm not seeing 
that.  -Warray-bounds warns about OOB access on size>1 trailing arrays, and 
always has (with the notable exception of the comment I made above, which I 
think is a bug).



Comment at: clang/test/CodeGen/bounds-checking-fam.c:23
 // CHECK-LABEL: define {{.*}} @{{.*}}test_one{{.*}}(
 int test_one(struct One *p, int i) {
   // CHECK-STRICT-0-NOT: @__ubsan

Should there be a "test_zero" case?  Such as:

```
struct Zero {
   int a[0];
};

int test_zero(struct Zero *p, int i) {
   return p->a[i] + (p->a)[i];
}
```

Likewise, for C99 compilation, should there also be a case with a trailing 
incomplete array?

```
struct Inc {
   int x;
   int a[];
};

int test_inc(struct Inc *p, int i) {
   return p->a[i] + (p->a)[i];
}
```




Comment at: clang/test/SemaCXX/array-bounds.cpp:211-213
+struct foo {
+  char c[ONE]; // expected-note {{array 'c' declared here}}
+};

This may be a bit beyond the scope of this change, but I think there are some 
problems with -Warray-bounds behavior that should be considered here as well, 
having to do with *nesting* of structs that have trailing size=1 arrays.

Consider https://godbolt.org/z/ex6P8bqY9, in which this code:

```
#define LEN 1

typedef struct S1 {
int x;
double y;
int tail[3];
} S1;

typedef struct S {
int   x;
S1s1;
double y;
int   tail[1];
} S;

void fooS(S *s) {
s->tail[2] = 10;
s->tail[5] = 20;
(s->tail)[10] = 200;
s->s1.tail[10] = (s->s1.tail)[10] + 1;
}
```
produces a warning (as you'd expect).  But if you change the size of the 
trailing array in the **nested** struct to 1, you no longer get a warning: 
https://godbolt.org/z/dWET3E9d6

Note also that testing these examples with trunk build repeats the bug I 
mentioned in an earlier comment, where -fsfa=1 causes an expected 
-Warray-bounds warning to be dropped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126864

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

[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

In D112374#3653967 , @JDevlieghere 
wrote:

> I don't. I think reverting your change was well within the guidelines 
> outlined by LLVM's patch reversion policy: 
> https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy
>
> Additionally, I think you could've given me a little bit more time to catch 
> up on the discussion here. The code review policy and practices 
> (https://llvm.org/docs/CodeReview.html#code-reviews-speed-and-reciprocity) 
> recommend pinging every few days to once per week depending on how urgent the 
> patch is.
>
> By relanding, you broke the bots again 
> (https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45354/#showFailuresLink)
>  and I'm forced to revert this change a second time. Please refrain from 
> landing this again until we've settled on a way forward.

I agree with @JDevlieghere here. In general, it's reasonable that the patch was 
reverted when it was found to break other things in the LLVM project, and 
reasonable to expect the original author's cooperation in resolving that 
breakage before the patch gets reapplied--especially when the patch is as large 
and far-reaching as this one is.

As an aside, this also patch breaks one of our internal tidy checkers. Likely 
the fix we'll need for that checker is simple, but it'll take a bit of research 
for me to understand what needs to happen in our internal code. Thanks for your 
patience and willingness to help your colleagues understand and adapt to the 
change you've authored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374

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

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

I believe this exposed another odd issue where a true positive (enabled by this 
commit) disappears when unrelated code is not present.   Bug filed as: 
https://bugs.llvm.org/show_bug.cgi?id=51950.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103314

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

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

I believe this commit exposed a new false-positive bug in [core.DivideZero].  
I've filed the report here: https://bugs.llvm.org/show_bug.cgi?id=51940


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103314

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

[PATCH] D104285: [analyzer] Retrieve value by direct index from list initialization of constant array declaration.

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

In D104285#2847547 , @ASDenysPetrov 
wrote:

> I don't think we should revert b30521c28a4d 
>  because 
> it corrects symbol representation in CSA and fixes two bugs: 
> https://bugs.llvm.org/show_bug.cgi?id=37503 and 
> https://bugs.llvm.org/show_bug.cgi?id=49007. Another point of non-revert is 
> that your cases were previously hidden in CSA core and that's good to find 
> them. I'm afraid it's a dubious idea to return back old bugs in favor of not 
> seeing new ones.

Valid point.  Thanks for considering the question.  I agree it's better to move 
forward and get this patch working.  :)   I just wish I had the bandwidth to 
help more...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

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

[PATCH] D104285: [analyzer] Retrieve value by direct index from list initialization of constant array declaration.

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

While this patch resolves the issue captured in 
https://bugs.llvm.org/show_bug.cgi?id=50604, it actually introduces a *new* 
bug.  Perhaps this is what you were alluding to?  Here's a reproducer which 
doesn't fail on main (with or without the problematic b30521c28a4d 
 commit), 
but it *does* fail with this proposed patch:

  eahcmrh@seroius03977[21:50][repo/eahcmrh/ltebb]$ cat ~/pr50604-newbug.c 
  static float const dt[12] =
  {
0., 0.0235, 0.0470, 0.0706, 0.0941, 0.1176,
0.1411, 0.1646, 0.1881, 0.2117, 0.2352, 0.2587
  };
  void foo(float s)
  {
(void)( s + dt[0]) ;
  }
  eahcmrh@seroius03977[21:57][repo/eahcmrh/ltebb]$ 
/proj/bbi/eahcmrh/arcpatch-D104285/compiler-clang/bin/clang -Xanalyzer 
-analyzer-werror --analyze ~/pr50604-newbug.c 
  /home/eahcmrh/pr50604-newbug.c:8:13: error: The right operand of '+' is a 
garbage value [core.UndefinedBinaryOperatorResult]
(void)( s + dt[0]) ;
  ^ ~
  1 error generated.

I'll upload this reproducer to the bug report as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

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

[PATCH] D104285: [analyzer] Retrieve value by direct index from list initialization of constant array declaration.

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

In D104285#2836190 , @ASDenysPetrov 
wrote:

>> I think the presence of the initializer list in the test case is not 
>> necessary to trigger the spurious warnings
>
> Could you please provide some test cases that you think will uncover other 
> issues. I'll add them to the test set.

I tested locally with this patch and found that my guess was incorrect-- I 
couldn't trigger the incorrect behavior without an initializer list.  So I 
think your code and testcases are good as they are!

> I also have to mention one point of what this patch do more. Consider next:
>
>   int const arr[2][2] = {{1, 2}, {3, 4}}; // global space
>   int const *ptr = [0][0];
>   ptr[3]; // represented as ConcreteInt(0) 
>   arr[1][1]; // represented as reg_$0 [2]},1 S64b,int}>
>
> As you can see, now the access through the raw pointer is more presice as 
> through the multi-level indexing. I didn't want to synchronyze those 
> retrieved values both to be `reg_$0`. I've seen a way to handle it more 
> sophisticatedly.
> I'm gonna do the same for the multi-level indexing (aka `arr[1][2][3]`).

I don't understand -- probably I don't have enough experience with analyzer 
state dumps to know what I should find surprising or better in this example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

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

[PATCH] D104285: [analyzer] Retrieve value by direct index from list initialization of constant array declaration.

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

I've looked this over and tested it locally, and I'm pretty sure it's a good 
patch.  If it were solely up to me, I'd accept this patch as-is.  I don't think 
I should assume I have enough experience in this area though...  @NoQ , could 
you take a look over this, and accept it if you think it's safe and reasonable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

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

[PATCH] D104285: [analyzer] Retrieve value by direct index from list initialization of constant array declaration.

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: clang/include/clang/AST/Expr.h:4961
+  /// - `nullptr` for invalid index (`i < 0` or `i >= array_size`).
+  const Expr *getExprForConstArrayByRawIndex(int64_t Idx) const;
+

I think in most (all?) other methods in this class, array indices are unsigned 
in the API.  If the array index itself comes from an expression that is 
negative (i.e., a literal negative integer, or an constant expression that 
evaluates to a negative number), that has to be handled correctly, but I'm not 
sure this is the right place to do it.  As this code stands, if an integer 
literal used used, which is greater than LONG_MAX, but less than ULONG_MAX, it 
will be end up being treated as invalid in this method, won't it?



Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1671
   if (auto CI = R->getIndex().getAs()) {
 int64_t i = CI->getValue().getSExtValue();
+const Expr *E = InitList->getExprForConstArrayByRawIndex(i);

I see where you got the int64_t from -- that's what getSExtValue() returns.  
So, if the literal const index value in the expr is greater than LONG_MAX (but 
less than ULONG_MAX, of course), this would assert.  That seems undesirable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

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

[PATCH] D104285: [analyzer] Retrieve value by direct index from list initialization of constant array declaration.

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

In D104285#2823305 , 
@chrish_ericsson_atx wrote:

> I'm not sure about whether or not this patch would only work for constant 
> arrays with initializer lists.  If it does only work for such arrays, then I 
> wonder whether the fix is broad enough -- I haven't tested (yet), but I think 
> the presence of the initializer list in the test case is not necessary to 
> trigger the spurious warnings about garbage/undefined values.  I'll try it 
> tomorrow morning...

I tested with an unpatched build using a reproducer without an initializer 
list, and didn't get the spurious warnings -- so your approach seems correct to 
me now.   I've also tested your patch and I believe it gives correct behavior.




Comment at: clang/lib/AST/Type.cpp:149
+Extents.push_back(*CAT->getSize().getRawData());
+  } while (CAT = dyn_cast(CAT->getElementType()));
+  return Extents;




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

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

[PATCH] D104424: [Sema] Fix for PR50741

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Delivered as rGfc6ec9b98cf9 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104424

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

[PATCH] D104424: [Sema] Fix for PR50741

[Chris Hamilton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfc6ec9b98cf9: [Sema] Fix for PR50741 (authored by 
chrish_ericsson_atx).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104424

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/unbounded-array-bounds.c

Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -1,9 +1,6 @@
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
-// RUN:  --implicit-check-not 'past the last possible element'
-// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
-// RUN:  --implicit-check-not 'past the last possible element'
-// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
-// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -Wno-unused -verify=addr64,expected %s
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only -Wno-unused -verify=addr32,expected %s
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only -Wno-unused -verify=addr16,expected %s
 
 struct S {
   long long a;
@@ -12,61 +9,61 @@
   short d;
 };
 
-struct S s[];
+struct S s[]; // expected-warning {{tentative array definition}} expected-note {{declared here}} addr16-note {{declared here}}
 
 void f1() {
   ++s[3].a;
   ++s[7073650413200313099].b;
-  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
-  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  // addr16-warning@-1 {{array index 7073650413200313099 refers past the last possible element for an array in 16-bit address space containing 160-bit (20-byte) elements (max possible 3276 elements)}}
+  // addr32-warning@-2 {{array index 7073650413200313099 refers past the last possible element for an array in 32-bit address space containing 192-bit (24-byte) elements (max possible 178956970 elements)}}
+  // addr64-warning@-3 {{array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)}}
   ++s[7073650].c;
-  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  // addr16-warning@-1 {{array index 7073650 refers past the last possible element for an array in 16-bit address space containing 160-bit (20-byte) elements (max possible 3276 elements)}}
 }
 
-long long ll[];
+long long ll[]; // expected-warning {{tentative array definition}} expected-note {{declared here}} addr16-note {{declared here}} addr32-note {{declared here}}
 
 void f2() {
   ++ll[3];
   ++ll[2705843009213693952];
-  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
-  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  // addr16-warning@-1 {{array index 2705843009213693952 refers past the last possible element for an array in 16-bit address space containing 64-bit (8-byte) elements (max possible 8192 elements)}}
+  // addr32-warning@-2 {{array index 2705843009213693952 refers past the last possible element for an array in 32-bit address space containing 64-bit (8-byte) elements (max possible 536870912 elements)}}
+  // addr64-warning@-3 {{array index 2705843009213693952 refers past the last possible element for an array in 64-bit address space containing 64-bit (8-byte) elements (max possible 2305843009213693952 elements)}}
   ++ll[847073650];
-  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  // addr16-warning@-1 {{array index 847073650 refers past 

[PATCH] D104424: [Sema] Fix for PR50741

[Chris Hamilton via Phabricator via cfe-commits]

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

Thanks for the hints, @erichkeane !  Especially thanks for pointing out the 
root cause early on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104424

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

[PATCH] D104424: [Sema] Fix for PR50741

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 352711.
chrish_ericsson_atx edited the summary of this revision.
chrish_ericsson_atx added a comment.

Addressed review feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104424

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/unbounded-array-bounds.c

Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -1,9 +1,6 @@
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
-// RUN:  --implicit-check-not 'past the last possible element'
-// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
-// RUN:  --implicit-check-not 'past the last possible element'
-// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
-// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -Wno-unused -verify=addr64,expected %s
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only -Wno-unused -verify=addr32,expected %s
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only -Wno-unused -verify=addr16,expected %s
 
 struct S {
   long long a;
@@ -12,61 +9,61 @@
   short d;
 };
 
-struct S s[];
+struct S s[]; // expected-warning {{tentative array definition}} expected-note {{declared here}} addr16-note {{declared here}}
 
 void f1() {
   ++s[3].a;
   ++s[7073650413200313099].b;
-  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
-  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  // addr16-warning@-1 {{array index 7073650413200313099 refers past the last possible element for an array in 16-bit address space containing 160-bit (20-byte) elements (max possible 3276 elements)}}
+  // addr32-warning@-2 {{array index 7073650413200313099 refers past the last possible element for an array in 32-bit address space containing 192-bit (24-byte) elements (max possible 178956970 elements)}}
+  // addr64-warning@-3 {{array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)}}
   ++s[7073650].c;
-  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  // addr16-warning@-1 {{array index 7073650 refers past the last possible element for an array in 16-bit address space containing 160-bit (20-byte) elements (max possible 3276 elements)}}
 }
 
-long long ll[];
+long long ll[]; // expected-warning {{tentative array definition}} expected-note {{declared here}} addr16-note {{declared here}} addr32-note {{declared here}}
 
 void f2() {
   ++ll[3];
   ++ll[2705843009213693952];
-  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
-  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  // addr16-warning@-1 {{array index 2705843009213693952 refers past the last possible element for an array in 16-bit address space containing 64-bit (8-byte) elements (max possible 8192 elements)}}
+  // addr32-warning@-2 {{array index 2705843009213693952 refers past the last possible element for an array in 32-bit address space containing 64-bit (8-byte) elements (max possible 536870912 elements)}}
+  // addr64-warning@-3 {{array index 2705843009213693952 refers past the last possible element for an array in 64-bit address space containing 64-bit (8-byte) elements (max possible 2305843009213693952 elements)}}
   ++ll[847073650];
-  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  // addr16-warning@-1 {{array index 847073650 refers past the last possible element for an array in 

[PATCH] D104285: [analyzer] Retrieve value by direct index from list initialization of constant array declaration.

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

I'm not sure about whether or not this patch would only work for constant 
arrays with initializer lists.  If it does only work for such arrays, then I 
wonder whether the fix is broad enough -- I haven't tested (yet), but I think 
the presence of the initializer list in the test case is not necessary to 
trigger the spurious warnings about garbage/undefined values.  I'll try it 
tomorrow morning...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104285

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

[PATCH] D104424: [Sema] Fix for PR50741

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 352569.
chrish_ericsson_atx added a comment.

Updated summary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104424

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/unbounded-array-bounds.c

Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -1,9 +1,6 @@
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
-// RUN:  --implicit-check-not 'past the last possible element'
-// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
-// RUN:  --implicit-check-not 'past the last possible element'
-// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
-// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only  -verify=addr64,expected %s
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only  -verify=addr32,expected %s
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only  -verify=addr16,expected %s
 
 struct S {
   long long a;
@@ -12,61 +9,61 @@
   short d;
 };
 
-struct S s[];
+struct S s[]; // expected-warning {{tentative array definition}} expected-note {{declared here}} addr16-note {{declared here}}
 
 void f1() {
   ++s[3].a;
   ++s[7073650413200313099].b;
-  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
-  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  // addr16-warning@16 {{array index 7073650413200313099 refers past the last possible element for an array in 16-bit address space containing 160-bit (20-byte) elements (max possible 3276 elements)}}
+  // addr32-warning@16 {{array index 7073650413200313099 refers past the last possible element for an array in 32-bit address space containing 192-bit (24-byte) elements (max possible 178956970 elements)}}
+  // addr64-warning@16 {{array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)}}
   ++s[7073650].c;
-  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  // addr16-warning@20 {{array index 7073650 refers past the last possible element for an array in 16-bit address space containing 160-bit (20-byte) elements (max possible 3276 elements)}}
 }
 
-long long ll[];
+long long ll[]; // expected-warning {{tentative array definition}} expected-note {{declared here}} addr16-note {{declared here}} addr32-note {{declared here}}
 
 void f2() {
   ++ll[3];
   ++ll[2705843009213693952];
-  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
-  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  // addr16-warning@28 {{array index 2705843009213693952 refers past the last possible element for an array in 16-bit address space containing 64-bit (8-byte) elements (max possible 8192 elements)}}
+  // addr32-warning@28 {{array index 2705843009213693952 refers past the last possible element for an array in 32-bit address space containing 64-bit (8-byte) elements (max possible 536870912 elements)}}
+  // addr64-warning@28 {{array index 2705843009213693952 refers past the last possible element for an array in 64-bit address space containing 64-bit (8-byte) elements (max possible 2305843009213693952 elements)}}
   ++ll[847073650];
-  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  // addr16-warning@32 {{array index 847073650 refers past the last possible element for an array in 16-bit address space containing 64-bit (8-byte) elements (max possible 8192 elements)}}
+  // 

[PATCH] D104424: [Sema] Fix for PR50741

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx created this revision.
chrish_ericsson_atx added reviewers: erichkeane, aaron.ballman, abhinavgaba.
chrish_ericsson_atx requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Fixed crash when doing pointer math on a

  void pointer.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104424

Files:
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/unbounded-array-bounds.c

Index: clang/test/Sema/unbounded-array-bounds.c
===
--- clang/test/Sema/unbounded-array-bounds.c
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -1,9 +1,6 @@
-// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
-// RUN:  --implicit-check-not 'past the last possible element'
-// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
-// RUN:  --implicit-check-not 'past the last possible element'
-// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
-// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only  -verify=addr64,expected %s
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only  -verify=addr32,expected %s
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only  -verify=addr16,expected %s
 
 struct S {
   long long a;
@@ -12,61 +9,61 @@
   short d;
 };
 
-struct S s[];
+struct S s[]; // expected-warning {{tentative array definition}} expected-note {{declared here}} addr16-note {{declared here}}
 
 void f1() {
   ++s[3].a;
   ++s[7073650413200313099].b;
-  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
-  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  // addr16-warning@16 {{array index 7073650413200313099 refers past the last possible element for an array in 16-bit address space containing 160-bit (20-byte) elements (max possible 3276 elements)}}
+  // addr32-warning@16 {{array index 7073650413200313099 refers past the last possible element for an array in 32-bit address space containing 192-bit (24-byte) elements (max possible 178956970 elements)}}
+  // addr64-warning@16 {{array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)}}
   ++s[7073650].c;
-  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  // addr16-warning@20 {{array index 7073650 refers past the last possible element for an array in 16-bit address space containing 160-bit (20-byte) elements (max possible 3276 elements)}}
 }
 
-long long ll[];
+long long ll[]; // expected-warning {{tentative array definition}} expected-note {{declared here}} addr16-note {{declared here}} addr32-note {{declared here}}
 
 void f2() {
   ++ll[3];
   ++ll[2705843009213693952];
-  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
-  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  // addr16-warning@28 {{array index 2705843009213693952 refers past the last possible element for an array in 16-bit address space containing 64-bit (8-byte) elements (max possible 8192 elements)}}
+  // addr32-warning@28 {{array index 2705843009213693952 refers past the last possible element for an array in 32-bit address space containing 64-bit (8-byte) elements (max possible 536870912 elements)}}
+  // addr64-warning@28 {{array index 2705843009213693952 refers past the last possible element for an array in 64-bit address space containing 64-bit (8-byte) elements (max possible 2305843009213693952 elements)}}
   ++ll[847073650];
-  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
-  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  // addr16-warning@32 {{array index 847073650 refers past the last 

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

I'm aware that this commit triggers one failure in ASan/Windows -- I have 
posted a patch for review to address that here: https://reviews.llvm.org/D104141


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

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

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGce44fe199bbf: [Sema] Address-space sensitive check for 
unbounded arrays (v2) (authored by chrish_ericsson_atx).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx14.cpp

Index: clang/test/SemaCXX/constant-expression-cxx14.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx14.cpp
+++ clang/test/SemaCXX/constant-expression-cxx14.cpp
@@ -1027,8 +1027,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{array 'p' declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{the pointer incremented by 18446744073709551615 refers past the last possible element for an array in 64-bit address space containing 32-bit (4-byte) elements (max possible 4611686018427387904 elements)}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: the pointer incremented by 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} 

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 351491.
chrish_ericsson_atx added a comment.

Corrected APSInt.toString() usage to comply with intervening change 
61cdaf66fe22be2b5 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx14.cpp

Index: clang/test/SemaCXX/constant-expression-cxx14.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx14.cpp
+++ clang/test/SemaCXX/constant-expression-cxx14.cpp
@@ -1027,8 +1027,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{array 'p' declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{the pointer incremented by 18446744073709551615 refers past the last possible element for an array in 64-bit address space containing 32-bit (4-byte) elements (max possible 4611686018427387904 elements)}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: the pointer incremented by 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

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

Reverted commit due to buildbot failures -- will update shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

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

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe42a347b7440: [Sema] Address-space sensitive check for 
unbounded arrays (v2) (authored by chrish_ericsson_atx).

Changed prior to commit:
  https://reviews.llvm.org/D88174?vs=350722=351461#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx14.cpp

Index: clang/test/SemaCXX/constant-expression-cxx14.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx14.cpp
+++ clang/test/SemaCXX/constant-expression-cxx14.cpp
@@ -1027,8 +1027,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{array 'p' declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{the pointer incremented by 18446744073709551615 refers past the last possible element for an array in 64-bit address space containing 32-bit (4-byte) elements (max possible 4611686018427387904 elements)}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: the pointer incremented by 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // 

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 350722.
chrish_ericsson_atx added a comment.

Refreshed previously-reverted changeset and re-tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx14.cpp

Index: clang/test/SemaCXX/constant-expression-cxx14.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx14.cpp
+++ clang/test/SemaCXX/constant-expression-cxx14.cpp
@@ -1027,8 +1027,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{array 'p' declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{the pointer incremented by 18446744073709551615 refers past the last possible element for an array in 64-bit address space containing 32-bit (4-byte) elements (max possible 4611686018427387904 elements)}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: the pointer incremented by 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:3: warning: {{.*}} 

[PATCH] D89055: [analyzer] Wrong type cast occures during pointer dereferencing after type punning

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.
Herald added a subscriber: manas.



Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:763-765
+  if (const auto *ER = dyn_cast(R)) {
+R = StateMgr.getStoreManager().castRegion(ER, CastTy);
+return loc::MemRegionVal(R);

This causes new false-positive static analysis warnings from 
core.CallAndMessage and core.UndefinedBinaryOperatorResult, (and possibly 
others?), possibly because of loss of array extent information.  See 
https://bugs.llvm.org/show_bug.cgi?id=50604.  



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89055

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

[PATCH] D101108: [PR49761] Fix variadic arg handling in matcher

[Chris Hamilton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcae3b70cebc1: [PR49761] Fix variadic arg handling in matcher 
(authored by chrish_ericsson_atx).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101108

Files:
  clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h


Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4757,8 +4757,11 @@
 
   int ParamIndex = 0;
   bool Matched = false;
+  unsigned NumArgs = Node.getNumArgs();
+  if (FProto && FProto->isVariadic())
+NumArgs = std::min(NumArgs, FProto->getNumParams());
 
-  for (; ArgIndex < Node.getNumArgs(); ++ArgIndex, ++ParamIndex) {
+  for (; ArgIndex < NumArgs; ++ArgIndex, ++ParamIndex) {
 BoundNodesTreeBuilder ArgMatches(*Builder);
 if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()), 
Finder,
)) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
@@ -386,3 +386,18 @@
   do {
   } while (false && CondVar);
 }
+
+struct logger {
+  void (*debug)(struct logger *, const char *, ...);
+};
+
+int foo(void) {
+  struct logger *pl = 0;
+  int iterator = 0;
+  while (iterator < 10) {
+char *l_tmp_msg = 0;
+pl->debug(pl, "%d: %s\n", iterator, l_tmp_msg);
+iterator++;
+  }
+  return 0;
+}


Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4757,8 +4757,11 @@
 
   int ParamIndex = 0;
   bool Matched = false;
+  unsigned NumArgs = Node.getNumArgs();
+  if (FProto && FProto->isVariadic())
+NumArgs = std::min(NumArgs, FProto->getNumParams());
 
-  for (; ArgIndex < Node.getNumArgs(); ++ArgIndex, ++ParamIndex) {
+  for (; ArgIndex < NumArgs; ++ArgIndex, ++ParamIndex) {
 BoundNodesTreeBuilder ArgMatches(*Builder);
 if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()), Finder,
)) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
@@ -386,3 +386,18 @@
   do {
   } while (false && CondVar);
 }
+
+struct logger {
+  void (*debug)(struct logger *, const char *, ...);
+};
+
+int foo(void) {
+  struct logger *pl = 0;
+  int iterator = 0;
+  while (iterator < 10) {
+char *l_tmp_msg = 0;
+pl->debug(pl, "%d: %s\n", iterator, l_tmp_msg);
+iterator++;
+  }
+  return 0;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101108: [PR49761] Fix variadic arg handling in matcher

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 340094.
chrish_ericsson_atx added a comment.

Addressed feedback from aaron.ballman.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101108

Files:
  clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h


Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4757,8 +4757,11 @@
 
   int ParamIndex = 0;
   bool Matched = false;
+  unsigned NumArgs = Node.getNumArgs();
+  if (FProto && FProto->isVariadic())
+NumArgs = std::min(NumArgs, FProto->getNumParams());
 
-  for (; ArgIndex < Node.getNumArgs(); ++ArgIndex, ++ParamIndex) {
+  for (; ArgIndex < NumArgs; ++ArgIndex, ++ParamIndex) {
 BoundNodesTreeBuilder ArgMatches(*Builder);
 if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()), 
Finder,
)) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
@@ -386,3 +386,18 @@
   do {
   } while (false && CondVar);
 }
+
+struct logger {
+  void (*debug)(struct logger *, const char *, ...);
+};
+
+int foo(void) {
+  struct logger *pl = 0;
+  int iterator = 0;
+  while (iterator < 10) {
+char *l_tmp_msg = 0;
+pl->debug(pl, "%d: %s\n", iterator, l_tmp_msg);
+iterator++;
+  }
+  return 0;
+}


Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4757,8 +4757,11 @@
 
   int ParamIndex = 0;
   bool Matched = false;
+  unsigned NumArgs = Node.getNumArgs();
+  if (FProto && FProto->isVariadic())
+NumArgs = std::min(NumArgs, FProto->getNumParams());
 
-  for (; ArgIndex < Node.getNumArgs(); ++ArgIndex, ++ParamIndex) {
+  for (; ArgIndex < NumArgs; ++ArgIndex, ++ParamIndex) {
 BoundNodesTreeBuilder ArgMatches(*Builder);
 if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()), Finder,
)) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
@@ -386,3 +386,18 @@
   do {
   } while (false && CondVar);
 }
+
+struct logger {
+  void (*debug)(struct logger *, const char *, ...);
+};
+
+int foo(void) {
+  struct logger *pl = 0;
+  int iterator = 0;
+  while (iterator < 10) {
+char *l_tmp_msg = 0;
+pl->debug(pl, "%d: %s\n", iterator, l_tmp_msg);
+iterator++;
+  }
+  return 0;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D101108: [PR49761] Fix variadic arg handling in matcher

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4760-4765
+  unsigned numArgs = Node.getNumArgs();
+  if (FProto && FProto->isVariadic() && FProto->getNumParams() < numArgs) {
+numArgs = FProto->getNumParams();
+  }
 
+  for (; ArgIndex < numArgs; ++ArgIndex, ++ParamIndex) {

aaron.ballman wrote:
> 
Thanks Aaron!  Nice simplification.  I don't know why I can't remember the 
variable name style -- old habits die hard!  :)   Thanks for the feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101108

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

[PATCH] D101108: [PR49761] Fix variadic arg handling in matcher

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx created this revision.
chrish_ericsson_atx requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.

Mishandling of variadic arguments in a function call caused a crash
(runtime assert fail) in bugprone-infinite-loop tidy checker.  Fix
is to limit argument matching to the lesser of the number of variadic
params in the prototype or the number of actual args in the call.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D101108

Files:
  clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
  clang/include/clang/ASTMatchers/ASTMatchers.h


Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4757,8 +4757,12 @@
 
   int ParamIndex = 0;
   bool Matched = false;
+  unsigned numArgs = Node.getNumArgs();
+  if (FProto && FProto->isVariadic() && FProto->getNumParams() < numArgs) {
+numArgs = FProto->getNumParams();
+  }
 
-  for (; ArgIndex < Node.getNumArgs(); ++ArgIndex, ++ParamIndex) {
+  for (; ArgIndex < numArgs; ++ArgIndex, ++ParamIndex) {
 BoundNodesTreeBuilder ArgMatches(*Builder);
 if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()), 
Finder,
)) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
@@ -386,3 +386,18 @@
   do {
   } while (false && CondVar);
 }
+
+struct logger {
+  void (*debug)(struct logger *, const char *, ...);
+};
+
+int foo(void) {
+  struct logger *pl = 0;
+  int iterator = 0;
+  while (iterator < 10) {
+char *l_tmp_msg = 0;
+pl->debug(pl, "%d: %s\n", iterator, l_tmp_msg);
+iterator++;
+  }
+  return 0;
+}


Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -4757,8 +4757,12 @@
 
   int ParamIndex = 0;
   bool Matched = false;
+  unsigned numArgs = Node.getNumArgs();
+  if (FProto && FProto->isVariadic() && FProto->getNumParams() < numArgs) {
+numArgs = FProto->getNumParams();
+  }
 
-  for (; ArgIndex < Node.getNumArgs(); ++ArgIndex, ++ParamIndex) {
+  for (; ArgIndex < numArgs; ++ArgIndex, ++ParamIndex) {
 BoundNodesTreeBuilder ArgMatches(*Builder);
 if (ArgMatcher.matches(*(Node.getArg(ArgIndex)->IgnoreParenCasts()), Finder,
)) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop.cpp
@@ -386,3 +386,18 @@
   do {
   } while (false && CondVar);
 }
+
+struct logger {
+  void (*debug)(struct logger *, const char *, ...);
+};
+
+int foo(void) {
+  struct logger *pl = 0;
+  int iterator = 0;
+  while (iterator < 10) {
+char *l_tmp_msg = 0;
+pl->debug(pl, "%d: %s\n", iterator, l_tmp_msg);
+iterator++;
+  }
+  return 0;
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86743: [analyzer] Ignore VLASizeChecker case that could cause crash

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Is anything planned or in progress that would address this issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86743

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

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

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

Reverted due to another obscure test failure.  Working to diagnose.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

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

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd9ee935679e7: [Sema] Address-space sensitive check for 
unbounded arrays (v2) (authored by chrish_ericsson_atx).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{the pointer incremented by 18446744073709551615 refers past the last possible element for an array in 64-bit address space containing 32-bit (4-byte) elements (max possible 4611686018427387904 elements)}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: the pointer incremented by 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // 

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

I'm going to land this as-is, based on Bevin's LGTM and my own confidence that 
the 10 lines I added are correct (enough).  @aaron.ballman , please reopen this 
review if you have additional comments or concerns.  Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

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

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Ping.  :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

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

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:14054
+  if (IndexNegated) {
+index.setIsUnsigned(false);
 index = -index;

This call to index.setIsUnsigned(false) is the only unreviewed part of this 
change in this file.



Comment at: clang/test/Sema/unbounded-array-bounds.c:72-80
+void f6() {
+  int ints[] = {1, 3, 5, 7, 8, 6, 4, 5, 9};
+  int const n_ints = sizeof(ints) / sizeof(int);
+  unsigned long long const N = 3;
+
+  int *middle = [0] + n_ints / 2;
+  // Should NOT produce a warning.

This function is the only unreviewed part of this change in this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88174

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

[PATCH] D88174: [Sema] Address-space sensitive check for unbounded arrays (v2)

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx created this revision.
chrish_ericsson_atx added reviewers: aaron.ballman, rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
chrish_ericsson_atx requested review of this revision.

Check applied to unbounded (incomplete) arrays and pointers to spot
cases where the computed address is beyond the largest possible
addressable extent of the array, based on the address space in which the
array is delcared, or which the pointer refers to.

Check helps to avoid cases of nonsense pointer math and array indexing
which could lead to linker failures or runtime exceptions.  Of
particular interest when building for embedded systems with small
address spaces.

This is version 2 of this patch -- version 1 had some testing issues
due to a sign error in existing code.  That error is corrected and
lit test for this chagne is extended to verify the fix.

Originally reviewed/accepted by: aaron.ballman
Original revision: https://reviews.llvm.org/D86796


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88174

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{the pointer incremented by 18446744073709551615 refers past the last possible element for an array in 64-bit address space containing 32-bit (4-byte) elements (max possible 4611686018427387904 elements)}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past 

[PATCH] D87942: [Analyzer] GNU named variadic macros in Plister

[Chris Hamilton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2697d138a65a: [Analyzer] GNU named variadic macros in 
Plister (authored by chrish_ericsson_atx).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87942

Files:
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  
clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  clang/test/Analysis/plist-macros-with-expansion.cpp

Index: clang/test/Analysis/plist-macros-with-expansion.cpp
===
--- clang/test/Analysis/plist-macros-with-expansion.cpp
+++ clang/test/Analysis/plist-macros-with-expansion.cpp
@@ -529,3 +529,17 @@
 // FIXME: Stringify and escape __VA_ARGS__ correctly.
 // CHECK: nameSTRINGIFIED_VA_ARGS
 // CHECK-NEXT: expansionvariadicCFunction(x, Additional supply depots required., );x = 0;
+
+// bz44493: Support GNU-style named variadic arguments in plister
+#define BZ44493_GNUVA(i, args...)  --(i);
+
+int bz44493(void) {
+  int a = 2;
+  BZ44493_GNUVA(a);
+  BZ44493_GNUVA(a, "arg2");
+  (void)(10 / a); // expected-warning{{Division by zero}}
+  return 0;
+}
+
+// CHECK: nameBZ44493_GNUVA
+// CHECK-NEXT: expansion--(a);
Index: clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -6787,6 +6787,142 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line537
+   col3
+   file0
+  
+  
+   line537
+   col5
+   file0
+  
+ 
+end
+ 
+  
+   line539
+   col3
+   file0
+  
+  
+   line539
+   col15
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line539
+  col3
+  file0
+ 
+ ranges
+ 
+   
+
+ line539
+ col3
+ file0
+
+
+ line539
+ col26
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ The value 0 is assigned to a
+ message
+ The value 0 is assigned to a
+
+
+ kindevent
+ location
+ 
+  line540
+  col13
+  file0
+ 
+ ranges
+ 
+   
+
+ line540
+ col10
+ file0
+
+
+ line540
+ col15
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line539
+  col3
+  file0
+ 
+ nameBZ44493_GNUVA
+ expansion--(a);
+
+   
+   descriptionDivision by zero
+   categoryLogic error
+   typeDivision by zero
+   check_namecore.DivideZero
+   
+   issue_hash_content_of_line_in_context21c6d180d8c8c30cf730b7a7136980a9
+  issue_context_kindfunction
+  issue_contextbz44493
+  issue_hash_function_offset4
+  location
+  
+   line540
+   col13
+   file0
+  
+  ExecutedLines
+  
+   0
+   
+536
+537
+538
+539
+540
+   
+  
+  
  
  files
  
Index: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -1132,7 +1132,7 @@
   std::string MacroName = PP.getSpelling(TheTok);
 
   const auto *II = PP.getIdentifierInfo(MacroName);
-  assert(II && "Failed to acquire the IndetifierInfo for the macro!");
+  assert(II && "Failed to acquire the IdentifierInfo for the macro!");
 
   const MacroInfo *MI = getMacroInfoForLocation(PP, SM, II, ExpanLoc);
   // assert(MI && "The macro must've been defined at it's expansion location!");
@@ -1180,9 +1180,16 @@
   //   * > 1, then tok::comma is a part of the current arg.
   int ParenthesesDepth = 1;
 
-  // If we encounter __VA_ARGS__, we will lex until the closing tok::r_paren,
-  // even if we lex a tok::comma and ParanthesesDepth == 1.
-  const IdentifierInfo *__VA_ARGS__II = PP.getIdentifierInfo("__VA_ARGS__");
+  // If we encounter the variadic arg, we will lex until the closing
+  // tok::r_paren, even if we lex a tok::comma and ParanthesesDepth == 1.
+  const IdentifierInfo *VariadicParamII = PP.getIdentifierInfo("__VA_ARGS__");
+  if (MI->isGNUVarargs()) {
+// If macro uses GNU-style variadic args, the param name is user-supplied,
+// an not "__VA_ARGS__".  E.g.:
+//   #define FOO(a, b, myvargs...)
+// 

[PATCH] D87942: [Analyzer] GNU named variadic macros in Plister

[Chris Hamilton via Phabricator via cfe-commits]

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

Thanks for the quick feedback, Kristof!  I changed the macro to be just a 
decrement operator, and triggered DBZ with that.  Added the checks as you 
suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87942

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

[PATCH] D87942: [Analyzer] GNU named variadic macros in Plister

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 293154.
chrish_ericsson_atx added a comment.

Addressed feedback from @Szelethus


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87942

Files:
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  
clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  clang/test/Analysis/plist-macros-with-expansion.cpp

Index: clang/test/Analysis/plist-macros-with-expansion.cpp
===
--- clang/test/Analysis/plist-macros-with-expansion.cpp
+++ clang/test/Analysis/plist-macros-with-expansion.cpp
@@ -529,3 +529,17 @@
 // FIXME: Stringify and escape __VA_ARGS__ correctly.
 // CHECK: nameSTRINGIFIED_VA_ARGS
 // CHECK-NEXT: expansionvariadicCFunction(x, Additional supply depots required., );x = 0;
+
+// bz44493: Support GNU-style named variadic arguments in plister
+#define BZ44493_GNUVA(i, args...)  --(i);
+
+int bz44493(void) {
+  int a = 2;
+  BZ44493_GNUVA(a);
+  BZ44493_GNUVA(a, "arg2");
+  (void)(10 / a); // expected-warning{{Division by zero}}
+  return 0;
+}
+
+// CHECK: nameBZ44493_GNUVA
+// CHECK-NEXT: expansion--(a);
Index: clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -6787,6 +6787,142 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line537
+   col3
+   file0
+  
+  
+   line537
+   col5
+   file0
+  
+ 
+end
+ 
+  
+   line539
+   col3
+   file0
+  
+  
+   line539
+   col15
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line539
+  col3
+  file0
+ 
+ ranges
+ 
+   
+
+ line539
+ col3
+ file0
+
+
+ line539
+ col26
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ The value 0 is assigned to a
+ message
+ The value 0 is assigned to a
+
+
+ kindevent
+ location
+ 
+  line540
+  col13
+  file0
+ 
+ ranges
+ 
+   
+
+ line540
+ col10
+ file0
+
+
+ line540
+ col15
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ Division by zero
+ message
+ Division by zero
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line539
+  col3
+  file0
+ 
+ nameBZ44493_GNUVA
+ expansion--(a);
+
+   
+   descriptionDivision by zero
+   categoryLogic error
+   typeDivision by zero
+   check_namecore.DivideZero
+   
+   issue_hash_content_of_line_in_context21c6d180d8c8c30cf730b7a7136980a9
+  issue_context_kindfunction
+  issue_contextbz44493
+  issue_hash_function_offset4
+  location
+  
+   line540
+   col13
+   file0
+  
+  ExecutedLines
+  
+   0
+   
+536
+537
+538
+539
+540
+   
+  
+  
  
  files
  
Index: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
===
--- clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
+++ clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
@@ -1132,7 +1132,7 @@
   std::string MacroName = PP.getSpelling(TheTok);
 
   const auto *II = PP.getIdentifierInfo(MacroName);
-  assert(II && "Failed to acquire the IndetifierInfo for the macro!");
+  assert(II && "Failed to acquire the IdentifierInfo for the macro!");
 
   const MacroInfo *MI = getMacroInfoForLocation(PP, SM, II, ExpanLoc);
   // assert(MI && "The macro must've been defined at it's expansion location!");
@@ -1180,9 +1180,16 @@
   //   * > 1, then tok::comma is a part of the current arg.
   int ParenthesesDepth = 1;
 
-  // If we encounter __VA_ARGS__, we will lex until the closing tok::r_paren,
-  // even if we lex a tok::comma and ParanthesesDepth == 1.
-  const IdentifierInfo *__VA_ARGS__II = PP.getIdentifierInfo("__VA_ARGS__");
+  // If we encounter the variadic arg, we will lex until the closing
+  // tok::r_paren, even if we lex a tok::comma and ParanthesesDepth == 1.
+  const IdentifierInfo *VariadicParamII = PP.getIdentifierInfo("__VA_ARGS__");
+  if (MI->isGNUVarargs()) {
+// If macro uses GNU-style variadic args, the param name is user-supplied,
+// an not "__VA_ARGS__".  E.g.:
+//   #define FOO(a, b, myvargs...)
+// In this case, just use the last parameter:
+VariadicParamII = *(MacroParams.rbegin());
+  }
 
   for (const 

[PATCH] D87942: [Analyzer] GNU named variadic macros in Plister

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: clang/test/Analysis/plist-macros-with-expansion.cpp:536
+  do {   \
+  } while (0)
+

Seems the do {} while (0) idiom is a problem, because the plist output gets 
formatted differently on windows than it does on Linux.   (Extra space).  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87942

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

[PATCH] D87942: [Analyzer] GNU named variadic macros in Plister

[Chris Hamilton via Phabricator via cfe-commits]

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

Added support for GNU named variadic macros in
macro expansion for plist generation.

Fix for https://bugs.llvm.org/show_bug.cgi?id=44493


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87942

Files:
  clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp
  
clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
  clang/test/Analysis/plist-macros-with-expansion.cpp

Index: clang/test/Analysis/plist-macros-with-expansion.cpp
===
--- clang/test/Analysis/plist-macros-with-expansion.cpp
+++ clang/test/Analysis/plist-macros-with-expansion.cpp
@@ -529,3 +529,17 @@
 // FIXME: Stringify and escape __VA_ARGS__ correctly.
 // CHECK: nameSTRINGIFIED_VA_ARGS
 // CHECK-NEXT: expansionvariadicCFunction(x, Additional supply depots required., );x = 0;
+
+// bz44493: Support GNU-style named variadic arguments in plister
+#define bz44493_log(args...) \
+  do {   \
+  } while (0)
+
+int bz44493(void) {
+  int x = 1;
+  bz44493_log("arg");
+  bz44493_log("arg", "arg2");
+  int a;
+  a & 0xffc0; // expected-warning{{expression result unused}}
+  return 0;
+}
Index: clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
===
--- clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
+++ clang/test/Analysis/Inputs/expected-plists/plist-macros-with-expansion.cpp.plist
@@ -6787,6 +6787,221 @@

   
   
+  
+   path
+   
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line539
+   col3
+   file0
+  
+  
+   line539
+   col5
+   file0
+  
+ 
+end
+ 
+  
+   line540
+   col3
+   file0
+  
+  
+   line540
+   col13
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line542
+  col3
+  file0
+ 
+ ranges
+ 
+   
+
+ line542
+ col3
+ file0
+
+
+ line542
+ col7
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ a declared without an initial value
+ message
+ a declared without an initial value
+
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line542
+   col3
+   file0
+  
+  
+   line542
+   col5
+   file0
+  
+ 
+end
+ 
+  
+   line543
+   col3
+   file0
+  
+  
+   line543
+   col3
+   file0
+  
+ 
+   
+  
+
+
+ kindcontrol
+ edges
+  
+   
+start
+ 
+  
+   line543
+   col3
+   file0
+  
+  
+   line543
+   col3
+   file0
+  
+ 
+end
+ 
+  
+   line543
+   col5
+   file0
+  
+  
+   line543
+   col5
+   file0
+  
+ 
+   
+  
+
+
+ kindevent
+ location
+ 
+  line543
+  col5
+  file0
+ 
+ ranges
+ 
+   
+
+ line543
+ col3
+ file0
+
+
+ line543
+ col3
+ file0
+
+   
+ 
+ depth0
+ extended_message
+ The left operand of  is a garbage value
+ message
+ The left operand of  is a garbage value
+
+   
+   macro_expansions
+   
+
+ location
+ 
+  line540
+  col3
+  file0
+ 
+ namebz44493_log
+ expansiondo { } while(0)
+
+
+ location
+ 
+  line541
+  col3
+  file0
+ 
+ namebz44493_log
+ expansiondo { } while(0)
+
+   
+   descriptionThe left operand of  is a garbage value
+   categoryLogic error
+   typeResult of operation is garbage or undefined
+   check_namecore.UndefinedBinaryOperatorResult
+   
+   issue_hash_content_of_line_in_context7515419ef7ac803c938efbeaf5ed1c6f
+  issue_context_kindfunction
+  issue_contextbz44493
+  issue_hash_function_offset5
+  location
+  
+   line543
+   col5
+   file0
+  
+  ExecutedLines
+  
+   0
+   
+538
+539
+540
+541
+542
+543
+   
+  
+  
  
  files
  
Index: 

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGda55e9ba1273: [Sema] Address-space sensitive index check for 
unbounded arrays (authored by chrish_ericsson_atx).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{the pointer incremented by 18446744073709551615 refers past the last possible element for an array in 64-bit address space containing 32-bit (4-byte) elements (max possible 4611686018427387904 elements)}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,70 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: the pointer incremented by 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // 

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 11 inline comments as done.
chrish_ericsson_atx added a comment.

Addressed all feedback from Aaron, except for two comments about reachability 
that I don't understand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

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

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 291184.
chrish_ericsson_atx added a comment.

Addressed feedback from Aaron Ballman


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{the pointer incremented by 18446744073709551615 refers past the last possible element for an array in 64-bit address space containing 32-bit (4-byte) elements (max possible 4611686018427387904 elements)}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,70 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: array index 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: the pointer incremented by 7073650413200313099 refers past the last possible element for an array in 64-bit address space containing 256-bit (32-byte) elements (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:3: warning: {{.*}} past the last possible element 

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Thank you for the comments, @aaron.ballman .   I'll update with the changes you 
requested shortly.  I did have some requests for clarification of you, though.  
Thanks!




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8841-8844
+def warn_array_index_exceeds_max_addressable_bounds : Warning<
+  "array index %0 refers past the last possible element for an array in %1-bit 
"
+  "address space containing %2-bit (%3-byte) elements (max possible %4 
element%s5)">,
+  InGroup;

aaron.ballman wrote:
> I'd combine this with the above diagnostic given how similar they are:
> ```
> def warn_exceeds_max_addressable_bounds : Warning<
>   "%select{array index %1|the pointer incremented by %1}0 refers past the 
> last possible element for an array in %2-bit "
>   "address space containing %3-bit (%4-byte) elements (max possible %5 
> element%s6)">,
>   InGroup;
> ```
I was attempting to follow the pattern set by the preceding four definitions, 
which keep pointer math warnings and array index warnings separated, but are 
otherwise nearly identical.  If there's value in that pattern for those other 
warnings, I would assume the same value applies to keeping these separated.  
Please re-ping if you disagree, otherwise, I'll leave these as two separate 
warnings.



Comment at: clang/lib/Sema/SemaChecking.cpp:14063
+  if (isUnboundedArray) {
+if (index.isUnsigned() || !index.isNegative()) {
+  const auto  = getASTContext();

aaron.ballman wrote:
> ebevhan wrote:
> > This could be early return to avoid the indentation.
> +1 to using early return here. I might even get rid of `isUnboundedArray` and 
> just use `!BaseType`
@ebevhan's comment about early return was actually addressed in a previous 
diff... I should have marked it as such.  My apologies.

I'd prefer to keep `isUnboundedArray` (with the case corrected, of course), as 
it seems much clearer than `!BaseType` in terms of expressing what we are 
actually checking here, and why.



Comment at: clang/lib/Sema/SemaChecking.cpp:14100
+  // dependent CharUnits)
+  DiagRuntimeBehavior(BaseExpr->getBeginLoc(), BaseExpr,
+  PDiag(DiagID)

aaron.ballman wrote:
> It's not clear to me whether we need to pay the extra expense of doing 
> reachability checks for the diagnostic -- do you have evidence that there 
> would be a high false positive rate if we always emit the diagnostic?
Sorry-- I'm not following you here, @aaron.ballman. What reachability check are 
you referring to?  The diagnostic isn't conditioned on anything (at least not 
here, where your comment appears...), so I'm not sure what change you are 
suggesting that would "always emit the diagnostic"...  Sorry to be slow on the 
uptake here... Could you clarify that for me?



Comment at: clang/lib/Sema/SemaChecking.cpp:14111
+// Try harder to find a NamedDecl to point at in the note.
+while (const ArraySubscriptExpr *ASE =
+   dyn_cast(BaseExpr))

aaron.ballman wrote:
> You can use `const auto *` here since the type is spelled out in the 
> initialization. Same below.
Fair point -- this was just cut-and-paste from the code I had to duplicate in 
order to address the early return comment from @ebevhan.  :)  I'll change it in 
both places.



Comment at: clang/lib/Sema/SemaChecking.cpp:14121
+  if (ND)
+DiagRuntimeBehavior(ND->getBeginLoc(), BaseExpr,
+PDiag(diag::note_array_declared_here) << ND);

aaron.ballman wrote:
> Similar comment here about reachability.
Similar request for clarification.  ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

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

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

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

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Thanks for the excellent feedback, @ebevhan .   @aaron.ballman , @krememek , or 
@rsmith , could one of you take a look at this change and if it's acceptable, 
please approve it?   I have not requested commit privileges yet, either, so I 
will need your assistance to land this change, assuming it's acceptable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

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

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 289819.
chrish_ericsson_atx added a comment.

NC push did not resolve failed test.  Rebased in hopes that whatever has 
broken the build has been resolved in the intervening commits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{refers past the last possible element}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,70 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  p += 7073650;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+struct BQ {
+  struct S 

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 289815.
chrish_ericsson_atx added a comment.

NC.  Pushing null change in hopes of re-triggering testing.

Unit test that failed is low-level LLVM test, which doesn't even exercise the 
code I've changed here, so I'm assuming it's a spurious failure in the CI
machinery.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{refers past the last possible element}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,70 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  p += 7073650;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element 

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaChecking.cpp:13966
   if (index.isUnsigned() || !index.isNegative()) {
-// It is possible that the type of the base expression after
-// IgnoreParenCasts is incomplete, even though the type of the base
-// expression before IgnoreParenCasts is complete (see PR39746 for an
-// example). In this case we have no information about whether the array
-// access exceeds the array bounds. However we can still diagnose an array
-// access which precedes the array bounds.
-if (BaseType->isIncompleteType())
-  return;
+if (isUnboundedArray) {
+  const auto  = getASTContext();

chrish_ericsson_atx wrote:
> ebevhan wrote:
> > chrish_ericsson_atx wrote:
> > > ebevhan wrote:
> > > > It might simplify the patch to move this condition out of the tree and 
> > > > just early return for the other case. That is:
> > > > 
> > > > ```
> > > > if (isUnboundedArray) {
> > > >   if (!(index.isUnsigned() || !index.isNegative()))
> > > > return;
> > > > 
> > > >   ...
> > > >   return;
> > > > }
> > > > 
> > > > if (index.isUnsigned() ...
> > > > ```
> > > There's a bit more code (starting at line 14094 in this patch set) that 
> > > applies in all cases, so an early return here would prevent the "Array 
> > > declared here" note from being generated.
> > Ah, the note.
> > 
> > I wonder if it wouldn't be cleaner (and avoid indenting the entire block) 
> > if that was just duplicated.
> Hard to say which is cleaner -- it's a tradeoff.  Nesting level would be 
> shallower if that code was duplicated, but then again, the duplication 
> increases the chance of an incomplete fix should an issue be discovered in 
> that code.  Overall code would be slightly longer, as well (adding about 16 
> lines of code, but removing only 4).   To me, the current strategy feels more 
> surgical, but I'll change it if you feel more strongly about it than I do.   
> Please re-ping if you do.
I reconsidered and made the changes you suggested.  Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

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

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 289785.
chrish_ericsson_atx added a comment.

Refactored as ebevhan suggested to simplify patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{refers past the last possible element}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,70 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  p += 7073650;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+struct BQ {
+  struct S bigblock[3276];
+};
+
+struct BQ bq[];
+
+void f5() {
+  ++bq[0].bigblock[0].a;
+  

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 2 inline comments as done.
chrish_ericsson_atx added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:13966
   if (index.isUnsigned() || !index.isNegative()) {
-// It is possible that the type of the base expression after
-// IgnoreParenCasts is incomplete, even though the type of the base
-// expression before IgnoreParenCasts is complete (see PR39746 for an
-// example). In this case we have no information about whether the array
-// access exceeds the array bounds. However we can still diagnose an array
-// access which precedes the array bounds.
-if (BaseType->isIncompleteType())
-  return;
+if (isUnboundedArray) {
+  const auto  = getASTContext();

ebevhan wrote:
> chrish_ericsson_atx wrote:
> > ebevhan wrote:
> > > It might simplify the patch to move this condition out of the tree and 
> > > just early return for the other case. That is:
> > > 
> > > ```
> > > if (isUnboundedArray) {
> > >   if (!(index.isUnsigned() || !index.isNegative()))
> > > return;
> > > 
> > >   ...
> > >   return;
> > > }
> > > 
> > > if (index.isUnsigned() ...
> > > ```
> > There's a bit more code (starting at line 14094 in this patch set) that 
> > applies in all cases, so an early return here would prevent the "Array 
> > declared here" note from being generated.
> Ah, the note.
> 
> I wonder if it wouldn't be cleaner (and avoid indenting the entire block) if 
> that was just duplicated.
Hard to say which is cleaner -- it's a tradeoff.  Nesting level would be 
shallower if that code was duplicated, but then again, the duplication 
increases the chance of an incomplete fix should an issue be discovered in that 
code.  Overall code would be slightly longer, as well (adding about 16 lines of 
code, but removing only 4).   To me, the current strategy feels more surgical, 
but I'll change it if you feel more strongly about it than I do.   Please 
re-ping if you do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

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

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 289753.
chrish_ericsson_atx marked 2 inline comments as done.
chrish_ericsson_atx added a comment.

Addressed reviewer feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{refers past the last possible element}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,70 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  p += 7073650;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+struct BQ {
+  struct S bigblock[3276];
+};
+
+struct BQ bq[];
+
+void f5() {
+  

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 3 inline comments as done.
chrish_ericsson_atx added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:13981
+bool overflow;
+llvm::APInt product(index);
+product += 1;

ebevhan wrote:
> What if index is wider than AddrBits, but the active bits are fewer? Then you 
> might miss out on triggering the overflow case in the multiplication.
Line 13984 checks for active bits of product being less than AddrBits, which is 
the same case (since product, by definition, has same width as index).  So I 
think this is covered.  If I've misunderstood, please re-ping.



Comment at: clang/lib/Sema/SemaChecking.cpp:13992
+  MaxElems =
+  MaxElems.zext(std::max(AddrBits << 1, apElemBytes.getBitWidth()));
+  MaxElems += 1;

ebevhan wrote:
> Should this not be AddrBits + 1 if you want to add 1 below?
AddrBits + 1 would also work.  I chose AddrBits << 1 figuring that the width 
would end up being a nice clean power of 2, but that's not necessarily true.  
Functionally, the effect of either approach is identical.  I suppose + 1 is a 
bit more obvious, though.  I'll make this change.



Comment at: clang/lib/Sema/SemaChecking.cpp:13993
+  MaxElems.zext(std::max(AddrBits << 1, apElemBytes.getBitWidth()));
+  MaxElems += 1;
+  if (MaxElems.getBitWidth() < apElemBytes.getBitWidth())

ebevhan wrote:
> Though, why is the +1 here? Isn't this already the maximum number of elements?
Initial value of MaxElems is APInt::getMaxValue(AddrBits), which is the index 
of the last addressable CharUnit in the address space.  Adding 1 makes it the 
total number of addressable CharUnits in the address space, which is what we 
want as the numerator for computing total number of elements of a given size 
that will fit in that address space.




Comment at: clang/lib/Sema/SemaChecking.cpp:13994-13997
+  if (MaxElems.getBitWidth() < apElemBytes.getBitWidth())
+MaxElems = MaxElems.zext(apElemBytes.getBitWidth());
+  else if (apElemBytes.getBitWidth() < MaxElems.getBitWidth())
+apElemBytes = apElemBytes.zext(MaxElems.getBitWidth());

ebevhan wrote:
> MaxElems should already be at a sufficient width here because of the earlier 
> max. You can probably just do apElemBytes = 
> apElemBytes.zextOrTrunc(MaxElems.getBitWidth())?
Yep -- you are right.  index and apElemBytes are already guaranteed to have 
equal width (>= AddrBits), and MaxElems is guaranteed to have the same width or 
double width.  So the single zext of apElemBytes will do fine.  Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

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

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 2 inline comments as done.
chrish_ericsson_atx added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:13989
+  MaxElems <<= AddrBits;
+  MaxElems /= ElemBytes;
+

ebevhan wrote:
> The size calculations here could probably be simplified by doing something 
> like this:
> 
> * If getActiveBits of the index is greater than AddrBits, it's indexing 
> outside
> * Construct an AddrBits-wide APInt containing the index value
> * Use umul_ovf with getTypeSizeInChars(ElementType); if that overflows, it's 
> indexing outside
> 
Refactored as suggested.  I agree -- it's cleaner this way (eliminates the 
loop), and avoids a divide when no diag is required.  But it's still hard to 
read because of all the APInt bitwidth hijinks necessary to do the math without 
tripping asserts. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

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

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 289503.
chrish_ericsson_atx added a comment.

Updating D86796 : [Sema] Address-space 
sensitive index check for unbounded arrays

Refactored math as suggested by Bevin Hansson.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{refers past the last possible element}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,70 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  p += 7073650;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

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

I will tinker with the math to simplify as you suggest.  Working with APInt and 
APSInt seems to promulgate sensitive and brittle code, which makes trying 
alternative expressions more tedious than I'd like (which is why I bailed on an 
earlier attempt to simplify this).  However, that same observation about 
brittle code supports the goal that simpler math would be safer, as there would 
presumably be fewer opportunities for AP/APSInt to misbehave as they interact.




Comment at: clang/lib/Sema/SemaChecking.cpp:13966
   if (index.isUnsigned() || !index.isNegative()) {
-// It is possible that the type of the base expression after
-// IgnoreParenCasts is incomplete, even though the type of the base
-// expression before IgnoreParenCasts is complete (see PR39746 for an
-// example). In this case we have no information about whether the array
-// access exceeds the array bounds. However we can still diagnose an array
-// access which precedes the array bounds.
-if (BaseType->isIncompleteType())
-  return;
+if (isUnboundedArray) {
+  const auto  = getASTContext();

ebevhan wrote:
> It might simplify the patch to move this condition out of the tree and just 
> early return for the other case. That is:
> 
> ```
> if (isUnboundedArray) {
>   if (!(index.isUnsigned() || !index.isNegative()))
> return;
> 
>   ...
>   return;
> }
> 
> if (index.isUnsigned() ...
> ```
There's a bit more code (starting at line 14094 in this patch set) that applies 
in all cases, so an early return here would prevent the "Array declared here" 
note from being generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

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

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 288995.
chrish_ericsson_atx added a comment.

Corrected formatting (per git-clang-format)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{refers past the last possible element}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  p += 7073650;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
Index: clang/test/Sema/const-eval.c
===
--- 

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 288649.
chrish_ericsson_atx added a comment.

Removed Change-Id from commit log message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{refers past the last possible element}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  p += 7073650;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
Index: clang/test/Sema/const-eval.c
===
--- 

[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx created this revision.
Herald added subscribers: cfe-commits, arphaman.
Herald added a project: clang.
chrish_ericsson_atx requested review of this revision.

Check applied to unbounded (incomplete) arrays and pointers
to spot cases where the computed address is beyond the
largest possible addressable extent of the array, based
on the address space in which the array is delcared, or
which the pointer refers to.

Check helps to avoid cases of nonsense pointer math and
array indexing which could lead to linker failures or
runtime exceptions.  Of particular interest when building
for embedded systems with small address spaces.

Change-Id: I836042912f0f7ba4ee774ac03d2fb47d987709e2


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86796

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/const-eval.c
  clang/test/Sema/unbounded-array-bounds.c
  clang/test/SemaCXX/constant-expression-cxx1y.cpp

Index: clang/test/SemaCXX/constant-expression-cxx1y.cpp
===
--- clang/test/SemaCXX/constant-expression-cxx1y.cpp
+++ clang/test/SemaCXX/constant-expression-cxx1y.cpp
@@ -1018,8 +1018,9 @@
 }
 
 constexpr void PR28739(int n) { // expected-error {{never produces a constant}}
-  int *p = 
+  int *p =   // expected-note {{declared here}}
   p += (__int128)(unsigned long)-1; // expected-note {{cannot refer to element 18446744073709551615 of non-array object in a constant expression}}
+  // expected-warning@-1 {{refers past the last possible element}}
 }
 
 constexpr void Void(int n) {
Index: clang/test/Sema/unbounded-array-bounds.c
===
--- /dev/null
+++ clang/test/Sema/unbounded-array-bounds.c
@@ -0,0 +1,58 @@
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-X86-ADDR64 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple i386-pc-linux-gnu   -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-I386-ADDR32 %s \
+// RUN:  --implicit-check-not 'past the last possible element'
+// RUN: %clang_cc1 -triple avr-pc-linux-gnu-fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-AVR-ADDR16 %s  \
+// RUN:  --implicit-check-not 'past the last possible element'
+
+struct S {
+  long long a;
+  char b;
+  long long c;
+  short d;
+};
+
+struct S s[];
+
+void f1() {
+  ++s[3].a;
+  ++s[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++s[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+long long ll[];
+
+void f2() {
+  ++ll[3];
+  ++ll[2705843009213693952];
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 2305843009213693952 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+  ++ll[847073650];
+  // CHECK-I386-ADDR32: :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 536870912 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 8192 elements)
+}
+
+void f3(struct S p[]) {
+  ++p[3].a;
+  ++p[7073650413200313099].b;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:5: warning: {{.*}} past the last possible element {{.*}} in 32-bit {{.*}} (max possible 178956970 elements)
+  // CHECK-AVR-ADDR16:  :[[@LINE-3]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+  ++p[7073650].c;
+  // CHECK-AVR-ADDR16:  :[[@LINE-1]]:5: warning: {{.*}} past the last possible element {{.*}} in 16-bit {{.*}} (max possible 3276 elements)
+}
+
+void f4(struct S *p) {
+  p += 3;
+  p += 7073650413200313099;
+  // CHECK-X86-ADDR64:  :[[@LINE-1]]:3: warning: {{.*}} past the last possible element {{.*}} in 64-bit {{.*}} (max possible 576460752303423488 elements)
+  // CHECK-I386-ADDR32: :[[@LINE-2]]:3: warning: {{.*}} past the last 

[PATCH] D74746: [clang][doxygen] Fix -Wdocumentation warning for tag typedefs

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

I reported this issue back in November, as 
https://bugs.llvm.org/show_bug.cgi?id=44143.  This change fixes the bug -- I'll 
mark it as such.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74746



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

[PATCH] D70203: [AST] Attach comment in `/** doc */ typedef struct A {} B` to B as well as A.

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

In D70203#1760299 , @bjope wrote:

> Hi @sammccall . 
>  Just a heads up. Looks like this might have caused: 
> https://bugs.llvm.org/show_bug.cgi?id=44143 .


Yes indeed.  I locally reverted commit a433e714 
 and it 
resolved my issue.  It's odd, though.  The unittest Sam added is nearly 
identical with the reproducer I posted.  Only obvious difference I see at first 
blush is that my reproducer uses the same identifier for the struct name and 
the typedef name, whereas Sam's unittest uses a unique identifier for each.  
Semantically, there is no difference, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70203



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

[PATCH] D62953: [Syntax] Do not glue multiple empty PP expansions to a single mapping

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added inline comments.



Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:447
+tryConsumeSpelledUntil(File, EndOffset + 1, SpelledIndex).hasValue();
+(void)HitMapping;
+assert(!HitMapping && "recursive macro expansion?");

What is intended by this line?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62953



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

@Szelethus, firstly, thank you for landing this change. I really appreciate it! 
  Secondly, apologies for misspelling your name earlier.  And lastly, thank you 
for your patient explanation of how to get the diffs updated correctly in a 
Phabricator review.  I think my mistake was that I made the change and the 
updates updates in a private branch, and not directly off master, and then 
duplicated the changes on master.  For one of those sets of changes, I amended 
the first commit with the second round of changes, and I think I confused 
myself by doing that.  In any case, lesson learned, and thank you again for 
your coaching!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx added a comment.

Kristoff, if you wouldn't mind, since you offered earlier, please go ahead and 
commit this change as-is, since it was accepted.  I ran into some non-technical 
issues with my follow-up changes and I'm going to be unavailable for several 
weeks.  To mitigate risk and work for my team, I'd like to submit the newer 
changes separately (and will reference this review in that changeset when I do, 
of course), after I return to work.

And-- thank you for your help and feedback!  I appreciated this upstreaming 
process (especially for what seemed like a fairly small/simple chnage).  I 
expect there will be many more.


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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx updated this revision to Diff 215433.
chrish_ericsson_atx added a comment.

Follow-up on reviewer feedback.  Changed from blacklisting LValueToRValue to 
whitelisting IntegralCast.  This was a good call -- additional testing with 
different cast kinds showed that the assertion tripped for other casts besides 
LValueToRValue, e.g., FloatToIntegral.  I couldn't see any casts other than 
Integral where the enum check seemed appropriate.  Testing with only 
IntegralCast enabled gave expected (correct) results.

Also reformatted the new regtest file per reviewer comments.


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

https://reviews.llvm.org/D66014

Files:
  clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  clang/test/Analysis/enum-cast-out-of-range.c


Index: clang/test/Analysis/enum-cast-out-of-range.c
===
--- clang/test/Analysis/enum-cast-out-of-range.c
+++ clang/test/Analysis/enum-cast-out-of-range.c
@@ -2,33 +2,34 @@
 // RUN:   -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
 // RUN:   -verify %s
 
-enum unscoped_unspecified_t {
-  unscoped_unspecified_0 = -4,
-  unscoped_unspecified_1,
-  unscoped_unspecified_2 = 1,
-  unscoped_unspecified_3,
-  unscoped_unspecified_4 = 4
+enum En_t {
+  En_0 = -4,
+  En_1,
+  En_2 = 1,
+  En_3,
+  En_4 = 4
 };
 
 void unscopedUnspecifiedCStyle() {
-  enum unscoped_unspecified_t InvalidBeforeRangeBegin = (enum 
unscoped_unspecified_t)(-5); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t ValidNegativeValue1 = (enum 
unscoped_unspecified_t)(-4); // OK.
-  enum unscoped_unspecified_t ValidNegativeValue2 = (enum 
unscoped_unspecified_t)(-3); // OK.
-  enum unscoped_unspecified_t InvalidInsideRange1 = (enum 
unscoped_unspecified_t)(-2); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t InvalidInsideRange2 = (enum 
unscoped_unspecified_t)(-1); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t InvalidInsideRange3 = (enum 
unscoped_unspecified_t)(0); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t ValidPositiveValue1 = (enum 
unscoped_unspecified_t)(1); // OK.
-  enum unscoped_unspecified_t ValidPositiveValue2 = (enum 
unscoped_unspecified_t)(2); // OK.
-  enum unscoped_unspecified_t InvalidInsideRange4 = (enum 
unscoped_unspecified_t)(3); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
-  enum unscoped_unspecified_t ValidPositiveValue3 = (enum 
unscoped_unspecified_t)(4); // OK.
-  enum unscoped_unspecified_t InvalidAfterRangeEnd = (enum 
unscoped_unspecified_t)(5); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum En_t Below= (enum En_t)(-5); // expected-warning {{not in the valid 
range}}
+  enum En_t NegVal1  = (enum En_t)(-4); // OK.
+  enum En_t NegVal2  = (enum En_t)(-3); // OK.
+  enum En_t InRange1 = (enum En_t)(-2); // expected-warning {{not in the valid 
range}}
+  enum En_t InRange2 = (enum En_t)(-1); // expected-warning {{not in the valid 
range}}
+  enum En_t InRange3 = (enum En_t)(0);  // expected-warning {{not in the valid 
range}}
+  enum En_t PosVal1  = (enum En_t)(1);  // OK.
+  enum En_t PosVal2  = (enum En_t)(2);  // OK.
+  enum En_t InRange4 = (enum En_t)(3);  // expected-warning {{not in the valid 
range}}
+  enum En_t PosVal3  = (enum En_t)(4);  // OK.
+  enum En_t Above= (enum En_t)(5);  // expected-warning {{not in the valid 
range}}
 }
 
-enum unscoped_unspecified_t unused;
+enum En_t unused;
 void unusedExpr() {
-// following line is not something that EnumCastOutOfRangeChecker should 
evaluate.  checker should either ignore this line
-// or process it without producing any warnings.  However, compilation 
will (and should) still generate a warning having 
-// nothing to do with this checker.
+// Following line is not something that EnumCastOutOfRangeChecker should
+// evaluate.  Checker should either ignore this line or process it without
+// producing any warnings.  However, compilation will (and should) still
+// generate a warning having nothing to do with this checker.
 unused; // expected-warning {{expression result unused}}
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -91,10 +91,21 @@
 
 void 

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 5 inline comments as done.
chrish_ericsson_atx added a comment.

In D66014#1627858 , @Szelethus wrote:

> LGTM, thanks! Do you need someone to commit this on your behalf? Also, could 
> you please make the comments capitalized, terminated, and fitting in 80 
> columns?


I have updated the comments and line formatting as you recommended.  Given that 
this change is already "Accepted", can I (should I) upload new differential for 
this change, or should this be delivered as-is, and I'll upload the new diffs 
as a new/separate change?

And yes, I will need someone to commit on my behalf.  I'm too new to have 
commit privs. :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

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



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

NoQ wrote:
> chrish_ericsson_atx wrote:
> > NoQ wrote:
> > > chrish_ericsson_atx wrote:
> > > > NoQ wrote:
> > > > > If you look at the list of cast kinds, you'll be shocked to see 
> > > > > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > > > > definitely stands out (because it's the only one that has very little 
> > > > > to do with actual casting), i'd still be much more comfortable if we 
> > > > > *whitelist* the casts that we check, rather than blacklist them.
> > > > > 
> > > > > Can you take a look at the list and find which casts are we actually 
> > > > > talking about and hardcode them here?
> > > > I'm happy to cross-check a list; however, I'm not aware of the list you 
> > > > are referring to.  Would you mind providing a bit more detail?
> > > See **[[ 
> > > https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
> > >  | include/clang/AST/OperationKinds.def ]]**.
> > Ah.   Okay -- That is a list I've seen before, of course...  :)   
> > 
> > Hmm...  regarding whitelisting versus blacklisting...  In this case, it 
> > seems to me that switching to whitelisting casts that we know we should be 
> > checking increases the risk that we would introduce a new bug -- 
> > specifically that we'd accidentally leave out cast type that should have 
> > been included, which would disable a legitimate check (that some users 
> > might already be relying on).  By contrast, blacklisting the one cast type 
> > we know should *not* be checked adds zero new risk and still fixes this bug.
> > 
> > The sense of risk for me comes partly from being fairly new to working on 
> > clang AST code -- this is my first change.   It strikes me that if I were 
> > to feel confident about having the "correct" whitelist, I would have to 
> > understand every cast type fairly deeply.  Given that I see several cast 
> > kinds in that list that I know nothing about, I'm concerned with the level 
> > of effort required, and that I still won't be able to fully mitigate the 
> > risk.
> > 
> > Can you help me understand your rationale for preferring a whitelist in 
> > this case?  Or, do you feel I've overstated the risk here?  I'm not 
> > emotionally invested in being proven correct-- just want to learn!  :)
> > 
> Generally, if you're not sure, try to not to warn. False positives are worse 
> than false negatives. It is natural to start with a small set of cases in 
> which you're sure, and then slowly cover more and more cases as you 
> investigate further. Leave a comment if you're unsure if you covered all 
> cases, so that people knew that the code is potentially incomplete and it's a 
> good place to look for potential improvements.
> 
> When you're actively developing the checker, you should rely on your own 
> experiments with the real-world code that you plan to analyze. When the 
> checker is still in alpha, it's fine for it to be more aggressive than 
> necessary, because "you're still experimenting with it", but this will need 
> to be addressed when the checker is moved out of alpha.
> 
> In particular, you have the following tricks at your disposal:
> - Take a large codebase, run your checker on it (you'll need to do this 
> regularly anyway) and include the cast kind in the warning text. This will 
> give you a nice list of casts that you want to support (and probably also a 
> list of casts that you //don't// want to support).
> - If you want to be notified of false negatives by your power-users, instead 
> of a whitelist put an assertion (immediately before emitting the report) that 
> the cast kind is one of the known cast kinds. This is a bit evil, of course, 
> but it only affects the users that run analysis with assertions on, which 
> means custom-built clang, which means power-users that actively want to 
> report these bugs.
> - If you don't know what code does a specific AST node kind represent, as a 
> last resort you can always assert that this kind of node never gets 
> constructed and see which clang tests fail.
> - If you want to have some protection against newly introduced cast kinds, 
> make a `switch (getCastKind())` and don't add a `default:` case into it. This 
> way anybody who introduces a new cast kind would get a compiler warning 
> ("unhandled case in switch") and will be forced to take a look at how this 
> code should handle the new cast kind.
Okay.  Very fair points.  I've been focused too much on released production 
code lately-- the mantra there is to be a surgical as possible, and minimize 
risk of any changes 

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

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

In D66014#1625733 , @Szelethus wrote:

> Oh, btw, thank you for working on this!


Glad to help!




Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

NoQ wrote:
> chrish_ericsson_atx wrote:
> > NoQ wrote:
> > > If you look at the list of cast kinds, you'll be shocked to see 
> > > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > > definitely stands out (because it's the only one that has very little to 
> > > do with actual casting), i'd still be much more comfortable if we 
> > > *whitelist* the casts that we check, rather than blacklist them.
> > > 
> > > Can you take a look at the list and find which casts are we actually 
> > > talking about and hardcode them here?
> > I'm happy to cross-check a list; however, I'm not aware of the list you are 
> > referring to.  Would you mind providing a bit more detail?
> See **[[ 
> https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
>  | include/clang/AST/OperationKinds.def ]]**.
Ah.   Okay -- That is a list I've seen before, of course...  :)   

Hmm...  regarding whitelisting versus blacklisting...  In this case, it seems 
to me that switching to whitelisting casts that we know we should be checking 
increases the risk that we would introduce a new bug -- specifically that we'd 
accidentally leave out cast type that should have been included, which would 
disable a legitimate check (that some users might already be relying on).  By 
contrast, blacklisting the one cast type we know should *not* be checked adds 
zero new risk and still fixes this bug.

The sense of risk for me comes partly from being fairly new to working on clang 
AST code -- this is my first change.   It strikes me that if I were to feel 
confident about having the "correct" whitelist, I would have to understand 
every cast type fairly deeply.  Given that I see several cast kinds in that 
list that I know nothing about, I'm concerned with the level of effort 
required, and that I still won't be able to fully mitigate the risk.

Can you help me understand your rationale for preferring a whitelist in this 
case?  Or, do you feel I've overstated the risk here?  I'm not emotionally 
invested in being proven correct-- just want to learn!  :)



Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 3 inline comments as done.
chrish_ericsson_atx added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

NoQ wrote:
> If you look at the list of cast kinds, you'll be shocked to see ridiculously 
> weird cornercases. Even though lvalue-to-rvalue cast definitely stands out 
> (because it's the only one that has very little to do with actual casting), 
> i'd still be much more comfortable if we *whitelist* the casts that we check, 
> rather than blacklist them.
> 
> Can you take a look at the list and find which casts are we actually talking 
> about and hardcode them here?
I'm happy to cross-check a list; however, I'm not aware of the list you are 
referring to.  Would you mind providing a bit more detail?



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:121-122
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
   DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
 

Szelethus wrote:
> So this is where the assertion comes from, and will eventually lead to 
> `SValBuilder::evalEQ`, which calls `SValBuilder::evalBinOp`, where this will 
> fire on line 427:
> ```
> assert(op == BO_Add);
> ```
> Seems like this happens because `unused`'s value in your testcase will be 
> retrieved as a `Loc`, while the values in the enum are (correctly) `NonLoc`, 
> and `SValBuilder::evalBinOp` thinks this is some sort of pointer arithmetic 
> (`5 + ptr` etc).
> 
> How about instead of checking for LValueToRValue cast, we check whether 
> `ValueToCast` is `Loc`, and bail out if so? That sounds like a more general 
> solution, but I didn't sit atop of this for hours.
Is this the only case where ValueToCast will be Loc?   I was unsure about the 
implications of Loc vs. NonLoc in this code, and I didn't want to risk breaking 
a check that should have been valid.  That's why I opted for bailing on an 
LValueToRValue cast at a higher level-- that seemed much safer to me.  I 
certainly might be missing something, but I couldn't find any evidence that an 
LValueToRValue cast should ever need to be checked for enum range errors.   I 
will certainly defer to your judgement, but I'd like to have a better 
understanding of why looking for ValueToCast == Loc would actually be more 
correct.



Comment at: clang/test/Analysis/enum-cast-out-of-range.c:27-34
+enum unscoped_unspecified_t unused;
+void unusedExpr() {
+// following line is not something that EnumCastOutOfRangeChecker should 
evaluate.  checker should either ignore this line
+// or process it without producing any warnings.  However, compilation 
will (and should) still generate a warning having 
+// nothing to do with this checker.
+unused; // expected-warning {{expression result unused}}
+}

NoQ wrote:
> I guess this covers D33672#1537287!
> 
> That said, there's one thing about this test that i don't understand. Why 
> does this AST contain an implicit lvalue-to-rvalue cast at all? This looks 
> like a (most likely otherwise harmless) bug in the AST; if i understand 
> correctly, this should be a freestanding `DeclRefExpr`.
It sure looks like D33672 is the same bug, so yes, I bet it will fix that one.  
 This fix also addresses https://bugs.llvm.org/show_bug.cgi?id=41388.  (Forgive 
me if there's a way to directly link to bugs.llvm.org with this markup.)

Not sure about whether the AST should have represented this node as a 
DeclRefExpr-- that certainly makes some sense, but the LValueToRValue cast 
isn't really wrong either.   At the end of the day, this statement is useless, 
so I'm not sure it matters (much) how the AST represents it.  Representing it 
as LValueToRValue cast wouldn't cause side effects, would it? (E.g.,  cause IR 
to contain explicit dereferencing code?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66014: Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

EnumCastOutOfRangeChecker should not perform enum range checks on 
LValueToRValue casts, since this type of cast does not actually change the 
underlying type.   Performing the unnecessary check actually triggered an 
assertion failure deeper in EnumCastOutOfRange for certain input (which is 
captured in the accompanying test code).


Repository:
  rC Clang

https://reviews.llvm.org/D66014

Files:
  clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  clang/test/Analysis/enum-cast-out-of-range.c
  clang/test/Analysis/enum-cast-out-of-range.cpp


Index: clang/test/Analysis/enum-cast-out-of-range.cpp
===
--- clang/test/Analysis/enum-cast-out-of-range.cpp
+++ clang/test/Analysis/enum-cast-out-of-range.cpp
@@ -150,7 +150,15 @@
   scoped_specified_t InvalidAfterRangeEnd = (scoped_specified_t)(5); // 
expected-warning {{The value provided to the cast expression is not in the 
valid range of values for the enum}}
 }
 
-void rangeContstrained1(int input) {
+unscoped_unspecified_t unused;
+void unusedExpr() {
+// following line is not something that EnumCastOutOfRangeChecker should 
evaluate.  checker should either ignore this line
+// or process it without producing any warnings.  However, compilation 
will (and should) still generate a warning having 
+// nothing to do with this checker.
+unused; // expected-warning {{expression result unused}}
+}
+
+void rangeConstrained1(int input) {
   if (input > -5 && input < 5)
 auto value = static_cast(input); // OK. Being 
conservative, this is a possibly good value.
 }
Index: clang/test/Analysis/enum-cast-out-of-range.c
===
--- /dev/null
+++ clang/test/Analysis/enum-cast-out-of-range.c
@@ -0,0 +1,34 @@
+// RUN: %clang_analyze_cc1 \
+// RUN:   -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
+// RUN:   -verify %s
+
+enum unscoped_unspecified_t {
+  unscoped_unspecified_0 = -4,
+  unscoped_unspecified_1,
+  unscoped_unspecified_2 = 1,
+  unscoped_unspecified_3,
+  unscoped_unspecified_4 = 4
+};
+
+void unscopedUnspecifiedCStyle() {
+  enum unscoped_unspecified_t InvalidBeforeRangeBegin = (enum 
unscoped_unspecified_t)(-5); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum unscoped_unspecified_t ValidNegativeValue1 = (enum 
unscoped_unspecified_t)(-4); // OK.
+  enum unscoped_unspecified_t ValidNegativeValue2 = (enum 
unscoped_unspecified_t)(-3); // OK.
+  enum unscoped_unspecified_t InvalidInsideRange1 = (enum 
unscoped_unspecified_t)(-2); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum unscoped_unspecified_t InvalidInsideRange2 = (enum 
unscoped_unspecified_t)(-1); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum unscoped_unspecified_t InvalidInsideRange3 = (enum 
unscoped_unspecified_t)(0); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum unscoped_unspecified_t ValidPositiveValue1 = (enum 
unscoped_unspecified_t)(1); // OK.
+  enum unscoped_unspecified_t ValidPositiveValue2 = (enum 
unscoped_unspecified_t)(2); // OK.
+  enum unscoped_unspecified_t InvalidInsideRange4 = (enum 
unscoped_unspecified_t)(3); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+  enum unscoped_unspecified_t ValidPositiveValue3 = (enum 
unscoped_unspecified_t)(4); // OK.
+  enum unscoped_unspecified_t InvalidAfterRangeEnd = (enum 
unscoped_unspecified_t)(5); // expected-warning {{The value provided to the 
cast expression is not in the valid range of values for the enum}}
+}
+
+enum unscoped_unspecified_t unused;
+void unusedExpr() {
+// following line is not something that EnumCastOutOfRangeChecker should 
evaluate.  checker should either ignore this line
+// or process it without producing any warnings.  However, compilation 
will (and should) still generate a warning having 
+// nothing to do with this checker.
+unused; // expected-warning {{expression result unused}}
+}
+
Index: clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -91,6 +91,11 @@
 
 void EnumCastOutOfRangeChecker::checkPreStmt(const CastExpr *CE,
  CheckerContext ) const {
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze