[Pitch] @noSanitize attribute for functions

Evolution Pitches
1

In embedded environments, it can be useful to disable sanitizer instrumentation on individual functions. For example, ASAN may generate false positives or crash when accessing MMIO regions. In other cases, it may be desirable to disable instrumentation on particularly hot functions to improve the performance of sanitized builds.

I'd like to add a new attribute @noSanitize(<kind>) which can be applied to functions to disable instrumentation for a single sanitizer, similar to clang's __attribute__((no_sanitize("<kind>"))). This attribute would prevent Swift from adding the sanitize_address (or sanitize_thread, sanitize_memtag, etc.) attribute to a function when emitting LLVM IR.

@noSanitize(address)
func unsanitizedLoad(_ p: UnsafePointer<UInt32>) -> UInt32 { p.pointee }

Proposal

Implementation

9 Likes
2

Definitely seems worth having, and there's clear precedent for it.

Some random thoughts based on the proposal text:

  • The proposal suggests using multiple @noSanitize attributes to apply multiple suppressions; e.g., @noSanitize(address) @noSanitize(thread). What about taking a comma-delimited list of sanitizer names instead, as the Clang attribute does: @noSanitize(address, thread)?

  • coverage should also be supported for disabling, as Clang does.

  • The proposal and implementation list specific sanitizers that are supported. My read of the Clang version of this attribute is that it supports exactly the same list of sanitizers that can be specified in the -fsanitize=/-fno-sanitize= command line flag, plus "coverage". To avoid drift/future maintenance issues, should a Swift @noSanitize attribute simply also do the same? That is, instead of hardcoding a list, just parse the name as if it was the command line argument, and it's up to the relevant CodeGen pass to query the ones it cares about. Essentially, do the same preprocessor metaprogramming Clang does to generate a bitmask from Sanitizers.def to make this faster than repeated string comparisons.

  • Closures are left as a future direction. Is there a reason to not just support it now?

  • Likewise, variable initializations are interesting in Swift. Globals and static members are lazily initialized, so I wonder if we would need to support @noSanitize on those in order to make sure the entire initialization clause, including synthesized code, is (un)instrumented correctly.

    • Indeed, for completeness we might even need it for regular properties, because in something like struct S { var x = foo() }, you can ask for foo() to disable sanitizers, but the compiler would still instrument the variable initialization expression of S.x that wraps the call to foo().
3

  • The proposal suggests using multiple @noSanitize attributes to apply multiple suppressions; e.g., @noSanitize(address) @noSanitize(thread). What about taking a comma-delimited list of sanitizer names instead, as the Clang attribute does: @noSanitize(address, thread)?

  • coverage should also be supported for disabling, as Clang does

I think this makes sense.

  • The proposal and implementation list specific sanitizers that are supported. My read of the Clang version of this attribute is that it supports exactly the same list of sanitizers that can be specified in the -fsanitize=/-fno-sanitize= command line flag, plus "coverage". To avoid drift/future maintenance issues, should a Swift @noSanitizeattribute simply also do the same? That is, instead of hardcoding a list, just parse the name as if it was the command line argument, and it's up to the relevant CodeGen pass to query the ones it cares about. Essentially, do the same preprocessor metaprogramming Clang does to generate a bitmask from Sanitizers.def to make this faster than repeated string comparisons.

There are also lots of weird implications and unintuitive behaviors e.g. the fsanitize=fuzzer bit implies fsanitize=fuzzer-no-link (source). And no_sanitize("address") implies no_sanitize("kernel-address") (source). And no_sanitize("fuzzer") does nothing -- you usually instead want no_sanitize("coverage") (godbolt).

I feel that having @noSanitize correspond 1:1 with -sanitize options implies that noSanitize(x) "undos" all of the effects of -sanitize=x on an individual function -- but that's definitely not something we (or clang) can promise. We could go ahead and automatically support a @noSanitize kind for every -sanitize option, but I feel it will quickly become a burden if we care about defining how these various options interact.

  • Indeed, for completeness we might even need it for regular properties, because in something like struct S { var x = foo() }, you can ask for foo() to disable sanitizers, but the compiler would still instrument the variable initialization expression of S.x that wraps the call to foo().

I haven't thought a lot about this, but presumably we would need to distinguish between "please don't instrument this variable initialization" from "please don't put redzones around this variable" (which is how clang's no_sanitize("address") works for globals right now, I believe). I'd prefer to not have to resolve this question right now, and just support the attribute on functions.

1 Like
4

I can understand this point of view and I don't entirely disagree with it. To argue the other side, this is very much a power-user feature, so I worry less about someone stumbling on it and using it with an incorrect sanitizer name and not understanding why it doesn't work, especially since Clang is already in the same boat.

For features in that class, I kind of lean toward whatever makes future compiler implementation work easier to ensure that we don't accidentally drop something on the floor (like add a new kind of sanitizer but forget to support it in the attribute). But maybe that's because I've been bitten recently by features where a capability was added in one part of the compiler but not another, and it caused a mismatch in the support that the compiler claimed existed, so I want to be proactive about avoiding repeats of that.

So maybe there's a middle ground. What you've currently implemented is an allowlist, but what if we flipped it to a blocklist instead? You could still generate a bitmask for every known sanitizer, and just diagnose the ones that are known to not be supported by the attribute (or that don't exist at all). In that case, the failure mode is safer, because if a new sanitizer is added and woven through CodeGen:

  • With an allowlist (as currently proposed): If the sanitizer does not make sense to disable, no issue. If it could be disabled, but the compiler engineer forgets to add the attribute to @noSanitize, it becomes impossible to disable the new sanitizer for some block of code.
  • With a blocklist (my proposed alternative): If the sanitizer can be disabled, it will automatically be supported by @noSanitize. If the sanitizer does not make sense to disable, the worst that happens is the attribute silently accepts it and does nothing.

This is getting into implementation detail territory rather than feature design, but since the list of sanitizers was explicitly called out in the proposal document, it seems relevant here.

1 Like
5

Nice, this seems useful!

+1, so long as it is allowed to still write it separately, and let the compiler merge... I think that can be important for macro use cases?

Inlining a @noSanitize callee into a caller that is still being instrumented would silently re-instrument the callee's body, defeating the attribute. To preserve the guarantee, the SIL performance inliner refuses to inline a @noSanitize(<kind>) callee into a caller that does not carry the same @noSanitize(<kind>) when that sanitizer is enabled for the current build.

The restriction is one-directional: a regular (sanitized) callee may still be inlined into a @noSanitize caller (and thus may lose its instrumentation). Users who want a @noSanitize function to be inlined into ordinary sanitized code should either mark the caller with a matching @noSanitize or accept that the callee will remain an out-of-line call in sanitized builds. @inline(__always) does not override this restriction.

It seems to me we can do a bit better in this case, maybe one of these?

  • warn (as part of a diagnostic group) in callers of @inline(always) + @noSanitize when their sanitization options will prevent the optimization and offer a fix-it to propagate the sanitization option to the callers function, which programmers could elevate to an error with -warnings-as-error or suppress per call site with @diagnose
  • warn/error on functions that combine those attributes, if we think they are inappropriate to combine given these restrictions

@inlinable has different behavior, but we could consider warning when it is combined with @noSanatize since it makes it harder to inline? maybe this could be a disabled by default diagnostic group.

6

Oh and should the attribute be accepted on other top level declarations that may contain functions, propagating to their members lexically?

@noSanitize(address)
struct Device: ~Copyable {
  init() { ... }

  deinit { ... }

  var status: UInt32 {
    get { ... }
    set { ... }
  }

  subscript(i: Int) -> UInt8 { ... }
}

If so, it could be a good candidate for file level defaults support!

default @noSanitize(address)

struct Device: ~Copyable {
  init() { ... }

  deinit { ... }

  var status: UInt32 {
    get { ... }
    set { ... }
  }

  subscript(i: Int) -> UInt8 { ... }
}

struct Mouse ...

Note that supporting file-level defaults would ideally have a way to reenable sanitization, and would turn the globals future direction into a source break.