On Thu, 20 May 2021 at 11:08, Martin Liška <mli...@suse.cz> wrote: > On 5/20/21 10:45 AM, Marco Elver wrote: > > On Thu, 20 May 2021 at 10:33, Martin Liška <mli...@suse.cz> wrote: > >> Hello. > >> > >> The patch implements one missing attribute which can be used for > >> per-function > >> disabling of coverage sanitization. > >> > >> Patch can bootstrap on x86_64-linux-gnu and survives regression tests. > > > > Thanks for implementing this so quickly. One thing I just have to > > double check, is if this works with always_inline (not instrumented if > > inlined into no_sanitize), > > No, in this case it's instrumented (if caller has not set the attribute).
This one I'm not sure of, because in too many places, we assume always_inline behaves like a macro (see below). > > and inline (not inlined if inline fn is > > no_sanitize_coverage). > > No again, it's not blocking inlining. I think that's fine, as long as the inlined code is still instrumented according to the attribute (which I think is the case based on what you say above). I think this came up with other no_sanitize [1] based on what I had written to you last year [2]. [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-June/547618.html [2] https://lore.kernel.org/lkml/canpmjnnrz5ovkb6pe7k6gjfogbht_zhypkng9ad+kjndzk7...@mail.gmail.com/ > > Just like the other no_sanitize* do. The test doesn't mention them. > > It's aligned with similar issue that was discussed some time ago: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94722#c9 That's fair, but keep in mind all -fsanitize are not for use in production, unlike stack protector. no_sanitize_coverage needs to be aligned with the behavior of other existing no_sanitize*. Of course, whether to inline or not I don't mind, as long as no_sanitize means the code is never instrumented as discussed in [2]. AFAIK, it just so happens that to achieve that for the other sanitizers, the easiest option was to not inline "inline no_sanitize*" functions. But always_inline needs to behave in line with other no_sanitize*, i.e. if inlined into no_sanitize_coverage, never instrument: https://lore.kernel.org/lkml/CANpmjNMTsY_8241bS7=xafqvzhflrvekv_um4aduwe_kh3r...@mail.gmail.com/ This is also what Clang will do for no_sanitize_coverage. Thanks, -- Marco > Martin > > > > > Thanks, > > -- Marco > > > >> Ready to be installed? > >> Thanks, > >> Martin > >> > >> gcc/ChangeLog: > >> > >> * asan.h (sanitize_coverage_p): New function. > >> * doc/extend.texi: Document it. > >> * fold-const.c (fold_range_test): Use sanitize_flags_p > >> instead of flag_sanitize_coverage. > >> (fold_truth_andor): Likewise. > >> * sancov.c: Likewise. > >> * tree-ssa-ifcombine.c (ifcombine_ifandif): Likewise. > >> > >> gcc/c-family/ChangeLog: > >> > >> * c-attribs.c (handle_no_sanitize_coverage_attribute): New. > >> > >> gcc/testsuite/ChangeLog: > >> > >> * gcc.dg/sancov/attribute.c: New test. > >> --- > >> gcc/asan.h | 10 ++++++++++ > >> gcc/c-family/c-attribs.c | 20 ++++++++++++++++++++ > >> gcc/doc/extend.texi | 6 ++++++ > >> gcc/fold-const.c | 4 ++-- > >> gcc/sancov.c | 4 ++-- > >> gcc/testsuite/gcc.dg/sancov/attribute.c | 15 +++++++++++++++ > >> gcc/tree-ssa-ifcombine.c | 4 +++- > >> 7 files changed, 58 insertions(+), 5 deletions(-) > >> create mode 100644 gcc/testsuite/gcc.dg/sancov/attribute.c > >> > >> diff --git a/gcc/asan.h b/gcc/asan.h > >> index f110f1db563..8c0b2baf170 100644 > >> --- a/gcc/asan.h > >> +++ b/gcc/asan.h > >> @@ -249,4 +249,14 @@ sanitize_flags_p (unsigned int flag, const_tree fn = > >> current_function_decl) > >> return result_flags; > >> } > >> > >> +/* Return true when coverage sanitization should happend for FN function. > >> */ > >> + > >> +static inline bool > >> +sanitize_coverage_p (const_tree fn = current_function_decl) > >> +{ > >> + return (flag_sanitize_coverage > >> + && lookup_attribute ("no_sanitize_coverage", > >> + DECL_ATTRIBUTES (fn)) == NULL_TREE); > >> +} > >> + > >> #endif /* TREE_ASAN */ > >> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > >> index ccf9e4ccf0b..671b27c3200 100644 > >> --- a/gcc/c-family/c-attribs.c > >> +++ b/gcc/c-family/c-attribs.c > >> @@ -62,6 +62,8 @@ static tree handle_no_address_safety_analysis_attribute > >> (tree *, tree, tree, > >> int, bool *); > >> static tree handle_no_sanitize_undefined_attribute (tree *, tree, tree, > >> int, > >> bool *); > >> +static tree handle_no_sanitize_coverage_attribute (tree *, tree, tree, > >> int, > >> + bool *); > >> static tree handle_asan_odr_indicator_attribute (tree *, tree, tree, > >> int, > >> bool *); > >> static tree handle_stack_protect_attribute (tree *, tree, tree, int, > >> bool *); > >> @@ -449,6 +451,8 @@ const struct attribute_spec c_common_attribute_table[] > >> = > >> handle_no_sanitize_thread_attribute, NULL }, > >> { "no_sanitize_undefined", 0, 0, true, false, false, false, > >> handle_no_sanitize_undefined_attribute, > >> NULL }, > >> + { "no_sanitize_coverage", 0, 0, true, false, false, false, > >> + handle_no_sanitize_coverage_attribute, NULL > >> }, > >> { "asan odr indicator", 0, 0, true, false, false, false, > >> handle_asan_odr_indicator_attribute, NULL }, > >> { "warning", 1, 1, true, false, false, false, > >> @@ -1211,6 +1215,22 @@ handle_no_sanitize_undefined_attribute (tree *node, > >> tree name, tree, int, > >> return NULL_TREE; > >> } > >> > >> +/* Handle a "no_sanitize_coverage" attribute; arguments as in > >> + struct attribute_spec.handler. */ > >> + > >> +static tree > >> +handle_no_sanitize_coverage_attribute (tree *node, tree name, tree, int, > >> + bool *no_add_attrs) > >> +{ > >> + if (TREE_CODE (*node) != FUNCTION_DECL) > >> + { > >> + warning (OPT_Wattributes, "%qE attribute ignored", name); > >> + *no_add_attrs = true; > >> + } > >> + > >> + return NULL_TREE; > >> +} > >> + > >> /* Handle an "asan odr indicator" attribute; arguments as in > >> struct attribute_spec.handler. */ > >> > >> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi > >> index 826804e6149..3ddeb0dee3a 100644 > >> --- a/gcc/doc/extend.texi > >> +++ b/gcc/doc/extend.texi > >> @@ -3415,6 +3415,12 @@ The @code{no_sanitize_undefined} attribute on > >> functions is used > >> to inform the compiler that it should not check for undefined behavior > >> in the function when compiling with the @option{-fsanitize=undefined} > >> option. > >> > >> +@item no_sanitize_coverage > >> +@cindex @code{no_sanitize_coverage} function attribute > >> +The @code{no_sanitize_coverage} attribute on functions is used > >> +to inform the compiler that it should not do coverage-guided > >> +fuzzing code instrumentation (@option{-fsanitize-coverage}). > >> + > >> @item no_split_stack > >> @cindex @code{no_split_stack} function attribute > >> @opindex fsplit-stack > >> diff --git a/gcc/fold-const.c b/gcc/fold-const.c > >> index 3be9c15e6b2..d088187a057 100644 > >> --- a/gcc/fold-const.c > >> +++ b/gcc/fold-const.c > >> @@ -6016,7 +6016,7 @@ fold_range_test (location_t loc, enum tree_code > >> code, tree type, > >> logical_op_non_short_circuit > >> = param_logical_op_non_short_circuit; > >> if (logical_op_non_short_circuit > >> - && !flag_sanitize_coverage > >> + && !sanitize_coverage_p () > >> && lhs != 0 && rhs != 0 > >> && (code == TRUTH_ANDIF_EXPR || code == TRUTH_ORIF_EXPR) > >> && operand_equal_p (lhs, rhs, 0)) > >> @@ -9652,7 +9652,7 @@ fold_truth_andor (location_t loc, enum tree_code > >> code, tree type, > >> logical_op_non_short_circuit > >> = param_logical_op_non_short_circuit; > >> if (logical_op_non_short_circuit > >> - && !flag_sanitize_coverage > >> + && !sanitize_coverage_p () > >> && (code == TRUTH_AND_EXPR > >> || code == TRUTH_ANDIF_EXPR > >> || code == TRUTH_OR_EXPR > >> diff --git a/gcc/sancov.c b/gcc/sancov.c > >> index d656c37cae9..9cfbd425def 100644 > >> --- a/gcc/sancov.c > >> +++ b/gcc/sancov.c > >> @@ -313,9 +313,9 @@ public: > >> return new pass_sancov<O0> (m_ctxt); > >> } > >> virtual bool > >> - gate (function *) > >> + gate (function *fun) > >> { > >> - return flag_sanitize_coverage && (!O0 || !optimize); > >> + return sanitize_coverage_p (fun->decl) && (!O0 || !optimize); > >> } > >> virtual unsigned int > >> execute (function *fun) > >> diff --git a/gcc/testsuite/gcc.dg/sancov/attribute.c > >> b/gcc/testsuite/gcc.dg/sancov/attribute.c > >> new file mode 100644 > >> index 00000000000..bf6dbd4bae7 > >> --- /dev/null > >> +++ b/gcc/testsuite/gcc.dg/sancov/attribute.c > >> @@ -0,0 +1,15 @@ > >> +/* { dg-do compile } */ > >> +/* { dg-options "-fsanitize-coverage=trace-pc -fdump-tree-optimized" } */ > >> + > >> +void foo(void) > >> +{ > >> +} > >> + > >> +void > >> +__attribute__((no_sanitize_coverage)) > >> +bar(void) > >> +{ > >> +} > >> + > >> + > >> +/* { dg-final { scan-tree-dump-times "__builtin___sanitizer_cov_trace_pc > >> \\(\\)" 1 "optimized" } } */ > >> diff --git a/gcc/tree-ssa-ifcombine.c b/gcc/tree-ssa-ifcombine.c > >> index 836a12d6b80..f93e04aa4df 100644 > >> --- a/gcc/tree-ssa-ifcombine.c > >> +++ b/gcc/tree-ssa-ifcombine.c > >> @@ -40,6 +40,8 @@ along with GCC; see the file COPYING3. If not see > >> #include "gimplify-me.h" > >> #include "tree-cfg.h" > >> #include "tree-ssa.h" > >> +#include "attribs.h" > >> +#include "asan.h" > >> > >> #ifndef LOGICAL_OP_NON_SHORT_CIRCUIT > >> #define LOGICAL_OP_NON_SHORT_CIRCUIT \ > >> @@ -567,7 +569,7 @@ ifcombine_ifandif (basic_block inner_cond_bb, bool > >> inner_inv, > >> if (param_logical_op_non_short_circuit != -1) > >> logical_op_non_short_circuit > >> = param_logical_op_non_short_circuit; > >> - if (!logical_op_non_short_circuit || flag_sanitize_coverage) > >> + if (!logical_op_non_short_circuit || sanitize_coverage_p ()) > >> return false; > >> /* Only do this optimization if the inner bb contains only the > >> conditional. */ > >> if (!gsi_one_before_end_p (gsi_start_nondebug_after_labels_bb > >> (inner_cond_bb))) > >> -- > >> 2.31.1 > >> >
