[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
tbaeder marked an inline comment as done.
Closed by commit rGf8cedc642d9b: [clang] Never wrap a nullptr in 
CXXNewExpr::getArraySize() (authored by tbaeder).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119525

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/AST/issue53742.cpp

Index: clang/test/AST/issue53742.cpp
===
--- /dev/null
+++ clang/test/AST/issue53742.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+struct Data {
+  char *a;
+  char *b;
+  bool *c;
+};
+
+int main() {
+  Data in;
+  in.a = new char[](); // expected-error {{cannot determine allocated array size from initializer}}
+  in.c = new bool[100]();
+  in.b = new char[100]();
+}
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11912,9 +11912,9 @@
 
   // Transform the size of the array we're allocating (if any).
   Optional ArraySize;
-  if (Optional OldArraySize = E->getArraySize()) {
+  if (E->isArray()) {
 ExprResult NewArraySize;
-if (*OldArraySize) {
+if (Optional OldArraySize = E->getArraySize()) {
   NewArraySize = getDerived().TransformExpr(*OldArraySize);
   if (NewArraySize.isInvalid())
 return ExprError();
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -2132,10 +2132,10 @@
   if (E->isParenTypeId())
 OS << "(";
   std::string TypeS;
-  if (Optional Size = E->getArraySize()) {
+  if (E->isArray()) {
 llvm::raw_string_ostream s(TypeS);
 s << '[';
-if (*Size)
+if (Optional Size = E->getArraySize())
   (*Size)->printPretty(s, Helper, Policy);
 s << ']';
   }
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9427,7 +9427,7 @@
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional ArraySize = E->getArraySize()) {
+  if (Optional ArraySize = E->getArraySize()) {
 const Expr *Stripped = *ArraySize;
 for (; auto *ICE = dyn_cast(Stripped);
  Stripped = ICE->getSubExpr())
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -2261,15 +2261,32 @@
 
   bool isArray() const { return CXXNewExprBits.IsArray; }
 
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto *Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
+
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() const {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto *Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
 
   unsigned getNumPlacementArgs() const {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -54,6 +54,14 @@
   There is an analogous ``zero_call_used_regs`` attribute to allow for finer
   control of this feature.
 
+Bug Fixes
+--
+- ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
+  wrapping a ``nullptr`` when the ``CXXNewExpr`` did not have an array
+  size expression. This was fixed and ``::getArraySize()`` will now always
+  either return ``None`` or a ``llvm::Optional`` wrapping a valid ``Expr*``.
+  This fixes `Issue #53742`_.
+
 Improvements to Clang's diagnostics
 ^^^
 
@@ -83,7 +91,8 @@
 - Added support for parameter pack expansion in `clang::annotate`.
 
 - The ``overloadable`` attribute can now be written in all of the syntactic
-  locations a declaration attribute may appear. Fixes PR53805.
+  locations a declaration attr

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

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



Comment at: clang/docs/ReleaseNotes.rst:54
+--
+- ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
+  wrapping a ``nullptr`` when the ``CXXNewExpr`` did not have an array

aaron.ballman wrote:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > Just added a reference to the bug that was fixed.
> > I was wondering about this. Where does the "PRXX" syntax come from? 
> > Since the repo and issues are on github now, I don't see how it makes sense 
> > to call it a PR (it's an issue, not a pull request) and github doesn't 
> > linkify those, while it does when using the `#xx` syntax (which isn't 
> > relevant in this case, but it is when using it in git commit messages). I 
> > have seen other people use the same syntax to refer to issues. I'd probably 
> > just add an actual link to the github issue here if that's permitted. 
> > Thanks for the suggestion though, I'm on PTO this week so don't land this 
> > until Monday  :)
> > I was wondering about this. Where does the "PRXX" syntax come from?
> 
> "Problem Report" -- ancient terminology.
> 
> > I have seen other people use the same syntax to refer to issues. I'd 
> > probably just add an actual link to the github issue here if that's 
> > permitted.
> 
> TBH, I think that's an even better suggestion (linking to the issue). One 
> concern I have is that it's really hard to tell whether the number is a 
> bugzilla issue number or a GitHub issue number (I suppose we should all 
> assume they're always github issue numbers these days though), so I wasn't 
> keen on having a number with no prefix to it. But if we're linking to what's 
> been fixed, then there's never a chance for confusion.
> 
> > Thanks for the suggestion though, I'm on PTO this week so don't land this 
> > until Monday :)
> 
> Sounds good to me, enjoy your PTO!
I added a link for the new entry and replaced the old PR mention with a link as 
well, so it's consistent at least.


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 410233.

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

https://reviews.llvm.org/D119525

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/AST/issue53742.cpp

Index: clang/test/AST/issue53742.cpp
===
--- /dev/null
+++ clang/test/AST/issue53742.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+struct Data {
+  char *a;
+  char *b;
+  bool *c;
+};
+
+int main() {
+  Data in;
+  in.a = new char[](); // expected-error {{cannot determine allocated array size from initializer}}
+  in.c = new bool[100]();
+  in.b = new char[100]();
+}
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11912,9 +11912,9 @@
 
   // Transform the size of the array we're allocating (if any).
   Optional ArraySize;
-  if (Optional OldArraySize = E->getArraySize()) {
+  if (E->isArray()) {
 ExprResult NewArraySize;
-if (*OldArraySize) {
+if (Optional OldArraySize = E->getArraySize()) {
   NewArraySize = getDerived().TransformExpr(*OldArraySize);
   if (NewArraySize.isInvalid())
 return ExprError();
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -2132,10 +2132,10 @@
   if (E->isParenTypeId())
 OS << "(";
   std::string TypeS;
-  if (Optional Size = E->getArraySize()) {
+  if (E->isArray()) {
 llvm::raw_string_ostream s(TypeS);
 s << '[';
-if (*Size)
+if (Optional Size = E->getArraySize())
   (*Size)->printPretty(s, Helper, Policy);
 s << ']';
   }
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9427,7 +9427,7 @@
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional ArraySize = E->getArraySize()) {
+  if (Optional ArraySize = E->getArraySize()) {
 const Expr *Stripped = *ArraySize;
 for (; auto *ICE = dyn_cast(Stripped);
  Stripped = ICE->getSubExpr())
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -2261,15 +2261,32 @@
 
   bool isArray() const { return CXXNewExprBits.IsArray; }
 
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto *Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
+
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() const {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto *Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
 
   unsigned getNumPlacementArgs() const {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -54,6 +54,14 @@
   There is an analogous ``zero_call_used_regs`` attribute to allow for finer
   control of this feature.
 
+Bug Fixes
+--
+- ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
+  wrapping a ``nullptr`` when the ``CXXNewExpr`` did not have an array
+  size expression. This was fixed and ``::getArraySize()`` will now always
+  either return ``None`` or a ``llvm::Optional`` wrapping a valid ``Expr*``.
+  This fixes `Issue #53742`_.
+
 Improvements to Clang's diagnostics
 ^^^
 
@@ -83,7 +91,8 @@
 - Added support for parameter pack expansion in `clang::annotate`.
 
 - The ``overloadable`` attribute can now be written in all of the syntactic
-  locations a declaration attribute may appear. Fixes PR53805.
+  locations a declaration attribute may appear.
+  This fixes `Issue #53805`_.
 
 Windows Support
 ---
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:54
+--
+- ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
+  wrapping a ``nullptr`` when the ``CXXNewExpr`` did not have an array

tbaeder wrote:
> aaron.ballman wrote:
> > Just added a reference to the bug that was fixed.
> I was wondering about this. Where does the "PRXX" syntax come from? Since 
> the repo and issues are on github now, I don't see how it makes sense to call 
> it a PR (it's an issue, not a pull request) and github doesn't linkify those, 
> while it does when using the `#xx` syntax (which isn't relevant in this 
> case, but it is when using it in git commit messages). I have seen other 
> people use the same syntax to refer to issues. I'd probably just add an 
> actual link to the github issue here if that's permitted. Thanks for the 
> suggestion though, I'm on PTO this week so don't land this until Monday  :)
> I was wondering about this. Where does the "PRXX" syntax come from?

"Problem Report" -- ancient terminology.

> I have seen other people use the same syntax to refer to issues. I'd probably 
> just add an actual link to the github issue here if that's permitted.

TBH, I think that's an even better suggestion (linking to the issue). One 
concern I have is that it's really hard to tell whether the number is a 
bugzilla issue number or a GitHub issue number (I suppose we should all assume 
they're always github issue numbers these days though), so I wasn't keen on 
having a number with no prefix to it. But if we're linking to what's been 
fixed, then there's never a chance for confusion.

> Thanks for the suggestion though, I'm on PTO this week so don't land this 
> until Monday :)

Sounds good to me, enjoy your PTO!


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/docs/ReleaseNotes.rst:54
+--
+- ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
+  wrapping a ``nullptr`` when the ``CXXNewExpr`` did not have an array

aaron.ballman wrote:
> Just added a reference to the bug that was fixed.
I was wondering about this. Where does the "PRXX" syntax come from? Since 
the repo and issues are on github now, I don't see how it makes sense to call 
it a PR (it's an issue, not a pull request) and github doesn't linkify those, 
while it does when using the `#xx` syntax (which isn't relevant in this 
case, but it is when using it in git commit messages). I have seen other people 
use the same syntax to refer to issues. I'd probably just add an actual link to 
the github issue here if that's permitted. Thanks for the suggestion though, 
I'm on PTO this week so don't land this until Monday  :)


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Continues to LGTM, though I had a recommendation for the release note. Feel 
free to land when you'd like.




Comment at: clang/docs/ReleaseNotes.rst:54
+--
+- ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
+  wrapping a ``nullptr`` when the ``CXXNewExpr`` did not have an array

Just added a reference to the bug that was fixed.


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 409173.

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

https://reviews.llvm.org/D119525

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/AST/issue53742.cpp

Index: clang/test/AST/issue53742.cpp
===
--- /dev/null
+++ clang/test/AST/issue53742.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+struct Data {
+  char *a;
+  char *b;
+  bool *c;
+};
+
+int main() {
+  Data in;
+  in.a = new char[](); // expected-error {{cannot determine allocated array size from initializer}}
+  in.c = new bool[100]();
+  in.b = new char[100]();
+}
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11918,9 +11918,9 @@
 
   // Transform the size of the array we're allocating (if any).
   Optional ArraySize;
-  if (Optional OldArraySize = E->getArraySize()) {
+  if (E->isArray()) {
 ExprResult NewArraySize;
-if (*OldArraySize) {
+if (Optional OldArraySize = E->getArraySize()) {
   NewArraySize = getDerived().TransformExpr(*OldArraySize);
   if (NewArraySize.isInvalid())
 return ExprError();
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -2132,10 +2132,10 @@
   if (E->isParenTypeId())
 OS << "(";
   std::string TypeS;
-  if (Optional Size = E->getArraySize()) {
+  if (E->isArray()) {
 llvm::raw_string_ostream s(TypeS);
 s << '[';
-if (*Size)
+if (Optional Size = E->getArraySize())
   (*Size)->printPretty(s, Helper, Policy);
 s << ']';
   }
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9427,7 +9427,7 @@
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional ArraySize = E->getArraySize()) {
+  if (Optional ArraySize = E->getArraySize()) {
 const Expr *Stripped = *ArraySize;
 for (; auto *ICE = dyn_cast(Stripped);
  Stripped = ICE->getSubExpr())
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -2261,15 +2261,32 @@
 
   bool isArray() const { return CXXNewExprBits.IsArray; }
 
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto *Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
+
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() const {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto *Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
 
   unsigned getNumPlacementArgs() const {
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -49,6 +49,13 @@
 
 -  ...
 
+Bug Fixes
+--
+- ``CXXNewExpr::getArraySize()`` previously returned a ``llvm::Optional``
+  wrapping a ``nullptr`` when the ``CXXNewExpr`` did not have an array
+  size expression. This was fixed and ``::getArraySize()`` will now always
+  either return ``None`` or a ``llvm::Optional`` wrapping a valid ``Expr*``.
+
 Improvements to Clang's diagnostics
 ^^^
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D119525#3317623 , @tbaeder wrote:

> In D119525#3314383 , @aaron.ballman 
> wrote:
>
>> Can you also add a release note for the change?
>
> Would this go in the "libclang" section in `clang/docs/ReleaseNotes.rst`? Or 
> somewhere else?

Nope, I'd add a new section titled "Bug Fixes" after "Major New Features" and 
then add it there.


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

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

In D119525#3314383 , @aaron.ballman 
wrote:

> Can you also add a release note for the change?

Would this go in the "libclang" section in `clang/docs/ReleaseNotes.rst`? Or 
somewhere else?


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

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



Comment at: clang/lib/AST/StmtPrinter.cpp:2135
   std::string TypeS;
-  if (Optional Size = E->getArraySize()) {
+  if (E->isArray()) {
 llvm::raw_string_ostream s(TypeS);

aaron.ballman wrote:
> tbaeder wrote:
> > I changed this just for clarity, not doing it did not cause any tests to 
> > fail. Not sure if this is tested at all.
> -ast-print is... pretty terrible (I've had half a mind to put up an RFC 
> asking if we should remove it entirely, that's how unmaintained it is), so I 
> suspect it's not tested.
Alright, I'll leave it at that then. I briefly tried to test the code but 
didn't succeed.


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 407912.

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

https://reviews.llvm.org/D119525

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/AST/issue53742.cpp

Index: clang/test/AST/issue53742.cpp
===
--- /dev/null
+++ clang/test/AST/issue53742.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+struct Data {
+  char *a;
+  char *b;
+  bool *c;
+};
+
+int main() {
+  Data in;
+  in.a = new char[](); // expected-error {{cannot determine allocated array size from initializer}}
+  in.c = new bool[100]();
+  in.b = new char[100]();
+}
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11918,9 +11918,9 @@
 
   // Transform the size of the array we're allocating (if any).
   Optional ArraySize;
-  if (Optional OldArraySize = E->getArraySize()) {
+  if (E->isArray()) {
 ExprResult NewArraySize;
-if (*OldArraySize) {
+if (Optional OldArraySize = E->getArraySize()) {
   NewArraySize = getDerived().TransformExpr(*OldArraySize);
   if (NewArraySize.isInvalid())
 return ExprError();
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -2132,10 +2132,10 @@
   if (E->isParenTypeId())
 OS << "(";
   std::string TypeS;
-  if (Optional Size = E->getArraySize()) {
+  if (E->isArray()) {
 llvm::raw_string_ostream s(TypeS);
 s << '[';
-if (*Size)
+if (Optional Size = E->getArraySize())
   (*Size)->printPretty(s, Helper, Policy);
 s << ']';
   }
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9427,7 +9427,7 @@
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional ArraySize = E->getArraySize()) {
+  if (Optional ArraySize = E->getArraySize()) {
 const Expr *Stripped = *ArraySize;
 for (; auto *ICE = dyn_cast(Stripped);
  Stripped = ICE->getSubExpr())
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -2261,15 +2261,32 @@
 
   bool isArray() const { return CXXNewExprBits.IsArray; }
 
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto *Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
+
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() const {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto *Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
 
   unsigned getNumPlacementArgs() const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from some minor nits. Can you also add a release note for the change?




Comment at: clang/include/clang/AST/ExprCXX.h:2271
+
+if (auto Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))





Comment at: clang/include/clang/AST/ExprCXX.h:2285
+
+if (auto Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))





Comment at: clang/lib/AST/StmtPrinter.cpp:2135
   std::string TypeS;
-  if (Optional Size = E->getArraySize()) {
+  if (E->isArray()) {
 llvm::raw_string_ostream s(TypeS);

tbaeder wrote:
> I changed this just for clarity, not doing it did not cause any tests to 
> fail. Not sure if this is tested at all.
-ast-print is... pretty terrible (I've had half a mind to put up an RFC asking 
if we should remove it entirely, that's how unmaintained it is), so I suspect 
it's not tested.


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/lib/AST/StmtPrinter.cpp:2135
   std::string TypeS;
-  if (Optional Size = E->getArraySize()) {
+  if (E->isArray()) {
 llvm::raw_string_ostream s(TypeS);

I changed this just for clarity, not doing it did not cause any tests to fail. 
Not sure if this is tested at all.


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 407885.

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

https://reviews.llvm.org/D119525

Files:
  clang/include/clang/AST/ExprCXX.h
  clang/lib/AST/ExprConstant.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/Sema/TreeTransform.h
  clang/test/AST/issue53742.cpp

Index: clang/test/AST/issue53742.cpp
===
--- /dev/null
+++ clang/test/AST/issue53742.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+struct Data {
+  char *a;
+  char *b;
+  bool *c;
+};
+
+int main() {
+  Data in;
+  in.a = new char[](); // expected-error {{cannot determine allocated array size from initializer}}
+  in.c = new bool[100]();
+  in.b = new char[100]();
+}
Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -11918,9 +11918,9 @@
 
   // Transform the size of the array we're allocating (if any).
   Optional ArraySize;
-  if (Optional OldArraySize = E->getArraySize()) {
+  if (E->isArray()) {
 ExprResult NewArraySize;
-if (*OldArraySize) {
+if (Optional OldArraySize = E->getArraySize()) {
   NewArraySize = getDerived().TransformExpr(*OldArraySize);
   if (NewArraySize.isInvalid())
 return ExprError();
Index: clang/lib/AST/StmtPrinter.cpp
===
--- clang/lib/AST/StmtPrinter.cpp
+++ clang/lib/AST/StmtPrinter.cpp
@@ -2132,10 +2132,10 @@
   if (E->isParenTypeId())
 OS << "(";
   std::string TypeS;
-  if (Optional Size = E->getArraySize()) {
+  if (E->isArray()) {
 llvm::raw_string_ostream s(TypeS);
 s << '[';
-if (*Size)
+if (Optional Size = E->getArraySize())
   (*Size)->printPretty(s, Helper, Policy);
 s << ']';
   }
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9427,7 +9427,7 @@
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional ArraySize = E->getArraySize()) {
+  if (Optional ArraySize = E->getArraySize()) {
 const Expr *Stripped = *ArraySize;
 for (; auto *ICE = dyn_cast(Stripped);
  Stripped = ICE->getSubExpr())
Index: clang/include/clang/AST/ExprCXX.h
===
--- clang/include/clang/AST/ExprCXX.h
+++ clang/include/clang/AST/ExprCXX.h
@@ -2261,15 +2261,32 @@
 
   bool isArray() const { return CXXNewExprBits.IsArray; }
 
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
+
+  /// This might return None even if isArray() returns true,
+  /// since there might not be an array size expression.
+  /// If the result is not-None, it will never wrap a nullptr.
   Optional getArraySize() const {
 if (!isArray())
   return None;
-return cast_or_null(getTrailingObjects()[arraySizeOffset()]);
+
+if (auto Result =
+cast_or_null(getTrailingObjects()[arraySizeOffset()]))
+  return Result;
+
+return None;
   }
 
   unsigned getNumPlacementArgs() const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D119525#3313554 , @tbaeder wrote:

> This is a pretty big gotcha at best and I feel like `getArraySize()` should 
> return `None` in that case instead... thoughts?

I think it is a gotcha -- some places protect against a null pointer 
(https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/TreeTransform.h#L11923
 and 
https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/StmtPrinter.cpp#L2138)
 while other places assume the pointer isn't null 
(https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGExprCXX.cpp#L731
 and 
https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ExprConstant.cpp#L9431).
 So I think a better approach is to fix up the interface so it doesn't return 
nullptr rather than play whack-a-mole forever with the API (and fix up the 
places currently checking for nullptr explicitly).


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

https://reviews.llvm.org/D119525

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

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

tbaeder updated this revision to Diff 407785.
tbaeder added a comment.

This is a pretty big gotcha at best and I feel like `getArraySize()` should 
return `None` in that case instead... thoughts?


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

https://reviews.llvm.org/D119525

Files:
  clang/lib/AST/ExprConstant.cpp
  clang/test/AST/issue53742.cpp


Index: clang/test/AST/issue53742.cpp
===
--- /dev/null
+++ clang/test/AST/issue53742.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+struct Data {
+  char *a;
+  char *b;
+  bool *c;
+};
+
+int main() {
+  Data in;
+  in.a = new char[](); // expected-error {{cannot determine allocated array 
size from initializer}}
+  in.c = new bool[100]();
+  in.b = new char[100]();
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9427,7 +9427,8 @@
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional ArraySize = E->getArraySize()) {
+  Optional ArraySize = E->getArraySize();
+  if (ArraySize && *ArraySize) {
 const Expr *Stripped = *ArraySize;
 for (; auto *ICE = dyn_cast(Stripped);
  Stripped = ICE->getSubExpr())


Index: clang/test/AST/issue53742.cpp
===
--- /dev/null
+++ clang/test/AST/issue53742.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only %s -verify
+
+struct Data {
+  char *a;
+  char *b;
+  bool *c;
+};
+
+int main() {
+  Data in;
+  in.a = new char[](); // expected-error {{cannot determine allocated array size from initializer}}
+  in.c = new bool[100]();
+  in.b = new char[100]();
+}
Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9427,7 +9427,8 @@
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional ArraySize = E->getArraySize()) {
+  Optional ArraySize = E->getArraySize();
+  if (ArraySize && *ArraySize) {
 const Expr *Stripped = *ArraySize;
 for (; auto *ICE = dyn_cast(Stripped);
  Stripped = ICE->getSubExpr())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119525: [clang] Fix crash when array size is missing in initializer

[Timm Bäder via Phabricator via cfe-commits]

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

This fixes the crash from https://github.com/llvm/llvm-project/issues/53742

`CXXNewExpr::getArraySize()` may return a `llvm::Optional<>` pointing to a 
`nullptr`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119525

Files:
  clang/lib/AST/ExprConstant.cpp


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9427,8 +9427,8 @@
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional ArraySize = E->getArraySize()) {
-const Expr *Stripped = *ArraySize;
+  Optional ArraySize = E->getArraySize();
+  if (const Expr *Stripped = *ArraySize) {
 for (; auto *ICE = dyn_cast(Stripped);
  Stripped = ICE->getSubExpr())
   if (ICE->getCastKind() != CK_NoOp &&


Index: clang/lib/AST/ExprConstant.cpp
===
--- clang/lib/AST/ExprConstant.cpp
+++ clang/lib/AST/ExprConstant.cpp
@@ -9427,8 +9427,8 @@
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional ArraySize = E->getArraySize()) {
-const Expr *Stripped = *ArraySize;
+  Optional ArraySize = E->getArraySize();
+  if (const Expr *Stripped = *ArraySize) {
 for (; auto *ICE = dyn_cast(Stripped);
  Stripped = ICE->getSubExpr())
   if (ICE->getCastKind() != CK_NoOp &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits