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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Randstruct.h:31-35
+using llvm::SmallVectorImpl;
+
+bool randomizeStructureLayout(const ASTContext &Context, llvm::StringRef Name,
+                              llvm::ArrayRef<Decl *> Fields,
+                              SmallVectorImpl<Decl *> &FinalOrdering);
----------------
Might as well be consistent with the other type names.


================
Comment at: clang/include/clang/Driver/Options.td:2122-2133
+def frandomize_layout_seed_EQ
+    : Joined<["-"], "frandomize-layout-seed=">,
+      MetaVarName<"<seed>">,
+      Group<f_clang_Group>,
+      Flags<[CC1Option]>,
+      HelpText<"The seed used by the randomize structure layout feature">;
+def frandomize_layout_seed_file_EQ
----------------
You should condense these a bit to keep the style the same as the surrounding code.


================
Comment at: clang/lib/AST/Randstruct.cpp:71-75
+  std::unique_ptr<Bucket> CurrentBucket = nullptr;
+
+  // The current bucket containing the run of adjacent bitfields to ensure they
+  // remain adjacent.
+  std::unique_ptr<BitfieldRunBucket> CurrentBitfieldRun = nullptr;
----------------
Default ctor already does the right thing, so no need to do the initialization.


================
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:
> > 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?
> Okay, finally tested it with the GCC plugin. It doesn't produce a diagnostic. I'm not sure if that's the correct behavior, but at least it matches. How is such a thing handled with similar attributes?
> Okay, finally tested it with the GCC plugin. It doesn't produce a diagnostic. I'm not sure if that's the correct behavior, but at least it matches. How is such a thing handled with similar attributes?

As you might be unsurprised to learn: inconsistently. :-D However, we typically try to make attributes mutually exclusive (e.g., `hot` and `cold`, `global` and `device`, etc).

Unfortunately, I gave you some advice earlier to combine into one semantic attribute and that may have been less-than-awesome advice. Attr.td supports the ability to trivially define to attributes as mutually exclusive (e.g., `def : MutualExclusions<[Hot, Cold]>;` but this works on the *attribute* level and not the *spelling* level.

It's probably easier for you to split the definition back into two attributes in Attr.td and just use the `MutualExclusions` support we already have. Sorry for that churn! If you wanted to (and I certainly don't insist), another option would be to modify ClangAttrEmitter.cpp and Attr.td to support mutual exclusion syntax on the spelling level, so you could do something like:
```
def : MutualExclusions<[
  RandomizeLayout<GCC<"randomize_layout">>,
  RandomizeLayout<GCC<"no_randomize_layout">>
]>;
```
where you specify the attribute and the spelling, then you could leave `RandomizeLayout` as a single semantic spelling. But again, I don't insist.


================
Comment at: clang/unittests/AST/RandstructTest.cpp:154-158
+#ifdef _WIN32
+  const field_names Expected = {"lettuce", "bacon", "mayonnaise", "tomato"};
+#else
+  const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"};
+#endif
----------------
Any idea what's gone wrong here? (Do we have a bug to file because these come out reversed? If so, can you add a FIXME comment here that we expect this test to change someday?)


================
Comment at: clang/unittests/AST/RandstructTest.cpp:208-213
+// FIXME: Clang trips an assertion in the DiagnosticsEngine when the warning is
+// emitted while running under the test suite:
+// clang/lib/Frontend/TextDiagnosticPrinter.cpp:150: virtual void
+// clang::TextDiagnosticPrinter::HandleDiagnostic(clang::DiagnosticsEngine::Level,
+// const clang::Diagnostic&): Assertion `TextDiag && "UnExpected diagnostic
+// outside source file processing"' failed.
----------------
Is the assertion unrelated to the changes in your patch and you just happen to hit it with this test? (I get worried when tests trigger assertions.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121556/new/

https://reviews.llvm.org/D121556