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

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.


================
Comment at: clang/include/clang/Sema/Sema.h:505
+    // AlignPackInfo::getFromRawEncoding, it should not be inspected directly.
+    uint32_t getRawEncoding(const AlignPackInfo &Info) const {
+      std::uint32_t Encoding{};
----------------
this getter do not need to take any parameters. You could just use `*this`.
Or, to make it consistent with the getFromRawEncoding, we could make this a static function as well and keep the parameter.


================
Comment at: clang/include/clang/Sema/Sema.h:506
+    uint32_t getRawEncoding(const AlignPackInfo &Info) const {
+      std::uint32_t Encoding{};
+      if (Info.IsAIXStack())
----------------
Since this new structure have the same size as the uint32_t, have you consider using memcpy to burn the data directly into the uint32_t, and vice versa?


================
Comment at: clang/include/clang/Sema/Sema.h:570
+    /// \brief True if it is a AIX #pragma align/pack stack.
+    bool AIXStack;
+
----------------
It might be better to rename it XLStack, as this is the XL compiler behavior on AIX. Other compilers on AIX, like gcc, could have different behavior.


================
Comment at: clang/include/clang/Sema/Sema.h:637
   PragmaStack<MSVtorDispMode> VtorDispStack;
-  // #pragma pack.
-  // Sentinel to represent when the stack is set to mac68k alignment.
-  static const unsigned kMac68kAlignmentSentinel = ~0U;
-  PragmaStack<unsigned> PackStack;
+  PragmaStack<AlignPackInfo> PackStack;
   // The current #pragma pack values and locations at each #include.
----------------
Xiangling_L wrote:
> jasonliu wrote:
> > We should consider renaming PackStack to AlignPackStack across Clang. Maybe even as a NFC first. As it is right now, clang already uses one stack to record those two informations from `Pragma align` and `Pragma pack`. Leave it as PackStack is too confusing, and people could actually ignore the pragma Align when developing with PackStack. 
> That's a good point. I will create a NFC accordingly.
Please post this NFC when you have time.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:699-700
+        IsMac68kAlign(false),
+        IsNaturalAlign(!Context.getTargetInfo().getTriple().isOSAIX() ? true
+                                                                      : false),
+        IsMsStruct(false), UnfilledBitsInLastUnit(0),
----------------



================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1322
   HandledFirstNonOverlappingEmptyField =
-      !Context.getTargetInfo().defaultsToAIXPowerAlignment();
+      !Context.getTargetInfo().defaultsToAIXPowerAlignment() || IsNaturalAlign;
 
----------------
Not sure if this would have a real effect, but reading this code concerns me a little bit:
We would set `HandledFirstNonOverlappingEmptyField` based on `IsNaturalAlign`, but for AIX, that bit might only get set at line 1343.
It would look like we have a read before write to the variable here. 


================
Comment at: clang/lib/Sema/SemaAttr.cpp:232
     Action = Sema::PSK_Push_Set;
-    Alignment = 0;
+    if (IsAIX)
+      ModeVal = AlignPackInfo::Natural;
----------------
XLPragmaPack is used to control pack and align stack semantic behavior.
Someone on AIX might want to turn off this compatible pack and align with XL behavior (to get the compatible behavior with clang on other platform for easier porting purpose), but they would still prefer to have the natural alignment to work correctly.
So it might not be desirable to query this option for this natural alignment behavior.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:413
+  // FIXME: PackStack may contain both #pragma align and #pragma pack
+  // information, we should warn about both modified align mode and alignment.
   if (PrevPackState.CurrentValue != PackStack.CurrentValue) {
----------------
I don't think there is a preferable solution yet. Pointing out what could go wrong is good enough here.
Same for the FIXME below.


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

https://reviews.llvm.org/D87702