[PATCH] D87702: [Frontend] Add pragma align natural and sort out pragma pack stack effect

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:858
+def err_pragma_pack_invalid_alignment : Error<
+  "expected #pragma pack parameter to be '1', '2', '4', '8', or '16'">;
 def warn_pragma_pack_non_default_at_include : Warning<
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:893
+def warn_pragma_pack_identifer_not_supported : Warning<
+  "specifying an identifier within pragma pack is not supported, identifier is ignored">,
+  InGroup<PragmaPack>;
----------------
If the user wrote an identifier, it seems like there's a strong chance that ignoring the identifier will result in incorrect behavior that would lead to hard-to-find ODR issues. Should this scenario be an error rather than a warning where the identifier is ignored?


================
Comment at: clang/include/clang/Sema/Sema.h:486
+        : PackAttr(true), AlignMode(M), PackNumber(Num), XLStack(IsXL) {
+      assert(Num == PackNumber && "Unexpected value.");
+    }
----------------
The string literal here doesn't really convey what's being asserted -- it took me a while to figure out that this is trying to catch truncation issues when `Num` cannot be represented by an `unsigned char`.


================
Comment at: clang/include/clang/Sema/Sema.h:494
+
+    AlignPackInfo(bool IsXL) : AlignPackInfo(Native, IsXL) {}
+
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:512
+    static AlignPackInfo getFromRawEncoding(unsigned Encoding) {
+      static_assert(sizeof(AlignPackInfo) == sizeof(uint32_t),
+                    "Size is not correct");
----------------
I would feel more comfortable with this assertion if the class was using bit-fields to pack the values together. As it stands, we're kind of hoping that `bool`, `Mode`, and `unsigned char` will all pack in a particular way (and `bool`'s representation is implementation-defined).


================
Comment at: clang/include/clang/Sema/Sema.h:527
+
+    unsigned char getPackNumber() const { return PackNumber; }
+
----------------
Given that the ctor takes an `int` for this value, should this be returning an `int`?


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1314
 void ItaniumRecordLayoutBuilder::InitializeLayout(const Decl *D) {
+
   if (const RecordDecl *RD = dyn_cast<RecordDecl>(D)) {
----------------
Unintended whitespace change?


================
Comment at: clang/lib/Sema/SemaAttr.cpp:69-71
+  if (IsXLPragma && M == AlignPackInfo::Natural) {
+    RD->addAttr(AlignNaturalAttr::CreateImplicit(Context));
   }
----------------



================
Comment at: clang/test/Sema/aix-pragma-pack-and-align.c:231
+
+// expected-no-warning
----------------
Is this comment intentional?


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

https://reviews.llvm.org/D87702