Skip to content

Commit

Permalink
[Clang] Support MSPropertyRefExpr as placement arg to new-expression (#…
Browse files Browse the repository at this point in the history
…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 #65053

---------

Co-authored-by: Erich Keane <ekeane@nvidia.com>
  • Loading branch information
Sirraide and erichkeane committed Jan 17, 2024
1 parent 04a69a1 commit af0ee61
Show file tree
Hide file tree
Showing 14 changed files with 375 additions and 42 deletions.
30 changes: 30 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,30 @@ Improvements to Clang's diagnostics
enclosing template must be a definition.
- Clang now diagnoses function/variable templates that shadow their own template parameters, e.g. ``template<class T> void T();``.

- Clang now emits more descriptive diagnostics for 'unusual' expressions (e.g. incomplete index
expressions on matrix types or builtin functions without an argument list) as placement-args
to new-expressions.

Before:

.. code-block:: text
error: no matching function for call to 'operator new'
13 | new (__builtin_memset) S {};
| ^ ~~~~~~~~~~~~~~~~~~
note: candidate function not viable: no known conversion from '<builtin fn type>' to 'int' for 2nd argument
5 | void* operator new(__SIZE_TYPE__, int);
| ^
After:

.. code-block:: text
error: builtin functions must be directly called
13 | new (__builtin_memset) S {};
| ^
Improvements to Clang's time-trace
----------------------------------
Expand Down Expand Up @@ -774,6 +798,12 @@ Bug Fixes in This Version
- Fix assertion failure when call noreturn-attribute function with musttail
attribute.
Fixes (`#76631 <https://github.com/llvm/llvm-project/issues/76631>`_)
- The MS ``__noop`` builtin without an argument list is now accepted
in the placement-args of new-expressions, matching MSVC's behaviour.
- Fix an issue that caused MS ``__decspec(property)`` accesses as well as
Objective-C++ property accesses to not be converted to a function call
to the getter in the placement-args of new-expressions.
Fixes (`#65053 <https://github.com/llvm/llvm-project/issues/65053>`_)

Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/Sema/Sema.h
Original file line number Diff line number Diff line change
Expand Up @@ -2112,6 +2112,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
Expand Down
12 changes: 4 additions & 8 deletions clang/lib/Sema/SemaExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/Sema/SemaExprCXX.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) &&
Expand Down
52 changes: 52 additions & 0 deletions clang/test/CodeGenCXX/ms-property-new.cpp
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
12 changes: 12 additions & 0 deletions clang/test/CodeGenCXX/placement-new-ms-__noop.cpp
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;
}
46 changes: 46 additions & 0 deletions clang/test/CodeGenObjCXX/property-placement-new.mm
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>;
}
}
69 changes: 35 additions & 34 deletions clang/test/SemaCXX/builtin-std-move.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// RUN: %clang_cc1 -std=c++17 -verify %s
// RUN: %clang_cc1 -std=c++17 -verify %s -DNO_CONSTEXPR
// RUN: %clang_cc1 -std=c++20 -verify %s
// RUN: %clang_cc1 -std=c++17 -verify=cxx17,expected %s
// RUN: %clang_cc1 -std=c++17 -verify=cxx17,expected %s -DNO_CONSTEXPR
// RUN: %clang_cc1 -std=c++20 -verify=cxx20,expected %s

namespace std {
#ifndef NO_CONSTEXPR
Expand All @@ -12,6 +12,7 @@ namespace std {
template<typename T> CONSTEXPR T &&move(T &x) {
static_assert(T::moveable, "instantiated move"); // expected-error {{no member named 'moveable' in 'B'}}
// expected-error@-1 {{no member named 'moveable' in 'C'}}
// expected-error@-2 {{no member named 'moveable' in 'D'}}
return static_cast<T&&>(x);
}

Expand All @@ -23,6 +24,7 @@ namespace std {

template<typename T> CONSTEXPR auto move_if_noexcept(T &x) -> typename ref<T, noexcept(T(static_cast<T&&>(x)))>::type {
static_assert(T::moveable, "instantiated move_if_noexcept"); // expected-error {{no member named 'moveable' in 'B'}}
// expected-error@-1 {{no member named 'moveable' in 'D'}}
return static_cast<typename ref<T, noexcept(T(static_cast<T&&>(x)))>::type>(x);
}

Expand All @@ -36,6 +38,7 @@ namespace std {
template<typename T> CONSTEXPR T &&forward(typename remove_reference<T>::type &x) {
static_assert(T::moveable, "instantiated forward"); // expected-error {{no member named 'moveable' in 'B'}}
// expected-error@-1 {{no member named 'moveable' in 'C'}}
// expected-error@-2 {{no member named 'moveable' in 'D'}}
return static_cast<T&&>(x);
}
template<typename T> CONSTEXPR T &&forward(typename remove_reference<T>::type &&x) {
Expand Down Expand Up @@ -67,21 +70,25 @@ namespace std {
CONSTEXPR auto forward_like(T &&t) -> ForwardLikeRetType<U, T> {
using TT = typename remove_reference<T>::type;
static_assert(TT::moveable, "instantiated as_const"); // expected-error {{no member named 'moveable' in 'B'}}
// expected-error@-1 {{no member named 'moveable' in 'D'}}
return static_cast<ForwardLikeRetType<U, T>>(t);
}

template<typename T> CONSTEXPR const T &as_const(T &x) {
static_assert(T::moveable, "instantiated as_const"); // expected-error {{no member named 'moveable' in 'B'}}
// expected-error@-1 {{no member named 'moveable' in 'D'}}
return x;
}

template<typename T> CONSTEXPR T *addressof(T &x) {
static_assert(T::moveable, "instantiated addressof"); // expected-error {{no member named 'moveable' in 'B'}}
// expected-error@-1 {{no member named 'moveable' in 'D'}}
return __builtin_addressof(x);
}

template<typename T> CONSTEXPR T *__addressof(T &x) {
static_assert(T::moveable, "instantiated __addressof"); // expected-error {{no member named 'moveable' in 'B'}}
// expected-error@-1 {{no member named 'moveable' in 'D'}}
return __builtin_addressof(x);
}
}
Expand Down Expand Up @@ -116,42 +123,20 @@ A &forward_rval_as_lval() {
}

struct B {};
B &&(*pMove)(B&) = std::move; // #1 expected-note {{instantiation of}}
B &&(*pMoveIfNoexcept)(B&) = &std::move_if_noexcept; // #2 expected-note {{instantiation of}}
B &&(*pForward)(B&) = &std::forward<B>; // #3 expected-note {{instantiation of}}
B &&(*pForwardLike)(B&) = &std::forward_like<int&&, B&>; // #4 expected-note {{instantiation of}}
const B &(*pAsConst)(B&) = &std::as_const; // #5 expected-note {{instantiation of}}
B *(*pAddressof)(B&) = &std::addressof; // #6 expected-note {{instantiation of}}
B *(*pUnderUnderAddressof)(B&) = &std::__addressof; // #7 expected-note {{instantiation of}}
B &&(*pMove)(B&) = std::move; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
B &&(*pMoveIfNoexcept)(B&) = &std::move_if_noexcept; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
B &&(*pForward)(B&) = &std::forward<B>; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
B &&(*pForwardLike)(B&) = &std::forward_like<int&&, B&>; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
const B &(*pAsConst)(B&) = &std::as_const; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
B *(*pAddressof)(B&) = &std::addressof; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
B *(*pUnderUnderAddressof)(B&) = &std::__addressof; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
int (*pUnrelatedMove)(B, B) = std::move;

struct C {};
C &&(&rMove)(C&) = std::move; // #8 expected-note {{instantiation of}}
C &&(&rForward)(C&) = std::forward<C>; // #9 expected-note {{instantiation of}}
C &&(&rMove)(C&) = std::move; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
C &&(&rForward)(C&) = std::forward<C>; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
int (&rUnrelatedMove)(B, B) = std::move;

#if __cplusplus <= 201703L
// expected-warning@#1 {{non-addressable}}
// expected-warning@#2 {{non-addressable}}
// expected-warning@#3 {{non-addressable}}
// expected-warning@#4 {{non-addressable}}
// expected-warning@#5 {{non-addressable}}
// expected-warning@#6 {{non-addressable}}
// expected-warning@#7 {{non-addressable}}
// expected-warning@#8 {{non-addressable}}
// expected-warning@#9 {{non-addressable}}
#else
// expected-error@#1 {{non-addressable}}
// expected-error@#2 {{non-addressable}}
// expected-error@#3 {{non-addressable}}
// expected-error@#4 {{non-addressable}}
// expected-error@#5 {{non-addressable}}
// expected-error@#6 {{non-addressable}}
// expected-error@#7 {{non-addressable}}
// expected-error@#8 {{non-addressable}}
// expected-error@#9 {{non-addressable}}
#endif

void attribute_const() {
int n;
std::move(n); // expected-warning {{ignoring return value}}
Expand All @@ -163,6 +148,22 @@ void attribute_const() {
std::as_const(n); // expected-warning {{ignoring return value}}
}

struct D {
void* operator new(__SIZE_TYPE__, D&&(*)(D&));
void* operator new(__SIZE_TYPE__, D*(*)(D&));
void* operator new(__SIZE_TYPE__, const D&(*)(D&));
};

void placement_new() {
new (std::move<D>) D; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
new (std::move_if_noexcept<D>) D; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
new (std::forward<D>) D; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
new (std::forward_like<D>) D; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
new (std::addressof<D>) D; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
new (std::__addressof<D>) D; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
new (std::as_const<D>) D; // cxx17-warning {{non-addressable}} cxx20-error {{non-addressable}} expected-note {{instantiation of}}
}

namespace std {
template<typename T> int &move(T);
}
Expand Down

0 comments on commit af0ee61

Please sign in to comment.