Hi Attachment is my updated path. The implementation of parse_sanitizer_options is not elegance enough. Mixing handling flags of fsanitize is easy to make mistakes.
ChangeLog:
gcc/ChangeLog:
2017-09-05 Wish Wu <wishwu...@gmail.com>
* asan.c (initialize_sanitizer_builtins):
Build function type list of trace-cmp.
* builtin-types.def (BT_FN_VOID_UINT8_UINT8):
Define function type of trace-cmp.
(BT_FN_VOID_UINT16_UINT16): Likewise.
(BT_FN_VOID_UINT32_UINT32): Likewise.
(BT_FN_VOID_FLOAT_FLOAT): Likewise.
(BT_FN_VOID_DOUBLE_DOUBLE): Likewise.
(BT_FN_VOID_UINT64_PTR): Likewise.
* common.opt: Add options of sanitize coverage.
* flag-types.h (enum sanitize_coverage_code):
Declare flags of sanitize coverage.
* fold-const.c (fold_range_test):
Disable non-short-circuit feature when sanitize coverage is enabled.
(fold_truth_andor): Likewise.
* tree-ssa-ifcombine.c (ifcombine_ifandif): Likewise.
* opts.c (COVERAGE_SANITIZER_OPT):
Define coverage sanitizer options.
(get_closest_sanitizer_option): Make OPT_fsanitize_,
OPT_fsanitize_recover_ and OPT_fsanitize_coverage_ to use same
function.
(parse_sanitizer_options): Likewise.
(common_handle_option): Add OPT_fsanitize_coverage_.
* sancov.c (instrument_comparison): Instrument comparisons.
(instrument_switch): Likewise.
(sancov_pass): Add trace-cmp support.
* sanitizer.def (BUILT_IN_SANITIZER_COV_TRACE_CMP1):
Define builtin functions of trace-cmp.
(BUILT_IN_SANITIZER_COV_TRACE_CMP2): Likewise.
(BUILT_IN_SANITIZER_COV_TRACE_CMP4): Likewise.
(BUILT_IN_SANITIZER_COV_TRACE_CMP8): Likewise.
(BUILT_IN_SANITIZER_COV_TRACE_CMPF): Likewise.
(BUILT_IN_SANITIZER_COV_TRACE_CMPD): Likewise.
(BUILT_IN_SANITIZER_COV_TRACE_SWITCH): Likewise.
gcc/testsuite/ChangeLog:
2017-09-05 Wish Wu <wishwu...@gmail.com>
* gcc.dg/sancov/basic3.c: New test.
Thank you every much for improving my codes.
Wish Wu
------------------------------------------------------------------
From:Jakub Jelinek <ja...@redhat.com>
Time:2017 Sep 5 (Tue) 01:34
To:Wish Wu <weixi....@antfin.com>
Cc:Dmitry Vyukov <dvyu...@google.com>; gcc-patches <gcc-patches@gcc.gnu.org>;
Jeff Law <l...@redhat.com>; wishwu007 <wishwu...@gmail.com>
Subject:Re: Add support to trace comparison instructions and switch statements
On Mon, Sep 04, 2017 at 09:36:40PM +0800, 吴潍浠(此彼) wrote:
> gcc/ChangeLog:
>
> 2017-09-04 Wish Wu <wishwu...@gmail.com>
>
> * asan.c (initialize_sanitizer_builtins):
> * builtin-types.def (BT_FN_VOID_UINT8_UINT8):
> (BT_FN_VOID_UINT16_UINT16):
> (BT_FN_VOID_UINT32_UINT32):
> (BT_FN_VOID_FLOAT_FLOAT):
> (BT_FN_VOID_DOUBLE_DOUBLE):
> (BT_FN_VOID_UINT64_PTR):
> * common.opt:
> * flag-types.h (enum sanitize_coverage_code):
> * opts.c (COVERAGE_SANITIZER_OPT):
> (get_closest_sanitizer_option):
> (parse_sanitizer_options):
> (common_handle_option):
> * sancov.c (instrument_cond):
> (instrument_switch):
> (sancov_pass):
> * sanitizer.def (BUILT_IN_SANITIZER_COV_TRACE_CMP1):
> (BUILT_IN_SANITIZER_COV_TRACE_CMP2):
> (BUILT_IN_SANITIZER_COV_TRACE_CMP4):
> (BUILT_IN_SANITIZER_COV_TRACE_CMP8):
> (BUILT_IN_SANITIZER_COV_TRACE_CMPF):
> (BUILT_IN_SANITIZER_COV_TRACE_CMPD):
> (BUILT_IN_SANITIZER_COV_TRACE_SWITCH):
mklog just generates a template, you need to fill in the details
on what has been changed or added or removed. See other ChangeLog
entries etc. to see what is expected.
> For code :
> void bar (void);
> void
> foo (int x)
> {
> if (x == 21 || x == 64 || x == 98 || x == 135)
> bar ();
> }
> GIMPLE IL on x86_64:
> 1
> 2 ;; Function foo (foo, funcdef_no=0, decl_uid=2161, cgraph_uid=0,
> symbol_order=0)
> 3
> 4 foo (int x)
> 5 {
...
That is with -O0 though? With -O2 you'll see that it changes.
IMNSHO you really want to also handle the GIMPLE_ASSIGN with tcc_comparison
class rhs_code. Shouldn't be that hard to handle that within
instrument_cond, just the way how you extract lhs and rhs from the insn will
differ based on if it is a GIMPLE_COND or GIMPLE_ASSIGN (and in that case
also for tcc_comparison rhs_code or for COND_EXPR with tcc_comparison first
operand).
And I really think we should change the 2 LOGICAL_OP_NON_SHORT_CIRCUIT
uses in fold-const.c and one in tree-ssa-ifcombine.c with
LOGICAL_OP_NON_SHORT_CIRCUIT
&& !flag_sanitize_coverage
with a comment that for sanitize coverage we want to avoid this optimization
because it negatively affects it.
@@ -1611,38 +1631,51 @@ parse_sanitizer_options (const char *p, location_t
}
/* Check to see if the string matches an option class name. */
- for (i = 0; sanitizer_opts[i].name != NULL; ++i)
- if (len == sanitizer_opts[i].len
- && memcmp (p, sanitizer_opts[i].name, len) == 0)
+ for (i = 0; opts[i].name != NULL; ++i)
+ if (len == opts[i].len
+ && memcmp (p, opts[i].name, len) == 0)
{
- /* Handle both -fsanitize and -fno-sanitize cases. */
- if (value && sanitizer_opts[i].flag == ~0U)
+ if (code == OPT_fsanitize_coverage_)
{
- if (code == OPT_fsanitize_)
- {
- if (complain)
- error_at (loc, "%<-fsanitize=all%> option is not valid");
- }
+ if (value)
+ flags |= opts[i].flag;
else
- flags |= ~(SANITIZE_THREAD | SANITIZE_LEAK
- | SANITIZE_UNREACHABLE | SANITIZE_RETURN);
+ flags &= ~opts[i].flag;
+ found = true;
+ break;
}
- else if (value)
+ else
{
- /* Do not enable -fsanitize-recover=unreachable and
- -fsanitize-recover=return if -fsanitize-recover=undefined
- is selected. */
- if (code == OPT_fsanitize_recover_
- && sanitizer_opts[i].flag == SANITIZE_UNDEFINED)
- flags |= (SANITIZE_UNDEFINED
- & ~(SANITIZE_UNREACHABLE | SANITIZE_RETURN));
+ /* Handle both -fsanitize and -fno-sanitize cases. */
+ if (value && opts[i].flag == ~0U)
+ {
+ if (code == OPT_fsanitize_)
+ {
+ if (complain)
+ error_at (loc,
+ "%<-fsanitize=all%> option is not valid");
+ }
+ else
+ flags |= ~(SANITIZE_THREAD | SANITIZE_LEAK
+ | SANITIZE_UNREACHABLE | SANITIZE_RETURN);
+ }
+ else if (value)
+ {
+ /* Do not enable -fsanitize-recover=unreachable and
+ -fsanitize-recover=return if -fsanitize-recover=undefined
+ is selected. */
+ if (code == OPT_fsanitize_recover_
+ && opts[i].flag == SANITIZE_UNDEFINED)
+ flags |= (SANITIZE_UNDEFINED
+ & ~(SANITIZE_UNREACHABLE | SANITIZE_RETURN));
+ else
+ flags |= opts[i].flag;
+ }
else
- flags |= sanitizer_opts[i].flag;
+ flags &= ~opts[i].flag;
+ found = true;
+ break;
}
- else
- flags &= ~sanitizer_opts[i].flag;
- found = true;
- break;
}
I don't see a need for this hunk. For code == OPT_fsanitize_coverage_
(where you know that there is no entry with ~0U flag value and also
know that code is not OPT_fsanitize_recover_) I think it will
just DTRT without any changes.
namespace {
There should be a function comment before the function here, explaining
what the function is for.
+static void
+instrument_cond (gimple_stmt_iterator *gsi, gimple *stmt)
+{
+ tree lhs = gimple_cond_lhs (stmt);
+ tree rhs = gimple_cond_rhs (stmt);
+ tree lhs_type = TREE_TYPE (lhs);
+ tree rhs_type = TREE_TYPE (rhs);
+
+ HOST_WIDE_INT size_in_bytes = MAX (int_size_in_bytes (lhs_type),
+ int_size_in_bytes (rhs_type));
+ if (size_in_bytes == -1)
+ return;
As I said, GIMPLE_COND operands should have the same (or compatible)
type, so there is no need to use rhs_type at all, nor test it.
And there is no need to test size_in_bytes before the if, just do
it right before the switch in there (and no need to special case -1,
because it is like any other default: handled by return;).
+ else if (SCALAR_FLOAT_TYPE_P (lhs_type) && SCALAR_FLOAT_TYPE_P (rhs_type))
Again, no need to test both.
+ {
+ if (TYPE_MODE (lhs_type) == TYPE_MODE (double_type_node)
+ || TYPE_MODE (rhs_type) == TYPE_MODE (double_type_node))
Or here.
+ {
+ fncode = BUILT_IN_SANITIZER_COV_TRACE_CMPD;
+ to_type = double_type_node;
+ }
+ else if (TYPE_MODE (lhs_type) == TYPE_MODE (float_type_node)
+ || TYPE_MODE (rhs_type) == TYPE_MODE (float_type_node))
Or here. Just use type instead of lhs_type.
+ {
+ fncode = BUILT_IN_SANITIZER_COV_TRACE_CMPF;
+ to_type = float_type_node;
+ }
+
+ }
+ if (to_type != NULL_TREE)
+ {
+ gimple_seq seq = NULL;
+
+ gimple_seq_add_stmt (&seq, build_type_cast (to_type, lhs));
+ tree clhs = gimple_assign_lhs (gimple_seq_last_stmt (seq));
+
+ gimple_seq_add_stmt (&seq, build_type_cast (to_type, rhs));
+ tree crhs = gimple_assign_lhs (gimple_seq_last_stmt (seq));
If the var already has the right type, that will just create waste
that further opts will need to clean up. Better:
if (!useless_type_conversion_p (to_type, lhs_type)) // or s/lhs_// if you do
the above
{
gimple_seq_add_stmt (&seq, build_type_cast (to_type, lhs));
lhs = gimple_assign_lhs (gimple_seq_last_stmt (seq));
gimple_seq_add_stmt (&seq, build_type_cast (to_type, rhs));
rhs = gimple_assign_rhs (gimple_seq_last_stmt (seq));
}
or perhaps even
if (!useless_type_conversion_p (to_type, type))
{
if (TREE_CODE (lhs) == INTEGER_CST)
lhs = fold_convert (to_type, lhs);
else
{
gimple_seq_add_stmt (&seq, build_type_cast (to_type, lhs));
lhs = gimple_assign_lhs (gimple_seq_last_stmt (seq));
}
...
}
+ tree index = gimple_switch_index (switch_stmt);
+
+ HOST_WIDE_INT size_in_bytes = int_size_in_bytes (TREE_TYPE (index));
+ if (size_in_bytes == -1)
+ return;
Well, you want to punt not just when it is -1, but also when it is
> 8.
+
+ unsigned i, n = gimple_switch_num_labels (switch_stmt), num = 0;
+ for (i = 0; i < n; ++i)
I think gimple_switch_label (switch_stmt, 0) must be the
default label and thus have no low/high case, so you should
use for (i = 1; i < n; ++i).
+ for (i = 0; i < n; ++i)
Ditto.
Jakub
gcc-svn-201709052016.patch
Description: Binary data
