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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
