[PATCH] D107394: [AIX] "aligned" attribute does not decrease alignment

[Steven Wan via Phabricator via cfe-commits]

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