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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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

LGTM



================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1891
+  llvm::Triple Target = Context.getTargetInfo().getTriple();
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
+                                 Context.getLangOpts().getClangABICompat() <=
----------------
dblaikie wrote:
> rsmith wrote:
> > `isPOD` is C++ standard specific, and our ABI rule really shouldn't be. Does GCC use the C++98 rules here, the C++11 rules, or something else? (Hopefully the GCC behavior doesn't change between `-std=c++98` and `-std=c++11`!)
> > 
> > From a quick test, it looks like GCC doesn't pack fields whose types are classes with base classes, even if they're trivially-copyable and standard-layout, suggesting that it's probably using the C++98 POD rules.
> I /think/ `CXXRecordDecl::isPOD` doesn't use a language-varying definition, according to its documentation at least:
> https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/include/clang/AST/DeclCXX.h#L1124
> & the code that sets the field that the accessor returns looks, to me at least, consistent with that claim: https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/lib/AST/DeclCXX.cpp#L983
> 
> But if there's another way I should spell this to make it more clear/correct, more test cases to add to show the difference between these definitions - I'm open to that... 
Ah, right you are, I was thinking of `QualType::isPODType`, which does depend on the language mode: https://github.com/llvm/llvm-project/blob/1e3a02162db20264e9615b1346420c8d199cb347/clang/lib/AST/Type.cpp#L2350

No action necessary here, I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117616