[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Hmm, I guess it might be the C++11 definition, as suggested - since a base class (even a public one) seems to classify the type as "non pod" as far as GCC is concerned (

In D117616#3298001 <https://reviews.llvm.org/D117616#3298001>, @dblaikie wrote:

> In D117616#3295859 <https://reviews.llvm.org/D117616#3295859>, @Bhramar.vatsa wrote:
>
>> @dblaikie 
>> The condition `FieldClass->isPOD()` returns false for the following case (when considering field 'struct foo t' of 'struct foo1') :
>>
>>   class foo {
>>      foo() = default;
>>      int _a;
>>   };
>>   
>>   struct foo1 {
>>       struct foo t;
>>   } t1;
>>
>> The same code though doesn't give any warning for gcc: https://godbolt.org/z/f4chraerY
>>
>> This is because the way it works for CXXRecordDecl : https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/lib/AST/DeclCXX.cpp#L928
>>
>> So, there seems to be a difference the way GCC is handling this case, in comparison to how now clang handles it.
>>
>> For the same case, `D->getType().isCXX11PODType()` (or `isPODType()`) indicates it to be a POD type. So, we think that this should be further changed such that it doesn't break the code that works with GCC.
>
> Sorry, was a bit confused by the discussion of warnings and such - but, yes, this does seem to be a remaining divergence in the layout between Clang (after this patch was committed) and GCC: https://godbolt.org/z/GEM5q4fd3
>
> @rsmith do you have a semi-exhaustive list of the variations in POD-ness I should probably test to better understand which definition GCC is using here? Reading the cppreference on POD, aggregate, standard layout, and trivial there are a lot of dimensions and I was wondering if you had a quick-ish summary so I hopefully don't miss cases & figure out exactly how this is meant to be sliced?
>
> I'll work on some godbolt probes/test cases for now to see what I can come up with.

Posted https://reviews.llvm.org/D119051 at least with the existing test case - thanks for that!

Open to more robust test case suggestions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117616