Page MenuHomePhabricator

[Frontend] Add pragma align natural and sort out pragma pack stack effect
Changes PlannedPublic

Authored by Xiangling_L on Sep 15 2020, 8:29 AM.

Details

Summary
  1. Implementing the natural align for AIX
  2. Sort out pragma pack stack effect
  3. Add -faix-pragma-stack option to enable AIX pragma stack effect

Related RFCs are here:
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058064.html

http://lists.llvm.org/pipermail/cfe-dev/2018-May/058065.html

Diff Detail

Event Timeline

Xiangling_L created this revision.Sep 15 2020, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 8:29 AM
Xiangling_L requested review of this revision.Sep 15 2020, 8:29 AM
Xiangling_L edited the summary of this revision. (Show Details)

Removed redundant header file include;

Fix the PragmaStack is empty assertion failure when we do: #pragma pack(2) then #pragma align(reset)

jasonliu added inline comments.Sep 18 2020, 1:56 PM
clang/include/clang/Sema/Sema.h
66

Do we need cmath?

487

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?

503

I don't see this function gets referenced in this patch.

513

I think a normal operator== signature would look like this:
bool operator==(const AlignPackInfo &Info) const;

514

This could return true when PackAttr in AlignPackInfo are not the same. Wouldn't that cause an issue?

518

You should be able to implement this using "operator==" right?
e.g. return !(*this == Info);

523
clang/lib/AST/RecordLayoutBuilder.cpp
697–702

This could probably confuse people, as natural alignment is the default on most target except AIX.

clang/lib/Sema/SemaAttr.cpp
403–404

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.

Xiangling_L marked 9 inline comments as done.Sep 23 2020, 1:36 PM
Xiangling_L added inline comments.
clang/include/clang/Sema/Sema.h
487

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?

514

(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.

clang/lib/Sema/SemaAttr.cpp
403–404

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.

Xiangling_L marked 3 inline comments as done.

Addressed the comments;

jasonliu added inline comments.Thu, Oct 8, 1:57 PM
clang/include/clang/Basic/LangOptions.def
340

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?

clang/include/clang/Sema/Sema.h
487

Using a char instead of an int could make this class more compact, especially when we actually don't need to represent a big range.
I understand there is a warning/error somewhere to prevent out of range from happening. But an assert here would make sure that no one could pass in an unexpected value into here by accident when developing.

493

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?

497

We could have a empty stack on AIX that returns false when it was asked if it's an AIX stack?
It's a bit confusing here.

514

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?

637

We should consider renaming PackStack to AlignPackStack across Clang. Maybe even as a NFC first. As it is right now, clang already uses one stack to record those two informations from Pragma align and Pragma pack. Leave it as PackStack is too confusing, and people could actually ignore the pragma Align when developing with PackStack.

clang/lib/Sema/SemaAttr.cpp
367

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.

403–404

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.

clang/lib/Serialization/ASTReader.cpp
7914

I think we need to do something more to handle the PragmaPackStack here.
It seems in ASTReader, we would always use CurrentValue.getAlignMode() for all the AlignPackInfo.
Does that mean we are actually not able to capture the align mode even when there is an align mode set for the current entry?

clang/lib/Serialization/ASTWriter.cpp
4162

I think we would miss the serialization of pragma align mode, if we only record the pack number here.

Xiangling_L marked 8 inline comments as done.Tue, Oct 13, 1:49 PM
Xiangling_L added inline comments.
clang/include/clang/Basic/LangOptions.def
340

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.

clang/include/clang/Sema/Sema.h
493

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.

514

Yes, it's confusing but your understanding is correct. For other targets, they actually only use PackNumber to compare if two CurrentValue equal.

clang/lib/Sema/SemaAttr.cpp
367

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.

403–404

Thanks for putting this out. I am gonna put a FIXME for now and we may fix it in a later patch.

clang/lib/Serialization/ASTWriter.cpp
4162

I will work on this. Thanks.

Xiangling_L planned changes to this revision.Tue, Oct 13, 1:49 PM
Xiangling_L marked 6 inline comments as done.