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

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

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

shirley breuer via cfe-dev

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


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Sort out and correct pragma align/pack stack effect

shirley breuer via cfe-dev
ping.

On Fri, Nov 27, 2020 at 3:09 PM Xiangling Liao <[hidden email]> 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


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Sort out and correct pragma align/pack stack effect

shirley breuer via cfe-dev
ping.

On Tue, Dec 1, 2020 at 10:16 AM Xiangling Liao <[hidden email]> wrote:
ping.

On Fri, Nov 27, 2020 at 3:09 PM Xiangling Liao <[hidden email]> 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


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev