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

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

rsmith added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:239
 
+- gcc doesn't pack non-packed non-pod members in packed structs. Clang
+  historically did perform such packing. Clang now matches the gcc behavior
----------------
"non-packed" here seems a little unclear. Does this mean "GCC doesn't pack non-POD members in packed structs unless the packed attribute is also specified on the member." ?

(Also I believe the preferred spelling for GCC is capitalized.)


================
Comment at: clang/docs/ReleaseNotes.rst:242
+  (except on Darwin and PS4). You can switch back to the old ABI behavior with
+  the flag: -fclang-abi-compat=13.0.
+
----------------



================
Comment at: clang/include/clang/Basic/LangOptions.h:185
+    /// Attempt to be ABI-compatible with code generated by Clang 13.0.x.
+    /// This causes clang to apply to pack non-pod members of packed structs.
+    Ver13,
----------------



================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1891
+  llvm::Triple Target = Context.getTargetInfo().getTriple();
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
+                                 Context.getLangOpts().getClangABICompat() <=
----------------
`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.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1895
+                                 Target.isPS4() || Target.isOSDarwin())) ||
+                     D->hasAttr<PackedAttr>();
 
----------------
Would it make sense to warn here if `Packed && !FieldPacked`, like GCC does?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117616