This is an archive of the discontinued LLVM Phabricator instance.

[scudo] Use require_constant_initialization
ClosedPublic

Authored by vitalybuka on Apr 29 2021, 1:51 AM.

Details

Diff Detail

Event Timeline

vitalybuka created this revision.Apr 29 2021, 1:51 AM
vitalybuka requested review of this revision.Apr 29 2021, 1:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2021, 1:51 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
Harbormaster completed remote builds in B101582: Diff 341438.

Is this related to the GWP-ASan initialization issues?
I am unclear if we need to rework the init() vs initLinkInitialized() functions as well?

compiler-rt/lib/scudo/standalone/mutex.h
53

{} for Fuchsia as well?

cryptoad added inline comments.Apr 29 2021, 8:16 AM
compiler-rt/lib/scudo/standalone/primary64.h
293–294

This will probably have to be initialized as well for Fuchsia since it's not empty.

304

Same here.

vitalybuka marked 3 inline comments as done.Apr 29 2021, 9:52 AM

Is this related to the GWP-ASan initialization issues?

Yes. I tried to figure out rules for static initialization and looks like there is no way to be sure for such complex class without attribute or constinit of c++20.

I am unclear if we need to rework the init() vs initLinkInitialized() functions as well?

It's probably unrelated to this patch.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 29 2021, 9:52 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
vitalybuka reopened this revision.Apr 29 2021, 9:55 AM

Landed by mistake, reverting.

vitalybuka updated this revision to Diff 341558.EditedApr 29 2021, 9:57 AM

addressed @cryptoad comments

I probably missed something for android or Fuchsia, but diagnostics messages make it pretty clear what needs to be initialized and it should be easy to fix during integrations.

hctim added inline comments.Apr 29 2021, 3:40 PM
compiler-rt/lib/scudo/standalone/internal_defs.h
54

Judging by https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86974, looks like this isn't supported on gcc (and I see us bumping to c++2a for constinit) :(. Ah well, I guess we only get the diagnostic on clang.

Looking into it further, looks like gcc/clang-O0 don't elide the GWP-ASan dynamic initializer because the parent Scudo allocator is default-initialized, and the GWP-ASan initializer is value-initialized with nonzero values. This patch effectively makes the Scudo allocator's implicit construct constexpr by initializing all its members, and triggers static constant initialization of the Scudo and GWP-ASan allocators.

It looks like https://reviews.llvm.org/D87478 removed the blocker (from https://reviews.llvm.org/D69265#inline-624315) to making Scudo's allocator constexpr, so now this is possible :).

I wrote a comment for posterity that would be good to include here, as it hopefully helps with debugging:

// This check is only available on Clang. This is essentialy an alias of C++20's
// 'constinit' specifier which will take care of this when (if?) we can ask all
// libc's that use Scudo to compile us with C++20. Dynamic initialization is
// bad; Scudo is designed to be lazy-initializated on the first call to
// malloc/free (and friends), and this generally happens in the loader somewhere
// in libdl's init. After the loader is done, control is transferred to libc's
// initialization, and the dynamic initializers are run. If there's a dynamic
// initializer for Scudo, then it will clobber the already-initialized Scudo,
// and re-initialize all its members back to default values, causing various
// explosions. Unfortunately, marking scudo::Allocator<>'s constructor as
// 'constexpr' isn't sufficient to prevent dynamic initialization, as default
// initialization is fine under 'constexpr' (but not 'constinit'). Clang at -O0,
// and gcc at all opt levels will emit a dynamic initializer for any
// constant-initialized variables if there is a mix of default-initialized and
// constant-initialized variables.
//
// If you're looking at this because your build failed, you probably introduced
// a new member to scudo::Allocator<> (possibly transiently) that didn't have an
// initializer. The fix is easy - just add one.

While constexpr != constinit, it might be helpful to explicitly define the default constructor for scudo::Allocator<> as constexpr (constexpr Allocator() {}) as well.

vitalybuka added inline comments.Apr 29 2021, 4:56 PM
compiler-rt/lib/scudo/standalone/internal_defs.h
54

Judging by https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86974, looks like this isn't supported on gcc (and I see us bumping to c++2a for constinit) :(. Ah well, I guess we only get the diagnostic on clang.

I guess that's fine. At least on clang if I remove attribute it still makes it const initialized.

Looking into it further, looks like gcc/clang-O0 don't elide the GWP-ASan dynamic initializer because the parent Scudo allocator is default-initialized, and the GWP-ASan initializer is value-initialized with nonzero values. This patch effectively makes the Scudo allocator's implicit construct constexpr by initializing all its members, and triggers static constant initialization of the Scudo and GWP-ASan allocators.

constexpr is not enough https://godbolt.org/z/s51zfv9sE

vitalybuka added inline comments.Apr 29 2021, 4:59 PM
compiler-rt/lib/scudo/standalone/internal_defs.h
54

As part of this patch I had all touched classes as constexpr, but as it's not relevant I reverted them.

hctim added inline comments.Apr 29 2021, 5:07 PM
compiler-rt/lib/scudo/standalone/internal_defs.h
54

I guess that's fine. At least on clang if I remove attribute it still makes it const initialized.

Sorry for any confusion, I mean to say the attribute is good as a defensive layer of compile-time checking and we should definitely keep it, even if it's just for clang.

constexpr is not enough https://godbolt.org/z/s51zfv9sE

Yep, from the comment:

// Unfortunately, marking scudo::Allocator<>'s constructor as
// 'constexpr' isn't sufficient to prevent dynamic initialization, as default
// initialization is fine under 'constexpr' (but not 'constinit')

Although I should have written "it makes the implicit constructor constexpr (this doesn't necessarily fix the problem), and also having no more non-default-initialized members elides the dynamic initializer".

constexpr on the c'tor just gives us a little bit of compile-time diagnostics on gcc (which the attribute is significantly better but clang-only).

vitalybuka added inline comments.Apr 29 2021, 5:08 PM
compiler-rt/lib/scudo/standalone/internal_defs.h
54

according standard "The constexpr specifier declares that it is *possible* to evaluate the value of the function or variable at compile time.", but with -O0 compiler does not bother.

Even O1
O1: https://godbolt.org/z/ov6ecoEqn
O2: https://godbolt.org/z/je4fff4bh

Add comment explaining SCUDO_REQUIRE_CONSTANT_INITIALIZATION

vitalybuka marked an inline comment as done.Apr 29 2021, 5:16 PM
vitalybuka added inline comments.
compiler-rt/lib/scudo/standalone/internal_defs.h
54

Now I see, you are proposing comment :)
I'll add it.

vitalybuka marked an inline comment as done.Apr 30 2021, 1:55 PM

ping

hctim accepted this revision.Apr 30 2021, 1:56 PM

LGTM! Thank you!

This revision is now accepted and ready to land.Apr 30 2021, 1:56 PM
This revision was landed with ongoing or failed builds.May 1 2021, 1:47 AM
This revision was automatically updated to reflect the committed changes.