[PATCH] D121556: [randstruct] Add randomize structure layout support

[Aaron Ballman via Phabricator via cfe-commits]

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