[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:66
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
+#include <cmath>
 #include <deque>
----------------
Do we need cmath?


================
Comment at: clang/include/clang/Sema/Sema.h:488
+    AlignPackInfo(AlignPackInfo::Mode M, int Num, bool IsAIX)
+        : PackAttr(true), AlignMode(M), PackNumber(Num), AIXStack(IsAIX) {}
+
----------------
I noticed PackNumber is an unsigned char and we are passing an int type into it.
Could we add an assertion in the constructor to make sure Num would never be something that's going to get truncated when it converts to an unsigned char?


================
Comment at: clang/include/clang/Sema/Sema.h:504
+    unsigned getPackNumber() const { return PackNumber; }
+    void setPackNumber(int Num) { PackNumber = Num; }
+
----------------
I don't see this function gets referenced in this patch. 


================
Comment at: clang/include/clang/Sema/Sema.h:514
+
+    bool operator==(AlignPackInfo Info) const {
+      return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
----------------
I think a normal operator== signature would look like this:
bool operator==(const AlignPackInfo &Info) const;


================
Comment at: clang/include/clang/Sema/Sema.h:515
+    bool operator==(AlignPackInfo Info) const {
+      return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+    }
----------------
This could return true when `PackAttr` in AlignPackInfo are not the same. Wouldn't that cause an issue?


================
Comment at: clang/include/clang/Sema/Sema.h:519
+    bool operator!=(AlignPackInfo Info) const {
+      return AlignMode != Info.AlignMode || PackNumber != Info.PackNumber;
+    }
----------------
You should be able to implement this using "operator==" right?
e.g. return !(*this == Info);



================
Comment at: clang/include/clang/Sema/Sema.h:524
+    /// \brief True if this is a pragma pack attribute,
+    //         not a pragma align attribute.
+    bool PackAttr;
----------------



================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:697
         InferAlignment(false), Packed(false), IsUnion(false),
-        IsMac68kAlign(false), IsMsStruct(false), UnfilledBitsInLastUnit(0),
-        LastBitfieldTypeSize(0), MaxFieldAlignment(CharUnits::Zero()),
-        DataSize(0), NonVirtualSize(CharUnits::Zero()),
+        IsMac68kAlign(false), IsNaturalAlign(false), IsMsStruct(false),
+        UnfilledBitsInLastUnit(0), LastBitfieldTypeSize(0),
----------------
This could probably confuse people, as natural alignment is the default on most target except AIX.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:403
   // Warn about modified alignment after #includes.
   if (PrevPackState.CurrentValue != PackStack.CurrentValue) {
     Diag(IncludeLoc, diag::warn_pragma_pack_modified_after_include);
----------------
Since we changed the PackStack for it to contain AlignPackInfo instead of unsigned. 
This stack no longer only contains Pack information. So we need to rethink about how this diagnostic and the one follows should work.
i.e. What's the purpose of these diagnostic? Is it still only for pragma pack report? If so, what we are doing here is not correct, since the `CurrentValue` could be different, not because of the pragma pack change, but because of the pragma align change.
If it's not only for pragma pack any more, but also intend to detect the pragma align interaction, then possibly function name and diagnostic needs some modification, as they don't match the intent any more.


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

https://reviews.llvm.org/D87702