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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 9 inline comments as done.
Xiangling_L added inline comments.


================
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>;
----------------
aaron.ballman wrote:
> 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?
Could you show a more concrete example or give more details on how possible incorrect behavior would lead to hard-to-find ODR issues?


================
Comment at: clang/include/clang/Sema/Sema.h:486
+        : PackAttr(true), AlignMode(M), PackNumber(Num), XLStack(IsXL) {
+      assert(Num == PackNumber && "Unexpected value.");
+    }
----------------
aaron.ballman wrote:
> 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`.
Sorry for that, I would make it clearer.


================
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");
----------------
aaron.ballman wrote:
> 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).
Yeah, good point. I will move it back to previous bitfields version.


================
Comment at: clang/include/clang/Sema/Sema.h:527
+
+    unsigned char getPackNumber() const { return PackNumber; }
+
----------------
aaron.ballman wrote:
> Given that the ctor takes an `int` for this value, should this be returning an `int`?
As we know the pack number would not exceed 16 bytes, so the intention of using `unsigned char` here is to save space taken by AlignPackInfo. And that's why I added the diagnostics and assertion in the ctor to make sure the value won't be truncated.


================
Comment at: clang/test/Sema/aix-pragma-pack-and-align.c:231
+
+// expected-no-warning
----------------
aaron.ballman wrote:
> Is this comment intentional?
Yes, my intention is to test no pragma pack or prama align is unterminated.


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

https://reviews.llvm.org/D87702