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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
