Page MenuHomePhabricator

[GWP-ASan] Configuration options [3].
ClosedPublic

Authored by hctim on May 30 2019, 1:29 PM.

Details

Summary

See D60593 for further information.

This patch introduces the configuration options for GWP-ASan. In general, we expect the supporting allocator to populate the options struct, and give that to GWP-ASan during initialisation. For allocators that are okay with pulling in sanitizer_common, we also provide an optional parser that populates the gwp_asan::Options struct with values provided in the GWP_ASAN_OPTIONS environment variable.

This patch contains very little logic, and all of the testable components (i.e. the optional parser's internal logic) is tested as part of the sanitizer_common testbed.

Diff Detail

Repository
rL LLVM

Event Timeline

hctim created this revision.May 30 2019, 1:29 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, Restricted Project, mgorny, kubamracek. · View Herald Transcript
morehouse added inline comments.May 31 2019, 3:02 PM
compiler-rt/lib/gwp_asan/optional/options_parser.cpp
39 ↗(On Diff #202284)

If the above functions are private, they can go in an unnamed namespace or be made static.

57 ↗(On Diff #202284)

What does this do? Do we need it?

60 ↗(On Diff #202284)

Should we return early? Maybe the user will later set Enabled = true. In that case, we skipped the below validation and Printf initialization.

89 ↗(On Diff #202284)

I think this is unnecessary since we check __gwp_asan_default_options != nullptr before calling it.

compiler-rt/lib/gwp_asan/options.inc
31 ↗(On Diff #202284)

I think 2^31 will overflow. So technically only 2^31 - 1 is supported.

42 ↗(On Diff #202284)

This wording is a bit confusing. There are a few uses of the phrase "previously installed handler", but they refer to different handlers in each case.

hctim marked 9 inline comments as done.May 31 2019, 3:45 PM
hctim added inline comments.
compiler-rt/lib/gwp_asan/optional/options_parser.cpp
57 ↗(On Diff #202284)

In this patchset, nothing. New patchest it initialises verbosity, which is used to report unrecognised flags.

60 ↗(On Diff #202284)

I don't think we should be targeting live-enable/disable of GWP-ASan at this point in time, only at start time, unless I misunderstand the premise of the question.

@vlad.tsyrklevich WDYT?

compiler-rt/lib/gwp_asan/options.inc
31 ↗(On Diff #202284)

Also added a sanity check in the optional parser.

42 ↗(On Diff #202284)

Changed it. WDYT?

hctim updated this revision to Diff 202500.May 31 2019, 3:45 PM
hctim marked 4 inline comments as done.
  • Merge branch 'master' into gwp_asan/options
  • Updated from Matt's comments.
morehouse added inline comments.May 31 2019, 3:59 PM
compiler-rt/lib/gwp_asan/optional/options_parser.cpp
81 ↗(On Diff #202500)

Hm.. So now 2^31 - 1 is no longer supported?

60 ↗(On Diff #202284)

In that case, why support modifying options at runtime at all?

getOptions should return a const-ref, not a pointer and all initialization should be done through initOptions.

compiler-rt/lib/gwp_asan/options.inc
38 ↗(On Diff #202500)

"will forward the signal to any previously-installed handler, and user"

39 ↗(On Diff #202500)

further signal

40 ↗(On Diff #202500)

sam. Note

compiler-rt/lib/gwp_asan/optional/options_parser.cpp
60 ↗(On Diff #202284)

Agree that live-enable/disable doesn't really make sense, and that we should change the interface to be a const ref instead.

hctim marked 7 inline comments as done.Jun 3 2019, 10:24 AM
hctim updated this revision to Diff 202751.Jun 3 2019, 10:24 AM
  • Matt's comments #3.
morehouse accepted this revision.Jun 4 2019, 9:21 AM
morehouse added inline comments.
compiler-rt/lib/gwp_asan/optional/options_parser.cpp
80 ↗(On Diff #202751)

Isn't it impossible for SampleRate to be larger than INT_MAX?

This revision is now accepted and ready to land.Jun 4 2019, 9:21 AM
hctim updated this revision to Diff 202966.Jun 4 2019, 9:57 AM
hctim marked an inline comment as done.
  • Matt's last round of comments.
  • Merge branch 'master' into gwp_asan/options
This revision was automatically updated to reflect the committed changes.