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

[Xiangling Liao via Phabricator via cfe-commits]

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


================
Comment at: clang/include/clang/Basic/LangOptions.def:340
 
+LANGOPT(AIXPragmaPack, 1, 0, "AIX #pragma pack handling")
+
----------------
jasonliu wrote:
> Not sure if AIXPragmaPack is the best name here. 
> It's more like IBM xl pragma pack handling on AIX.
> Would it be better if we name it `XLPragmaPackOnAIX`?
Just a record of the offline discussion: `XLPragmaPack` is sufficient here, following the convention of how we named our C++ ABI on AIX as XLABI.


================
Comment at: clang/include/clang/Sema/Sema.h:493
+          PackNumber(M == Packed ? 1
+                                 : (M == Mac68k ? Mac68kAlignmentSentinel
+                                                : UninitPackVal)),
----------------
jasonliu wrote:
> I think one of the idea is to use `enum Mode::Mac68k` to replace the need of Mac68kAlignmentSentinel.
> Is there any reason that you would still need PackNumber to contain `Mac68kAlignmentSentinel`?
AIX and other targets have different ways to compare if two `CurrentValue` equal. Other targets use only `PackNumber ` while AIX use both align mode and PackNumber. So this sentinel `Mac68kAlignmentSentinel` is added to support this.


================
Comment at: clang/include/clang/Sema/Sema.h:515
+    bool operator==(AlignPackInfo Info) const {
+      return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+    }
----------------
jasonliu wrote:
> Xiangling_L wrote:
> > jasonliu wrote:
> > > This could return true when `PackAttr` in AlignPackInfo are not the same. Wouldn't that cause an issue?
> > (1) You mean we have two `AlignPackInfo` with same AlignMode and PackNumber, but one is PackAttr and the other one is AlignAttr?
> > The example I can think of is:
> > 
> > 
> > ```
> > a)#pragma align(packed)
> >   #pragma pack(1)   //AlignMode = Packed, PackNumber = 1
> > 
> > b) #pragma align(packed)  //AlignMode = Packed, PackNumber = 1
> > ```
> > 
> > But I don't think we have any issue in this case. Before and after my patch, a == b.
> > Please let me know any other cases concerning you if any.
> > 
> > (2) However, your concerns leads me to think of another case, where behavior changes with my patch.
> > 
> > ```
> > a) 
> > #pragma align(natural)
> > #pragma pack(1)   /AlignMode = Native,  PackNumber = 1
> > 
> > b)
> > #pragma align(packed) ///AlignMode = Packed, PackNumber = 1
> > 
> > ```
> > Without this patch, a == b for other targets.
> > And I suppose a != b for AIX.
> > 
> In your first example, if I understand correctly,
> a) would return true for IsPackAttr()
> b) would return false for IsPackAttr()
> and yet a == b ?
> I think that's confusing. 
> 
> Any reason why you don't want to just compare all the data members to make sure they are all equal?
Yes, it's confusing but your understanding is correct. For other targets, they actually only use `PackNumber` to compare if two CurrentValue equal.


================
Comment at: clang/lib/Sema/SemaAttr.cpp:367
+  // AIX pragma pack does not support identifier syntax.
+  if (getLangOpts().AIXPragmaPack && !SlotLabel.empty()) {
+    Diag(PragmaLoc, diag::warn_pragma_pack_identifer_not_supported);
----------------
jasonliu wrote:
> Although IBM xl compiler does not support this form, do we see a harm for us to support this form in clang on AIX?
> Also, if this is indeed not desired to support, we could move this check to the top of this function for an early return. 
We may consider supporting this form in the future, but I don't think we need to cover it in this patch. And we don't support it by passing `StringRef()` instead, so we still need to wait for a `Info` constructed for us.


================
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);
----------------
jasonliu wrote:
> Xiangling_L wrote:
> > jasonliu wrote:
> > > 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.
> > Thanks for pointing this out. I agree that what we are doing here is not correct. 
> > The original commit[45b40147117668ce65bff4f6a240bdae4ad4bf7d] message shows:
> > 
> > ```
> >     This commit adds a new -Wpragma-pack warning. It warns in the following cases:
> >     
> >     - When a translation unit is missing terminating #pragma pack (pop) directives.
> >     - When entering an included file if the current alignment value as determined
> >       by '#pragma pack' directives is different from the default alignment value.
> >     - When leaving an included file that changed the state of the current alignment
> >       value.
> > ```
> > 
> > So it looks these warnings are used only for `pragma pack`.
> This might not be enough to make the diagnostic in `DiagnoseNonDefaultPragmaPack()` and  `DiagnoseUnterminatedPragmaPack()` work sensibly, when pragma align is involved. 
> One example being if the source contains:
> 
> `#pragma align = natural`
> 
> We would get:
> ```
> clang pragmapack.c -S 
> pragmapack.c:1:9: warning: unterminated '#pragma pack (push, ...)' at end of file [-Wpragma-pack]
> #pragma align = natural 
>         ^
> pragmapack.c:1:9: note: did you intend to use '#pragma pack (pop)' instead of '#pragma pack()'?
> 1 warning generated.
> 
> ```
> 
> But this seems like an existing bug, as without your change, I'm still seeing this "incorrect" message.
Thanks for putting this out. I am gonna put a `FIXME` for now and we may fix it in a later patch.


================
Comment at: clang/lib/Serialization/ASTWriter.cpp:4162
   RecordData Record;
-  Record.push_back(SemaRef.PackStack.CurrentValue);
+  Record.push_back(SemaRef.PackStack.CurrentValue.getPackNumber());
   AddSourceLocation(SemaRef.PackStack.CurrentPragmaLocation, Record);
----------------
jasonliu wrote:
> I think we would miss the serialization of pragma align mode, if we only record the pack number here. 
I will work on this. Thanks.


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

https://reviews.llvm.org/D87702