[PATCH] D58723: [scudo][standalone] Add bytemap classes

[Matt Morehouse via Phabricator via llvm-commits]

morehouse accepted this revision.
morehouse added inline comments.


================
Comment at: lib/scudo/standalone/bytemap.h:22
+    Map = reinterpret_cast<u8 *>(map(nullptr, Size, "scudo:bytemap"));
+  }
+  void init() { initLinkerInitialized(); }
----------------
cryptoad wrote:
> morehouse wrote:
> > morehouse wrote:
> > > morehouse wrote:
> > > > cryptoad wrote:
> > > > > morehouse wrote:
> > > > > > Having two init functions is a little confusing.  Would it make more sense to have a constexpr constructor and a single init() function?
> > > > > With this I followed the way it was done in sanitizer_common with an init function that would skip the zero initialization if not needed (most our structures are either mapped or static). Would you have an example of what you are suggesting, I am not sure I fully understand how to organize it?
> > > > Essentially, make the zero-initialization explicit in the `constexpr` constructor, and then do all dynamic initialization in `init`.
> > > > 
> > > > ```
> > > > class FlatByteMap {
> > > >   constexpr FlatByteMap() : Map(nullptr) {}
> > > >   void init() {
> > > >     Map = reinterpret_cast<u8 *>(map(nullptr, Size, "scudo:bytemap"));
> > > >   }
> > > > };
> > > > 
> > > > class TwoLevelByteMap {
> > > >   constexpr TwoLevelByteMap() : Level1Map(nullptr), Mutex{} {}
> > > >   void init() {
> > > >     Level1Map = reinterpret_cast<atomic_uptr *>(
> > > >         map(nullptr, sizeof(atomic_uptr) * Level1Size, "scudo:bytemap"));
> > > >   }
> > > > };
> > > > ```
> > > > 
> > > > The `constexpr` constructor will initialize any statics / globals at compile time while still working at runtime for non-static instantiations.  Then the call to `init` completes any dynamic initialization required.
> > > The linker-init function will work fine, but since we're reimplementing much of sanitizer_common for Scudo now, maybe it's a good opportunity to modernize a bit.
> > You'll probably also need to refactor `StaticSpinMutex` to use a `constexpr` constructor for initialization.  In fact, we could probably completely remove `StaticSpinMutex` and just give `SpinMutex` a `constexpr` constructor.
> > 
> > ```
> > class SpinMutex {
> >   constexpr SpinMutex() : State{} {}
> > };
> > ```
> > 
> > Then `SpinMutex` can be used as either global or local.
> I went into the rabbit hole of changing those and started hitting issues along the way (with __thread).
> If that's Ok with you I'd like to land this in it's current shape and revisit that later.
SGTM


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58723