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

[Bill Wendling via Phabricator via cfe-commits]

void marked 7 inline comments as done.
void added inline comments.


================
Comment at: clang/include/clang/AST/Decl.h:4067
+
+  void setIsRandomized(bool V) const { RecordDeclBits.IsRandomized = V; }
+
----------------
aaron.ballman wrote:
> A setter that is marked `const` does not spark joy for me... Can we use friendship rather than mutability to solve this?
It was done to prevent a `const_cast`. It looks like the other setters are non-const. I'll see if I can get around that.


================
Comment at: clang/include/clang/AST/Randstruct.h:34
+
+extern std::string SEED;
+
----------------
aaron.ballman wrote:
> Doing this with a global variable is unfortunate; it could make things harder when we multithread the frontend. Can we solve this without a global? (And if not, does this global need to be a `ManagedStatic`?)
Maybe this could be moved to `LangOpts`?


================
Comment at: clang/include/clang/Basic/Attr.td:3938-3939
+def RandomizeLayout : InheritableAttr {
+  let Spellings = [GCC<"randomize_layout">, Declspec<"randomize_layout">,
+    Keyword<"randomize_layout">];
+  let Subjects = SubjectList<[Record]>;
----------------
aaron.ballman wrote:
> Microsoft does not implement this `__declspec`, correct? If they don't, we shouldn't either (even if GCC does) unless there's a *very* good reason to do so. That's Microsoft's design space we're encroaching on.
> 
> I'd also like to understand the justification for adding a new keyword for this.
Good point. I'll remove those.


================
Comment at: clang/lib/AST/DeclBase.cpp:28
 #include "clang/AST/ExternalASTSource.h"
+#include "clang/AST/Randstruct.h"
 #include "clang/AST/Stmt.h"
----------------
aaron.ballman wrote:
> Is this include necessary?
Hmm...that must have been from a previous change. Removed.


================
Comment at: clang/lib/AST/Randstruct.cpp:72
+  // All of the Buckets produced by best-effort cache-line algorithm.
+  // TODO: Figure out a better initial size.
+  SmallVector<std::unique_ptr<randstruct::Bucket>, 16> Buckets;
----------------
aaron.ballman wrote:
> This size seems as defensible as most others; do you plan to do more here, or should this comment be removed?
It's probably not worth looking into unless it becomes an issue. I'll remove it.


================
Comment at: clang/lib/AST/Randstruct.cpp:99
+
+    if (FD->isBitField() && !FD->isZeroLengthBitField(Context)) {
+      // Start a bitfield run if this is the first bitfield we have found.
----------------
aaron.ballman wrote:
> Oh cool, we do bit-field runs!
> 
> How should we handle anonymous bit-fields (if anything special should be done for them at all)? People usually use them for padding between bit-fields, so it's not clear to me how to treat them when randomizing.
Good question! I'm not sure either. On one hand, I'm rather concerned about changing bitfield order in general, but it appears to be something that GCC's plugin does, so...



================
Comment at: clang/lib/AST/Randstruct.cpp:179
+
+  SmallVector<FieldDecl *, 64> RandomizedFields;
+
----------------
aaron.ballman wrote:
> This one seems a bit large to me; any reason not to use `16` again?
The code above looks to apply mostly to bitfield runs. This is for all fields in a structure. I assumed (without proof) that this will always be larger than the bitfield runs. :-)


================
Comment at: clang/lib/Parse/ParseDeclCXX.cpp:2063-2069
+      if (RD->isRandomized()) {
+        for (Decl *D : FinalOrdering)
+          RD->removeDecl(D);
+
+        for (Decl *D : FinalOrdering)
+          RD->addDecl(D);
+      }
----------------
aaron.ballman wrote:
> I think this works okay for C, but I think if we ever attempted to provide this functionality in C++, the calls to `addDecl()` would be rather expensive because it eventually calls `addedMember()` which calculates information about the structure (whether it's copy constructible, has a trivial destructor, etc) and we don't need to redo any of that work.
> 
> However, for now, I think this is fine.
God help the person who does this for C++! :-)

Joking aside, supporting C++ will probably result in a almost total rewrite of this feature.


================
Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:2503
+
+  // FIXME: Any way to get a handle to a RecordDecl struct here?
+  clang::randstruct::checkForBadCasts(S, AC);
----------------
aaron.ballman wrote:
> No, this function is only called when popping a function scope, and so the only declaration it has access to is the `FunctionDecl`. Or do I misunderstand the question?
Sounds good to me. Is this the best place for this check then?


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121556