[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

ilya added a comment.

Kind ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

ilya updated this revision to Diff 238106.
ilya added a comment.

Address rsmith's comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Parser/cxx-ambig-decl-expr.cpp
  clang/test/SemaCXX/array-bounds.cpp

Index: clang/test/SemaCXX/array-bounds.cpp
===
--- clang/test/SemaCXX/array-bounds.cpp
+++ clang/test/SemaCXX/array-bounds.cpp
@@ -27,7 +27,7 @@
 };
 
 void f1(int a[1]) {
-  int val = a[3]; // no warning for function argumnet
+  int val = a[3]; // no warning for function argument
 }
 
 void f2(const int (&a)[2]) { // expected-note {{declared here}}
@@ -133,7 +133,7 @@
 
 int test_sizeof_as_condition(int flag) {
   int arr[2] = { 0, 0 }; // expected-note {{array 'arr' declared here}}
-  if (flag) 
+  if (flag)
 return sizeof(char) != sizeof(char) ? arr[2] : arr[1];
   return sizeof(char) == sizeof(char) ? arr[2] : arr[1]; // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
 }
@@ -241,7 +241,7 @@
 }
 
 int test_pr11007_aux(const char * restrict, ...);
-  
+
 // Test checking with varargs.
 void test_pr11007() {
   double a[5]; // expected-note {{array 'a' declared here}}
@@ -309,3 +309,33 @@
 foo(); // expected-note 1{{in instantiation of function template specialization}}
   };
 }
+
+namespace PR44343 {
+  const unsigned int array[2] = {0, 1}; // expected-note 5{{array 'array' declared here}}
+
+  const int i1 = (const int)array[2]; // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  const int i2 = static_cast(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  const int &i3 = reinterpret_cast(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  unsigned int &i4 = const_cast(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  int i5 = int(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  const unsigned int *i6 = &(1 > 0 ? array[2] : array[1]); // no warning for one-past-end element's address retrieval
+
+  // Test dynamic cast
+  struct Base {
+virtual ~Base();
+  };
+  struct Derived : Base {
+  };
+  Base baseArr[2]; // expected-note {{array 'baseArr' declared here}}
+  Derived *d1 = dynamic_cast(&baseArr[2]); // FIXME: Should actually warn because dynamic_cast accesses the vptr
+  Derived &d2 = dynamic_cast(baseArr[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+
+  // Test operator `&` in combination with operators `.` and `->`
+  struct A {
+int n;
+  };
+  A a[2]; // expected-note {{array 'a' declared here}}
+  int *n = &a[2].n; // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  A *aPtr[2]; // expected-note {{array 'aPtr' declared here}}
+  int *n2 = &aPtr[2]->n; // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+}
Index: clang/test/Parser/cxx-ambig-decl-expr.cpp
===
--- clang/test/Parser/cxx-ambig-decl-expr.cpp
+++ clang/test/Parser/cxx-ambig-decl-expr.cpp
@@ -24,7 +24,7 @@
 
   // This is array indexing not an array declarator because a comma expression
   // is not syntactically a constant-expression.
-  int(x[1,1]); // expected-warning 2{{unused}}
+  int(x[1,0]); // expected-warning 2{{unused}}
 
   // This is array indexing not an array declaration because a braced-init-list
   // is not syntactically a constant-expression.
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13368,62 +13368,63 @@
 << ND->getDeclName());
 }
 
-void Sema::CheckArrayAccess(const Expr *expr) {
-  int AllowOnePastEnd = 0;
-  while (expr) {
-expr = expr->IgnoreParenImpCasts();
-switch (expr->getStmtClass()) {
-  case Stmt::ArraySubscriptExprClass: {
-const ArraySubscriptExpr *ASE = cast(expr);
-CheckArrayAccess(ASE->getBase(), ASE->getIdx(), ASE,
- AllowOnePastEnd > 0);
-expr = ASE->getBase();
-break;
-  }
-  case Stmt::MemberExprClass: {
-expr = cast(expr)->getBase();
-break;
-  }
-  case Stmt::OMPArraySectionExprClass: {
-const OMPArraySectionExpr *ASE = cast(expr);
-if (ASE->getLowerBound())
-  CheckArrayAccess(ASE->getBase(), ASE->getLowerBound(),
-   /*ASE=*/nullptr, AllowOnePastEnd > 0);
-return;
-  }
-  case Stmt::U

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

ilya marked 4 inline comments as done.
ilya added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:13384
   case Stmt::MemberExprClass: {
 expr = cast(expr)->getBase();
 break;

rsmith wrote:
> ilya wrote:
> > rsmith wrote:
> > > Hmm, don't we need to do different things for dot and arrow in this case?
> > There are several test cases for an out of bounds access on an array member 
> > using dot and arrow operators in array-bounds.cpp. Do you have a specific 
> > test case for which you think the code is broken?
> > There are several test cases for an out of bounds access on an array member 
> > using dot and arrow operators in array-bounds.cpp. Do you have a specific 
> > test case for which you think the code is broken?
> 
> Sure. There's a false negative for this:
> 
> ```
> struct A { int n; };
> A *a[4];
> int *n = &a[4]->n;
> ```
> 
> ... because we incorrectly visit the left-hand side of the `->` with 
> `AllowOnePastEnd == 1`. The left-hand side of `->` is subject to 
> lvalue-to-rvalue conversion, so can't be one-past-the-end regardless of the 
> context in which the `->` appears.
> ... because we incorrectly visit the left-hand side of the -> with 
> AllowOnePastEnd == 1. The left-hand side of -> is subject to lvalue-to-rvalue 
> conversion, so can't be one-past-the-end regardless of the context in which 
> the -> appears.

Thanks for clarifying. So basically we don't want to allow one-past-end for 
MemberExpr?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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

[PATCH] D69764: [clang-format] Add Left/Right Const (East/West , Before/After) fixer capability

[Ilya Mirsky via Phabricator via cfe-commits]

ilya added a comment.

In D69764#1732235 , @aaron.ballman 
wrote:

> I like the functionality, but am slightly opposed to using "east/west" 
> terminology -- that's not a ubiquitous phrase and it takes a bit of thinking 
> before it makes sense. I think "left/right" is likely to be more universally 
> understood.
>
> Also, should this apply to other qualifiers like `volatile` or `restrict`? If 
> so, the name `ConstStyle` should probably be `CVQualifierStyle` or something 
> else.


+1.
In addition to `volatile` and `restrict`, in my organization we'd also be 
interested in applying this to address space qualifiers (custom keywords added 
in our downstream fork). Can this be generalized to accept a map of qualifier 
keywords and its desired orientation?


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

https://reviews.llvm.org/D69764



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

[PATCH] D69764: [clang-format] Add Left/Right Const (East/West , Before/After) fixer capability

[Ilya Mirsky via Phabricator via cfe-commits]

ilya added a comment.

In D69764#1775842 , @MyDeveloperDay 
wrote:

> In D69764#1775835 , @ilya wrote:
>
> > In D69764#1732235 , @aaron.ballman 
> > wrote:
> >
> > > I like the functionality, but am slightly opposed to using "east/west" 
> > > terminology -- that's not a ubiquitous phrase and it takes a bit of 
> > > thinking before it makes sense. I think "left/right" is likely to be more 
> > > universally understood.
> > >
> > > Also, should this apply to other qualifiers like `volatile` or 
> > > `restrict`? If so, the name `ConstStyle` should probably be 
> > > `CVQualifierStyle` or something else.
> >
> >
> > +1.
> >  In addition to `volatile` and `restrict`, in my organization we'd also be 
> > interested in applying this to address space qualifiers (custom keywords 
> > added in our downstream fork). Can this be generalized to accept a map of 
> > qualifier keywords and its desired orientation?
>
>
> Could you give me an example of what you mean?


As described in section 5 of the embedded C standard ISO/IEC J TC1 SC22 WG14 
N1169 , our 
downstream llvm fork supports several address spaces, let's denote them `__as1` 
and `__as2`. These address space qualifiers behave similarly to `const`, so the 
following are semantically equal:

  __as1 int foo;
  int __as1 bar;

So for us it would be nice to be able to specify alignment options for a 
dynamic list of custom qualifiers. Something like the following:

  QualifierStyle:
const: Right
volatile:  Right
__as1: Right
__as2: Right

But I don't know if this can be (easily) supported in a .clang-format file, 
since the style options are defined as (static) enums. I realize my proposal 
might be out of the scope of this patch, but I wanted to get some opinions from 
the community.


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

https://reviews.llvm.org/D69764



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

[PATCH] D69764: [clang-format] Add Left/Right Const (East/West , Before/After) fixer capability

[Ilya Mirsky via Phabricator via cfe-commits]

ilya added a comment.

> I think this would likely make everything much more complicated, but perhaps 
> we should think about this for the configuration at least now so we future 
> proof ourselves.

What you've outlined looks good to me.
Of course we will be happy to contribute, given there will be an interest from 
the community, but as you said, for now at least let's keep this in mind for 
the configuration and naming.


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

https://reviews.llvm.org/D69764



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

[PATCH] D71666: [clang-tidy] Fix readability-const-return-type identifying the wrong `const` token

[Ilya Mirsky via Phabricator via cfe-commits]

ilya created this revision.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.

Replace tidy::utils::lexer::getConstQualifyingToken with a corrected and also
generalized to other qualifiers variant - getQualifyingToken.

Fixes: http://llvm.org/PR44326


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71666

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -37,6 +37,9 @@
 template 
 typename std::add_const::type n15(T v) { return v; }
 
+template 
+struct MyStruct {};
+
 template 
 class Klazz {
 public:
@@ -128,10 +131,46 @@
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz'
 // CHECK-FIXES: Klazz p12() {}
 
+const Klazz> p33() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz> p33() {}
+
 const Klazz* const p13() {}
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
 // CHECK-FIXES: const Klazz* p13() {}
 
+const Klazz* const volatile p14() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
+// CHECK-FIXES: const Klazz* volatile p14() {}
+
+const MyStruct<0 < 1> p34() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p34() {}
+
+MyStruct<0 < 1> const p35() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p35() {}
+
+Klazz const> const p36() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz const> p36() {}
+
+const Klazz const> *const p37() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz const> *p37() {}
+
+Klazz> const p38() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz> p38() {}
+
+const Klazz> p39() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz> p39() {}
+
+const Klazz 1)>> p40() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz 1)>> p40() {}
+
 // re-declaration of p15.
 const int p15();
 // CHECK-FIXES: int p15();
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.h
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.h
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.h
@@ -96,9 +96,14 @@
 /// token in ``Range`` that is responsible for const qualification. ``Range``
 /// must be valid with respect to ``SM``.  Returns ``None`` if no ``const``
 /// tokens are found.
-llvm::Optional getConstQualifyingToken(CharSourceRange Range,
-  const ASTContext &Context,
-  const SourceManager &SM);
+///
+/// Similar to ``getConstQualifyingToken`` above, but generalized to any
+/// of the following qualifier tokens: kw_const, kw_volatile, kw_restrict,
+/// kw___cm, kw___lpm.
+llvm::Optional getQualifyingToken(tok::TokenKind TK,
+ CharSourceRange Range,
+ const ASTContext &Context,
+ const SourceManager &SM);
 
 } // namespace lexer
 } // namespace utils
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
@@ -102,15 +102,20 @@
   return false;
 }
 
-llvm::Optional getConstQualifyingToken(CharSourceRange Range,
-  const ASTContext &Context,
-  const SourceManager &SM) {
+llvm::Optional getQualifyingToken(tok::TokenKind TK,
+ CharSourceRange Range,
+ const ASTContext &Context,
+ const SourceManager &SM) {
+  assert((TK == tok::kw_const || TK == tok::kw_volatile ||
+  TK == tok::kw_restrict) &&
+ "TK is not a qualifier keyword");
   std::pair LocInfo = SM.getDecomposedLoc(Range.getBegin());
   StringRef File = SM.getBufferData(LocInfo.first);
   Lexer RawLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(),
  File.begin(), File.data() + LocInfo.second, File.end());
-  llvm::Optional FirstConstTok;
-  Token LastTokInRange;
+  llvm::Optional LastMatchBeforeTemplate;
+  llvm::Optional La

[PATCH] D71666: [clang-tidy] Fix readability-const-return-type identifying the wrong `const` token

[Ilya Mirsky via Phabricator via cfe-commits]

ilya updated this revision to Diff 234560.
ilya added a comment.

Fix documentation in LexerUtils.h


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71666

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -37,6 +37,9 @@
 template 
 typename std::add_const::type n15(T v) { return v; }
 
+template 
+struct MyStruct {};
+
 template 
 class Klazz {
 public:
@@ -128,10 +131,46 @@
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz'
 // CHECK-FIXES: Klazz p12() {}
 
+const Klazz> p33() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz> p33() {}
+
 const Klazz* const p13() {}
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
 // CHECK-FIXES: const Klazz* p13() {}
 
+const Klazz* const volatile p14() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
+// CHECK-FIXES: const Klazz* volatile p14() {}
+
+const MyStruct<0 < 1> p34() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p34() {}
+
+MyStruct<0 < 1> const p35() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p35() {}
+
+Klazz const> const p36() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz const> p36() {}
+
+const Klazz const> *const p37() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz const> *p37() {}
+
+Klazz> const p38() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz> p38() {}
+
+const Klazz> p39() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz> p39() {}
+
+const Klazz 1)>> p40() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz 1)>> p40() {}
+
 // re-declaration of p15.
 const int p15();
 // CHECK-FIXES: int p15();
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.h
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.h
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.h
@@ -92,13 +92,14 @@
  const SourceManager &SM,
  const LangOptions &LangOpts);
 
-/// Assuming that ``Range`` spans a const-qualified type, returns the ``const``
-/// token in ``Range`` that is responsible for const qualification. ``Range``
-/// must be valid with respect to ``SM``.  Returns ``None`` if no ``const``
+/// Assuming that ``Range`` spans a CVR-qualified type, returns the
+/// token in ``Range`` that is responsible for the qualification. ``Range``
+/// must be valid with respect to ``SM``.  Returns ``None`` if no qualifying
 /// tokens are found.
-llvm::Optional getConstQualifyingToken(CharSourceRange Range,
-  const ASTContext &Context,
-  const SourceManager &SM);
+llvm::Optional getQualifyingToken(tok::TokenKind TK,
+ CharSourceRange Range,
+ const ASTContext &Context,
+ const SourceManager &SM);
 
 } // namespace lexer
 } // namespace utils
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
@@ -102,15 +102,20 @@
   return false;
 }
 
-llvm::Optional getConstQualifyingToken(CharSourceRange Range,
-  const ASTContext &Context,
-  const SourceManager &SM) {
+llvm::Optional getQualifyingToken(tok::TokenKind TK,
+ CharSourceRange Range,
+ const ASTContext &Context,
+ const SourceManager &SM) {
+  assert((TK == tok::kw_const || TK == tok::kw_volatile ||
+  TK == tok::kw_restrict) &&
+ "TK is not a qualifier keyword");
   std::pair LocInfo = SM.getDecomposedLoc(Range.getBegin());
   StringRef File = SM.getBufferData(LocInfo.first);
   Lexer RawLexer(SM.getLocForStartOfFile(LocInfo.first), Context.getLangOpts(),
  File.begin(), File.data() 

[PATCH] D71714: [Sema] NFC: Remove trailing spaces and fix a typo in a test file

[Ilya Mirsky via Phabricator via cfe-commits]

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

[Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array 
item

Fixes: http://llvm.org/PR44343


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71714

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


Index: clang/test/SemaCXX/array-bounds.cpp
===
--- clang/test/SemaCXX/array-bounds.cpp
+++ clang/test/SemaCXX/array-bounds.cpp
@@ -27,7 +27,7 @@
 };
 
 void f1(int a[1]) {
-  int val = a[3]; // no warning for function argumnet
+  int val = a[3]; // no warning for function argument
 }
 
 void f2(const int (&a)[2]) { // expected-note {{declared here}}
@@ -133,7 +133,7 @@
 
 int test_sizeof_as_condition(int flag) {
   int arr[2] = { 0, 0 }; // expected-note {{array 'arr' declared here}}
-  if (flag) 
+  if (flag)
 return sizeof(char) != sizeof(char) ? arr[2] : arr[1];
   return sizeof(char) == sizeof(char) ? arr[2] : arr[1]; // expected-warning 
{{array index 2 is past the end of the array (which contains 2 elements)}}
 }
@@ -241,7 +241,7 @@
 }
 
 int test_pr11007_aux(const char * restrict, ...);
-  
+
 // Test checking with varargs.
 void test_pr11007() {
   double a[5]; // expected-note {{array 'a' declared here}}
@@ -309,3 +309,23 @@
 foo(); // expected-note 1{{in instantiation of function template 
specialization}}
   };
 }
+
+namespace PR44343 {
+  const unsigned int array[2] = {0, 1}; // expected-note 4{{array 'array' 
declared here}}
+
+  const int i1 = (const int)array[2]; // expected-warning {{array index 2 is 
past the end of the array (which contains 2 elements)}}
+  const int i2 = static_cast(array[2]); // expected-warning {{array 
index 2 is past the end of the array (which contains 2 elements)}}
+  const int &i3 = reinterpret_cast(array[2]); // expected-warning 
{{array index 2 is past the end of the array (which contains 2 elements)}}
+  unsigned int &i4 = const_cast(array[2]); // expected-warning 
{{array index 2 is past the end of the array (which contains 2 elements)}}
+
+  struct Base {
+virtual ~Base();
+  };
+
+  struct Derived : Base {
+  };
+
+  Base baseArr[2]; // expected-note {{array 'baseArr' declared here}}
+  Derived *d1 = dynamic_cast(&baseArr[2]); // no warning for 
one-past-end element's address retrieval
+  Derived &d2 = dynamic_cast(baseArr[2]); // expected-warning 
{{array index 2 is past the end of the array (which contains 2 elements)}}
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13421,6 +13421,15 @@
   CheckArrayAccess(Arg);
 return;
   }
+  case Stmt::CStyleCastExprClass:
+  case Stmt::CXXStaticCastExprClass:
+  case Stmt::CXXDynamicCastExprClass:
+  case Stmt::CXXReinterpretCastExprClass:
+  case Stmt::CXXConstCastExprClass: {
+const auto *ECE = cast(expr);
+expr = ECE->getSubExpr();
+break;
+  }
   default:
 return;
 }


Index: clang/test/SemaCXX/array-bounds.cpp
===
--- clang/test/SemaCXX/array-bounds.cpp
+++ clang/test/SemaCXX/array-bounds.cpp
@@ -27,7 +27,7 @@
 };
 
 void f1(int a[1]) {
-  int val = a[3]; // no warning for function argumnet
+  int val = a[3]; // no warning for function argument
 }
 
 void f2(const int (&a)[2]) { // expected-note {{declared here}}
@@ -133,7 +133,7 @@
 
 int test_sizeof_as_condition(int flag) {
   int arr[2] = { 0, 0 }; // expected-note {{array 'arr' declared here}}
-  if (flag) 
+  if (flag)
 return sizeof(char) != sizeof(char) ? arr[2] : arr[1];
   return sizeof(char) == sizeof(char) ? arr[2] : arr[1]; // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
 }
@@ -241,7 +241,7 @@
 }
 
 int test_pr11007_aux(const char * restrict, ...);
-  
+
 // Test checking with varargs.
 void test_pr11007() {
   double a[5]; // expected-note {{array 'a' declared here}}
@@ -309,3 +309,23 @@
 foo(); // expected-note 1{{in instantiation of function template specialization}}
   };
 }
+
+namespace PR44343 {
+  const unsigned int array[2] = {0, 1}; // expected-note 4{{array 'array' declared here}}
+
+  const int i1 = (const int)array[2]; // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  const int i2 = static_cast(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  const int &i3 = reinterpret_cast(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  unsigned int &i4 = const_cast(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+
+  struct Base {
+virtual ~Base();
+  };

[PATCH] D71666: [clang-tidy] Fix readability-const-return-type identifying the wrong `const` token

[Ilya Mirsky via Phabricator via cfe-commits]

ilya updated this revision to Diff 235156.
ilya added a comment.

Update LexerUtils.h documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71666

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -37,6 +37,9 @@
 template 
 typename std::add_const::type n15(T v) { return v; }
 
+template 
+struct MyStruct {};
+
 template 
 class Klazz {
 public:
@@ -128,10 +131,46 @@
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz'
 // CHECK-FIXES: Klazz p12() {}
 
+const Klazz> p33() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz> p33() {}
+
 const Klazz* const p13() {}
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
 // CHECK-FIXES: const Klazz* p13() {}
 
+const Klazz* const volatile p14() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
+// CHECK-FIXES: const Klazz* volatile p14() {}
+
+const MyStruct<0 < 1> p34() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p34() {}
+
+MyStruct<0 < 1> const p35() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p35() {}
+
+Klazz const> const p36() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz const> p36() {}
+
+const Klazz const> *const p37() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz const> *p37() {}
+
+Klazz> const p38() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz> p38() {}
+
+const Klazz> p39() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz> p39() {}
+
+const Klazz 1)>> p40() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz 1)>> p40() {}
+
 // re-declaration of p15.
 const int p15();
 // CHECK-FIXES: int p15();
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.h
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.h
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.h
@@ -92,13 +92,15 @@
  const SourceManager &SM,
  const LangOptions &LangOpts);
 
-/// Assuming that ``Range`` spans a const-qualified type, returns the ``const``
-/// token in ``Range`` that is responsible for const qualification. ``Range``
-/// must be valid with respect to ``SM``.  Returns ``None`` if no ``const``
+/// Assuming that ``Range`` spans a CVR-qualified type, returns the
+/// token in ``Range`` that is responsible for the qualification. ``Range``
+/// must be valid with respect to ``SM``.  Returns ``None`` if no qualifying
 /// tokens are found.
-llvm::Optional getConstQualifyingToken(CharSourceRange Range,
-  const ASTContext &Context,
-  const SourceManager &SM);
+/// Note: doesn't support member function qualifiers.
+llvm::Optional getQualifyingToken(tok::TokenKind TK,
+ CharSourceRange Range,
+ const ASTContext &Context,
+ const SourceManager &SM);
 
 } // namespace lexer
 } // namespace utils
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
@@ -102,15 +102,20 @@
   return false;
 }
 
-llvm::Optional getConstQualifyingToken(CharSourceRange Range,
-  const ASTContext &Context,
-  const SourceManager &SM) {
+llvm::Optional getQualifyingToken(tok::TokenKind TK,
+ CharSourceRange Range,
+ const ASTContext &Context,
+ const SourceManager &SM) {
+  assert((TK == tok::kw_const || TK == tok::kw_volatile ||
+  TK == tok::kw_restrict) &&
+ "TK is not a qualifier keyword");
   std::pair LocInfo = SM.getDecomposedLoc(Range.getBegin());
   StringRef File = SM.getBufferData(LocInfo.first);
   Lexer RawLexer(SM.getLocForStartOfFile(LocInfo.first), Context.get

[PATCH] D71666: [clang-tidy] Fix readability-const-return-type identifying the wrong `const` token

[Ilya Mirsky via Phabricator via cfe-commits]

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

@aaron.ballman, thanks for reviewing. Do you mind submitting this, as I don't 
have commit access?




Comment at: clang-tools-extra/clang-tidy/utils/LexerUtils.h:95-97
+/// Assuming that ``Range`` spans a CVR-qualified type, returns the
+/// token in ``Range`` that is responsible for the qualification. ``Range``
+/// must be valid with respect to ``SM``.  Returns ``None`` if no qualifying

aaron.ballman wrote:
> Should we add a comment that this is not for getting a member function 
> qualifier? (I am assuming that case was never intended to work, but if it 
> does work, should we also support ref qualifiers in that case?)
That's a good point. For the purposes that this function is currently being 
used, I don't see a reason to support member function qualifiers - updated the 
documentation accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71666



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

[PATCH] D71666: [clang-tidy] Fix readability-const-return-type identifying the wrong `const` token

[Ilya Mirsky via Phabricator via cfe-commits]

ilya updated this revision to Diff 235157.
ilya marked an inline comment as done.
ilya added a comment.

Doxygen format the documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71666

Files:
  clang-tools-extra/clang-tidy/readability/ConstReturnTypeCheck.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/readability-const-return-type.cpp
@@ -37,6 +37,9 @@
 template 
 typename std::add_const::type n15(T v) { return v; }
 
+template 
+struct MyStruct {};
+
 template 
 class Klazz {
 public:
@@ -128,10 +131,46 @@
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz'
 // CHECK-FIXES: Klazz p12() {}
 
+const Klazz> p33() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz> p33() {}
+
 const Klazz* const p13() {}
 // CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
 // CHECK-FIXES: const Klazz* p13() {}
 
+const Klazz* const volatile p14() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz *
+// CHECK-FIXES: const Klazz* volatile p14() {}
+
+const MyStruct<0 < 1> p34() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p34() {}
+
+MyStruct<0 < 1> const p35() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const MyStruct<0 < 1>'
+// CHECK-FIXES: MyStruct<0 < 1> p35() {}
+
+Klazz const> const p36() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz const> p36() {}
+
+const Klazz const> *const p37() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz const> *p37() {}
+
+Klazz> const p38() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz> p38() {}
+
+const Klazz> p39() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz<
+// CHECK-FIXES: Klazz> p39() {}
+
+const Klazz 1)>> p40() {}
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: return type 'const Klazz 1)>> p40() {}
+
 // re-declaration of p15.
 const int p15();
 // CHECK-FIXES: int p15();
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.h
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.h
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.h
@@ -92,13 +92,15 @@
  const SourceManager &SM,
  const LangOptions &LangOpts);
 
-/// Assuming that ``Range`` spans a const-qualified type, returns the ``const``
-/// token in ``Range`` that is responsible for const qualification. ``Range``
-/// must be valid with respect to ``SM``.  Returns ``None`` if no ``const``
+/// Assuming that ``Range`` spans a CVR-qualified type, returns the
+/// token in ``Range`` that is responsible for the qualification. ``Range``
+/// must be valid with respect to ``SM``.  Returns ``None`` if no qualifying
 /// tokens are found.
-llvm::Optional getConstQualifyingToken(CharSourceRange Range,
-  const ASTContext &Context,
-  const SourceManager &SM);
+/// \note: doesn't support member function qualifiers.
+llvm::Optional getQualifyingToken(tok::TokenKind TK,
+ CharSourceRange Range,
+ const ASTContext &Context,
+ const SourceManager &SM);
 
 } // namespace lexer
 } // namespace utils
Index: clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
===
--- clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
+++ clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
@@ -102,15 +102,20 @@
   return false;
 }
 
-llvm::Optional getConstQualifyingToken(CharSourceRange Range,
-  const ASTContext &Context,
-  const SourceManager &SM) {
+llvm::Optional getQualifyingToken(tok::TokenKind TK,
+ CharSourceRange Range,
+ const ASTContext &Context,
+ const SourceManager &SM) {
+  assert((TK == tok::kw_const || TK == tok::kw_volatile ||
+  TK == tok::kw_restrict) &&
+ "TK is not a qualifier keyword");
   std::pair LocInfo = SM.getDecomposedLoc(Range.getBegin());
   StringRef File = SM.getBufferData(LocInfo.first);
   Lexer RawLexer(SM.getLocFor

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

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

In D71714#1791464 , @riccibruno wrote:

> These are not the only AST nodes representing cast expressions (there is also 
> `CXXFunctionalCastExpr`, ...). What about replacing the 
> `IgnoreParenImpCasts()` above by `IgnoreParenCasts()` ? Incidentally doing 
> this uncovers another test (`Parser/cxx-ambig-decl-expr.cpp )` where this 
> diagnostic is triggered.


Thanks, using `IgnoreParenCasts` is even better.




Comment at: clang/lib/Sema/SemaChecking.cpp:13384
   case Stmt::MemberExprClass: {
 expr = cast(expr)->getBase();
 break;

rsmith wrote:
> Hmm, don't we need to do different things for dot and arrow in this case?
There are several test cases for an out of bounds access on an array member 
using dot and arrow operators in array-bounds.cpp. Do you have a specific test 
case for which you think the code is broken?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

ilya updated this revision to Diff 235184.
ilya added a comment.

1. Refactor `CheckArrayAccess()` to take `AllowPastTheEnd` parameter to avoid a 
false positive array out-of-bounds warning for `&(cond ? arr1[N] : arr2[N])`.
2. Use `IgnoreParenCasts()` instead of `IgnoreParenImpCasts()` in 
`CheckArrayAccess()` to avoid false negatives when explicit cast is involved.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714

Files:
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Parser/cxx-ambig-decl-expr.cpp
  clang/test/SemaCXX/array-bounds.cpp

Index: clang/test/SemaCXX/array-bounds.cpp
===
--- clang/test/SemaCXX/array-bounds.cpp
+++ clang/test/SemaCXX/array-bounds.cpp
@@ -27,7 +27,7 @@
 };
 
 void f1(int a[1]) {
-  int val = a[3]; // no warning for function argumnet
+  int val = a[3]; // no warning for function argument
 }
 
 void f2(const int (&a)[2]) { // expected-note {{declared here}}
@@ -133,7 +133,7 @@
 
 int test_sizeof_as_condition(int flag) {
   int arr[2] = { 0, 0 }; // expected-note {{array 'arr' declared here}}
-  if (flag) 
+  if (flag)
 return sizeof(char) != sizeof(char) ? arr[2] : arr[1];
   return sizeof(char) == sizeof(char) ? arr[2] : arr[1]; // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
 }
@@ -241,7 +241,7 @@
 }
 
 int test_pr11007_aux(const char * restrict, ...);
-  
+
 // Test checking with varargs.
 void test_pr11007() {
   double a[5]; // expected-note {{array 'a' declared here}}
@@ -309,3 +309,25 @@
 foo(); // expected-note 1{{in instantiation of function template specialization}}
   };
 }
+
+namespace PR44343 {
+  const unsigned int array[2] = {0, 1}; // expected-note 5{{array 'array' declared here}}
+
+  const int i1 = (const int)array[2]; // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  const int i2 = static_cast(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  const int &i3 = reinterpret_cast(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  unsigned int &i4 = const_cast(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  int i5 = int(array[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+  const unsigned int *i6 = &(1 > 0 ? array[2] : array[1]); // no warning for one-past-end element's address retrieval
+
+  struct Base {
+virtual ~Base();
+  };
+
+  struct Derived : Base {
+  };
+
+  Base baseArr[2]; // expected-note {{array 'baseArr' declared here}}
+  Derived *d1 = dynamic_cast(&baseArr[2]); // no warning for one-past-end element's address retrieval
+  Derived &d2 = dynamic_cast(baseArr[2]); // expected-warning {{array index 2 is past the end of the array (which contains 2 elements)}}
+}
Index: clang/test/Parser/cxx-ambig-decl-expr.cpp
===
--- clang/test/Parser/cxx-ambig-decl-expr.cpp
+++ clang/test/Parser/cxx-ambig-decl-expr.cpp
@@ -24,7 +24,7 @@
 
   // This is array indexing not an array declarator because a comma expression
   // is not syntactically a constant-expression.
-  int(x[1,1]); // expected-warning 2{{unused}}
+  int(x[1,0]); // expected-warning 2{{unused}}
 
   // This is array indexing not an array declaration because a braced-init-list
   // is not syntactically a constant-expression.
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13368,62 +13368,63 @@
 << ND->getDeclName());
 }
 
-void Sema::CheckArrayAccess(const Expr *expr) {
-  int AllowOnePastEnd = 0;
-  while (expr) {
-expr = expr->IgnoreParenImpCasts();
-switch (expr->getStmtClass()) {
-  case Stmt::ArraySubscriptExprClass: {
-const ArraySubscriptExpr *ASE = cast(expr);
-CheckArrayAccess(ASE->getBase(), ASE->getIdx(), ASE,
- AllowOnePastEnd > 0);
-expr = ASE->getBase();
-break;
-  }
-  case Stmt::MemberExprClass: {
-expr = cast(expr)->getBase();
-break;
-  }
-  case Stmt::OMPArraySectionExprClass: {
-const OMPArraySectionExpr *ASE = cast(expr);
-if (ASE->getLowerBound())
-  CheckArrayAccess(ASE->getBase(), ASE->getLowerBound(),
-   /*ASE=*/nullptr, AllowOnePastEnd > 0);
-return;
-  }
-  case Stmt::UnaryOperatorClass: {
-// Only unwrap the * and & unary operators
-const UnaryOperator *UO = cast(expr);
-expr = UO->getSubExpr();
-switch (UO->getOpcode()) {
-  case UO_Ad

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

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

Implemented @riccibruno's and @rsmith's comments.
While I agree with the general notion about the flaws with the current 
implementation, I feel that the more major refactoring proposed in this review 
is out of the scope of this minor fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

ilya added a comment.

One last kind ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

ilya added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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

[PATCH] D71714: [Sema] Fix -Warray-bounds false negative when casting an out-of-bounds array item

[Ilya Mirsky via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaChecking.cpp:13384
   case Stmt::MemberExprClass: {
 expr = cast(expr)->getBase();
 break;

ebevhan wrote:
> ilya wrote:
> > rsmith wrote:
> > > ilya wrote:
> > > > rsmith wrote:
> > > > > Hmm, don't we need to do different things for dot and arrow in this 
> > > > > case?
> > > > There are several test cases for an out of bounds access on an array 
> > > > member using dot and arrow operators in array-bounds.cpp. Do you have a 
> > > > specific test case for which you think the code is broken?
> > > > There are several test cases for an out of bounds access on an array 
> > > > member using dot and arrow operators in array-bounds.cpp. Do you have a 
> > > > specific test case for which you think the code is broken?
> > > 
> > > Sure. There's a false negative for this:
> > > 
> > > ```
> > > struct A { int n; };
> > > A *a[4];
> > > int *n = &a[4]->n;
> > > ```
> > > 
> > > ... because we incorrectly visit the left-hand side of the `->` with 
> > > `AllowOnePastEnd == 1`. The left-hand side of `->` is subject to 
> > > lvalue-to-rvalue conversion, so can't be one-past-the-end regardless of 
> > > the context in which the `->` appears.
> > > ... because we incorrectly visit the left-hand side of the -> with 
> > > AllowOnePastEnd == 1. The left-hand side of -> is subject to 
> > > lvalue-to-rvalue conversion, so can't be one-past-the-end regardless of 
> > > the context in which the -> appears.
> > 
> > Thanks for clarifying. So basically we don't want to allow one-past-end for 
> > MemberExpr?
> > Thanks for clarifying. So basically we don't want to allow one-past-end for 
> > MemberExpr?
> 
> I think the point is rather that when the MemberExpr isArrow, we want to 
> think of it as performing an implicit dereference first, and thus we should 
> do `AllowOnePastEnd--` before recursing if that is the case.
> I think the point is rather that when the MemberExpr isArrow, we want to 
> think of it as performing an implicit dereference first, and thus we should 
> do AllowOnePastEnd-- before recursing if that is the case.

@ebevhan and I have discussed this offline, and we believe that there's no 
reason to allow an //address-of// of a member of a one past-the-end value, 
using either '.' or '->' operators, regardless of the fact that referencing a 
member of a one past-the-end value using '.' is merely a pointer arithmetic and 
doesn't imply dereferencing, as with operator '->'.

We couldn't find any reference in the [[ 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4659.pdf| C++ 2017 
N4659 ]] regarding member access of a one past-the-end value. Only mentions of 
one past-the-end in the context of iterators (27.2.1 p. 7) and pointer 
arithmetic (8.3.1 p. 3).

[[ http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf | ISO C99 ]] 
describes in 6.5.3.2 p. 3 that if the operand to the '&' operator is the result 
of an unary '*' operator (or array subscript, implictly), the two operators 
"cancel" each other, but it says nothing about the case when there's a member 
expression "in-between", (the member expression section, 6.5.2.3, says nothing 
about that either).

@rsmith, what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71714



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