rjmccall added inline comments.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1975
+ bool AlignAttrCanDecreaseAlignment =
+ AlignIsRequired && (Ty->getAs<TypedefType>() != nullptr || FieldPacked);
+
----------------
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.
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
