Page MenuHomePhabricator

[GWP-ASan] Respect GWP_ASAN_DEFAULT_ENABLED compile-time macro
ClosedPublic

Authored by mcgrathr on Nov 13 2020, 2:38 PM.

Details

Summary

If the containing allocator build uses -DGWP_ASAN_DEFAULT_ENABLED=false
then the option will default to false. For e.g. Scudo, this is simpler
and more efficient than using -DSCUDO_DEFAULT_OPTIONS=... to set gwp-asan
options that have to be parsed from the string at startup.

Diff Detail

Event Timeline

mcgrathr created this revision.Nov 13 2020, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 13 2020, 2:38 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
mcgrathr requested review of this revision.Nov 13 2020, 2:38 PM
hctim added a comment.Nov 13 2020, 4:35 PM

I'm not sure adding this complexity here is warranted, why is -DGWP_ASAN_DEFAULT_ENABLED=false better than -DSCUDO_DEFAULT_OPTIONS=GWP_ASAN_Enabled=false?

This does have some niceties that it patches up the text description, but nothing else in Scudo does that if you use -DSCUDO_DEFAULT_OPTIONS, and the inconsistency might be misleading.

I'm not sure adding this complexity here is warranted, why is -DGWP_ASAN_DEFAULT_ENABLED=false better than -DSCUDO_DEFAULT_OPTIONS=GWP_ASAN_Enabled=false?

The complexity of one optional macro with a Boolean value seems quite minimal to me. The benefit is having the value simply linker-initialized rather than having every process on every device repeat the same runtime work at startup to parse a string and poke a variable (possibly causing an otherwise unnecessary COW page).

This does have some niceties that it patches up the text description, but nothing else in Scudo does that if you use -DSCUDO_DEFAULT_OPTIONS, and the inconsistency might be misleading.

I don't particularly care about the description text, it just seemed easy to do. If you'd prefer to leave the description text fixed (and thus inconsistent with non-default defaults but consistent with other means of changing the defaults) that's fine with me.

Thanks for the review!

hctim accepted this revision.Nov 17 2020, 2:11 PM

I'm not sure adding this complexity here is warranted, why is -DGWP_ASAN_DEFAULT_ENABLED=false better than -DSCUDO_DEFAULT_OPTIONS=GWP_ASAN_Enabled=false?

The complexity of one optional macro with a Boolean value seems quite minimal to me. The benefit is having the value simply linker-initialized rather than having every process on every device repeat the same runtime work at startup to parse a string and poke a variable (possibly causing an otherwise unnecessary COW page).

Sure, this sounds like a good enough reason to me.

compiler-rt/lib/gwp_asan/options.inc
16

Nit: Remove -DGWP_ASAN_DEFAULT_ENABLED_STRING and use a compiler-rt-like stringify:

#define GWP_ASAN_STRINGIFY_(S) #S
#define GWP_ASAN_STRINGIFY(S) GWP_ASAN_STRINGIFY_(S)

#ifndef GWP_ASAN_DEFAULT_ENABLED
#define GWP_ASAN_DEFAULT_ENABLED true
#endif
GWP_ASAN_OPTION(bool, Enabled, GWP_ASAN_DEFAULT_ENABLED,
                "Is GWP-ASan enabled? Defaults to " GWP_ASAN_STRINGIFY(
                    GWP_ASAN_DEFAULT_ENABLED) ".")
This revision is now accepted and ready to land.Nov 17 2020, 2:11 PM
mcgrathr updated this revision to Diff 306157.Nov 18 2020, 10:33 AM
mcgrathr marked an inline comment as done.

macro nit

updated macro

This revision was landed with ongoing or failed builds.Nov 18 2020, 10:34 AM
This revision was automatically updated to reflect the committed changes.