[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/Sema/Sema.h:488
+    AlignPackInfo(AlignPackInfo::Mode M, int Num, bool IsAIX)
+        : PackAttr(true), AlignMode(M), PackNumber(Num), AIXStack(IsAIX) {}
+
----------------
jasonliu wrote:
> 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?
I think the warning/error `expected #pragma pack parameter to be '1', '2', '4', '8', or '16'` have already guaranteed that for us? Or maybe using `unsigned int` makes people more comfortable?


================
Comment at: clang/include/clang/Sema/Sema.h:515
+    bool operator==(AlignPackInfo Info) const {
+      return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+    }
----------------
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.



================
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:
> 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`.


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

https://reviews.llvm.org/D87702