Xiangling_L marked 8 inline comments as done.
Xiangling_L added inline comments.
================
Comment at: clang/include/clang/Basic/LangOptions.def:340
+LANGOPT(AIXPragmaPack, 1, 0, "AIX #pragma pack handling")
+
----------------
jasonliu wrote:
> 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`?
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.
================
Comment at: clang/include/clang/Sema/Sema.h:493
+ PackNumber(M == Packed ? 1
+ : (M == Mac68k ? Mac68kAlignmentSentinel
+ : UninitPackVal)),
----------------
jasonliu wrote:
> 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`?
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.
================
Comment at: clang/include/clang/Sema/Sema.h:515
+ bool operator==(AlignPackInfo Info) const {
+ return AlignMode == Info.AlignMode && PackNumber == Info.PackNumber;
+ }
----------------
jasonliu wrote:
> Xiangling_L wrote:
> > 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.
> >
> 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?
Yes, it's confusing but your understanding is correct. For other targets, they
actually only use `PackNumber` to compare if two CurrentValue equal.
================
Comment at: clang/lib/Sema/SemaAttr.cpp:367
+ // AIX pragma pack does not support identifier syntax.
+ if (getLangOpts().AIXPragmaPack && !SlotLabel.empty()) {
+ Diag(PragmaLoc, diag::warn_pragma_pack_identifer_not_supported);
----------------
jasonliu wrote:
> 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.
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.
================
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:
> Xiangling_L wrote:
> > 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`.
> 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.
Thanks for putting this out. I am gonna put a `FIXME` for now and we may fix it
in a later patch.
================
Comment at: clang/lib/Serialization/ASTWriter.cpp:4162
RecordData Record;
- Record.push_back(SemaRef.PackStack.CurrentValue);
+ Record.push_back(SemaRef.PackStack.CurrentValue.getPackNumber());
AddSourceLocation(SemaRef.PackStack.CurrentPragmaLocation, Record);
----------------
jasonliu wrote:
> I think we would miss the serialization of pragma align mode, if we only
> record the pack number here.
I will work on this. Thanks.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87702/new/
https://reviews.llvm.org/D87702
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
