Page MenuHomePhabricator

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

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonliu added inline comments.Sep 18 2020, 1:56 PM
clang/include/clang/Sema/Sema.h
483

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?

clang/lib/AST/RecordLayoutBuilder.cpp
702

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

clang/lib/Sema/SemaAttr.cpp
411

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
483

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?

510

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

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.Oct 8 2020, 1:57 PM
clang/include/clang/Basic/LangOptions.def
357

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
483

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.

489

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?

493

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.

510

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?

669–670

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
376

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.

411

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
7896

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
4161–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.Oct 13 2020, 1:49 PM
Xiangling_L added inline comments.
clang/include/clang/Basic/LangOptions.def
357

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
489

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.

510

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
376

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.

411

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
4161–4162

I will work on this. Thanks.

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

Addressed the comments;
Handle PCH for AIX;

Xiangling_L added inline comments.Nov 25 2020, 7:51 AM
clang/include/clang/Sema/Sema.h
669–670

That's a good point. I will create a NFC accordingly.

Update the logic to tell if two AlignPackInfo are equal;

Adjusted the logic of operator == for class AlignPackInfo;
Removed mac68k sentinel;

jasonliu added inline comments.Dec 22 2020, 8:37 AM
clang/include/clang/Sema/Sema.h
501

this getter do not need to take any parameters. You could just use *this.
Or, to make it consistent with the getFromRawEncoding, we could make this a static function as well and keep the parameter.

502

Since this new structure have the same size as the uint32_t, have you consider using memcpy to burn the data directly into the uint32_t, and vice versa?

566

It might be better to rename it XLStack, as this is the XL compiler behavior on AIX. Other compilers on AIX, like gcc, could have different behavior.

669–670

Please post this NFC when you have time.

clang/lib/AST/RecordLayoutBuilder.cpp
699–700
1319–1320

Not sure if this would have a real effect, but reading this code concerns me a little bit:
We would set HandledFirstNonOverlappingEmptyField based on IsNaturalAlign, but for AIX, that bit might only get set at line 1343.
It would look like we have a read before write to the variable here.

clang/lib/Sema/SemaAttr.cpp
231

XLPragmaPack is used to control pack and align stack semantic behavior.
Someone on AIX might want to turn off this compatible pack and align with XL behavior (to get the compatible behavior with clang on other platform for easier porting purpose), but they would still prefer to have the natural alignment to work correctly.
So it might not be desirable to query this option for this natural alignment behavior.

412

I don't think there is a preferable solution yet. Pointing out what could go wrong is good enough here.
Same for the FIXME below.

Xiangling_L marked 2 inline comments as done.Dec 29 2020, 7:43 AM
Xiangling_L added inline comments.
clang/include/clang/Sema/Sema.h
669–670

Thanks for reminding me of this. A NFC patch is posted here: https://reviews.llvm.org/D93901

Xiangling_L marked an inline comment as done.

Rebase on renaming NFC patch

Xiangling_L marked 6 inline comments as done.
  • used memcpy to burn AlignPackInfo into uint32_t and vice versa;
  • named XL on AIX stack effect and related options after "xl" instead of "aix"
  • adjusted natural align related code
Xiangling_L edited the summary of this revision. (Show Details)Dec 30 2020, 12:39 PM
aaron.ballman added inline comments.Jan 4 2021, 11:40 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
858
893

If the user wrote an identifier, it seems like there's a strong chance that ignoring the identifier will result in incorrect behavior that would lead to hard-to-find ODR issues. Should this scenario be an error rather than a warning where the identifier is ignored?

clang/include/clang/Sema/Sema.h
486

The string literal here doesn't really convey what's being asserted -- it took me a while to figure out that this is trying to catch truncation issues when Num cannot be represented by an unsigned char.

494
512

I would feel more comfortable with this assertion if the class was using bit-fields to pack the values together. As it stands, we're kind of hoping that bool, Mode, and unsigned char will all pack in a particular way (and bool's representation is implementation-defined).

527

Given that the ctor takes an int for this value, should this be returning an int?

clang/lib/AST/RecordLayoutBuilder.cpp
1314

Unintended whitespace change?

clang/lib/Sema/SemaAttr.cpp
69–71
clang/test/Sema/aix-pragma-pack-and-align.c
232

Is this comment intentional?

Xiangling_L marked 9 inline comments as done.Jan 6 2021, 1:30 PM
Xiangling_L added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
893

Could you show a more concrete example or give more details on how possible incorrect behavior would lead to hard-to-find ODR issues?

clang/include/clang/Sema/Sema.h
486

Sorry for that, I would make it clearer.

512

Yeah, good point. I will move it back to previous bitfields version.

527

As we know the pack number would not exceed 16 bytes, so the intention of using unsigned char here is to save space taken by AlignPackInfo. And that's why I added the diagnostics and assertion in the ctor to make sure the value won't be truncated.

clang/test/Sema/aix-pragma-pack-and-align.c
232

Yes, my intention is to test no pragma pack or prama align is unterminated.

Xiangling_L marked 5 inline comments as done.

Addressed the comments;
Rebased the patch on latest master;

aaron.ballman added inline comments.Jan 8 2021, 6:12 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
893

I'm not super familiar with this feature, so what I'm thinking of may not be a real issue in practice. The case I was thinking of is where the user does #pragma pack(push, r1, 2) to push a packing by name followed by #pragma pack(push, r2, 4), and the later pops come in the reverse order (#pragma pack(pop, r1) followed by #pragma pack(pop, r2)). The first pop will actually pop the r2 packing and not the r1 packing, which could cause the field layout to be different from what the user expected.

clang/include/clang/Sema/Sema.h
512

Oh, that's different from what I was expecting -- I was thinking you'd change the data member fields of the class to be bit-fields.

Either way works for me, though, so no change necessary unless you prefer that approach.

524–525

No else after a return.

527

We don't save any space with this function signature though -- the return value will most likely wind up being implicitly promoted to int anyway. I think the fact that we store it as a smaller datatype internally is an implementation detail and as far as the consumer of this class is concerned, the pack number is an integer type where we don't care about the size. tbh, because the pack numbers are always positive, I'd have the ctor and this function both deal with an unsigned rather than an int.

(Note, I'm not talking about the type of the underlying field -- I think it's fine to continue to use a smaller datatype there for space savings. I'm only talking about the types in the public interface.)

Xiangling_L marked 4 inline comments as done.

Rebased on latest master;
Addressed the comments;

aaron.ballman accepted this revision.Jan 12 2021, 5:17 AM

LGTM aside from a few small issues to fix.

clang/include/clang/Basic/DiagnosticSemaKinds.td
893

The identifier is no longer ignored now that this is an error rather than a warning.

clang/test/Sema/aix-pragma-pack-and-align.c
232

I don't think we have such a construct that's checked by -verify (and if we did, I'd say the test file is broken because it contains expected-warning comments).

Because you're passing -verify, any warnings that are triggered on a line without a matching expected-warning comment will be reported as a test failure, so you can safely remove this comment and still get the test coverage you want.

This revision is now accepted and ready to land.Jan 12 2021, 5:17 AM
jasonliu accepted this revision.Jan 12 2021, 4:01 PM

LGTM with minor nits.

clang/lib/Sema/SemaAttr.cpp
75
352–353

nit

376

Could we move this to the top of the function so that we could have a quick exit when we sees this?
If we are going to emit an error, the AlignPackStack.Act(PragmaLoc, Action, StringRef(), Info); is not necessary any more.

552
578
579
This revision was automatically updated to reflect the committed changes.
Xiangling_L marked 8 inline comments as done.