[Clang] Support MSPropertyRefExpr as placement arg to new-expression #75883

Sirraide · 2023-12-19T01:56:38Z

It seems we were forgetting to call checkArgsForPlaceholders on the placement arguments of new-expressions in Sema. I don't think that was intended—at least doing so doesn't seem to break anything—so this pr adds that.

This also fixes #65053

Sirraide · 2023-12-19T01:56:53Z

CC @AaronBallman

llvmbot · 2023-12-19T01:57:07Z

@llvm/pr-subscribers-clang

Author: None (Sirraide)

Changes

It seems we were forgetting to call checkArgsForPlaceholders on the placement arguments of new-expressions in Sema. I don't think that was intended—at least doing so doesn't seem to break anything—so this pr adds that.

This also fixes #65053

Full diff: https://github.com/llvm/llvm-project/pull/75883.diff

5 Files Affected:

(modified) clang/include/clang/Sema/Sema.h (+4)
(modified) clang/lib/Sema/SemaExpr.cpp (+4-8)
(modified) clang/lib/Sema/SemaExprCXX.cpp (+3)
(added) clang/test/CodeGenCXX/ms-property-new.cpp (+52)
(added) clang/test/SemaCXX/ms-property-new.cpp (+44)

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index a4f8fc1845b1ce..90be0a648f0124 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -2108,6 +2108,10 @@ class Sema final {
 
   bool CheckFunctionReturnType(QualType T, SourceLocation Loc);
 
+  /// Check an argument list for placeholders that we won't try to
+  /// handle later.
+  bool CheckArgsForPlaceholders(MultiExprArg args);
+
   /// Build a function type.
   ///
   /// This routine checks the function type according to C++ rules and
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index c7185d56cc9973..2442c1e104055b 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -5058,8 +5058,6 @@ static QualType getDependentArraySubscriptType(Expr *LHS, Expr *RHS,
   return Result->isDependentType() ? Result : Ctx.DependentTy;
 }
 
-static bool checkArgsForPlaceholders(Sema &S, MultiExprArg args);
-
 ExprResult Sema::ActOnArraySubscriptExpr(Scope *S, Expr *base,
                                          SourceLocation lbLoc,
                                          MultiExprArg ArgExprs,
@@ -5163,7 +5161,7 @@ ExprResult Sema::ActOnArraySubscriptExpr(Scope *S, Expr *base,
       return ExprError();
     ArgExprs[0] = result.get();
   } else {
-    if (checkArgsForPlaceholders(*this, ArgExprs))
+    if (CheckArgsForPlaceholders(ArgExprs))
       return ExprError();
   }
 
@@ -6912,15 +6910,13 @@ static bool isPlaceholderToRemoveAsArg(QualType type) {
   llvm_unreachable("bad builtin type kind");
 }
 
-/// Check an argument list for placeholders that we won't try to
-/// handle later.
-static bool checkArgsForPlaceholders(Sema &S, MultiExprArg args) {
+bool Sema::CheckArgsForPlaceholders(MultiExprArg args) {
   // Apply this processing to all the arguments at once instead of
   // dying at the first failure.
   bool hasInvalid = false;
   for (size_t i = 0, e = args.size(); i != e; i++) {
     if (isPlaceholderToRemoveAsArg(args[i]->getType())) {
-      ExprResult result = S.CheckPlaceholderExpr(args[i]);
+      ExprResult result = CheckPlaceholderExpr(args[i]);
       if (result.isInvalid()) hasInvalid = true;
       else args[i] = result.get();
     }
@@ -7194,7 +7190,7 @@ ExprResult Sema::BuildCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
   if (Result.isInvalid()) return ExprError();
   Fn = Result.get();
 
-  if (checkArgsForPlaceholders(*this, ArgExprs))
+  if (CheckArgsForPlaceholders(ArgExprs))
     return ExprError();
 
   if (getLangOpts().CPlusPlus) {
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 081b568762ae22..49f0921ce324fc 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -2286,6 +2286,9 @@ ExprResult Sema::BuildCXXNew(SourceRange Range, bool UseGlobal,
   bool PassAlignment = getLangOpts().AlignedAllocation &&
                        Alignment > NewAlignment;
 
+  if (CheckArgsForPlaceholders(PlacementArgs))
+    return ExprError();
+
   AllocationFunctionScope Scope = UseGlobal ? AFS_Global : AFS_Both;
   if (!AllocType->isDependentType() &&
       !Expr::hasAnyTypeDependentArguments(PlacementArgs) &&
diff --git a/clang/test/CodeGenCXX/ms-property-new.cpp b/clang/test/CodeGenCXX/ms-property-new.cpp
new file mode 100644
index 00000000000000..e21ec3d6702f62
--- /dev/null
+++ b/clang/test/CodeGenCXX/ms-property-new.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -emit-llvm -triple=x86_64-pc-win32 -fms-compatibility %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fms-compatibility -emit-pch -o %t %s
+// RUN: %clang_cc1 -emit-llvm -triple=x86_64-pc-win32 -fms-compatibility -include-pch %t -verify %s -o - | FileCheck %s
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+struct S {
+  int GetX() { return 42; }
+  __declspec(property(get=GetX)) int x;
+
+  int GetY(int i, int j) { return i+j; }
+  __declspec(property(get=GetY)) int y[];
+
+  void* operator new(__SIZE_TYPE__, int);
+};
+
+template <typename T>
+struct TS {
+  T GetT() { return T(); }
+  __declspec(property(get=GetT)) T t;
+
+  T GetR(T i, T j) { return i+j; }
+  __declspec(property(get=GetR)) T r[];
+};
+
+// CHECK-LABEL: main
+int main(int argc, char **argv) {
+  S *s;
+  TS<double> *ts;
+
+  // CHECK: [[X:%.+]] = call noundef i32 @"?GetX@S@@QEAAHXZ"(ptr {{[^,]*}} %{{.+}})
+  // CHECK-NEXT: call noundef ptr @"??2S@@SAPEAX_KH@Z"(i64 noundef 1, i32 noundef [[X]])
+  new (s->x) S;
+
+  // CHECK: [[Y:%.+]] = call noundef i32 @"?GetY@S@@QEAAHHH@Z"(ptr {{[^,]*}} %{{.+}}, i32 noundef 1, i32 noundef 2)
+  // CHECK-NEXT: call noundef ptr @"??2S@@SAPEAX_KH@Z"(i64 noundef 1, i32 noundef [[Y]])
+  new ((s->y)[1][2]) S;
+
+  // CHECK: [[T:%.+]] = call noundef double @"?GetT@?$TS@N@@QEAANXZ"(ptr {{[^,]*}} %{{.+}})
+  // CHECK-NEXT: [[TI:%.+]] = fptosi double [[T]] to i32
+  // CHECK-NEXT: call noundef ptr @"??2S@@SAPEAX_KH@Z"(i64 noundef 1, i32 noundef [[TI]])
+  new (ts->t) S;
+
+  // CHECK: [[R:%.+]] = call noundef double @"?GetR@?$TS@N@@QEAANNN@Z"(ptr {{[^,]*}} %{{.+}}, double {{[^,]*}}, double {{[^,]*}})
+  // CHECK-NEXT: [[RI:%.+]] = fptosi double [[R]] to i32
+  // CHECK-NEXT: call noundef ptr @"??2S@@SAPEAX_KH@Z"(i64 noundef 1, i32 noundef [[RI]])
+  new ((ts->r)[1][2]) S;
+}
+
+#endif
\ No newline at end of file
diff --git a/clang/test/SemaCXX/ms-property-new.cpp b/clang/test/SemaCXX/ms-property-new.cpp
new file mode 100644
index 00000000000000..d17b0fdeb76715
--- /dev/null
+++ b/clang/test/SemaCXX/ms-property-new.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_cc1 -ast-print -verify -triple=x86_64-pc-win32 -fms-compatibility %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fms-compatibility -emit-pch -o %t %s
+// RUN: %clang_cc1 -triple=x86_64-pc-win32 -fms-compatibility -include-pch %t -verify %s -ast-print -o - | FileCheck %s
+// expected-no-diagnostics
+
+#ifndef HEADER
+#define HEADER
+
+struct S {
+  int GetX() { return 42; }
+  __declspec(property(get=GetX)) int x;
+
+  int GetY(int i, int j) { return i+j; }
+  __declspec(property(get=GetY)) int y[];
+
+  void* operator new(__SIZE_TYPE__, int);
+};
+
+template <typename T>
+struct TS {
+  T GetT() { return T(); }
+  __declspec(property(get=GetT)) T t;
+
+  T GetR(T i, T j) { return i+j; }
+  __declspec(property(get=GetR)) T r[];
+};
+
+int main(int argc, char **argv) {
+  // CHECK: S *s;
+  // CHECK-NEXT: new (s->x) S;
+  // CHECK-NEXT: new ((s->y)[1][2]) S;
+  S *s;
+  new (s->x) S;
+  new ((s->y)[1][2]) S;
+
+  // CHECK-NEXT: TS<double> *ts;
+  // CHECK-NEXT: new (ts->t) S;
+  // CHECK-NEXT: new ((ts->r)[1][2]) S;
+  TS<double> *ts;
+  new (ts->t) S;
+  new ((ts->r)[1][2]) S;
+}
+
+#endif
\ No newline at end of file

clang/test/CodeGenCXX/ms-property-new.cpp

erichkeane · 2024-01-03T14:39:04Z

clang/lib/Sema/SemaExprCXX.cpp

@@ -2286,6 +2286,9 @@ ExprResult Sema::BuildCXXNew(SourceRange Range, bool UseGlobal,
  bool PassAlignment = getLangOpts().AlignedAllocation &&
                       Alignment > NewAlignment;

+  if (CheckArgsForPlaceholders(PlacementArgs))


This likely needs a good collection of tests to show what it changes for NON-property related 'new' operators.

I’ll take a look at what other expressions are actually affected by this other than MSPropertyRefExprs. (or do you mean we should just add more placement-new tests in general? Because I would assume we already have a bunch of those, but I haven’t checked to be fair?)

A bit of both, this should check the behavior of other expression types. This changes general 'placement new' behaviors, so it looks like it changes existing placement new behaviors that are seemingly untested, so should be showing off THOSE as well.

Makes sense. It seems like this affects only a small number of builtin types, so in theory, adding tests for every expression that could possibly have those types ought to do (if we don’t already have tests for those); I’m currently collecting a list of everything that could be affected by this.

The only expressions that this seems to affect is expressions for which isPlaceholderToRemoveAsArg returns true by passing them to CheckPlaceholderExpr; this only affects a handful of types:

BuiltinType::PseudoObject: MSProperty (subscript) properties and ObjC (subscript) properties; the latter are only relevant for Objective-C++ in this case. I’ll add tests for the latter as well (to the best of my abilities as I have never written a line of Objective-C or Objective-C++ before in my life). The comment in BuiltinTypes.def also mentions ‘VS.NET's __property declarations’, but I couldn’t find anything about __property in the rest of the codebase, and I’m candidly not entirely sure what this is in reference to, so I’m only adding tests for Objective-C++ for now.

BuiltinType::UnknownAny: For this, CheckPlaceholderExpr just emits an error, and I personally don’t see a way in which this would ever be valid as a placement-arg to a new expression, but I could be wrong here. Should we add tests for this case?

BuiltinType::BoundMember: s.foo and friends, where foo is a non-static member function; these are already invalid in placement-args, and CheckPlaceholderExpr similarly issues what looks to be the exact same error; we don’t seem to have any tests that check for this error, however, so I’ll add some for this as well.

BuiltinType::BuiltinFn: Only DeclRefExprs are affected here as this simply issues an error otherwise (which should be fine because passing a builtin function as a function argument doesn’t make sense anyway); if it is a builtin such as __builtin_memset, then this pr also ends up improving the error message from ‘no known conversion from '<builtin fn type>'’ to ‘builtin functions must be directly called’.

If it is a DeclRefExpr, there are two cases to consider: the first is the MS __noop builtin, which w/o this pr simply errors, but now, it is implicitly converted to a CallExpr that returns an int (which emits 0 in CG); this matches MSVCs behaviour, but I doubt we have a test for that, so I’ll add one as well.

The only other case that is affected here is if we have a standard-namespace builtin (e.g. std::move, std::forward_like, which before C++20 resulted in a warning, since C++20 in an error; this behaviour is unaffected by this change, but I’ll some tests for this as well because from what I can tell, there currently are no tests for this diagnostic at all.

BuiltinType::IncompleteMatrixIdx: From what I can tell, this is used for matrix types, specifically, for the type of an expression such as a[1] where a is a matrix type because matrix types always require two subscripts; since an expression of this type as a placement-arg is thus always invalid, this should error, but previously, we were emitting a less-than-ideal error (e.g. ‘no known conversion from '<incomplete matrix index type>' to 'int'’), whereas with this pr, we now get the error ‘single subscript expressions are not allowed for matrix values’. There does not seem to be a test for this that involves placement-new, so I’ll add a few as well.

BuiltinType::OMPArraySection, BuiltinType::OMPArrayShaping, and BuiltinType::OMPIterator: To my understanding, these cannot occur outside of OMP #pragmas, so they’re not relevant here.

tl;dr Overall, it seems like this pr also improves some error messages as well as support for MSVC’s __noop builtin. I’ll add some tests for the cases above (though again, for some of them, (e.g. the UnknownAny case), I’m not sure whether that’s necessary, so please let me know whether I should add some for those as well).

Oh, and Objective-C++ properties were suffering from the same problem, so this now also fixes those in new-expressions.

Alright, just got done adding some tests. Just let me know if there are more things that we should consider testing.

Also, regarding Objective-C++ properties: I tried following the examples in clang/test/AST/ast-dump-expr-json.m, but it didn’t seem to want to treat the subscripts as anything other than pointer arithmetic. It also doesn’t help that I’m not familiar w/ Objective-C/Objective-C++ at all. Could it be that Objective-C subscript properties are disabled in Objective-C++ mode?

erichkeane

1 issue with the test, else LGTM.

clang/test/SemaCXX/builtin-std-move.cpp

Sirraide · 2024-01-04T18:50:51Z

Just did a rebase and also updated the release notes to mention these changes

…ession

Sirraide · 2024-01-17T10:18:42Z

Just did another rebase to update the release notes.

Is there anything else that needs to be done for this pr?

erichkeane · 2024-01-17T14:21:17Z

Just did another rebase to update the release notes.

Is there anything else that needs to be done for this pr?

Nope! Once the CI is done, feel free to Squash & Merge.

Sirraide · 2024-01-17T17:46:21Z

Nope! Once the CI is done, feel free to Squash & Merge.

I would but I don’t have commit access, so someone else would have to do that for me. (Also, I do fear that in the time it takes for the CI jobs to complete, we’ll have another merge conflict w/ the release notes because they’re updated constantly.)

erichkeane · 2024-01-17T17:47:27Z

Nope! Once the CI is done, feel free to Squash & Merge.

I would but I don’t have commit access, so someone else would have to do that for me. (Also, I do fear that in the time it takes for the CI jobs to complete, we’ll have another merge conflict w/ the release notes because they’re updated constantly.)

I'll commit it when I see the CI done, and can do the release notes conflict. I wont bother re-running after conflict resolution.

Sirraide · 2024-01-17T17:47:55Z

I'll commit it when I see the CI done, and can do the release notes conflict. I wont bother re-running after conflict resolution.

Alright, thanks!

Sirraide · 2024-01-17T19:26:11Z

I’m curious, because I legitimately don’t know, is it common for CI on windows to take 9 hours?

erichkeane · 2024-01-17T19:43:34Z

I’m curious, because I legitimately don’t know, is it common for CI on windows to take 9 hours?

It is unfortunately becoming more and more common. The bot infrastructure is having trouble keeping up these days during the day.

…lvm#75883) It seems we were forgetting to call `checkArgsForPlaceholders` on the placement arguments of new-expressions in Sema. I don't think that was intended—at least doing so doesn't seem to break anything—so this pr adds that. This also fixes llvm#65053 --------- Co-authored-by: Erich Keane <ekeane@nvidia.com>

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 19, 2023

shafik requested review from AaronBallman and erichkeane December 19, 2023 04:17

erichkeane reviewed Jan 3, 2024

View reviewed changes

erichkeane reviewed Jan 4, 2024

View reviewed changes

clang/test/SemaCXX/builtin-std-move.cpp Outdated Show resolved Hide resolved

erichkeane reviewed Jan 4, 2024

View reviewed changes

clang/test/SemaCXX/builtin-std-move.cpp Outdated Show resolved Hide resolved

erichkeane approved these changes Jan 4, 2024

View reviewed changes

Sirraide force-pushed the main branch from 010de24 to 6db5fc6 Compare January 4, 2024 18:48

AaronBallman requested a review from erichkeane January 16, 2024 20:17

erichkeane approved these changes Jan 16, 2024

View reviewed changes

Sirraide added 6 commits January 17, 2024 11:13

[Clang] Support MSPropertyRefExpr as placement arg to new-expression

849b21f

[NFC] Add newline at end of file

f025348

[Clang] Add tests for placeholder exprs in placement-args of new-expr…

9b540da

…ession

[Clang] [NFC] Modernise expected directives

56c8413

[Clang] [NFC] Reorder expected directives

742f4c3

[Clang] [NFC] Update release notes

0ab9622

Sirraide force-pushed the main branch from 9805381 to 0ab9622 Compare January 17, 2024 10:17

Merge branch 'main' into main

fd03a66

erichkeane merged commit af0ee61 into llvm:main Jan 17, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] Support MSPropertyRefExpr as placement arg to new-expression #75883

[Clang] Support MSPropertyRefExpr as placement arg to new-expression #75883

Sirraide commented Dec 19, 2023

Sirraide commented Dec 19, 2023

llvmbot commented Dec 19, 2023

erichkeane Jan 3, 2024

Sirraide Jan 3, 2024

erichkeane Jan 3, 2024

Sirraide Jan 3, 2024

Sirraide Jan 4, 2024 •

edited

Sirraide Jan 4, 2024

Sirraide Jan 4, 2024

Sirraide Jan 4, 2024

erichkeane left a comment

Sirraide commented Jan 4, 2024

Sirraide commented Jan 17, 2024

erichkeane commented Jan 17, 2024

Sirraide commented Jan 17, 2024

erichkeane commented Jan 17, 2024

Sirraide commented Jan 17, 2024

Sirraide commented Jan 17, 2024

erichkeane commented Jan 17, 2024

[Clang] Support MSPropertyRefExpr as placement arg to new-expression #75883

[Clang] Support MSPropertyRefExpr as placement arg to new-expression #75883

Conversation

Sirraide commented Dec 19, 2023

Sirraide commented Dec 19, 2023

llvmbot commented Dec 19, 2023

erichkeane Jan 3, 2024

Choose a reason for hiding this comment

Sirraide Jan 3, 2024

Choose a reason for hiding this comment

erichkeane Jan 3, 2024

Choose a reason for hiding this comment

Sirraide Jan 3, 2024

Choose a reason for hiding this comment

Sirraide Jan 4, 2024 • edited

Choose a reason for hiding this comment

Sirraide Jan 4, 2024

Choose a reason for hiding this comment

Sirraide Jan 4, 2024

Choose a reason for hiding this comment

Sirraide Jan 4, 2024

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

Sirraide commented Jan 4, 2024

Sirraide commented Jan 17, 2024

erichkeane commented Jan 17, 2024

Sirraide commented Jan 17, 2024

erichkeane commented Jan 17, 2024

Sirraide commented Jan 17, 2024

Sirraide commented Jan 17, 2024

erichkeane commented Jan 17, 2024

Sirraide Jan 4, 2024 •

edited