stevewan added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1975
+ bool AlignAttrCanDecreaseAlignment =
+ AlignIsRequired && (Ty->getAs<TypedefType>() != nullptr || FieldPacked);
+
----------------
rjmccall wrote:
> Okay, so first off, the comment and variable names here make this sound very
> general when in fact this computation is only relevant to the AIX alignment
> upgrade logic. Also, please do not introduce new variables with broad scope
> named things like `Ty` that are actually referring to a very specific type
> and not, say, the unadulterated type of the field.
>
> It seems to me that the right way to think about this is that, while the AIX
> power alignment upgrade considers whether field type alignment is "required",
> it uses a different condition than the standard rule when making that
> decision. It's important to understand what that rule is exactly, and we
> can't see that from the current test cases. In particular, attributes on
> fields that define structs inline actually end up applying to both the field
> and the struct, which confounds unrelated issues. Consider:
>
> ```
> struct __attribute__((packed, aligned(2))) SS {
> double d;
> };
>
> struct S {
> // Neither the field nor the struct are packed, but the field type is.
> // Do we apply the alignment upgrade to S or not?
> struct SS s;
> };
> ```
>
> Regardless, I don't think this check for a typedef works; it's bypassing
> quite a bit of recursive logic for computing whether type alignment is
> required. For example, you could have an explicitly aligned typedef of an
> array type, and you'll lose that typedef here.
Thanks for the review.
> the comment and variable names here make this sound very general when in fact
> this computation is only relevant to the AIX alignment upgrade logic.
Moved the variable declarations back to the AIX alignment upgrade logic.
> It's important to understand what that rule is exactly, and we can't see
> that from the current test cases.
Updated the test cases accordingly.
> For example, you could have an explicitly aligned typedef of an array type,
> and you'll lose that typedef here.
Updated the check to examine the immediate type rather than the base element
type, this should take care of the array-type considerations.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107394/new/
https://reviews.llvm.org/D107394
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
