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

Fri Aug 12 15:20:36 PDT 2022

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

In D119051#3717934 <https://reviews.llvm.org/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