-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[Clang] Support MSPropertyRefExpr as placement arg to new-expression #75883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
849b21f
[Clang] Support MSPropertyRefExpr as placement arg to new-expression
Sirraide f025348
[NFC] Add newline at end of file
Sirraide 9b540da
[Clang] Add tests for placeholder exprs in placement-args of new-expr…
Sirraide 56c8413
[Clang] [NFC] Modernise expected directives
Sirraide 742f4c3
[Clang] [NFC] Reorder expected directives
Sirraide 0ab9622
[Clang] [NFC] Update release notes
Sirraide fd03a66
Merge branch 'main' into main
erichkeane File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| // RUN: %clang_cc1 -triple x86_64-windows-msvc -fms-extensions -std=c++11 -emit-llvm -o - %s | FileCheck %s | ||
|
|
||
| struct S { | ||
| void* operator new(__SIZE_TYPE__, int); | ||
| }; | ||
|
|
||
| int main() { | ||
| // CHECK: call {{.*}} ptr @"??2S@@SAPEAX_KH@Z"(i64 {{.*}} 1, i32 {{.*}} 0) | ||
| // CHECK: call {{.*}} ptr @"??2S@@SAPEAX_KH@Z"(i64 {{.*}} 1, i32 {{.*}} 0) | ||
| new (__noop) S; | ||
| new ((__noop)) S; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| // RUN: %clang_cc1 -x objective-c++ -std=c++11 %s -triple x86_64-apple-darwin10 -emit-llvm -o - | FileCheck %s | ||
|
|
||
| // CHECK: [[NAME:@.*]] = private unnamed_addr constant [9 x i8] c"position\00" | ||
| // CHECK: [[SEL:@.*]] = internal externally_initialized global ptr [[NAME]] | ||
|
|
||
| @interface I { | ||
| int position; | ||
| } | ||
| @property(nonatomic) int position; | ||
| @end | ||
|
|
||
| struct S { | ||
| void *operator new(__SIZE_TYPE__, int); | ||
| }; | ||
|
|
||
| template <typename T> | ||
| struct TS { | ||
| void *operator new(__SIZE_TYPE__, T); | ||
| }; | ||
|
|
||
| I *GetI(); | ||
|
|
||
| int main() { | ||
| @autoreleasepool { | ||
| // CHECK: [[I:%.+]] = alloca ptr | ||
| auto* i = GetI(); | ||
| i.position = 42; | ||
|
|
||
| // This is so we can find the next line more easily. | ||
| // CHECK: store double | ||
| double d = 42.0; | ||
|
|
||
| // CHECK: [[I1:%.+]] = load ptr, ptr [[I]] | ||
| // CHECK-NEXT: [[SEL1:%.+]] = load ptr, ptr [[SEL]] | ||
| // CHECK-NEXT: [[POS1:%.+]] = call {{.*}} i32 @objc_msgSend(ptr {{.*}} [[I1]], ptr {{.*}} [[SEL1]]) | ||
| // CHECK-NEXT: call {{.*}} ptr @_ZN1SnwEmi(i64 {{.*}} 1, i32 {{.*}} [[POS1]]) | ||
| new (i.position) S; | ||
|
|
||
| // CHECK: [[I2:%.+]] = load ptr, ptr [[I]] | ||
| // CHECK-NEXT: [[SEL2:%.+]] = load ptr, ptr [[SEL]] | ||
| // CHECK-NEXT: [[POS2:%.+]] = call {{.*}} i32 @objc_msgSend(ptr {{.*}} [[I2]], ptr {{.*}} [[SEL2]]) | ||
| // CHECK-NEXT: [[DBL:%.+]] = sitofp i32 [[POS2]] to double | ||
| // CHECK-NEXT: call {{.*}} ptr @_ZN2TSIdEnwEmd(i64 {{.*}} 1, double {{.*}} [[DBL]]) | ||
| new (i.position) TS<double>; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This likely needs a good collection of tests to show what it changes for NON-property related 'new' operators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only expressions that this seems to affect is expressions for which
isPlaceholderToRemoveAsArgreturnstrueby passing them toCheckPlaceholderExpr; 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 inBuiltinTypes.defalso mentions ‘VS.NET's__propertydeclarations’, but I couldn’t find anything about__propertyin 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,CheckPlaceholderExprjust 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.fooand friends, wherefoois a non-static member function; these are already invalid in placement-args, andCheckPlaceholderExprsimilarly 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: OnlyDeclRefExprs 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__noopbuiltin, which w/o this pr simply errors, but now, it is implicitly converted to aCallExprthat returns anint(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 asa[1]whereais 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, andBuiltinType::OMPIterator: To my understanding, these cannot occur outside of OMP#pragmas, so they’re not relevant here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr Overall, it seems like this pr also improves some error messages as well as support for MSVC’s
__noopbuiltin. 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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and Objective-C++ properties were suffering from the same problem, so this now also fixes those in new-expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?