[PATCH] D59254: [RFC] Implementation of Clang randstruct

[JF Bastien via Phabricator via cfe-commits]

jfb requested changes to this revision.
jfb added a comment.
This revision now requires changes to proceed.

I find it easier to understand the code by looking at the tests. When you add tests, please make sure you test for:

- Alignment attribute
- Packed attribute
- No unique address attribute
- Bit-fields
- Zero-width bit-field
- Zero or unsized array at end of struct
- C++ inheritance with vtables
- C++ virtual inheritance
- Anonymous unions (and the anonymous struct extension)
- Types with common initial sequence <http://eel.is/c++draft/class.mem#22>

I assume you only change field location, not their constructor's call order? i.e. `class S { std::string a, b; };` always constructs `a` before `b`, but might reorder them.

There are cases where a developer could observe the difference in field order. For example, `bit_cast` or `memcpy` of a type which got reordered. What are the gotchas? Can you diagnose them?



================
Comment at: clang/include/clang/AST/RandstructSeed.h:1
+#ifndef RANDSTRUCTSEED_H
+#define RANDSTRUCTSEED_H
----------------
This file needs the standard license comment.


================
Comment at: clang/include/clang/AST/RandstructSeed.h:6
+extern std::string RandstructSeed;
+extern bool RandstructAutoSelect;
+}
----------------
Returning a string is weird, and so it `extern` stuff. Is randomness a property of something in particular? Seems like `Module` or something similar might be the right place.


================
Comment at: clang/include/clang/AST/RecordFieldReorganizer.h:54
+  std::seed_seq Seq;
+  std::default_random_engine rng;
+};
----------------
vlad.tsyrklevich wrote:
> timpugh wrote:
> > connorkuehl wrote:
> > > pcc wrote:
> > > > I don't think we can use `default_random_engine` for this because the behaviour would need to be consistent between C++ standard library implementations, and the behaviour of `default_random_engine` is implementation defined. Similarly, I don't think that we can use `std::shuffle` (see note in https://en.cppreference.com/w/cpp/algorithm/random_shuffle ).
> > > Sure thing, we'll begin investigating alternatives.
> > @pcc  if we were to swap `default_random_engine` for a pre-defined random generator such as `mt19937_64` would this suffice? It is included in the random number library.
> > 
> > https://en.cppreference.com/w/cpp/numeric/random
> In that case the random numbers would be deterministic; however, std::shuffle would still vary by implementation (as mentioned in the note Peter linked to.) A quick search didn't reveal a deterministic shuffle in the LLVM code so you may have to implement one.
`Module::createRNG` was created to support randomization of things. You should look at the discussion that led to its addition: in parts it tries to offer stable output when it can, and the idea was that you could inject a seed if you wanted to.

Unfortunately the other randomization patches weren't committed, but they're also on Phabricator.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4161
+    char *c;
+  }__attribute__((randomize_layout)) __attribute__((no_randomize_layout));
+}];
----------------
I'm not sure you need to document what happens when both are present. If people do it the message they get should be sufficient.


================
Comment at: clang/lib/AST/RecordFieldReorganizer.cpp:95
+// FIXME: Is there a way to detect this? (i.e. on 32bit system vs 64?)
+const size_t CACHE_LINE = 64;
+
----------------
https://en.cppreference.com/w/cpp/thread/hardware_destructive_interference_size will provide this. I have an RFC on how to implement it, haven't gotten to it. Just leave a FIXME. 64 is the right value, we all know ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59254