b_sanitize as an array, with compiler checks #12648
Conversation
docs/markdown/Builtin-options.md
Outdated
| † The default and possible values of sanitizers changed in 1.4. Before 1.4 they | ||
| were string values, and restricted to a specific subset of values: `none`, | ||
| `address`, `thread`, `undefined`, `memory`, `leak`, or `address,undefined`. In | ||
| 1.4 it was changed to a free form array of sanitizers, which are checked by a | ||
| compiler and linker check. For backwards compatibility reasons | ||
| `get_option('b_sanitize')` continues to return a string value, which is | ||
| guaranteed to match the old string value if the array value contains the same | ||
| values (ie, `['undefined', 'address'] == 'address,undefined`). I a value not | ||
| allowed before 1.4 is used `b_sanitize` will return a string in undefined order. | ||
| A new value of `b_sanitizers` can be used for >= 1.4 to get a free form array, | ||
| with no ordering guarantees. |
There was a problem hiding this comment.
Could we do something like if the meson_version defined by the project is >= 1.4.0, return an array, else return string?
There was a problem hiding this comment.
That's what I did initially and others were absolutely against that. Having done CMake, I can see the concern
There was a problem hiding this comment.
Any reason you didn't want to continue with the b_sanitizers option? I think I would rather just have a new option than an array option that returns a string.
There was a problem hiding this comment.
I would rather not add extra complexity of having mutually exclusive options if we don't need them, in this case b_sanitize already accepts the same format that b_sanitizers would return anyway. If you don't like the get_option('b_sanitizers'), perhaps we could come up with something different there? like a new keyword for get_option() like, get_option('b_sanitize', version : 2) or something like that?
There was a problem hiding this comment.
What is the CMake concern that you have in mind?
I don't think the version number is a good idea, it adds a new layer of opt-ins that is unnecessary when we already use the project's Meson version for deprecations.
I think b_sanitizers is a better name than b_sanitize, so a rename is fine if you want to kick the can a little bit further down the road (in the sense that I cannot think of any such new name for b_pie). However, the rename should be done contextually with deprecation of b_sanitize; -Db_sanitize should be automatically converted to -Db_sanitizers, with the old name surviving only as an argument to get_option().
With respect to the order, I would return the sanitizers in sorted order for get_option('b_sanitize'), since it's a simple generalization of the special case address,undefined.
There was a problem hiding this comment.
The problem with a minimum version requirement is that up until now, the minimum version requirement was guaranteed to have no effect other than warnings control.
And not all projects use them at all! Which is "fine", but users get no real visibility into what version of meson you need, so they'd better upgrade regardless of which version of meson is provided by apt install.
Changing runtime behavior based on this, means that:
- people who bump their minimim version proactively may suddenly have a crash, and worse, that crash may be in conditional code that they didn't hit in local testing
- people who don't specify a minimum version either crash or don't crash, depending on which version of meson you use. CMake policies "don't have that issue" because the policy is separate from version and you opt into each one you want, and if you don't then you're just out of luck because you were expected to run a debug execution of cmake that reports deprecation warnings, then update your code (but not third-party functions) and flip the toggle and pray no third-party functions break
In contrast, renaming the option is the conservatively safe approach and cannot break projects, no matter how unlikely. And leaving aside developer implementation challenges, it's very straightforward for users.
There was a problem hiding this comment.
I obviously like my get_option(name, version : ...) the most. I agree with Eli that there are some real gotchas to the version based approach. The keyword argument, while not perfect, gets the best of both approaches: It's explicit, it has 100% backwards compatibility, it's easy to implement, and it's easy to use, we don't end up with a proliferation of options. It would probably be a pretty easy feature for a linter to look for too.
The problem I have with renaming is it's simply not scalable. Pretty soon we'll end up with dozens of renamed options, sometimes with clunky names (b_pie_feature?). An end user then has to figure out that while they run meson setup -Db_pie=disabled that the have to in their script call get_option('b_pie_feature'). And then when we get to Meson 2.0, we have to decide what to do there. Do we pull a python and break everyone's build scripts in the short term, but have better long term maintainability (dict.iteritems() -> dict.items()), or do we remove the obvious name and get stuck with get_option('b_pie_feature') forever? Renaming is the option I'm actively against.
There was a problem hiding this comment.
And not all projects use them at all!
Should a minimum meson version produce a warning of its own? And then no version is equivalent to '>=1.3'.
It's explicit, it has 100% backwards compatibility, it's easy to implement, and it's easy to use,
I agree with everything except easy to use: the user still has to figure out that they write -Db_pie=disabled but they get false (and they can't distinguish auto from disabled) without adding the rare "version" keyword. It's not totally different from renaming in this respect, it's a better version of renaming but it shares some of the disadvantages.
There was a problem hiding this comment.
Should a minimum meson version produce a warning of its own? And then no version is equivalent to '>=1.3'.
I'm very confused how this is supposed to solve the problem.
All projects without minimum versions that try to use this option would then immediately crash, instead of only crashing at some randomized date in the future. The goal isn't to have a flag day where everyone has to update their meson.build, but have that flag day be more predictable... the goal is to not have a flag day where lots of projects break.
There was a problem hiding this comment.
No, that would be if the default was >=1.4, not 1.3.
|
The first 4 commits could probably just be pulled straight into master. |
Indeed they can, but I've only looked at 3 of them so far. Pushed as 0c3e84b...730cce0 |
|
Yeah, that's fine. The Rust part is necessary for this series, but I don't think would otherwise be used |
dcbaker
force-pushed
the
submit/sanitizers-array
branch
2 times, most recently
from
b9ab0bf to
78c5c45
Compare
|
@eli-schwartz, @bonzini: I'd like to try to summarize where we are, please correct me if I'm wrong. We have three proposals:
Eli and I have reservations about 1. Another one I thought of last night is that this approach will be troublesome for Muon, since they have to lie and claim full 1.4 support to get the new behavior. Paolo likes option 1 the best. I like option 3 the best, and am completely opposed to 2 Eli is opposed to 1 Does this all sound right? |
|
I have a slight preference for 2, in the sense that I'm not fundamentally opposed to 3 but if I were the one implementing it I'd gravitate towards implementing 2. But I think 3 is workable. With that in mind, yes, I think that's about right. |
|
What I don't like of If we go for it I really think we should bite the bullet and add a policy like mechanism. It could be enabled per invocation with a Changing the name is fine with me but |
Followup to 7b7d2e0 which handles ASAN and UBSAN. It turns out that MSAN needs the same treatment. I've checked other sanitizers like HWASAN and TSAN - it looks like they may both need it too, but Meson doesn't currently suppose those anyway (see mesonbuild#12648). (TODO: Should we just set those anyway pre-emptively?) Signed-off-by: Sam James <[email protected]>
Followup to 7b7d2e0 which handles ASAN and UBSAN. It turns out that MSAN needs the same treatment. I've checked other sanitizers like HWASAN and TSAN - it looks like they may both need it too, but Meson doesn't currently suppose those anyway (see mesonbuild#12648). (TODO: Should we just set those anyway pre-emptively?) (TODO: docs) Signed-off-by: Sam James <[email protected]>
Followup to 7b7d2e0 which handles ASAN and UBSAN. It turns out that MSAN needs the same treatment. I've checked other sanitizers like HWASAN and TSAN - it looks like they may both need it too, but Meson doesn't currently suppose those anyway (see mesonbuild#12648). Signed-off-by: Sam James <[email protected]>
Followup to 7b7d2e0 which handles ASAN and UBSAN. It turns out that MSAN needs the same treatment. I've checked other sanitizers like HWASAN and TSAN - it looks like they may both need it too, but Meson doesn't currently suppose those anyway (see mesonbuild#12648). Signed-off-by: Sam James <[email protected]>
Followup to 7b7d2e0 which handles ASAN and UBSAN. It turns out that MSAN needs the same treatment. I've checked other sanitizers like HWASAN and TSAN - it looks like they may both need it too, but Meson doesn't currently suppose those anyway (see mesonbuild#12648). Signed-off-by: Sam James <[email protected]> Signed-off-by: Eli Schwartz <[email protected]>
Followup to 7b7d2e0 which handles ASAN and UBSAN. It turns out that MSAN needs the same treatment. I've checked other sanitizers like HWASAN and TSAN - it looks like they may both need it too, but Meson doesn't currently suppose those anyway (see mesonbuild#12648). Signed-off-by: Sam James <[email protected]> Signed-off-by: Eli Schwartz <[email protected]>
dcbaker
force-pushed
the
submit/sanitizers-array
branch
2 times, most recently
from
e9a4599 to
927e022
Compare
dcbaker
force-pushed
the
submit/sanitizers-array
branch
2 times, most recently
from
b26b0f0 to
74b38b1
Compare
dcbaker
force-pushed
the
submit/sanitizers-array
branch
4 times, most recently
from
7554836 to
a6722ff
Compare
|
@obilaniu, any chance you could help me get the cuda stuff working? I tried to set up cuda locally, but I'm having a real hard time getting it working (something about nixos + not having nvidia drivers I think) |
dcbaker
force-pushed
the
submit/sanitizers-array
branch
from
a6722ff to
6f8e283
Compare
|
Here is the traceback for CUDA: |
There are a lot of sanitizers these days, and trying to keep up with
them would be entirely too much work. Instead we'll do a compile check
to see if the option is valid. An array makes more sense than a string,
since users could just write `address,undefined` or they could write
`undefined,address`. With the array we get to handle that ourselves and
users writing code know that it's not safe to simply do a check like
```meson
if get_option('b_sanitize') == 'address,undefined'
...
endif
```
instead they need to do something like
```meson
opt = get_option('b_sanitizers')
if opt.contains('address') and opt.contains('undefined')
...
endif
```
To ensure backwards compatibility, `get_option('b_sanitize')` is
guaranteed to return a string with the values in the same order, unless
a new value is added.
Fixes mesonbuild#8283
Fixes mesonbuild#7761
Fixes mesonbuild#5154
Fixes mesonbuild#1582
dcbaker
force-pushed
the
submit/sanitizers-array
branch
from
6f8e283 to
db75bd1
Compare
|
@dcbaker I've rebased this PR and fixed the CUDA issue via pks-t@b87317b. Please feel free to cherry-pick that commit to fix the CUDA test failure. |
The current state of Meson's sanitizer support is that we have a hardcoded set of of string values that can be chosen from. There's several problems with this:
To solve all of this I've moved to doing a compiler check for sanitizer arguments, rather than using a hardcoded list. This makes configuration slightly slower when using sanitizers, but much more robust.
Second, I've moved from a string argument to an array. Because we currently support either a single string or
address,undefinedwe can do this transparently on the command line, the value ofaddress,undefinedis a valid array argument, as are single strings. So the command line of-Db_sanitize=...this is a 100% backwards compatible change. This simplifies things considerably as we don't have to deal with deprecations and collisions. In the Meson DSL,get_option('b_sanitize')has retained its current behavior of returning a string, with a guarantee that and order ofaddress,undefinedwill always returnaddress,undefined. If there are new values the value will be','.join(opts)with no order guarantees. A newget_option('b_sanitizers')will return an array of sanitizers with no ordering guarantees. This keeps things relatively simple, but also provides backwards compatibility guarantees.