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