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

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.


================
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;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556