[cfe-dev] [RFC] Sort out and correct pragma align/pack stack effect

[Xiangling Liao via cfe-dev]

ping.

On Fri, Nov 27, 2020 at 3:09 PM Xiangling Liao <xiangxdh at gmail.com> wrote:

> Hi,
>
>
>
> Recently we are trying to implement AIX pragma align/pack stack effect,
> but noticed current implementation model in Clang has some limitations.
>
>
>
> That results from mixing `#pragma pack` and `#pragma align` by using only an
> unsigned number to represent both so that they could override each other.
>
>
>
> Some examples we have been aware of are:
>
>
>
> 1. The current diagnostic for pragma pack does not produce a valid
> diagnostic when align pragma is involved. For example,
>
>
>
> *$ cat test_diag.c*
>
> *#pragma align = natural *
>
>
>
> *$ clang test_diag.c -S *
>
>
>
> *test_diag.c:1:9: warning: unterminated '#pragma pack (push, ...)' at end
> of file [-Wpragma-pack]*
>
> *#pragma align = natural *
>
> *        ^*
>
> *test_diag.c:1:9: note: did you intend to use '#pragma pack (pop)' instead
> of '#pragma pack()'?*
>
> *1 warning generated.*
>
>
>
> The diagnostic is misleading by indicating some `*'#pragma pack *` is
> unterminated. This is caused by not differentiating between align pragma
> and pack pragma in clang’s current align and pack stack model.
>
>
>
>
>
> 2*.*There exists platforms like AIX that cannot have pragma align/pack
> stack effect simply represented by an alignment value. The stack effect
> between align and pack pragmas on AIX is different.
>
>
>
> (1)They don’t override but create combinations with each other sometimes.
>
>
>
> Eg.
>
> #pragma align(natural)
>
> #pragma pack(2)
>
>
>
> In current Clang’s implementation we get pack(2) in effect here without
> knowing align mode information above it;
>
>
>
> However, XL on AIX would need to align objects to natural alignment first and then reduced by pack(2).
>
>
>
> (2)Additional complication comes in with `#pragma align(reset)` and `#pragma pack(pop)`.
>
>
>
> Eg.
>
> #pragma pack(2)
>
> #pragma align = packed
>
> #pragma pack(pop)
>
>
>
> In current Clang’s implementation we get pack(2) in effect here.
>
>
>
> However, for XL on AIX, `#pragma pack(pop)` won’t be able to pop the stack, because `#pragma align` sets a baseline, and there is no pragma pack applied to it. Without distinguishing pragma align and pramgma pack, the `#pragma align = packed` would be popped out mistakenly for AIX.
>
>
>
> *3.* Features added in later like PCH only record pragma alignment
> number. In platforms like AIX, baseline align mode information will be lost.
>
>
>
>
> Based on current limitations, we are proposing a new model by splitting up
> the alignment value into an align mode and a pack number so that we would
> be able to know what is the combined effect of align and pack in our stack.
>
>
>
> The proposed patch has been posted here as a reference:
>
> https://reviews.llvm.org/D87702
>
>
>
> This patch implements the proposed new model,  implements AIX pragma
> align/stack effect based on that, and updates pragma align/pack related PCH
> handling process.
>
>
>
> But there are several things I would like to point out particularly:
>
>
>
> (1) In `clang/include/clang/Sema/Sema.h`,
>
>     bool operator==(const AlignPackInfo &Info) const {
>
>       return std::tie(AlignMode, PackNumber, PackAttr, AIXStack) ==
>
>              std::tie(Info.AlignMode, Info.PackNumber, Info.PackAttr,
>
>                       Info.AIXStack);
>
> }
>
>
>
> We are trying to pursue a stricter “equal” definition of if two pragma are
> equal. That would change some previous behavior, for example:
>
>
>
> *a) *
>
> *#pragma align(packed)*
>
> *#pragma pack(1) // AlignMode = Packed,PackNumber = 1,IsPackAttr*
>
>
>
> *b)*
>
> *#pragma align(packed) //AlignMode = Packed,PackNumber = 1,  *
>
> *                        IsAlignAttr*
>
>
>
>
>
> Since we only care about unsigned numbers previously, these two pragma
> settings would be treated as equal.
>
>
>
> But now, they would not. So on platforms where the previous result is
> required, we would need to compare the `PackNumber` of `AlignPackInfo`
> objects instead of comparing themselves directly.
>
>
>
> (2) In the first diagnostic limitation mentioned above, we handle it by
> leaving a FIXME in the current patch in order to avoid scope creep. But we
> believe the new `AlignPackInfo` struct provides a good foundation to
> address this issue in the future.
>
>
>
> Please let me know if there are any questions about our current proposal
> and design. Your feedback is appreciated.
>
>
>
> Regards,
>
> Xiangling Liao
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20201201/dda4c9ec/attachment-0001.html>