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

[tbaeder]

Summary

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

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

[tbaeder]

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

[aaron.ballman]

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).

[tbaeder]

clang/lib/AST/StmtPrinter.cpp

2135

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

[aaron.ballman]

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

clang/include/clang/AST/ExprCXX.h

2271

2285

clang/lib/AST/StmtPrinter.cpp

2135

-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.

[tbaeder]

clang/lib/AST/StmtPrinter.cpp

2135

Alright, I'll leave it at that then. I briefly tried to test the code but didn't succeed.

[tbaeder]

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?

[aaron.ballman]

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.

[aaron.ballman]

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

clang/docs/ReleaseNotes.rst

59

Just added a reference to the bug that was fixed.

[tbaeder]

clang/docs/ReleaseNotes.rst

59

I was wondering about this. Where does the "PRXXXXXX" 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 #xxxxxx 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 :)

[aaron.ballman]

clang/docs/ReleaseNotes.rst

59

I was wondering about this. Where does the "PRXXXXXX" 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!

[tbaeder]

clang/docs/ReleaseNotes.rst

59

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.

clang/docs/ReleaseNotes.rst

 
 - Clang now supports the ``-fzero-call-used-regs`` feature for x86. The purpose
   of this feature is to limit Return-Oriented Programming (ROP) exploits and
   information leakage. It works by zeroing out a selected class of registers
   before function return --- e.g., all GPRs that are used within the function.
   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<https://github.com/llvm/llvm-project/issues/53742>`_.
+
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 New Compiler Flags
 ------------------

 - ...
 
 Attribute Changes in Clang
 --------------------------
 
 - 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<https://github.com/llvm/llvm-project/issues/53805>`_.
 
 Windows Support
 ---------------
 
 - Add support for MSVC-compatible ``/JMC``/``/JMC-`` flag in clang-cl (supports
   X86/X64/ARM/ARM64). ``/JMC`` could only be used when ``/Zi`` or ``/Z7`` is
   turned on. With this addition, clang-cl can be used in Visual Studio for the
   JustMyCode feature. Note, you may need to manually add ``/JMC`` as additional

clang/include/clang/AST/ExprCXX.h

public:
 
   FunctionDecl *getOperatorNew() const { return OperatorNew; }
   void setOperatorNew(FunctionDecl *D) { OperatorNew = D; }
   FunctionDecl *getOperatorDelete() const { return OperatorDelete; }
   void setOperatorDelete(FunctionDecl *D) { OperatorDelete = D; }
 
   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<Expr *> getArraySize() {
     if (!isArray())
       return None;
-    return cast_or_null<Expr>(getTrailingObjects<Stmt *>()[arraySizeOffset()]);
+
+    if (auto *Result =
+            cast_or_null<Expr>(getTrailingObjects<Stmt *>()[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<const Expr *> getArraySize() const {
     if (!isArray())
       return None;
-    return cast_or_null<Expr>(getTrailingObjects<Stmt *>()[arraySizeOffset()]);
+
+    if (auto *Result =
+            cast_or_null<Expr>(getTrailingObjects<Stmt *>()[arraySizeOffset()]))
+      return Result;
+
+    return None;
   }
 
   unsigned getNumPlacementArgs() const {
     return CXXNewExprBits.NumPlacementArgs;
   }
 
   Expr **getPlacementArgs() {
     return reinterpret_cast<Expr **>(getTrailingObjects<Stmt *>() +

clang/lib/AST/ExprConstant.cpp

bool PointerExprEvaluator::VisitCXXNewExpr(const CXXNewExpr *E) {
   }
 
   const Expr *Init = E->getInitializer();
   const InitListExpr *ResizedArrayILE = nullptr;
   const CXXConstructExpr *ResizedArrayCCE = nullptr;
   bool ValueInit = false;
 
   QualType AllocType = E->getAllocatedType();
-  if (Optional<const Expr*> ArraySize = E->getArraySize()) {
+  if (Optional<const Expr *> ArraySize = E->getArraySize()) {
     const Expr *Stripped = *ArraySize;
     for (; auto *ICE = dyn_cast<ImplicitCastExpr>(Stripped);
          Stripped = ICE->getSubExpr())
       if (ICE->getCastKind() != CK_NoOp &&
           ICE->getCastKind() != CK_IntegralCast)
         break;
 
     llvm::APSInt ArrayBound;

clang/lib/AST/StmtPrinter.cpp

for (unsigned i = 1; i < NumPlace; ++i) {
       OS << ", ";
       PrintExpr(E->getPlacementArg(i));
     }
     OS << ") ";
   }
   if (E->isParenTypeId())
     OS << "(";
   std::string TypeS;
-  if (Optional<Expr *> Size = E->getArraySize()) {
+  if (E->isArray()) {
     llvm::raw_string_ostream s(TypeS);
     s << '[';
-    if (*Size)
+    if (Optional<Expr *> Size = E->getArraySize())
       (*Size)->printPretty(s, Helper, Policy);
     s << ']';
   }
   E->getAllocatedType().print(OS, Policy, TypeS);
   if (E->isParenTypeId())
     OS << ")";
 
   CXXNewExpr::InitializationStyle InitStyle = E->getInitializationStyle();

clang/lib/Sema/TreeTransform.h

TreeTransform<Derived>::TransformCXXNewExpr(CXXNewExpr *E) {
   // Transform the type that we're allocating
   TypeSourceInfo *AllocTypeInfo =
       getDerived().TransformTypeWithDeducedTST(E->getAllocatedTypeSourceInfo());
   if (!AllocTypeInfo)
     return ExprError();
 
   // Transform the size of the array we're allocating (if any).
   Optional<Expr *> ArraySize;
-  if (Optional<Expr *> OldArraySize = E->getArraySize()) {
+  if (E->isArray()) {
     ExprResult NewArraySize;
-    if (*OldArraySize) {
+    if (Optional<Expr *> OldArraySize = E->getArraySize()) {
       NewArraySize = getDerived().TransformExpr(*OldArraySize);
       if (NewArraySize.isInvalid())
         return ExprError();
     }
     ArraySize = NewArraySize.get();
   }
 
   // Transform the placement arguments (if any).

clang/test/AST/issue53742.cpp

+// 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]();
+}