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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Apr 6 11:27:47 PDT 2022

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<[
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"};
+  const field_names Expected = {"mayonnaise", "bacon", "tomato", "lettuce"};
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.)

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list