Page MenuHomePhabricator

[SanitizeCoverage] Add support for NoSanitizeCoverage function attribute
ClosedPublic

Authored by melver on May 19 2021, 6:45 AM.

Details

Summary

We really ought to support no_sanitize("coverage") in line with other
sanitizers. This came up again in discussions on the Linux-kernel
mailing lists, because we currently do workarounds using objtool to
remove coverage instrumentation. Since that support is only on x86, to
continue support coverage instrumentation on other architectures, we
must support selectively disabling coverage instrumentation via function
attributes.

Unfortunately, for SanitizeCoverage, it has not been implemented as a
sanitizer via fsanitize= and associated options in Sanitizers.def, but
rolls its own option fsanitize-coverage. This meant that we never got
"automatic" no_sanitize attribute support.

Implement no_sanitize attribute support by special-casing the string
"coverage" in the NoSanitizeAttr implementation. To keep the feature as
unintrusive to existing IR generation as possible, define a new negative
function attribute NoSanitizeCoverage to propagate the information
through to the instrumentation pass.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=49035

Diff Detail

Event Timeline

melver created this revision.May 19 2021, 6:45 AM
melver requested review of this revision.May 19 2021, 6:45 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 19 2021, 6:45 AM
aaron.ballman added inline comments.May 19 2021, 7:07 AM
clang/docs/SanitizerCoverage.rst
319

I would expect to see this documentation also in AttrDocs.td within the NoSanitizeDocs.

clang/include/clang/Basic/Attr.td
2902–2906
melver updated this revision to Diff 346452.May 19 2021, 7:30 AM
melver marked 2 inline comments as done.

Address Aaron's comments

melver updated this revision to Diff 346484.May 19 2021, 9:19 AM

Test that always_inline works as expected with no_sanitize("coverage")

The clang parts look good to me, but someone else should do the final sign-off.

clang/lib/Sema/SemaDeclAttr.cpp
7292–7295

The formatting looks rather off here to my eyes, but if this is what clang-format produces for you, then I'm fine with it.

melver added inline comments.May 19 2021, 10:22 AM
clang/lib/Sema/SemaDeclAttr.cpp
7292–7295

Yeah, I had something else here, but then clang-format suggested this and I don't like it either.

Could remove the /*AllowGroups=*/ bit, for the sake of formatting better?

aaron.ballman added inline comments.May 19 2021, 10:23 AM
clang/lib/Sema/SemaDeclAttr.cpp
7292–7295

I'd rather have the comment + ugly formatting than pretty formatting + mysterious boolean literal, so I'd say let's just leave it as-is.

melver marked 2 inline comments as done.May 19 2021, 10:43 AM
melver updated this revision to Diff 346577.May 19 2021, 2:53 PM

Rest of long-tail of IR changes related to introducing a new attributes...

melver added a reviewer: pcc.May 19 2021, 2:54 PM
This revision is now accepted and ready to land.May 20 2021, 3:08 PM
vitalybuka accepted this revision.May 20 2021, 5:05 PM
vitalybuka added inline comments.
clang/lib/CodeGen/CodeGenFunction.cpp
757–760

consider adding method into CodeGenOptions and replace here and and the rest
ideally in a separate patch

clang/test/CodeGen/sanitize-coverage.c
23–87

you are adding tests for unrelated code.
Could you please land is as a separate NFC patch first?

llvm/lib/AsmParser/LLLexer.cpp
674

looking at the rest, seems "nosanitize_coverage" follows pattern better :)

melver added inline comments.May 21 2021, 7:25 AM
clang/test/CodeGen/sanitize-coverage.c
23–87

Which bits in particular? Just the

static inline __attribute__((__always_inline__)) void always_inlined_fn(int n) {
  if (n)
    x[n] = 42;
}
// CHECK-LABEL: define dso_local void @test_always_inline(
void test_always_inline(int n) {
  // CHECK-DAG: call void @__sanitizer_cov_trace_pc
  // CHECK-DAG: call void @__sanitizer_cov_trace_const_cmp
  always_inlined_fn(n);
}

part?

melver updated this revision to Diff 347062.May 21 2021, 9:22 AM
melver marked 3 inline comments as done.
  • Refactor test and SanitizeCoverage* checking.
  • s/no_sanitize_coverage/nosanitize_coverage/

Thanks!

melver added inline comments.May 21 2021, 9:23 AM
clang/lib/CodeGen/CodeGenFunction.cpp
757–760
clang/test/CodeGen/sanitize-coverage.c
23–87
llvm/lib/AsmParser/LLLexer.cpp
674

I wasn't sure myself, it's a bit inconsistent with "no_sanitize", but I've changed it. ;-)

Please have a look at the other 2 patches which are now a dependency for this. Once you're happy with all 3 I'll push them all together.
Thanks!

This revision was landed with ongoing or failed builds.May 25 2021, 4:00 AM
This revision was automatically updated to reflect the committed changes.