b_sanitize as an array, with compiler checks #12648

dcbaker · 2023-12-20T18:26:47Z

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:

some of those values don't work on some platforms, or specific versions of platforms, or on specific compilers on specific platforms. This makes it more than a simple "GCC has X, MSVC does not" issue. We really need to do a compiler/linker check
We don't support all of the possible values that a compiler might, and updates to Meson are required to udpate the list of supported values

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,undefined we can do this transparently on the command line, the value of address,undefined is 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 of address,undefined will always return address,undefined. If there are new values the value will be ','.join(opts) with no order guarantees. A new get_option('b_sanitizers') will return an array of sanitizers with no ordering guarantees. This keeps things relatively simple, but also provides backwards compatibility guarantees.

tristan957 · 2023-12-20T19:08:45Z

docs/markdown/Builtin-options.md

+† 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.


Could we do something like if the meson_version defined by the project is >= 1.4.0, return an array, else return string?

That's what I did initially and others were absolutely against that. Having done CMake, I can see the concern

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.

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?

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.

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.

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.

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.

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.

No, that would be if the default was >=1.4, not 1.3.

docs/markdown/snippets/b_sanitizer_changes.md

tristan957 · 2023-12-20T19:09:43Z

The first 4 commits could probably just be pulled straight into master.

eli-schwartz · 2023-12-20T19:20:31Z

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

dcbaker · 2023-12-20T19:22:05Z

Yeah, that's fine. The Rust part is necessary for this series, but I don't think would otherwise be used

dcbaker · 2024-01-11T17:03:41Z

@eli-schwartz, @bonzini: I'd like to try to summarize where we are, please correct me if I'm wrong. We have three proposals:

use the version to change the behavior
add new options for the new behavior
add a version keyword argument to get_option to change the behavior

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?

eli-schwartz · 2024-01-11T18:10:17Z

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.

bonzini · 2024-01-11T18:42:26Z

What I don't like of version: 2 is that it's very opaque and honestly I don't think it can ever go away. There won't be a Meson 2.0 in the foreseeable future IMO.

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 compat_policy: { 'b_sanitize_array': true } or with an add_compat_policy().

Changing the name is fine with me but b_pie_feature is bad. Maybe b_link_pie is acceptable?

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 <sam@gentoo.org>

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 <sam@gentoo.org>

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 <sam@gentoo.org>

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 <sam@gentoo.org> Signed-off-by: Eli Schwartz <eschwartz93@gmail.com>

mesonbuild/compilers/rust.py

mesonbuild/backend/vs2010backend.py

docs/markdown/Builtin-options.md

docs/markdown/snippets/b_sanitizer_changes.md

test cases/linuxlike/16 sanitizers/meson.build

dcbaker · 2024-09-20T20:49:52Z

@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)

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

tristan957 · 2024-09-23T16:29:37Z

Here is the traceback for CUDA:

Traceback (most recent call last):
  File "/__w/meson/meson/run_tests.py", line 306, in run_configure_inprocess
    returncode = mesonmain.run(commandlist, get_meson_script())
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/__w/meson/meson/mesonbuild/mesonmain.py", line 282, in run
    return CommandLineParser().run(args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/__w/meson/meson/mesonbuild/mesonmain.py", line 193, in run
    return errorhandler(e, command)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/__w/meson/meson/mesonbuild/mesonmain.py", line 34, in errorhandler
    raise e
  File "/__w/meson/meson/mesonbuild/mesonmain.py", line 191, in run
    return options.run_func(options)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/__w/meson/meson/mesonbuild/msetup.py", line 365, in run
    app.generate()
  File "/__w/meson/meson/mesonbuild/msetup.py", line 188, in generate
    return self._generate(env, capture, vslite_ctx)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/__w/meson/meson/mesonbuild/msetup.py", line 253, in _generate
    captured_compile_args = intr.backend.generate(capture, vslite_ctx)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/__w/meson/meson/mesonbuild/backend/ninjabackend.py", line 656, in generate
    self.generate_target(t)
  File "/__w/meson/meson/mesonbuild/backend/ninjabackend.py", line 1079, in generate_target
    elem = self.generate_link(target, outname, final_obj_list, linker, pch_objects, stdlib_args=stdlib_args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/__w/meson/meson/mesonbuild/backend/ninjabackend.py", line 3445, in generate_link
    commands += compilers.get_base_link_args(target.get_options(),
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/__w/meson/meson/mesonbuild/compilers/compilers.py", line 369, in get_base_link_args
    raise MesonException(f'Linker {linker.name_string()} does not support sanitizer arguments {sani_args}')
mesonbuild.utils.core.MesonException: Linker nvcc does not support sanitizer arguments ['-Xcompiler=-fsanitize=address\\,undefined']

dcbaker requested a review from jpakkane as a code owner December 20, 2023 18:26

tristan957 reviewed Dec 20, 2023

View reviewed changes

dcbaker force-pushed the submit/sanitizers-array branch 2 times, most recently from b9ab0bf to 78c5c45 Compare December 20, 2023 22:30

dcbaker added this to the 1.4.0 milestone Jan 8, 2024

thesamesam mentioned this pull request Feb 13, 2024

mtest: set MSAN_OPTIONS to abort by default #12852

Merged

dcbaker mentioned this pull request Feb 20, 2024

abseil: add 'enable_fuzzer' option mesonbuild/wrapdb#1412

Closed

dcbaker mentioned this pull request Apr 1, 2024

[Feature Request]: Add built-in option(s) to simplify hardening binaries #13024

Open

dcbaker force-pushed the submit/sanitizers-array branch 2 times, most recently from e9a4599 to 927e022 Compare September 18, 2024 23:01

eli-schwartz reviewed Sep 18, 2024

View reviewed changes

dcbaker force-pushed the submit/sanitizers-array branch from e002833 to b26b0f0 Compare September 19, 2024 01:13

compilers/rust: implement has_argument checks

1ab6058

dcbaker force-pushed the submit/sanitizers-array branch from b26b0f0 to 74b38b1 Compare September 19, 2024 01:44

bruchar1 removed this from the 1.4.0 milestone Sep 19, 2024

dcbaker force-pushed the submit/sanitizers-array branch 3 times, most recently from f13cc0f to 7554836 Compare September 19, 2024 17:19

dcbaker force-pushed the submit/sanitizers-array branch from 7554836 to a6722ff Compare September 20, 2024 16:56

dcbaker added 3 commits September 20, 2024 15:16

compilers/cuda: implement has_argument checks

a9edb6c

compilers: check for sanitizer arguments via a compiler check

017b996

dcbaker force-pushed the submit/sanitizers-array branch from a6722ff to 6f8e283 Compare September 20, 2024 22:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

b_sanitize as an array, with compiler checks #12648

b_sanitize as an array, with compiler checks #12648

dcbaker commented Dec 20, 2023

tristan957 Dec 20, 2023

dcbaker Dec 20, 2023

tristan957 Dec 20, 2023

dcbaker Dec 20, 2023

bonzini Dec 21, 2023

eli-schwartz Jan 9, 2024

dcbaker Jan 9, 2024 •

edited

Loading

bonzini Jan 9, 2024

eli-schwartz Jan 9, 2024

bonzini Jan 9, 2024

tristan957 commented Dec 20, 2023

eli-schwartz commented Dec 20, 2023

dcbaker commented Dec 20, 2023

dcbaker commented Jan 11, 2024

eli-schwartz commented Jan 11, 2024

bonzini commented Jan 11, 2024

dcbaker commented Sep 20, 2024 •

edited

Loading

tristan957 commented Sep 23, 2024

b_sanitize as an array, with compiler checks #12648

Are you sure you want to change the base?

b_sanitize as an array, with compiler checks #12648

Conversation

dcbaker commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristan957 commented Dec 20, 2023

eli-schwartz commented Dec 20, 2023

dcbaker commented Dec 20, 2023

dcbaker commented Jan 11, 2024

eli-schwartz commented Jan 11, 2024

bonzini commented Jan 11, 2024

dcbaker commented Sep 20, 2024 • edited Loading

tristan957 commented Sep 23, 2024

dcbaker Jan 9, 2024 •

edited

Loading

dcbaker commented Sep 20, 2024 •

edited

Loading