[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:893
+def warn_pragma_pack_identifer_not_supported : Warning<
+  "specifying an identifier within pragma pack is not supported, identifier is ignored">,
+  InGroup<PragmaPack>;
----------------
Xiangling_L wrote:
> 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?
I'm not super familiar with this feature, so what I'm thinking of may not be a real issue in practice. The case I was thinking of is where the user does `#pragma pack(push, r1, 2)` to push a packing by name followed by `#pragma pack(push, r2, 4)`, and the later pops come in the reverse order (`#pragma pack(pop, r1)` followed by `#pragma pack(pop, r2)`). The first pop will actually pop the `r2` packing and not the `r1` packing, which could cause the field layout to be different from what the user expected.


================
Comment at: clang/include/clang/Sema/Sema.h:524-525
+        return AlignPackInfo(M, PackNumber, IsXL);
+      else
+        return AlignPackInfo(M, IsXL);
+    }
----------------
No `else` after a `return`.


================
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");
----------------
Xiangling_L wrote:
> 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.
Oh, that's different from what I was expecting -- I was thinking you'd change the data member fields of the class to be bit-fields.

Either way works for me, though, so no change necessary unless you prefer that approach.


================
Comment at: clang/include/clang/Sema/Sema.h:527
+
+    unsigned char getPackNumber() const { return PackNumber; }
+
----------------
Xiangling_L wrote:
> 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.
We don't save any space with this function signature though -- the return value will most likely wind up being implicitly promoted to `int` anyway. I think the fact that we store it as a smaller datatype internally is an implementation detail and as far as the consumer of this class is concerned, the pack number is an integer type where we don't care about the size. tbh, because the pack numbers are always positive, I'd have the ctor and this function both deal with an `unsigned` rather than an `int`.

(Note, I'm not talking about the type of the underlying field -- I think it's fine to continue to use a smaller datatype there for space savings. I'm only talking about the types in the public interface.)


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

https://reviews.llvm.org/D87702