This is an archive of the discontinued LLVM Phabricator instance.

[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.

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
40

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

58

What does this do? Do we need it?

61

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

90

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

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

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

43

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
58

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

61

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
32

Also added a sanity check in the optional parser.

43

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
61

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.

82

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

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

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

40

further signal

41

sam. Note

compiler-rt/lib/gwp_asan/optional/options_parser.cpp
61

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

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.