subject:"\[PATCH\] D119051\: Extend the C\+\+03 definition of POD to include defaulted functions"

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-27 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D119051#3888667 , @rnk wrote:

> I realized I forgot some things I should've mentioned:
>
> - This probably deserves a release note, if it doesn't have one already that 
> I missed or forgot about

e4ec6ce8a75c208b49b163c81cda90dc7373c791 


> - We should tag #clang-vendors , 
> since this does change ABI. You've already done the work of guarding it with 
> -fclang-abi-compat, so that should be OK, we just need to use the 
> notification mechansim.

Yep yep - thanks @aaron.ballman for adding that.

In D119051#3889416 , @rnk wrote:

> In D119051#3888906 , @erichkeane 
> wrote:
>
>> I noticed in a downstream that this changes how we emit structs to IR 
>> sometimes (see https://godbolt.org/z/74xeq9rTj for an example, but the 
>> 'no-opaque-pointers' isn't necessary to cause this, just anything that 
>> causes emission of the struct).
>>
>> Are we OK with that?  Is the 'padding' arrays important in any way?
>
> The actual layout is different, it now agrees with GCC, see:
> https://godbolt.org/z/EK7Tb6YTT
>
> This is actually a quite significant ABI change, and there are various 
> mechanisms to stay on the old ABI.

Yep, +1 to that. (another dimension to look at, in terms of "is this the right 
LLVM IR" - basically the GCC Itanium interpretation is that defaulted special 
members shouldn't, themselves, change the ABI - so comparing Clang with/without 
the defaulted special member does (now, with the fix applied) have the same IR: 
https://godbolt.org/z/88WKxP6G9 )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-27 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added a comment.

In D119051#3888906 , @erichkeane 
wrote:

> I noticed in a downstream that this changes how we emit structs to IR 
> sometimes (see https://godbolt.org/z/74xeq9rTj for an example, but the 
> 'no-opaque-pointers' isn't necessary to cause this, just anything that causes 
> emission of the struct).
>
> Are we OK with that?  Is the 'padding' arrays important in any way?

The actual layout is different, it now agrees with GCC, see:
https://godbolt.org/z/EK7Tb6YTT

This is actually a quite significant ABI change, and there are various 
mechanisms to stay on the old ABI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-27 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a reviewer: clang-vendors.
aaron.ballman added a comment.

Adding clang-vendors because of the potential for disruption.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-27 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added a comment.

I noticed in a downstream that this changes how we emit structs to IR sometimes 
(see https://godbolt.org/z/74xeq9rTj for an example, but the 
'no-opaque-pointers' isn't necessary to cause this, just anything that causes 
emission of the struct).

Are we OK with that?  Is the 'padding' arrays important in any way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-27 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added a comment.

I realized I forgot some things I should've mentioned:

- This probably deserves a release note, if it doesn't have one already that I 
missed or forgot about
- We should tag #clang-vendors , 
since this does change ABI. You've already done the work of guarding it with 
-fclang-abi-compat, so that should be OK, we just need to use the notification 
mechansim.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-26 Thread David Blaikie via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7846d590033e: Extend the C++03 definition of POD to include 
defaulted functions (authored by dblaikie).

Changed prior to commit:
  https://reviews.llvm.org/D119051?vs=467024=470947#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/test/AST/conditionally-trivial-smfs.cpp
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,10 +1,12 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
 // RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=15
 // RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -656,3 +658,25 @@
 _Static_assert(__builtin_offsetof(C, b) == 3, "");
 }
 
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+  char c;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+_Static_assert(_Alignof(t2) == 1, "");
+struct t3 : t1 {
+  char c;
+};
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
+_Static_assert(sizeof(t3) == 8, "");
+#else
+_Static_assert(sizeof(t3) == 12, "");
+#endif
+}
Index: clang/test/AST/conditionally-trivial-smfs.cpp
===
--- clang/test/AST/conditionally-trivial-smfs.cpp
+++ clang/test/AST/conditionally-trivial-smfs.cpp
@@ -297,6 +297,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -316,6 +317,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -335,6 +337,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "moveAssign": {
 // CHECK-NEXT:"exists": true,
Index: clang/lib/Basic/Targets/OSTargets.h
===
--- clang/lib/Basic/Targets/OSTargets.h
+++ clang/lib/Basic/Targets/OSTargets.h
@@ -161,6 +161,10 @@
: TargetInfo::UnsignedLongLong)
: TargetInfo::getLeastIntTypeByWidth(BitWidth, IsSigned);
   }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+return false;
+  }
 };
 
 // DragonFlyBSD Target
@@ -558,6 +562,10 @@
   checkCallingConvention(CallingConv CC) const override {
 return (CC == CC_C) ? TargetInfo::CCCR_OK : TargetInfo::CCCR_Error;
   }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+return false;
+  }
 };
 
 // PS4 Target
Index: clang/lib/Basic/TargetInfo.cpp
===
---

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-25 Thread Reid Kleckner via Phabricator via cfe-commits

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-24 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping (hoping to ensure enough progress is made on this that we can get it in 
before the LLVM 16 branch - since it's complicated the last couple of releases 
already due to not having all the fallout of the ABI fixes in together)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-19 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-13 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Hmm - pinging this since my last comment didn't seem to send mail (at least not 
that appeared on llvm-commits archive)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-12 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1569
 
+  virtual bool areDefaultedSMFStillPOD(const LangOptions&) const;
+

rnk wrote:
> Please add a doc comment for this, with some history about how previously 
> explicitly defaulting certain special members would make types non-POD, and 
> that changing what is POD can change record layout, so some targets return 
> false to opt for the old, stable behavior.
Done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie updated this revision to Diff 467024.
dblaikie added a comment.

Add doc comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/test/AST/conditionally-trivial-smfs.cpp
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,10 +1,12 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
 // RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=14
 // RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -642,3 +644,33 @@
 _Static_assert(_Alignof(t1) == 1, "");
 _Static_assert(_Alignof(t2) == 1, "");
 } // namespace non_pod_packed
+
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+  char c;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+// 14 and below consider t1 non-pod, but pack it anyway
+// 15 considers it non-pod and doesn't pack it
+// 16 and up considers it pod and packs it again...
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT == 15
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");
+#endif
+struct t3 : t1 {
+  char c;
+};
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
+_Static_assert(sizeof(t3) == 8, "");
+#else
+_Static_assert(sizeof(t3) == 12, "");
+#endif
+}
Index: clang/test/AST/conditionally-trivial-smfs.cpp
===
--- clang/test/AST/conditionally-trivial-smfs.cpp
+++ clang/test/AST/conditionally-trivial-smfs.cpp
@@ -297,6 +297,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -316,6 +317,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -335,6 +337,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "moveAssign": {
 // CHECK-NEXT:"exists": true,
Index: clang/lib/Basic/Targets/OSTargets.h
===
--- clang/lib/Basic/Targets/OSTargets.h
+++ clang/lib/Basic/Targets/OSTargets.h
@@ -161,6 +161,10 @@
: TargetInfo::UnsignedLongLong)
: TargetInfo::getLeastIntTypeByWidth(BitWidth, IsSigned);
   }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+return false;
+  }
 };
 
 // DragonFlyBSD Target
@@ -558,6 +562,10 @@
   checkCallingConvention(CallingConv CC) const override {
 return (CC == CC_C) ? TargetInfo::CCCR_OK : TargetInfo::CCCR_Error;
   }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+return false;
+  }
 };
 
 // PS4 Target
Index: clang/lib/Basic/TargetInfo.cpp

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-10 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added a comment.

+#clang-vendors   for ABI changes




Comment at: clang/include/clang/Basic/TargetInfo.h:1569
 
+  virtual bool areDefaultedSMFStillPOD(const LangOptions&) const;
+

Please add a doc comment for this, with some history about how previously 
explicitly defaulting certain special members would make types non-POD, and 
that changing what is POD can change record layout, so some targets return 
false to opt for the old, stable behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-06 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:774-775
+if ((!Constructor->isDeleted() && !Constructor->isDefaulted()) ||
+(getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver15 || Target.isPS() || 
Target.isOSDarwin())) {
+  // C++ [class]p4:

rnk wrote:
> dblaikie wrote:
> > rnk wrote:
> > > I think this ought to be factored into a TargetInfo method, so we can 
> > > share the logic here and below somehow. Compare this for example with 
> > > `TargetInfo::getCallingConvKind`, which has a similar purpose.
> > Seems plausible - though the version is stored in LangOpts, which isn't 
> > currently plumbed through into TargetInfo - should it be plumbed through, 
> > or is there some layering thing there where targets shouldn't depend on 
> > lang opts?
> > 
> > Looks like it'd mostly involve passing LangOpts down here: 
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInstance.cpp#L107
> >  and plumbing it through all the `TargetInfo` ctors, possibly either 
> > storing `LangOpts&` in the `TargetInfo`, or computing the 
> > `DefaultedSMFArePOD` property in the ctor and storing that as a `bool` 
> > member in `TargetInfo` to return from some query function to be added to 
> > that hierarchy.
> > 
> > Or I guess like `getOSDefines` have `getDefaultedSMFArePOD` takes 
> > `LangOptions` as a parameter?
> My main concern is keeping complex target-specific conditions out of DeclCXX 
> to improve readability. Any way to achieve that sounds good to me.
> 
> I think I'd lean towards passing LangOpts as a parameter. After that, storing 
> `DefaultedSMFArePOD` on TargetInfo as a bool sounds good. You can set the 
> field in `TargetInfo::adjust` to read LangOpts, and then override that in PS 
> & Darwin specific ::adjust method overrides.
Fair enough - tried this out with a LongOpts parameter (I guess 
`getCallingConvKind` passes in just the boolean about whether it's the right 
ABI compatibility version - requiring the caller to check, but that seems a bit 
weird/specific? So I prefer passing in the whole LangOpts here and querying the 
right property inside TargetInfo)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-06 Thread David Blaikie via Phabricator via cfe-commits

dblaikie updated this revision to Diff 465956.
dblaikie added a comment.

Move condition to TargetInfo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/OSTargets.h
  clang/test/AST/conditionally-trivial-smfs.cpp
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,10 +1,12 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
 // RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=14
 // RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -642,3 +644,33 @@
 _Static_assert(_Alignof(t1) == 1, "");
 _Static_assert(_Alignof(t2) == 1, "");
 } // namespace non_pod_packed
+
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+  char c;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+// 14 and below consider t1 non-pod, but pack it anyway
+// 15 considers it non-pod and doesn't pack it
+// 16 and up considers it pod and packs it again...
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT == 15
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");
+#endif
+struct t3 : t1 {
+  char c;
+};
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
+_Static_assert(sizeof(t3) == 8, "");
+#else
+_Static_assert(sizeof(t3) == 12, "");
+#endif
+}
Index: clang/test/AST/conditionally-trivial-smfs.cpp
===
--- clang/test/AST/conditionally-trivial-smfs.cpp
+++ clang/test/AST/conditionally-trivial-smfs.cpp
@@ -297,6 +297,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -316,6 +317,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -335,6 +337,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "moveAssign": {
 // CHECK-NEXT:"exists": true,
Index: clang/lib/Basic/Targets/OSTargets.h
===
--- clang/lib/Basic/Targets/OSTargets.h
+++ clang/lib/Basic/Targets/OSTargets.h
@@ -161,6 +161,10 @@
: TargetInfo::UnsignedLongLong)
: TargetInfo::getLeastIntTypeByWidth(BitWidth, IsSigned);
   }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+return false;
+  }
 };
 
 // DragonFlyBSD Target
@@ -558,6 +562,10 @@
   checkCallingConvention(CallingConv CC) const override {
 return (CC == CC_C) ? TargetInfo::CCCR_OK : TargetInfo::CCCR_Error;
   }
+
+  bool areDefaultedSMFStillPOD(const LangOptions &) const override {
+return false;
+  }
 };
 
 // PS4 Target
Index:

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-05 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:774-775
+if ((!Constructor->isDeleted() && !Constructor->isDefaulted()) ||
+(getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver15 || Target.isPS() || 
Target.isOSDarwin())) {
+  // C++ [class]p4:

dblaikie wrote:
> rnk wrote:
> > I think this ought to be factored into a TargetInfo method, so we can share 
> > the logic here and below somehow. Compare this for example with 
> > `TargetInfo::getCallingConvKind`, which has a similar purpose.
> Seems plausible - though the version is stored in LangOpts, which isn't 
> currently plumbed through into TargetInfo - should it be plumbed through, or 
> is there some layering thing there where targets shouldn't depend on lang 
> opts?
> 
> Looks like it'd mostly involve passing LangOpts down here: 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInstance.cpp#L107
>  and plumbing it through all the `TargetInfo` ctors, possibly either storing 
> `LangOpts&` in the `TargetInfo`, or computing the `DefaultedSMFArePOD` 
> property in the ctor and storing that as a `bool` member in `TargetInfo` to 
> return from some query function to be added to that hierarchy.
> 
> Or I guess like `getOSDefines` have `getDefaultedSMFArePOD` takes 
> `LangOptions` as a parameter?
My main concern is keeping complex target-specific conditions out of DeclCXX to 
improve readability. Any way to achieve that sounds good to me.

I think I'd lean towards passing LangOpts as a parameter. After that, storing 
`DefaultedSMFArePOD` on TargetInfo as a bool sounds good. You can set the field 
in `TargetInfo::adjust` to read LangOpts, and then override that in PS & Darwin 
specific ::adjust method overrides.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-05 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D119051#3834985 , @rnk wrote:

> Relatedly, if we put this POD behavior query on TargetInfo, I wonder if it 
> makes sense to move the tail padding predicate from TargetCXXABI to 
> TargetInfo:
> https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/TargetCXXABI.h#L282
>
> The switch there is basically a switch over architectures.

Hmm, looked into this - but isn't most of `TargetCXXABI` a switch over CXXABIs, 
which are almost architectures/operating systems? Why is tail padding predicate 
different than the other tests in TargetCXXABI?

But I took a look at changing this in D135326 
 in case you want to check if this is what 
you had in mind - I'm not entirely sure it's better. The OS seems not be quite 
1:1 with the CXXABI groupings we have in TargetCXXABI... so one end result 
could just be calling back into the ABI to get the CXXABI kind and then 
returning based on that, which doesn't seem to be better...

Comment at: clang/lib/AST/DeclCXX.cpp:774-775
+if ((!Constructor->isDeleted() && !Constructor->isDefaulted()) ||
+(getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver15 || Target.isPS() || 
Target.isOSDarwin())) {
+  // C++ [class]p4:

rnk wrote:
> I think this ought to be factored into a TargetInfo method, so we can share 
> the logic here and below somehow. Compare this for example with 
> `TargetInfo::getCallingConvKind`, which has a similar purpose.
Seems plausible - though the version is stored in LangOpts, which isn't 
currently plumbed through into TargetInfo - should it be plumbed through, or is 
there some layering thing there where targets shouldn't depend on lang opts?

Looks like it'd mostly involve passing LangOpts down here: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/CompilerInstance.cpp#L107
 and plumbing it through all the `TargetInfo` ctors, possibly either storing 
`LangOpts&` in the `TargetInfo`, or computing the `DefaultedSMFArePOD` property 
in the ctor and storing that as a `bool` member in `TargetInfo` to return from 
some query function to be added to that hierarchy.

Or I guess like `getOSDefines` have `getDefaultedSMFArePOD` takes `LangOptions` 
as a parameter?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-04 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added a comment.

Relatedly, if we put this POD behavior query on TargetInfo, I wonder if it 
makes sense to move the tail padding predicate from TargetCXXABI to TargetInfo:
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/TargetCXXABI.h#L282

The switch there is basically a switch over architectures.




Comment at: clang/lib/AST/DeclCXX.cpp:774-775
+if ((!Constructor->isDeleted() && !Constructor->isDefaulted()) ||
+(getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver15 || Target.isPS() || 
Target.isOSDarwin())) {
+  // C++ [class]p4:

I think this ought to be factored into a TargetInfo method, so we can share the 
logic here and below somehow. Compare this for example with 
`TargetInfo::getCallingConvKind`, which has a similar purpose.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-04 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D119051#3747673 , @rnk wrote:

> In D119051#3747201 , @dblaikie 
> wrote:
>
>> So... my conclusion is that Clang's AArch64 appears to be correct for x86  
>> as well, and we should just rename the function and use it unconditionally, 
>> removing any use of Clang's AST POD property in the MSVC ABI handling?
>
> Sounds good to me, I think you did the hard work of verifying, thanks for 
> that. :)
>
> Anyway, removing this isPOD usage should be it's own patch, with tests. We 
> should already have test coverage for aarch64 that can be reused. @ayzhao, 
> can you help David with this? This is not C++20-related, but it is clang 
> frontend related.

Popping the stack, the use of AST's isPOD in Microsoft's ABI has been removed 
in favor of more nuanced custom implementation in the Microsoft ABI handling 
that fixed a few bugs along the way (D133817 
, D134688 ).

I've rebased this patch and addressed a test failure I hadn't spotted before, 
in `clang/test/AST/conditionally-trivial-smfs.cpp` - added some details to the 
patch description.
And now that the Microsoft ABI doesn't depend on the AST isPOD property at all, 
I've removed the previously proposed new test 
`clang/test/CodeGenCXX/return-abi.cpp` that was testing the Microsoft return 
ABI.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-04 Thread David Blaikie via Phabricator via cfe-commits

dblaikie updated this revision to Diff 465159.
dblaikie added a comment.

Remove Microsoft return ABI test, now that the Microsoft ABI implementation no 
longer depends on the AST isPOD property


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/DeclCXX.cpp
  clang/test/AST/conditionally-trivial-smfs.cpp
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,10 +1,12 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
 // RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=14
 // RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -642,3 +644,33 @@
 _Static_assert(_Alignof(t1) == 1, "");
 _Static_assert(_Alignof(t2) == 1, "");
 } // namespace non_pod_packed
+
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+  char c;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+// 14 and below consider t1 non-pod, but pack it anyway
+// 15 considers it non-pod and doesn't pack it
+// 16 and up considers it pod and packs it again...
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT == 15
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");
+#endif
+struct t3 : t1 {
+  char c;
+};
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
+_Static_assert(sizeof(t3) == 8, "");
+#else
+_Static_assert(sizeof(t3) == 12, "");
+#endif
+}
Index: clang/test/AST/conditionally-trivial-smfs.cpp
===
--- clang/test/AST/conditionally-trivial-smfs.cpp
+++ clang/test/AST/conditionally-trivial-smfs.cpp
@@ -297,6 +297,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -316,6 +317,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -335,6 +337,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "moveAssign": {
 // CHECK-NEXT:"exists": true,
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -36,6 +36,7 @@
 #include "clang/Basic/PartialDiagnostic.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/Specifiers.h"
+#include "clang/Basic/TargetInfo.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
@@ -768,12 +769,17 @@
 // Note that we have a user-declared constructor.
 data().UserDeclaredConstructor = true;
 
-// C++ [class]p4:
-//   A POD-struct is an aggregate class [...]
-// Since the POD bit is meant to be C++03 POD-ness, clear it even if
-// the type is technically an aggregate in C++0x since it

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-10-04 Thread David Blaikie via Phabricator via cfe-commits

dblaikie updated this revision to Diff 465158.
dblaikie added a comment.

rebase and fix an AST test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/DeclCXX.cpp
  clang/test/AST/conditionally-trivial-smfs.cpp
  clang/test/CodeGenCXX/return-abi.cpp
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,10 +1,12 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
 // RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=14
 // RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -642,3 +644,33 @@
 _Static_assert(_Alignof(t1) == 1, "");
 _Static_assert(_Alignof(t2) == 1, "");
 } // namespace non_pod_packed
+
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+  char c;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+// 14 and below consider t1 non-pod, but pack it anyway
+// 15 considers it non-pod and doesn't pack it
+// 16 and up considers it pod and packs it again...
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT == 15
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");
+#endif
+struct t3 : t1 {
+  char c;
+};
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
+_Static_assert(sizeof(t3) == 8, "");
+#else
+_Static_assert(sizeof(t3) == 12, "");
+#endif
+}
Index: clang/test/CodeGenCXX/return-abi.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/return-abi.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-pc-windows-msvc -Wno-return-type %s | FileCheck %s
+namespace nonpod {
+struct base {};
+struct t1 : base {
+int a;
+};
+t1 f1() {}
+// CHECK: define {{.*}}void {{.*}}nonpod{{.*}}(ptr
+}  // namespace nonpod
+namespace smf_defaulted_pod {
+struct t1 {
+t1() = default;
+int a;
+};
+t1 f1() {}
+// CHECK: define {{.*}}i32 {{.*}}smf_defaulted_pod{{.*}}()
+}  // namespace smf_defaulted_pod
+namespace pod {
+struct t1 {
+int a;
+};
+t1 f1() {}
+// CHECK: define {{.*}}i32 {{.*}}pod{{.*}}()
+}  // namespace pod
Index: clang/test/AST/conditionally-trivial-smfs.cpp
===
--- clang/test/AST/conditionally-trivial-smfs.cpp
+++ clang/test/AST/conditionally-trivial-smfs.cpp
@@ -297,6 +297,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -316,6 +317,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "isTrivial": true,
 // CHECK-NEXT:  "isTriviallyCopyable": true,
@@ -335,6 +337,7 @@
 // CHECK-NEXT:  "isAggregate": true,
 // CHECK-NEXT:  "isEmpty": true,
 // CHECK-NEXT:  "isLiteral": true,
+// CHECK-NEXT:  "isPOD": true,
 // CHECK-NEXT:  "isStandardLayout": true,
 // CHECK-NEXT:  "moveAssign": {
 // CHECK-NEXT:"exists": true,
Index: clang/lib/AST/DeclCXX.cpp

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-24 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added a subscriber: ayzhao.
rnk added a comment.

In D119051#3747201 , @dblaikie wrote:

> So... my conclusion is that Clang's AArch64 appears to be correct for x86  as 
> well, and we should just rename the function and use it unconditionally, 
> removing any use of Clang's AST POD property in the MSVC ABI handling?

Sounds good to me, I think you did the hard work of verifying, thanks for that. 
:)

Anyway, removing this isPOD usage should be it's own patch, with tests. We 
should already have test coverage for aarch64 that can be reused. @ayzhao, can 
you help David with this? This is not C++20-related, but it is clang frontend 
related.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-24 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Each godbolt link shows  MSVC 19.latest and clang, both in x86 and aarch64, 
with a reference pod and non-pod example at the start and end, and the variable 
test in the middle to see whether the codegen matches the pod or non-pod 
baselines above and below.

> 1. checked (second): has base classes

https://godbolt.org/z/d76fEGP1M - all agreed

> 2. checked (third): virtual functions/polymorphic

Oh, I guess if you can't have base classes then testing 'is polymorphic' is 
equivalent to 'has virtual functions'

This one's slightly less obvious to verify due to the existence of a virtual 
function changing the codegen of the `f1` test functions so comparing it to the 
pod/non-pod reference  & maybe me think about how to better isolate the test - 
though I don't think it's possible to totally isolate it, adding a virtual 
functions adds a vptr that has to be copied ,etc and extra globals for the 
vtalbe itself (MSVC doesn't home vtables), etc.

So I've got this: https://godbolt.org/z/nGa4drG4h - but it looks like they all 
agree, despite the extra variations in the output

> 3. only user-provided ctors, so it can have non-implicit (user-declared) but 
> defaulted ctors

User-defined ctor (`t1(int)` unrelated to any usage/copy operation, etc): 
https://godbolt.org/z/e79b1zeqr - everyone agrees
User-declared-but-not-user-defined ctor (`t1() = default`): Clang x86 is 
incorrect, believes it to be non-trivial - that's the original issue under 
discussion in this bug

> 4. specifically checking non-trivial copy assignment (no restriction on move 
> assignment and it could have any number of other member functions, I think?)

Oh, everyone allows a non-trivial non-special member function, I misread the 
Clang code - those don't affect retro-POD either, so everyone agrees on that. 
https://godbolt.org/z/hnMfqjov7

Restricting this to Special Member Functions, defaulted copy constructor... 
https://godbolt.org/z/sGvxcEPjo Clang x86 is incorrect

> 5. checked (first)

Used this as the baseline - everyone agrees.

> 6. nope/not relevant?



-

> 7. Presumably tested by "hasNonTrivialDestructor" and 
> "hasNonTrivialCopyAssignment"?

Having a field of a type with a protected member (property 5) - Clang x86 is 
incorrect: https://godbolt.org/z/GY48qxh3G
Having a field of a type with a non-trivial dtor - everyone agrees: 
https://godbolt.org/z/4dq9b43ac
Having a field of a type with a non-trivial copy-assignment - everyone agrees: 
https://godbolt.org/z/WEnKcz7oW

> 8. would a non-static data member initializer make the ctor user-provided? 
> Presumably not, so I don't think this property is checked at all, which seems 
> fair - how you pass something shouldn't be changed by how its constructed

Non-static data member initializer: https://godbolt.org/z/Mb1PYhjrP - Clang x86 
is incorrect

So... my conclusion is that Clang's AArch64 appears to be correct for x86  as 
well, and we should just rename the function and use it unconditionally, 
removing any use of Clang's AST POD property in the MSVC ABI handling?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-24 Thread David Blaikie via Phabricator via cfe-commits

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

In D119051#3724121 , @rnk wrote:

> Regarding the usage of isPOD in the MSVC ABI code, we should have a routine 
> similar to `isTrivialForAArch64MSVC` that works for the other MSVC 
> architectures that we support (x86, x64, and arm32). I don't think we should 
> try to rely on definition data bits. Clang is almost always tracking 
> something slightly different than what MSVC tracks, so we should write down 
> MSVC's logic once and keep it separate from Clang's notion of POD.
>
> I think you can work out MSVC's behavior with godbolt, but if you need a 
> hand, I volunteer @hans to take a stab at refactoring 
> `MicrosoftCXXABI::classifyReturnType`. I remember making lots of comments to 
> try to simplify the code during review, but everyone is always wary of scope 
> creep.

Well, my first attempt at this - essentially using `isTrivialForAArch64MSVC` 
for everything, including x86, doesn't fail any Clang tests...

Do we still have those various fuzzers Majnemer setup? I'd guess we'd need to 
fuzz around here again to discover what the intended behavior is?

Hmm, let's compare the things that make `PlainOldData` `false`...

1. has base classes
2. any virtual member function
3. non-implicit ctor (so even user-declared-but-defaulted ctor)
4. non-implicit member function (so even user-declared-but-defaulted assignment 
operators or dtor)
5. any non-public field member
6. some ObjC thing (non-trivial ObjC lifetime)
7. any field that is of C++98 POD type... (mostly a recursive definition, when 
it comes to record types - but then there's some rules about incomplete array 
types, which couldn't be field types anyway, and some other cases)
8. non-static data member initializers

Let's compare that with `isTrivialForAArch64MSVC`...

1. checked (second)
2. checked (third) - well, it checks "is polymorphic" rather than virtual 
functions, so does that change things if you've got virtual bases but no 
virtual functions, can you have that?
3. only user-provided ctors, so it can have non-implicit (user-declared) but 
defaulted ctors
4. specifically checking non-trivial copy assignment (no restriction on move 
assignment and it could have any number of other member functions, I think?)
5. checked (first)
6. nope/not relevant?
7. Presumably tested by "hasNonTrivialDestructor" and 
"hasNonTrivialCopyAssignment"?
8. would a non-static data member initializer make the ctor user-provided? 
Presumably not, so I don't think this property is checked at all, which seems 
fair - how you pass something shouldn't be changed by how its constructed

I'll test the cases where these things differ with the godbolt-like testing 
earlier...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-15 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added a subscriber: hans.
rnk added a comment.

Regarding the usage of isPOD in the MSVC ABI code, we should have a routine 
similar to `isTrivialForAArch64MSVC` that works for the other MSVC 
architectures that we support (x86, x64, and arm32). I don't think we should 
try to rely on definition data bits. Clang is almost always tracking something 
slightly different than what MSVC tracks, so we should write down MSVC's logic 
once and keep it separate from Clang's notion of POD.

I think you can work out MSVC's behavior with godbolt, but if you need a hand, 
I volunteer @hans to take a stab at refactoring 
`MicrosoftCXXABI::classifyReturnType`. I remember making lots of comments to 
try to simplify the code during review, but everyone is always wary of scope 
creep.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-15 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Do we need to gate this on use of `-fms-compatibility` as well? I'm not certain 
how cygwin factors in where it's sort of gcc and sort of msvc (perhaps the 
triple is sufficient for that?).




Comment at: clang/lib/AST/DeclCXX.cpp:892
+if ((!Method->isDeleted() && !Method->isDefaulted() &&
+ SMKind != SMF_MoveAssignment) ||
+(getLangOpts().getClangABICompat() <=

Ah, it took me a moment to realize that move assignment is called out here but 
not move construction because move construction is handled above.



Comment at: clang/test/SemaCXX/class-layout.cpp:663-665
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");

dblaikie wrote:
> aaron.ballman wrote:
> > 
> This doesn't compile under the first `RUN` line which uses C++98, I think?
Ah, fair point. I don't have a strong opinion, but you could do 
`-Dstatic_assert=_Static_assert` for that RUN line and not use the extension 
version. But it doesn't matter for the test functionality, so whichever 
approach sparks the most joy for you is fine by me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-15 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:892
+ LangOptions::ClangABI::Ver15)) {
+  // C++03 [class]p4:
+  //   A POD-struct is an aggregate class that has [...] no 
user-defined

dblaikie wrote:
> erichkeane wrote:
> > Does this language change much if there are constraints on this method?  We 
> > might need to consider that if we are breaking ABI here.
> Oh, sorry, I should've used more words - but this is intended as an ABI 
> break/fix. It's a place where Clang's ABI implementation is inconsistent with 
> GCC and MSVC - so this is intended to fix/make Clang compatible with those 
> ABIs where we are trying to be compatible with them. (& why we're checking 
> the ClangABI version here - so if you're asking for the old ABI you still get 
> it)
> 
> Perhaps I'm misunderstanding your questions/concerns?
Yeah, my concern is whether this behavior is different with a constraint on it. 
 That is, does the eligibility of the constructor matter for this case?  

This ctor/copy assign/etc might not pass a constraint check/never be callable.

So the question is whether we need to consider that here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-12 Thread David Blaikie via Phabricator via cfe-commits

dblaikie marked an inline comment as done and an inline comment as not done.
dblaikie added inline comments.



Comment at: clang/test/SemaCXX/class-layout.cpp:2-9
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=6 
-DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=14 
-DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=15 
-DCLANG_ABI_COMPAT=15

aaron.ballman wrote:
> No need for `-Wno-c++11-extensions` on these RUN lines, right (they already 
> specify c++11 specifically, so there are no extensions to warn about)?
ah, yeah - just need it on the first one


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-12 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a subscriber: hansw.
dblaikie added a comment.

In D119051#3717934 , @dblaikie wrote:

> I guess the other way to test for pod-for-purposes-of-ABI is IRgen. Looks 
> like MSVC isn't observable based on sizeof or alignof on these issues (the 
> previous godbolt shows MSVC's answer to alignment for the packed and size for 
> the trailing packing don't change based on any of the variations (pod, 
> non-pod, pod-with-defaulted-special-members)) - but should be observable 
> based on the returning ABI.
>
> Ah, here we go: https://godbolt.org/z/sd88zTjPP

Minor correction - this test was incorrect & I think demonstrates more/multiple 
MS ABI bugs. Looks like MS's ABI considers a NSDMI to still be 
pod-for-the-purposes-of-returning. So that's another bug (@hansw / @rnk - any 
interest in that? Want a bug or it or the like - here's a godbolt that shows it 
more clearly, using a base class instead, which MS does consider 
non-pod-for-purposes-of-returning: https://godbolt.org/z/aMhbe11a8 )

So using the base class instead (something both clang and MSVC agree on), 
here's a good demo of the defaulted SMF issue: https://godbolt.org/z/K31vGsejh

> So MSVC does consider the defaulted special member as still a valid 
> pod-for-purposes-of-ABI and clang is incorrect/not ABI compatible with this. 
> So MSVC does want this fix.

Adding a test (`clang/test/CodeGenCXX/return-abi.cpp`) here to test this. Seems 
that the Itanium ABI doesn't care about pod-ness for return (or parameter) ABI, 
probably only cares about "trivially copyable" ( 
https://godbolt.org/z/7n91YqecM ) - so doesn't seem like I can test the Itanium 
preferences for pod-ness in this codegen test. It seems to only show up in 
layout.

The quirk of the `SemaCXX/class-layout.cpp` that it's currently not testing the 
behavior at 14 and below (because of the packing-non-pod ABI bug that was fixed 
previously/recently/lead to this work) - I could change the test to use tail 
padding instead as a way to observe the pod-ness a bit more robustly? What do 
you think? I added the padding based test as well for comparison - could keep 
one, or the other, or both.

Comment at: clang/lib/AST/DeclCXX.cpp:892
+ LangOptions::ClangABI::Ver15)) {
+  // C++03 [class]p4:
+  //   A POD-struct is an aggregate class that has [...] no 
user-defined

erichkeane wrote:
> Does this language change much if there are constraints on this method?  We 
> might need to consider that if we are breaking ABI here.
Oh, sorry, I should've used more words - but this is intended as an ABI 
break/fix. It's a place where Clang's ABI implementation is inconsistent with 
GCC and MSVC - so this is intended to fix/make Clang compatible with those ABIs 
where we are trying to be compatible with them. (& why we're checking the 
ClangABI version here - so if you're asking for the old ABI you still get it)

Perhaps I'm misunderstanding your questions/concerns?

Comment at: clang/test/SemaCXX/class-layout.cpp:663-665
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");

aaron.ballman wrote:
> 
This doesn't compile under the first `RUN` line which uses C++98, I think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-12 Thread David Blaikie via Phabricator via cfe-commits

dblaikie updated this revision to Diff 452315.
dblaikie added a comment.

Add PS4/Darwin handling
Remove unnecessary warning suppression
Add MSVC return ABI testing
Add tail-padding based testing as well as the alignment based testing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/return-abi.cpp
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,10 +1,12 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
 // RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=14
 // RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -642,3 +644,33 @@
 _Static_assert(_Alignof(t1) == 1, "");
 _Static_assert(_Alignof(t2) == 1, "");
 } // namespace non_pod_packed
+
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+  char c;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+// 14 and below consider t1 non-pod, but pack it anyway
+// 15 considers it non-pod and doesn't pack it
+// 16 and up considers it pod and packs it again...
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT == 15
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");
+#endif
+struct t3 : t1 {
+  char c;
+};
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 15
+_Static_assert(sizeof(t3) == 8, "");
+#else
+_Static_assert(sizeof(t3) == 12, "");
+#endif
+}
Index: clang/test/CodeGenCXX/return-abi.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/return-abi.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -emit-llvm -o - -triple i686-pc-windows-msvc -Wno-return-type %s | FileCheck %s
+namespace nonpod {
+struct base {};
+struct t1 : base {
+int a;
+};
+t1 f1() {}
+// CHECK: define {{.*}}void {{.*}}nonpod{{.*}}(ptr
+}  // namespace nonpod
+namespace smf_defaulted_pod {
+struct t1 {
+t1() = default;
+int a;
+};
+t1 f1() {}
+// CHECK: define {{.*}}i32 {{.*}}smf_defaulted_pod{{.*}}()
+}  // namespace smf_defaulted_pod
+namespace pod {
+struct t1 {
+int a;
+};
+t1 f1() {}
+// CHECK: define {{.*}}i32 {{.*}}pod{{.*}}()
+}  // namespace pod
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3530,6 +3530,8 @@
 GenerateArg(Args, OPT_fclang_abi_compat_EQ, "12.0", SA);
   else if (Opts.getClangABICompat() == LangOptions::ClangABI::Ver14)
 GenerateArg(Args, OPT_fclang_abi_compat_EQ, "14.0", SA);
+  else if (Opts.getClangABICompat() == LangOptions::ClangABI::Ver15)
+GenerateArg(Args, OPT_fclang_abi_compat_EQ, "15.0", SA);
 
   if (Opts.getSignReturnAddressScope() ==
   LangOptions::SignReturnAddressScopeKind::All)
@@ -4022,6 +4024,8 @@
 Opts.setClangABICompat(LangOptions::ClangABI::Ver12);
   else if (Major <= 14)
 Opts.setClangABICompat(LangOptions::ClangABI::Ver14);
+  else if (Major <= 15)
+Opts.setClangABICompat(LangOptions::ClangABI::Ver15);
 } else if (Ver != "latest") {
   Diags.Report(diag::err_drv_invalid_value)
   << A->getAsString(Args) << A->getValue();
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-12 Thread Erich Keane via Phabricator via cfe-commits

erichkeane added inline comments.



Comment at: clang/lib/AST/DeclCXX.cpp:892
+ LangOptions::ClangABI::Ver15)) {
+  // C++03 [class]p4:
+  //   A POD-struct is an aggregate class that has [...] no 
user-defined

Does this language change much if there are constraints on this method?  We 
might need to consider that if we are breaking ABI here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-12 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

> In D119051#3715939 , @aaron.ballman 
> wrote:
>
>> In D119051#3714645 , @dblaikie 
>> wrote:
>>
>>> 
>>
>> I would have thought use of `__is_pod` would tell us, but I'm not seeing the 
>> behavior described in the test case when using that: 
>> https://godbolt.org/z/1vr3MK4KW Oddly, it seems that 
>> `QualType::isCXX11PODType()` doesn't look at `PlainOldData` at all! What is 
>> your expectation as to how the type trait should be behaving?
>
> Oh, yeah, seems @rsmith and I discussed this naming/expectations issue a bit 
> over here previously: https://reviews.llvm.org/D117616#inline-1132622

Ah, thank you for that!

In D119051#3717934 , @dblaikie wrote:

> I guess the other way to test for pod-for-purposes-of-ABI is IRgen. Looks 
> like MSVC isn't observable based on sizeof or alignof on these issues (the 
> previous godbolt shows MSVC's answer to alignment for the packed and size for 
> the trailing packing don't change based on any of the variations (pod, 
> non-pod, pod-with-defaulted-special-members)) - but should be observable 
> based on the returning ABI.
>
> Ah, here we go: https://godbolt.org/z/sd88zTjPP
>
> So MSVC does consider the defaulted special member as still a valid 
> pod-for-purposes-of-ABI and clang is incorrect/not ABI compatible with this. 
> So MSVC does want this fix.

Yeah this strikes me as the right kind of test to add for testing the ABI 
(codegen cares more about ABI than Sema, broadly speaking).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

I guess the other way to test for pod-for-purposes-of-ABI is IRgen. Looks like 
MSVC isn't observable based on sizeof or alignof on these issues (the previous 
godbolt shows MSVC's answer to alignment for the packed and size for the 
trailing packing don't change based on any of the variations (pod, non-pod, 
pod-with-defaulted-special-members)) - but should be observable based on the 
returning ABI.

Ah, here we go: https://godbolt.org/z/sd88zTjPP

So MSVC does consider the defaulted special member as still a valid 
pod-for-purposes-of-ABI and clang is incorrect/not ABI compatible with this. So 
MSVC does want this fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

> So I could test this other ways that actually impact layout - like whether 
> things can be packed in tail padding (can pack in tail padding for non-pod, 
> right?). Or we could ast dump and inspect the property directly? Maybe there 
> are some other options?

Here's what the tail-padding based testing looks like: 
https://godbolt.org/z/Te3d4fYjj (though the asserts all fail on MSVC... not 
sure what size MSVC makes these structs at all)

  namespace trailing {
  struct t1 {
  int x;
  char c;
  };
  struct t2 : t1 {
  char c;  
  };
  static_assert(sizeof(t2) == sizeof(int) * 3, "");
  } // namespace trailing
  namespace trailing_nonpod {
  struct t1 {
  protected:
  int x;
  char c;
  };
  struct t2 : t1  {
  char c;
  };
  static_assert(sizeof(t2) == sizeof(int) * 2, "");
  }  // namespace trailing_nonpod
  namespace trailing_smf_defaulted_pod {
  struct t1 {
  t1() = default;
  int a;
  char c;
  };
  struct t2 : t1 {
  char c;
  };
  static_assert(sizeof(t2) == sizeof(int) * 3, "");
  }

(GCC passes all these assertions, Clang (without this patch we're reviewing) 
fails the last of them)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

I guess the other packed behavior/ABI checking 
277123376ce08c98b07c154bf83e4092a5d4d3c6 


In D119051#3715939 , @aaron.ballman 
wrote:

> In D119051#3714645 , @dblaikie 
> wrote:
>
>> Realized maybe we don't need a separate driver flag for this at all, and 
>> rely only on the abi-compat flag? That seems to be how (at least some) other 
>> ABI compat changes have been handled, looking at other uses of `ClangABI` 
>> enum values.
>
> Agreed, I think this is the better approach.
>
>> There could be more testing than only the indirect result of the packing 
>> problem that first inspired this patch. Any suggestions on what might be the 
>> most direct way to test whether the type's been considered pod in this sense?
>
> I would have thought use of `__is_pod` would tell us, but I'm not seeing the 
> behavior described in the test case when using that: 
> https://godbolt.org/z/1vr3MK4KW Oddly, it seems that 
> `QualType::isCXX11PODType()` doesn't look at `PlainOldData` at all! What is 
> your expectation as to how the type trait should be behaving?

Oh, yeah, seems @rsmith and I discussed this naming/expectations issue a bit 
over here previously: https://reviews.llvm.org/D117616#inline-1132622

So I could test this other ways that actually impact layout - like whether 
things can be packed in tail padding (can pack in tail padding for non-pod, 
right?). Or we could ast dump and inspect the property directly? Maybe there 
are some other options?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.



>> There could be more testing than only the indirect result of the packing 
>> problem that first inspired this patch. Any suggestions on what might be the 
>> most direct way to test whether the type's been considered pod in this sense?
>
> I would have thought use of `__is_pod` would tell us, but I'm not seeing the 
> behavior described in the test case when using that: 
> https://godbolt.org/z/1vr3MK4KW Oddly, it seems that 
> `QualType::isCXX11PODType()` doesn't look at `PlainOldData` at all! What is 
> your expectation as to how the type trait should be behaving?

Ah, yeah, my understanding is the POD we're talking about is basically "POD for 
the purposes of Itanium layout" - independent of various language definitions 
of POD.

Which got me searching around in the code for uses of the data - it's used in 
some warnings, but also in the MSVC layout ( 
https://github.com/llvm/llvm-project/blob/c8cf669f6025bdf0fc227f3ddbbf3523a5b32f0b/clang/lib/CodeGen/MicrosoftCXXABI.cpp#L1123
 and it looks like MSVC proper uses the non-GCC-compatible choice here, so this 
patch shouldn't apply to MSVC compatibility mode... 
https://godbolt.org/z/Mvroof8n4 ).

I realized when removing the explicit flag I lost some of the overrides - for 
PS and Darwin. Since we're checking this property in two places and now it'll 
involve about 4 different checks (or should all these platforms imply the older 
ABI? At least that seems correct for PS and Darwin where their ABIs are defined 
by the older version of clang, but the MSVC case is weirder - since their ABI 
isn't defined by the older version of clang, and the ABI may need to change as 
we discover mismatches/mistakes in Clang's ABI implementation)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-11 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D119051#3714645 , @dblaikie wrote:

> Realized maybe we don't need a separate driver flag for this at all, and rely 
> only on the abi-compat flag? That seems to be how (at least some) other ABI 
> compat changes have been handled, looking at other uses of `ClangABI` enum 
> values.

Agreed, I think this is the better approach.

> There could be more testing than only the indirect result of the packing 
> problem that first inspired this patch. Any suggestions on what might be the 
> most direct way to test whether the type's been considered pod in this sense?

I would have thought use of `__is_pod` would tell us, but I'm not seeing the 
behavior described in the test case when using that: 
https://godbolt.org/z/1vr3MK4KW Oddly, it seems that 
`QualType::isCXX11PODType()` doesn't look at `PlainOldData` at all! What is 
your expectation as to how the type trait should be behaving?




Comment at: clang/test/SemaCXX/class-layout.cpp:2-9
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=6 
-DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=14 
-DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify 
-std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=15 
-DCLANG_ABI_COMPAT=15

No need for `-Wno-c++11-extensions` on these RUN lines, right (they already 
specify c++11 specifically, so there are no extensions to warn about)?



Comment at: clang/test/SemaCXX/class-layout.cpp:663-665
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-10 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Still waiting for the pre-merge checks to complete, but hopefully this is clean 
now.

Realized maybe we don't need a separate driver flag for this at all, and rely 
only on the abi-compat flag? That seems to be how (at least some) other ABI 
compat changes have been handled, looking at other uses of `ClangABI` enum 
values.

There could be more testing than only the indirect result of the packing 
problem that first inspired this patch. Any suggestions on what might be the 
most direct way to test whether the type's been considered pod in this sense?

Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5594
+unsigned Num;
+if (!Ver.consumeInteger(10, Num) && Num <= 13)
+  DefaultedSMFArePOD = false;

aaron.ballman wrote:
> Is Clang 13 still the correct thing to test for here, or should this be 16 
> these days?
Hmm - trunk is currently destined to be released as 16, yeah? So I think the 
right version would be 15, here. "If you want the old clang's ABI, ask for 
that, otherwise this is 16's ABI (new)"? Off-by-one error, of sorts. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-10 Thread David Blaikie via Phabricator via cfe-commits

dblaikie updated this revision to Diff 451686.
dblaikie added a comment.

Remove driver flag (just rely on ABI compat) & tidy up testing


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,10 +1,12 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=14
-// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-sie-ps5 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=15 -DCLANG_ABI_COMPAT=15
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=16 -DCLANG_ABI_COMPAT=16
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -642,3 +644,24 @@
 _Static_assert(_Alignof(t1) == 1, "");
 _Static_assert(_Alignof(t2) == 1, "");
 } // namespace non_pod_packed
+
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+// 14 and below consider t1 non-pod, but pack it anyway
+// 15 considers it non-pod and doesn't pack it
+// 16 and up considers it pod and packs it again...
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT == 15
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");
+#endif
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3530,6 +3530,8 @@
 GenerateArg(Args, OPT_fclang_abi_compat_EQ, "12.0", SA);
   else if (Opts.getClangABICompat() == LangOptions::ClangABI::Ver14)
 GenerateArg(Args, OPT_fclang_abi_compat_EQ, "14.0", SA);
+  else if (Opts.getClangABICompat() == LangOptions::ClangABI::Ver15)
+GenerateArg(Args, OPT_fclang_abi_compat_EQ, "15.0", SA);
 
   if (Opts.getSignReturnAddressScope() ==
   LangOptions::SignReturnAddressScopeKind::All)
@@ -4022,6 +4024,8 @@
 Opts.setClangABICompat(LangOptions::ClangABI::Ver12);
   else if (Major <= 14)
 Opts.setClangABICompat(LangOptions::ClangABI::Ver14);
+  else if (Major <= 15)
+Opts.setClangABICompat(LangOptions::ClangABI::Ver15);
 } else if (Ver != "latest") {
   Diags.Report(diag::err_drv_invalid_value)
   << A->getAsString(Args) << A->getValue();
Index: clang/lib/AST/DeclCXX.cpp
===
--- clang/lib/AST/DeclCXX.cpp
+++ clang/lib/AST/DeclCXX.cpp
@@ -768,12 +768,16 @@
 // Note that we have a user-declared

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-09 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Oh wow that's an awful lot of pings without any response; I'm very sorry you 
had that experience, so thank you for tagging me to try to get this unstuck!

The precommit CI test failures definitely look relevant and should be fixed up.




Comment at: clang/include/clang/Basic/LangOptions.def:219
 BENIGN_LANGOPT(AccessControl , 1, 1, "C++ access control")
+LANGOPT(DefaultedSMFArePOD, 1, 0, "Defaulted Special Members are allowed on 
POD types")
 LANGOPT(CharIsSigned  , 1, 1, "signed char")





Comment at: clang/include/clang/Driver/Options.td:1116
   PosFlag>;
+defm defaulted_smf_are_pod : BoolFOption<"defaulted-smf-are-pod",
+  LangOpts<"DefaultedSMFArePOD">, DefaultFalse,

Lol, well that's going to be fun when said out loud -- I come up with 
"defaulted smurfs are pod". :-D



Comment at: clang/include/clang/Driver/Options.td:1118-1119
+  LangOpts<"DefaultedSMFArePOD">, DefaultFalse,
+  NegFlag,
+  PosFlag>;
 def falign_functions : Flag<["-"], "falign-functions">, Group;





Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5592
+StringRef Ver = A->getValue();
+CmdArgs.push_back(Args.MakeArgString("-fclang-abi-compat=" + Ver));
+unsigned Num;

Are we going to get duplicates passed to -cc1, as we also do: 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5645



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5594
+unsigned Num;
+if (!Ver.consumeInteger(10, Num) && Num <= 13)
+  DefaultedSMFArePOD = false;

Is Clang 13 still the correct thing to test for here, or should this be 16 
these days?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-09 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a subscriber: aaron.ballman.
dblaikie added a comment.

@aaron.ballman is this something you're comfortable reviewing or could 
recommend anyone else who might be suitable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-08 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-08-01 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-07-25 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-07-17 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-07-08 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-06-27 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-06-21 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-06-13 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-06-06 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-06-01 Thread David Blaikie via Phabricator via cfe-commits

dblaikie marked 5 inline comments as done.
dblaikie added a comment.

In D119051#3550653 , @probinson wrote:

> I'm in the middle of upstreaming PS5 support and I still didn't think of 
> this... doh!

Ah, right - done!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-06-01 Thread David Blaikie via Phabricator via cfe-commits

dblaikie updated this revision to Diff 433472.
dblaikie added a comment.

Generalize to handle PS5


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/Driver/clang_f_opts.c
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,9 +1,9 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=14
-// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -641,3 +641,21 @@
 _Static_assert(_Alignof(t1) == 1, "");
 _Static_assert(_Alignof(t2) == 1, "");
 } // namespace non_pod_packed
+
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+#ifdef NO_DEFAULTED_SMF_ARE_POD
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");
+#endif
+}
Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -597,6 +597,16 @@
 // CHECK_DISABLE_DIRECT: -fobjc-disable-direct-methods-for-testing
 // CHECK_NO_DISABLE_DIRECT-NOT: -fobjc-disable-direct-methods-for-testing
 
+// RUN: %clang -### %s -fno-defaulted-smf-are-pod 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s 2>&1 | FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-scei-ps4 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-scei-ps5 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-scei-ps4 -fdefaulted-smf-are-pod 2>&1 | FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-unknown-darwin 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-unknown-darwin -fdefaulted-smf-are-pod 2>&1 | FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// CHECK-NO-DEFAULTED-SMF-ARE-POD-NOT: defaulted-smf-are-pod
+// CHECK-DEFAULTED-SMF-ARE-POD: -fdefaulted-smf-are-pod
+
 // RUN: %clang -### -S -fjmc -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefixes=CHECK_JMC_WARN,CHECK_NOJMC %s
 // RUN: %clang -### -S -fjmc -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefixes=CHECK_JMC_WARN_NOT_ELF,CHECK_NOJMC %s
 // RUN: %clang -### -S -fjmc -g -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_JMC %s
Index: clang/test/AST/ast-dump-record-definition-data-json.cpp
===
--- clang/test/AST/ast-dump-record-definition-data-json.cpp
+++ clang/test/AST/ast-dump-record-definition-data-json.cpp
@@ -772,6 +772,7 @@
 // CHECK-NEXT:   "isAggregate": true,
 // CHECK-NEXT:   "isEmpty": true,
 //

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-06-01 Thread Paul Robinson via Phabricator via cfe-commits

probinson added a comment.

I'm in the middle of upstreaming PS5 support and I still didn't think of 
this... doh!




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5588
 
+  bool DefaultedSMFArePOD = !RawTriple.isPS4() && !RawTriple.isOSDarwin();
+

`isPS4()` => `isPS()` please.  This should be in effect for both PS4 and PS5.



Comment at: clang/test/Driver/clang_f_opts.c:602
+// RUN: %clang -### %s 2>&1 | FileCheck 
--check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-scei-ps4 2>&1 | FileCheck 
--check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-scei-ps4 -fdefaulted-smf-are-pod 2>&1 | 
FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s

Please duplicate this line with `-target x86_64-sie-ps5`
(no need to duplicate both)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-05-27 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596
+  DefaultedSMFArePOD = false;
+  }
+

probinson wrote:
> dblaikie wrote:
> > probinson wrote:
> > > dblaikie wrote:
> > > > probinson wrote:
> > > > > Does this mean `-fclang-abi-compat` will override the PS4/Darwin 
> > > > > special case?  I think we don't want to do that.
> > > > I don't think so - this expression will make `DefaultedSMFArePOD` false 
> > > > when it's already false due to the target not being PS4 or Darwin, 
> > > > yeah? So it'll redundantly/harmlessly set it to false.
> > > No, I mean if it *is* PS4, it will turn true into false, and that's bad.  
> > > If I'm reading this correctly.
> > Right - let's see if I can explain how I'm understanding it at least - 
> > maybe I've got some code blindness from staring at it too long.
> > 
> > If it /is/ PS4, then this code:
> > ```
> > bool DefaultedSMFArePOD = !RawTriple.isPS4() && !RawTriple.isOSDarwin();
> > ```
> > Will amount to this:
> > ```
> > bool DefaultedSMFArePOD = !true && !RawTriple.isOSDarwin();
> > ```
> > ```
> > bool DefaultedSMFArePOD = false && !RawTriple.isOSDarwin();
> > ```
> > ```
> > bool DefaultedSMFArePOD = false;
> > ```
> > So then then the code at 5595 can only reinforce that, not change it - 
> > since it only sets the value to false. So my understanding is that the 
> > `-fclang-abi-compat` flag can not have any effect on the 
> > `DefaultedSMFArePOD` behavior of PS4. And it sounds like that's what you 
> > want? So I think we're good here?
> Those little bangs can be hard to see sometimes...  Also, I think this bit
> > already false due to the target not being PS4 or Darwin, 
> confused me, should have read
> > already false due to the target being PS4 or Darwin,
> and so what the code actually does is in fact what we want.
> 
> Thanks for your patience in explaining it, and I will go make an eye doctor 
> appointment!
>  I think this bit
> > already false due to the target not being PS4 or Darwin,
> confused me, should have read
> > already false due to the target being PS4 or Darwin,

Ah, right you are, sorry about that!

> Thanks for your patience in explaining it, and I will go make an eye doctor 
> appointment!

Sure thing - certainly sometimes consider whether or not switching to the C++ 
alternative tokens would be an improvement...
```
DefaultedSMFArePOD = not RawTriple.isPS4() and not RawTriple.isOSDarwin()
```
 ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-05-26 Thread Paul Robinson via Phabricator via cfe-commits

probinson added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596
+  DefaultedSMFArePOD = false;
+  }
+

dblaikie wrote:
> probinson wrote:
> > dblaikie wrote:
> > > probinson wrote:
> > > > Does this mean `-fclang-abi-compat` will override the PS4/Darwin 
> > > > special case?  I think we don't want to do that.
> > > I don't think so - this expression will make `DefaultedSMFArePOD` false 
> > > when it's already false due to the target not being PS4 or Darwin, yeah? 
> > > So it'll redundantly/harmlessly set it to false.
> > No, I mean if it *is* PS4, it will turn true into false, and that's bad.  
> > If I'm reading this correctly.
> Right - let's see if I can explain how I'm understanding it at least - maybe 
> I've got some code blindness from staring at it too long.
> 
> If it /is/ PS4, then this code:
> ```
> bool DefaultedSMFArePOD = !RawTriple.isPS4() && !RawTriple.isOSDarwin();
> ```
> Will amount to this:
> ```
> bool DefaultedSMFArePOD = !true && !RawTriple.isOSDarwin();
> ```
> ```
> bool DefaultedSMFArePOD = false && !RawTriple.isOSDarwin();
> ```
> ```
> bool DefaultedSMFArePOD = false;
> ```
> So then then the code at 5595 can only reinforce that, not change it - since 
> it only sets the value to false. So my understanding is that the 
> `-fclang-abi-compat` flag can not have any effect on the `DefaultedSMFArePOD` 
> behavior of PS4. And it sounds like that's what you want? So I think we're 
> good here?
Those little bangs can be hard to see sometimes...  Also, I think this bit
> already false due to the target not being PS4 or Darwin, 
confused me, should have read
> already false due to the target being PS4 or Darwin,
and so what the code actually does is in fact what we want.

Thanks for your patience in explaining it, and I will go make an eye doctor 
appointment!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-05-26 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.

Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596
+  DefaultedSMFArePOD = false;
+  }
+

probinson wrote:
> dblaikie wrote:
> > probinson wrote:
> > > Does this mean `-fclang-abi-compat` will override the PS4/Darwin special 
> > > case?  I think we don't want to do that.
> > I don't think so - this expression will make `DefaultedSMFArePOD` false 
> > when it's already false due to the target not being PS4 or Darwin, yeah? So 
> > it'll redundantly/harmlessly set it to false.
> No, I mean if it *is* PS4, it will turn true into false, and that's bad.  If 
> I'm reading this correctly.
Right - let's see if I can explain how I'm understanding it at least - maybe 
I've got some code blindness from staring at it too long.

If it /is/ PS4, then this code:
```
bool DefaultedSMFArePOD = !RawTriple.isPS4() && !RawTriple.isOSDarwin();
```
Will amount to this:
```
bool DefaultedSMFArePOD = !true && !RawTriple.isOSDarwin();
```
```
bool DefaultedSMFArePOD = false && !RawTriple.isOSDarwin();
```
```
bool DefaultedSMFArePOD = false;
```
So then then the code at 5595 can only reinforce that, not change it - since it 
only sets the value to false. So my understanding is that the 
`-fclang-abi-compat` flag can not have any effect on the `DefaultedSMFArePOD` 
behavior of PS4. And it sounds like that's what you want? So I think we're good 
here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-05-25 Thread Paul Robinson via Phabricator via cfe-commits

probinson added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596
+  DefaultedSMFArePOD = false;
+  }
+

dblaikie wrote:
> probinson wrote:
> > Does this mean `-fclang-abi-compat` will override the PS4/Darwin special 
> > case?  I think we don't want to do that.
> I don't think so - this expression will make `DefaultedSMFArePOD` false when 
> it's already false due to the target not being PS4 or Darwin, yeah? So it'll 
> redundantly/harmlessly set it to false.
No, I mean if it *is* PS4, it will turn true into false, and that's bad.  If 
I'm reading this correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-05-25 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596
+  DefaultedSMFArePOD = false;
+  }
+

probinson wrote:
> Does this mean `-fclang-abi-compat` will override the PS4/Darwin special 
> case?  I think we don't want to do that.
I don't think so - this expression will make `DefaultedSMFArePOD` false when 
it's already false due to the target not being PS4 or Darwin, yeah? So it'll 
redundantly/harmlessly set it to false.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-05-25 Thread Paul Robinson via Phabricator via cfe-commits

probinson added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5596
+  DefaultedSMFArePOD = false;
+  }
+

Does this mean `-fclang-abi-compat` will override the PS4/Darwin special case?  
I think we don't want to do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-05-24 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-05-24 Thread David Blaikie via Phabricator via cfe-commits

dblaikie updated this revision to Diff 431871.
dblaikie added a comment.
Herald added a subscriber: MaskRay.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/Driver/clang_f_opts.c
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,9 +1,9 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=14
-// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=14
+// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=14 -DCLANG_ABI_COMPAT=14
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -641,3 +641,21 @@
 _Static_assert(_Alignof(t1) == 1, "");
 _Static_assert(_Alignof(t2) == 1, "");
 } // namespace non_pod_packed
+
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+#ifdef NO_DEFAULTED_SMF_ARE_POD
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");
+#endif
+}
Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -597,6 +597,15 @@
 // CHECK_DISABLE_DIRECT: -fobjc-disable-direct-methods-for-testing
 // CHECK_NO_DISABLE_DIRECT-NOT: -fobjc-disable-direct-methods-for-testing
 
+// RUN: %clang -### %s -fno-defaulted-smf-are-pod 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s 2>&1 | FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-scei-ps4 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-scei-ps4 -fdefaulted-smf-are-pod 2>&1 | FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-unknown-darwin 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-unknown-darwin -fdefaulted-smf-are-pod 2>&1 | FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// CHECK-NO-DEFAULTED-SMF-ARE-POD-NOT: defaulted-smf-are-pod
+// CHECK-DEFAULTED-SMF-ARE-POD: -fdefaulted-smf-are-pod
+
 // RUN: %clang -### -S -fjmc -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefixes=CHECK_JMC_WARN,CHECK_NOJMC %s
 // RUN: %clang -### -S -fjmc -target x86_64-pc-windows-msvc %s 2>&1 | FileCheck -check-prefixes=CHECK_JMC_WARN_NOT_ELF,CHECK_NOJMC %s
 // RUN: %clang -### -S -fjmc -g -target x86_64-unknown-linux %s 2>&1 | FileCheck -check-prefix=CHECK_JMC %s
Index: clang/test/AST/ast-dump-record-definition-data-json.cpp
===
--- clang/test/AST/ast-dump-record-definition-data-json.cpp
+++ clang/test/AST/ast-dump-record-definition-data-json.cpp
@@ -772,6 +772,7 @@
 // CHECK-NEXT:   "isAggregate": true,
 // CHECK-NEXT:   "isEmpty": true,
 // CHECK-NEXT:   "isLiteral": true,
+// CHECK-NEXT:   "isPOD": true,
 // CHECK-NEXT:   "isStandardLayout":

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-03-07 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D119051#3350072 , @rnk wrote:

> I would structure this as a LangOpt, and feed the target checks and ABI 
> compat checks into the default setting for it. It could be something like 
> `DefaultedSMFArePOD` / `-f[no-]defaulted-smf-are-pod` (smf being special 
> member functions). The LangOpt default is true, and the driver fills in the 
> value from the target and ABI version in the case of no explicit flag.
>
> I think the DeclCXX changes are probably in the right place. I don't think it 
> makes sense to carry around separate `isPOD` bits according to the C++11 and 
> C++03 rules, just so we can use them later to make these two specific, known 
> ABI-impacting decisions.

Fair enough - had a go at that. Used your naming though in light of @jyknight's 
suggestion it might be overly specific (but figure we can bikeshed that in 
parallel with the implementation review)

In D119051#3350276 , @jyknight wrote:

> In D119051#3316026 , @dblaikie 
> wrote:
>
>> Ah, looks like this is the existing 
>> https://github.com/itanium-cxx-abi/cxx-abi/issues/66
>
> If you're going to change the ABI, you might as well tackle the rest of the 
> differences mentioned in that issue while you're in there. That is, neither 
> marking anything "= delete", nor creating a user-defined move assignment 
> operator should mark it non-pod according to the GCC-compatible rules.

Fair point - I think I've got those cases covered (see updated test case - 
happy to expand that further if desired - testing each case separately, or the 
like)? No member functions/ctor marked deleted, and no user-defined move 
assignment operator make the type non-pod.

I tested this a bit more robustly end-to-end like this:

  struct t1 {
t1() = default;
t1(const t1&) = delete;
t1(t1&&) = default;
void operator=(t1&&);
~t1() = delete;
int a;
char c;
  };
  struct t2 : t1 {
char c;
  };
  int x[sizeof(t2) == 3 * sizeof(int) ? 1 : -1];

$ clang-tot test.cpp -c  -target x86_64-scei-ps4 
test.cpp:13:7: error: 'x' declared as an array with a negative size
int x[sizeof(t2) == 3 * sizeof(int) ? 1 : -1];

  ^~

1 error generated.
$ clang-tot test.cpp -c  -target x86_64-scei-ps4 -fdefaulted-smf-are-pod
$ clang-tot test.cpp -c  -target x86_64-unknown-darwin 
test.cpp:13:7: error: 'x' declared as an array with a negative size
int x[sizeof(t2) == 3 * sizeof(int) ? 1 : -1];

  ^~

1 error generated.
$ clang-tot test.cpp -c  -target x86_64-unknown-darwin -fdefaulted-smf-are-pod
$ clang-tot test.cpp -c  -target x86_64-unknown-linux
$ clang-tot test.cpp -c  -target x86_64-unknown-linux -fno-defaulted-smf-are-pod
test.cpp:13:7: error: 'x' declared as an array with a negative size
int x[sizeof(t2) == 3 * sizeof(int) ? 1 : -1];

  ^~

1 error generated.

  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-03-07 Thread David Blaikie via Phabricator via cfe-commits

dblaikie updated this revision to Diff 413641.
dblaikie added a comment.
Herald added subscribers: dexonsmith, dang.
Herald added a project: All.

Add a -f driver flag for defaulted-smf-are-pod
Include the other cases from itanium ABI issue 66 (implicitly and explicitly 
deleted special members, and any form of move assignment operator)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/AST/DeclCXX.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/AST/ast-dump-record-definition-data-json.cpp
  clang/test/Driver/clang_f_opts.c
  clang/test/SemaCXX/class-layout.cpp

Index: clang/test/SemaCXX/class-layout.cpp
===
--- clang/test/SemaCXX/class-layout.cpp
+++ clang/test/SemaCXX/class-layout.cpp
@@ -1,9 +1,10 @@
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base
-// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=13
-// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=13 -DCLANG_ABI_COMPAT=13
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base -Wno-c++11-extensions -fno-defaulted-smf-are-pod -DNO_DEFAULTED_SMF_ARE_POD
+// RUN: %clang_cc1 -triple x86_64-apple-darwin%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=13
+// RUN: %clang_cc1 -triple x86_64-scei-ps4%s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -Wno-c++11-extensions -fclang-abi-compat=13 -DCLANG_ABI_COMPAT=13
 // expected-no-diagnostics
 
 #define SA(n, p) int a##n[(p) ? 1 : -1]
@@ -641,3 +642,21 @@
 _Static_assert(_Alignof(t1) == 1, "");
 _Static_assert(_Alignof(t2) == 1, "");
 } // namespace non_pod_packed
+
+namespace cxx11_pod {
+struct t1 {
+  t1() = default;
+  t1(const t1&) = delete;
+  ~t1() = delete;
+  t1(t1&&) = default;
+  int a;
+};
+struct t2 {
+  t1 v1;
+} __attribute__((packed));
+#ifdef NO_DEFAULTED_SMF_ARE_POD
+_Static_assert(_Alignof(t2) == 4, "");
+#else
+_Static_assert(_Alignof(t2) == 1, "");
+#endif
+}
Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -598,6 +598,15 @@
 // CHECK_DISABLE_DIRECT: -fobjc-disable-direct-methods-for-testing
 // CHECK_NO_DISABLE_DIRECT-NOT: -fobjc-disable-direct-methods-for-testing
 
+// RUN: %clang -### %s -fno-defaulted-smf-are-pod 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s 2>&1 | FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-scei-ps4 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-scei-ps4 -fdefaulted-smf-are-pod 2>&1 | FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-unknown-darwin 2>&1 | FileCheck --check-prefix=CHECK-NO-DEFAULTED-SMF-ARE-POD %s
+// RUN: %clang -### %s -target x86_64-unknown-darwin -fdefaulted-smf-are-pod 2>&1 | FileCheck --check-prefix=CHECK-DEFAULTED-SMF-ARE-POD %s
+// CHECK-NO-DEFAULTED-SMF-ARE-POD-NOT: defaulted-smf-are-pod
+// CHECK-DEFAULTED-SMF-ARE-POD: -fdefaulted-smf-are-pod
+
 // RUN: %clang -### -S -fjmc %s 2>&1 | FileCheck -check-prefixes=CHECK_JMC_WARN,CHECK_NOJMC %s
 // RUN: %clang -### -S -fjmc -g %s 2>&1 | FileCheck -check-prefix=CHECK_JMC %s
 // RUN: %clang -### -S -fjmc -g -fno-jmc %s 2>&1 | FileCheck -check-prefix=CHECK_NOJMC %s
Index: clang/test/AST/ast-dump-record-definition-data-json.cpp
===
---

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-28 Thread James Y Knight via Phabricator via cfe-commits

jyknight added a comment.

In D119051#3316026 , @dblaikie wrote:

> Ah, looks like this is the existing 
> https://github.com/itanium-cxx-abi/cxx-abi/issues/66

If you're going to change the ABI, you might as well tackle the rest of the 
differences mentioned in that issue while you're in there. That is, neither 
marking anything "= delete", nor creating a user-defined move assignment 
operator should mark it non-pod according to the GCC-compatible rules.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-28 Thread Reid Kleckner via Phabricator via cfe-commits

rnk added a comment.

I would structure this as a LangOpt, and feed the target checks and ABI compat 
checks into the default setting for it. It could be something like 
`DefaultedSMFArePOD` / `-f[no-]defaulted-smf-are-pod` (smf being special member 
functions). The LangOpt default is true, and the driver fills in the value from 
the target and ABI version in the case of no explicit flag.

I think the DeclCXX changes are probably in the right place. I don't think it 
makes sense to carry around separate `isPOD` bits according to the C++11 and 
C++03 rules, just so we can use them later to make these two specific, known 
ABI-impacting decisions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-28 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-21 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D119051#3315747 , @rjmccall wrote:

> And this should be raised as an Itanium issue.

Ah, looks like this is the existing 
https://github.com/itanium-cxx-abi/cxx-abi/issues/66


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

Thanks for chiming in!

In D119051#3315741 , @rjmccall wrote:

> Changing the C++03 POD definition is going to be substantially ABI-breaking 
> at this point.  Any logic to change it needs to be platform-specific.

Even if it's only related to the use of "= default"? Hrm. Got some examples I 
could try out?

Ah, I guess here: https://godbolt.org/z/P6n9xKTnM

  struct t1 {
int i;
char c;
t1() = default;
  };
  struct t2: t1 {
char c;
  };
  #ifdef __clang__
  static_assert(sizeof(t2) == 8, "");
  #else
  static_assert(sizeof(t2) == 12, "");
  #endif

Where would be the right place for the platform specific detection to happen 
then? Can the places I've proposed changing in DeclCXX.cpp be made 
target-specific there, or do we have to compute two different properties there 
and do the target-specific selection between them at some later point/in the 
record layout logic?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-11 Thread John McCall via Phabricator via cfe-commits

rjmccall added a comment.

And this should be raised as an Itanium issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-11 Thread John McCall via Phabricator via cfe-commits

rjmccall added a comment.

Changing the C++03 POD definition is going to be substantially ABI-breaking at 
this point.  Any logic to change it needs to be platform-specific.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-11 Thread David Blaikie via Phabricator via cfe-commits

dblaikie added a comment.

In D119051#3314138 , @Bhramar.vatsa 
wrote:

> Sorry, but I can only add a bit more confusion: 
> https://godbolt.org/z/fT19KTh34
> There are two cases, only differing in terms of user-defined constructor.
>
> Gcc and clang differs in the two cases. Gcc at least packs the second case 
> (without user defined constructor), but clang (trunk version) doesn't.

Right - but with this proposed patch applied, Clang's answer is consistent with 
GCC's behavior, I believe?

> Uncomment/define macro 'PROPS' to check that the type-traits indicate in both 
> cases that it can be considered POD.

Yes, I believe/understand that both of these types are C++11 POD, but the first 
example is not C++98/03 POD (and the second example isn't valid C++98/03, but 
as an extension, it is supported in C++98/03 and as far as I can tell (see the 
switch example in https://reviews.llvm.org/D119051#3305511 ) GCC deems the 
defaulted constructor to be still be C++98/03 POD, as an extension).

So, I believe your example is consistent with the theory I gave at the start of 
this patch: GCC is using the C++98/03 definition of POD, with an extension for 
defaulted functions, to determine whether to pack/align a member or not, and 
changing Clang's definition of 98/03 POD to match GCC's (by adding an extension 
for defaulted functions) is probably the right thing to do, so far as I can 
gather.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

2022-02-11 Thread Bhramar Vatsa via Phabricator via cfe-commits

Bhramar.vatsa added a comment.

Sorry, but I can only add a bit more confusion: https://godbolt.org/z/dzYhhxbz4
There are two cases, only differing in terms of user-defined constructor.

Gcc and clang differs in the two cases. Gcc at least packs the second case 
(without user defined constructor), but clang doesn't.

Uncomment/define macro 'PROPS' to check that the type-traits indicate in both 
cases that it can be considered POD.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119051

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

73 matches

Mail list logo