aaron.ballman added a comment.
Generally I think things are looking pretty good, but there's still an open
question and Precommit CI is still failing on Windows:
Failed Tests (7):
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.AnonymousStructsAndUnionsRetainFieldOrder
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.CheckAdjacentBitfieldsRemainAdjacentAfterRandomization
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.CheckVariableLengthArrayMemberRemainsAtEndOfStructure
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.MarkedRandomize
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.MultipleAttrsRandomize
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.RandstructDoesNotOverrideThePackedAttr
Clang-Unit ::
AST/./ASTTests.exe/StructureLayoutRandomization.ZeroWidthBitfieldsSeparateAllocationUnits
================
Comment at: clang/include/clang/Basic/Attr.td:3954-3968
+def RandomizeLayout : InheritableAttr {
+ let Spellings = [GCC<"randomize_layout">];
+ let Subjects = SubjectList<[Record]>;
+ let Documentation = [ClangRandstructDocs];
+ let LangOpts = [COnly];
+}
+
----------------
This gets rid of one of the AST nodes and uses an accessor instead. e.g., the
user can still write `[[gnu::no_randomize_layout]]` on a struct declaration,
we'll generate a `RandomizeLayoutAttr` for it, and you can call
`->isDisabled()` on an object to tell whether it's the `no_` spelling or not.
I don't feel super strong about this change, so if it turns out to make code
elsewhere significantly worse, feel free to ignore. My reasoning for combing is
that the `no` variant doesn't carry much semantic weight but eats up an
attribute kind value just the same; it seemed like a heavy use for an AST node.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8550-8556
+ case ParsedAttr::AT_RandomizeLayout:
+ handleSimpleAttribute<RandomizeLayoutAttr>(S, D, AL);
+ break;
+ case ParsedAttr::AT_NoRandomizeLayout:
+ // Drop the "randomize_layout" attribute if it's on the decl.
+ D->dropAttr<RandomizeLayoutAttr>();
+ break;
----------------
void wrote:
> aaron.ballman wrote:
> > I don't think this is sufficient. Consider redeclaration merging cases:
> > ```
> > struct [[gnu::randomize_layout]] S;
> > struct [[gnu::no_randomize_layout]] S {};
> >
> > struct [[gnu::no_randomize_layout]] T;
> > struct [[gnu::randomize_layout]] T {};
> > ```
> > I think if the user accidentally does something like this, it should be
> > diagnosed. I would have assumed this would warrant an error diagnostic
> > because the user is obviously confused as to what they want, but it seems
> > GCC ignores the subsequent diagnostic with a warning:
> > https://godbolt.org/z/1q8dazYPW.
> >
> > There's also the "confused attribute list" case which GCC just... does...
> > things... with: https://godbolt.org/z/rnfsn7hG1. I think we want to behave
> > more consistently with how we treat these cases.
> >
> > Either way, we shouldn't be silent.
> The GCC feature is done via a plugin in Linux. So godbolt probably won't work
> in this case. I'll check to see how GCC responds to these attribute
> situations.
Thoughts on this?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121556/new/
https://reviews.llvm.org/D121556
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
