* [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683)
@ 2016-04-26 13:02 Jakub Jelinek
2016-04-26 22:51 ` [PATCH] operand_equal_p checking " Jakub Jelinek
2016-04-26 23:00 ` [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683, take 2) Jakub Jelinek
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Jelinek @ 2016-04-26 13:02 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 9105 bytes --]
Hi!
As mentioned in the PR, the bug that has been eventually worked around
by making the C++ constexpr hash tables non-deletable is that
operand_equal_p, which is used in lots of hash tables as the equal function
(with last argument 0), may return true, i.e. a match, even for trees that
have different hash values (when iterative_hash_expr is used).
The following (included) patch changes inchash::add_expr, which is the underlying
function, to match what operand_equal_p does more closely, in particular
attempting to make sure that if operand_equal_p returns non-zero, then
the hash must be the same.
I've been using the attached hack; without this patch during x86_64-linux
and i686-linux yes,extra,rtl checking bootstraps there were 66931
notes (surprisingly only from the ivopts and gimple-ssa-strength-reduction
hash tables, no others), with the patch there are none.
Ok for trunk?
The debugging hack is too ugly and slows down the compiler (by artificially
increasing number of collisions), so it is not appropriate, but perhaps we
can add some internal only use OEP_* flag, pass it to the recursive calls
of operand_equal_p and if not set and flag_checking, verify
iterative_hash_expr equality in the outermost call).
2016-04-26 Jakub Jelinek <jakub@redhat.com>
PR sanitizer/70683
* tree.h (inchash::add_expr): Add FLAGS argument.
* tree.c (inchash::add_expr): Likewise. If not OEP_ADDRESS_OF,
use STRIP_NOPS first. For INTEGER_CST assert not OEP_ADDRESS_OF.
For REAL_CST and !HONOR_SIGNED_ZEROS (t) hash +/- 0 the same.
Formatting fix. Adjust recursive calls. For tcc_comparison,
if swap_tree_comparison (code) is smaller than code, hash that
and arguments in the other order. Hash CONVERT_EXPR the same
as NOP_EXPR. For OEP_ADDRESS_OF hash MEM_REF with 0 offset
of ADDR_EXPR of decl as the decl itself. Add or remove
OEP_ADDRESS_OF from recursive flags as needed. For
FMA_EXPR, WIDEN_MULT_{PLUS,MINUS}_EXPR hash the first two
operands commutatively and only the third one normally.
For internal CALL_EXPR hash in CALL_EXPR_IFN.
--- gcc/tree.h.jj 2016-04-22 18:21:32.000000000 +0200
+++ gcc/tree.h 2016-04-26 10:59:50.333534452 +0200
@@ -4759,7 +4759,7 @@ extern int simple_cst_equal (const_tree,
namespace inchash
{
-extern void add_expr (const_tree, hash &);
+extern void add_expr (const_tree, hash &, unsigned int = 0);
}
--- gcc/tree.c.jj 2016-04-22 18:21:32.000000000 +0200
+++ gcc/tree.c 2016-04-26 11:20:48.795359078 +0200
@@ -7769,7 +7769,7 @@ namespace inchash
This function is intended to produce the same hash for expressions which
would compare equal using operand_equal_p. */
void
-add_expr (const_tree t, inchash::hash &hstate)
+add_expr (const_tree t, inchash::hash &hstate, unsigned int flags)
{
int i;
enum tree_code code;
@@ -7781,6 +7781,9 @@ add_expr (const_tree t, inchash::hash &h
return;
}
+ if (!(flags & OEP_ADDRESS_OF))
+ STRIP_NOPS (t);
+
code = TREE_CODE (t);
switch (code)
@@ -7791,12 +7794,17 @@ add_expr (const_tree t, inchash::hash &h
hstate.merge_hash (0);
return;
case INTEGER_CST:
+ gcc_checking_assert (!(flags & OEP_ADDRESS_OF));
for (i = 0; i < TREE_INT_CST_NUNITS (t); i++)
hstate.add_wide_int (TREE_INT_CST_ELT (t, i));
return;
case REAL_CST:
{
- unsigned int val2 = real_hash (TREE_REAL_CST_PTR (t));
+ unsigned int val2;
+ if (!HONOR_SIGNED_ZEROS (t) && real_zerop (t))
+ val2 = rvc_zero;
+ else
+ val2 = real_hash (TREE_REAL_CST_PTR (t));
hstate.merge_hash (val2);
return;
}
@@ -7807,17 +7815,18 @@ add_expr (const_tree t, inchash::hash &h
return;
}
case STRING_CST:
- hstate.add ((const void *) TREE_STRING_POINTER (t), TREE_STRING_LENGTH (t));
+ hstate.add ((const void *) TREE_STRING_POINTER (t),
+ TREE_STRING_LENGTH (t));
return;
case COMPLEX_CST:
- inchash::add_expr (TREE_REALPART (t), hstate);
- inchash::add_expr (TREE_IMAGPART (t), hstate);
+ inchash::add_expr (TREE_REALPART (t), hstate, flags);
+ inchash::add_expr (TREE_IMAGPART (t), hstate, flags);
return;
case VECTOR_CST:
{
unsigned i;
for (i = 0; i < VECTOR_CST_NELTS (t); ++i)
- inchash::add_expr (VECTOR_CST_ELT (t, i), hstate);
+ inchash::add_expr (VECTOR_CST_ELT (t, i), hstate, flags);
return;
}
case SSA_NAME:
@@ -7831,16 +7840,17 @@ add_expr (const_tree t, inchash::hash &h
/* A list of expressions, for a CALL_EXPR or as the elements of a
VECTOR_CST. */
for (; t; t = TREE_CHAIN (t))
- inchash::add_expr (TREE_VALUE (t), hstate);
+ inchash::add_expr (TREE_VALUE (t), hstate, flags);
return;
case CONSTRUCTOR:
{
unsigned HOST_WIDE_INT idx;
tree field, value;
+ flags &= ~OEP_ADDRESS_OF;
FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (t), idx, field, value)
{
- inchash::add_expr (field, hstate);
- inchash::add_expr (value, hstate);
+ inchash::add_expr (field, hstate, flags);
+ inchash::add_expr (value, hstate, flags);
}
return;
}
@@ -7865,21 +7875,102 @@ add_expr (const_tree t, inchash::hash &h
/* DECL's have a unique ID */
hstate.add_wide_int (DECL_UID (t));
}
+ else if (tclass == tcc_comparison)
+ {
+ /* For comparisons that can be swapped, use the lower
+ tree code. */
+ enum tree_code ccode = swap_tree_comparison (code);
+ if (code < ccode)
+ ccode = code;
+ hstate.add_object (ccode);
+ inchash::add_expr (TREE_OPERAND (t, ccode != code), hstate, flags);
+ inchash::add_expr (TREE_OPERAND (t, ccode == code), hstate, flags);
+ }
+ else if (CONVERT_EXPR_CODE_P (code))
+ {
+ /* NOP_EXPR and CONVERT_EXPR are considered equal by
+ operand_equal_p. */
+ enum tree_code ccode = NOP_EXPR;
+ hstate.add_object (ccode);
+
+ /* Don't hash the type, that can lead to having nodes which
+ compare equal according to operand_equal_p, but which
+ have different hash codes. Make sure to include signedness
+ in the hash computation. */
+ hstate.add_int (TYPE_UNSIGNED (TREE_TYPE (t)));
+ inchash::add_expr (TREE_OPERAND (t, 0), hstate, flags);
+ }
+ /* For OEP_ADDRESS_OF, hash MEM_EXPR[&decl, 0] the same as decl. */
+ else if (code == MEM_REF
+ && (flags & OEP_ADDRESS_OF) != 0
+ && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR
+ && DECL_P (TREE_OPERAND (TREE_OPERAND (t, 0), 0))
+ && integer_zerop (TREE_OPERAND (t, 1)))
+ inchash::add_expr (TREE_OPERAND (TREE_OPERAND (t, 0), 0),
+ hstate, flags);
else
{
gcc_assert (IS_EXPR_CODE_CLASS (tclass));
+ unsigned int sflags = flags;
hstate.add_object (code);
+ switch (code)
+ {
+ case ADDR_EXPR:
+ gcc_checking_assert (!(flags & OEP_ADDRESS_OF));
+ flags |= OEP_ADDRESS_OF;
+ sflags = flags;
+ break;
+
+ case INDIRECT_REF:
+ case MEM_REF:
+ case TARGET_MEM_REF:
+ flags &= ~OEP_ADDRESS_OF;
+ sflags = flags;
+ break;
+
+ case ARRAY_REF:
+ case ARRAY_RANGE_REF:
+ case COMPONENT_REF:
+ case BIT_FIELD_REF:
+ sflags &= ~OEP_ADDRESS_OF;
+ break;
+
+ case COND_EXPR:
+ flags &= ~OEP_ADDRESS_OF;
+ break;
+
+ case FMA_EXPR:
+ case WIDEN_MULT_PLUS_EXPR:
+ case WIDEN_MULT_MINUS_EXPR:
+ {
+ /* The multiplication operands are commutative. */
+ inchash::hash one, two;
+ inchash::add_expr (TREE_OPERAND (t, 0), one, flags);
+ inchash::add_expr (TREE_OPERAND (t, 1), two, flags);
+ hstate.add_commutative (one, two);
+ inchash::add_expr (TREE_OPERAND (t, 2), two, flags);
+ return;
+ }
+
+ case CALL_EXPR:
+ if (CALL_EXPR_FN (t) == NULL_TREE)
+ hstate.add_int (CALL_EXPR_IFN (t));
+ break;
+
+ default:
+ break;
+ }
+
/* Don't hash the type, that can lead to having nodes which
compare equal according to operand_equal_p, but which
have different hash codes. */
- if (CONVERT_EXPR_CODE_P (code)
- || code == NON_LVALUE_EXPR)
+ if (code == NON_LVALUE_EXPR)
{
/* Make sure to include signness in the hash computation. */
hstate.add_int (TYPE_UNSIGNED (TREE_TYPE (t)));
- inchash::add_expr (TREE_OPERAND (t, 0), hstate);
+ inchash::add_expr (TREE_OPERAND (t, 0), hstate, flags);
}
else if (commutative_tree_code (code))
@@ -7889,13 +7980,14 @@ add_expr (const_tree t, inchash::hash &h
and then rehashing based on the order of their independent
hashes. */
inchash::hash one, two;
- inchash::add_expr (TREE_OPERAND (t, 0), one);
- inchash::add_expr (TREE_OPERAND (t, 1), two);
+ inchash::add_expr (TREE_OPERAND (t, 0), one, flags);
+ inchash::add_expr (TREE_OPERAND (t, 1), two, flags);
hstate.add_commutative (one, two);
}
else
for (i = TREE_OPERAND_LENGTH (t) - 1; i >= 0; --i)
- inchash::add_expr (TREE_OPERAND (t, i), hstate);
+ inchash::add_expr (TREE_OPERAND (t, i), hstate,
+ i == 0 ? flags : sflags);
}
return;
}
Jakub
[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 9816 bytes --]
--- gcc/fold-const.c.jj 2016-04-22 18:46:20.000000000 +0200
+++ gcc/fold-const.c 2016-04-26 10:37:43.537640231 +0200
@@ -3273,6 +3273,26 @@ operand_equal_p (const_tree arg0, const_
#undef OP_SAME
#undef OP_SAME_WITH_NULL
}
+
+int
+operand_equal_p2 (const_tree arg0, const_tree arg1, unsigned int kind)
+{
+ if (operand_equal_p (arg0, arg1, 0))
+ {
+ if (iterative_hash_expr (arg0, 0) != iterative_hash_expr (arg1, 0))
+ {
+ FILE *f = fopen ("/tmp/hash", "a");
+ fprintf (f, "%d %s %s %d %u %u\n", (int) BITS_PER_WORD,
+ main_input_filename ? main_input_filename : "-",
+ current_function_name (),
+ kind, iterative_hash_expr (arg0, 0),
+ iterative_hash_expr (arg1, 0));
+ fclose (f);
+ }
+ return 1;
+ }
+ return 0;
+}
\f
/* Similar to operand_equal_p, but see if ARG0 might have been made by
shorten_compare from ARG1 when ARG1 was being compared with OTHER.
--- gcc/asan.c.jj 2016-04-08 14:39:06.000000000 +0200
+++ gcc/asan.c 2016-04-26 10:38:31.976979750 +0200
@@ -402,9 +402,10 @@ struct asan_mem_ref_hasher : nofree_ptr_
/* Hash a memory reference. */
inline hashval_t
-asan_mem_ref_hasher::hash (const asan_mem_ref *mem_ref)
+asan_mem_ref_hasher::hash (const asan_mem_ref *)
{
- return iterative_hash_expr (mem_ref->start, 0);
+// return iterative_hash_expr (mem_ref->start, 0);
+ return 0;
}
/* Compare two memory references. We accept the length of either
@@ -414,7 +415,7 @@ inline bool
asan_mem_ref_hasher::equal (const asan_mem_ref *m1,
const asan_mem_ref *m2)
{
- return operand_equal_p (m1->start, m2->start, 0);
+ return operand_equal_p2 (m1->start, m2->start, 1);
}
static hash_table<asan_mem_ref_hasher> *asan_mem_ref_ht;
--- gcc/tree-ssa-loop-im.c.jj 2016-01-26 18:47:43.000000000 +0100
+++ gcc/tree-ssa-loop-im.c 2016-04-26 10:45:20.102414771 +0200
@@ -165,7 +165,7 @@ mem_ref_hasher::hash (const im_mem_ref *
inline bool
mem_ref_hasher::equal (const im_mem_ref *mem1, const tree_node *obj2)
{
- return operand_equal_p (mem1->mem.ref, (const_tree) obj2, 0);
+ return operand_equal_p2 (mem1->mem.ref, (const_tree) obj2, 6);
}
@@ -598,7 +598,8 @@ mem_ref_in_stmt (gimple *stmt)
return NULL;
gcc_assert (!store);
- hash = iterative_hash_expr (*mem, 0);
+// hash = iterative_hash_expr (*mem, 0);
+ hash = 0;
ref = memory_accesses.refs->find_with_hash (*mem, hash);
gcc_assert (ref != NULL);
@@ -1466,7 +1467,8 @@ gather_mem_refs_stmt (struct loop *loop,
}
else
{
- hash = iterative_hash_expr (*mem, 0);
+// hash = iterative_hash_expr (*mem, 0);
+ hash = 0;
slot = memory_accesses.refs->find_slot_with_hash (*mem, hash, INSERT);
if (*slot)
{
--- gcc/tree-if-conv.c.jj 2016-04-22 18:46:22.000000000 +0200
+++ gcc/tree-if-conv.c 2016-04-26 10:43:23.162009379 +0200
@@ -125,14 +125,15 @@ struct innermost_loop_behavior_hash : no
};
inline hashval_t
-innermost_loop_behavior_hash::hash (const value_type &e)
+innermost_loop_behavior_hash::hash (const value_type &)
{
- hashval_t hash;
-
- hash = iterative_hash_expr (e->base_address, 0);
- hash = iterative_hash_expr (e->offset, hash);
- hash = iterative_hash_expr (e->init, hash);
- return iterative_hash_expr (e->step, hash);
+// hashval_t hash;
+//
+// hash = iterative_hash_expr (e->base_address, 0);
+// hash = iterative_hash_expr (e->offset, hash);
+// hash = iterative_hash_expr (e->init, hash);
+// return iterative_hash_expr (e->step, hash);
+ return 0;
}
inline bool
@@ -150,16 +151,16 @@ innermost_loop_behavior_hash::equal (con
return false;
if (e1->base_address && e2->base_address
- && !operand_equal_p (e1->base_address, e2->base_address, 0))
+ && !operand_equal_p2 (e1->base_address, e2->base_address, 5))
return false;
if (e1->offset && e2->offset
- && !operand_equal_p (e1->offset, e2->offset, 0))
+ && !operand_equal_p2 (e1->offset, e2->offset, 5))
return false;
if (e1->init && e2->init
- && !operand_equal_p (e1->init, e2->init, 0))
+ && !operand_equal_p2 (e1->init, e2->init, 5))
return false;
if (e1->step && e2->step
- && !operand_equal_p (e1->step, e2->step, 0))
+ && !operand_equal_p2 (e1->step, e2->step, 5))
return false;
return true;
--- gcc/gimple-ssa-strength-reduction.c.jj 2016-01-18 20:37:54.000000000 +0100
+++ gcc/gimple-ssa-strength-reduction.c 2016-04-26 10:38:52.122705059 +0200
@@ -406,16 +406,17 @@ struct cand_chain_hasher : nofree_ptr_ha
};
inline hashval_t
-cand_chain_hasher::hash (const cand_chain *p)
+cand_chain_hasher::hash (const cand_chain *)
{
- tree base_expr = p->base_expr;
- return iterative_hash_expr (base_expr, 0);
+// tree base_expr = p->base_expr;
+// return iterative_hash_expr (base_expr, 0);
+ return 0;
}
inline bool
cand_chain_hasher::equal (const cand_chain *chain1, const cand_chain *chain2)
{
- return operand_equal_p (chain1->base_expr, chain2->base_expr, 0);
+ return operand_equal_p2 (chain1->base_expr, chain2->base_expr, 2);
}
/* Hash table embodying a mapping from base exprs to chains of candidates. */
--- gcc/tree-hash-traits.h.jj 2016-01-04 18:50:34.000000000 +0100
+++ gcc/tree-hash-traits.h 2016-04-26 10:38:11.915253296 +0200
@@ -29,16 +29,17 @@ struct tree_operand_hash : ggc_ptr_hash
};
inline hashval_t
-tree_operand_hash::hash (const value_type &t)
+tree_operand_hash::hash (const value_type &)
{
- return iterative_hash_expr (t, 0);
+// return iterative_hash_expr (t, 0);
+ return 0;
}
inline bool
tree_operand_hash::equal (const value_type &t1,
const compare_type &t2)
{
- return operand_equal_p (t1, t2, 0);
+ return operand_equal_p2 (t1, t2, 0);
}
/* Hasher for tree decls. Pointer equality is enough here, but the DECL_UID
--- gcc/tree-ssa-loop-ivopts.c.jj 2016-04-22 18:46:21.000000000 +0200
+++ gcc/tree-ssa-loop-ivopts.c 2016-04-26 10:46:58.102076888 +0200
@@ -285,8 +285,8 @@ iv_common_cand_hasher::equal (const iv_c
const iv_common_cand *ccand2)
{
return (ccand1->hash == ccand2->hash
- && operand_equal_p (ccand1->base, ccand2->base, 0)
- && operand_equal_p (ccand1->step, ccand2->step, 0)
+ && operand_equal_p2 (ccand1->base, ccand2->base, 7)
+ && operand_equal_p2 (ccand1->step, ccand2->step, 7)
&& (TYPE_PRECISION (TREE_TYPE (ccand1->base))
== TYPE_PRECISION (TREE_TYPE (ccand2->base))));
}
@@ -322,7 +322,7 @@ iv_inv_expr_hasher::equal (const iv_inv_
const iv_inv_expr_ent *expr2)
{
return expr1->hash == expr2->hash
- && operand_equal_p (expr1->expr, expr2->expr, 0);
+ && operand_equal_p2 (expr1->expr, expr2->expr, 7);
}
struct ivopts_data
@@ -3139,8 +3139,9 @@ record_common_cand (struct ivopts_data *
ent.base = base;
ent.step = step;
- ent.hash = iterative_hash_expr (base, 0);
- ent.hash = iterative_hash_expr (step, ent.hash);
+// ent.hash = iterative_hash_expr (base, 0);
+// ent.hash = iterative_hash_expr (step, ent.hash);
+ ent.hash = 0;
slot = data->iv_common_cand_tab->find_slot (&ent, INSERT);
if (*slot == NULL)
@@ -4673,7 +4674,8 @@ get_expr_id (struct ivopts_data *data, t
struct iv_inv_expr_ent **slot;
ent.expr = expr;
- ent.hash = iterative_hash_expr (expr, 0);
+// ent.hash = iterative_hash_expr (expr, 0);
+ ent.hash = 0;
slot = data->inv_expr_tab->find_slot (&ent, INSERT);
if (*slot)
return (*slot)->id;
--- gcc/fold-const.h.jj 2016-01-11 16:19:41.000000000 +0100
+++ gcc/fold-const.h 2016-04-26 10:35:31.791436620 +0200
@@ -87,6 +87,7 @@ extern void fold_undefer_overflow_warnin
extern void fold_undefer_and_ignore_overflow_warnings (void);
extern bool fold_deferring_overflow_warnings_p (void);
extern int operand_equal_p (const_tree, const_tree, unsigned int);
+extern int operand_equal_p2 (const_tree, const_tree, unsigned int);
extern int multiple_of_p (tree, const_tree, const_tree);
#define omit_one_operand(T1,T2,T3)\
omit_one_operand_loc (UNKNOWN_LOCATION, T1, T2, T3)
--- gcc/trans-mem.c.jj 2016-01-26 18:47:43.000000000 +0100
+++ gcc/trans-mem.c 2016-04-26 10:42:19.947871317 +0200
@@ -957,9 +957,10 @@ struct log_entry_hasher : pointer_hash <
/* Htab support. Return hash value for a `tm_log_entry'. */
inline hashval_t
-log_entry_hasher::hash (const tm_log_entry *log)
+log_entry_hasher::hash (const tm_log_entry *)
{
- return iterative_hash_expr (log->addr, 0);
+// return iterative_hash_expr (log->addr, 0);
+ return 0;
}
/* Htab support. Return true if two log entries are the same. */
@@ -985,7 +986,7 @@ log_entry_hasher::equal (const tm_log_en
if (log1->addr == log2->addr)
return true;
- return operand_equal_p (log1->addr, log2->addr, 0);
+ return operand_equal_p2 (log1->addr, log2->addr, 3);
}
/* Htab support. Free one tm_log_entry. */
@@ -3454,21 +3455,22 @@ struct tm_memop_hasher : free_ptr_hash <
/* Htab support. Return a hash value for a `tm_memop'. */
inline hashval_t
-tm_memop_hasher::hash (const tm_memop *mem)
+tm_memop_hasher::hash (const tm_memop *)
{
- tree addr = mem->addr;
+// tree addr = mem->addr;
/* We drill down to the SSA_NAME/DECL for the hash, but equality is
actually done with operand_equal_p (see tm_memop_eq). */
- if (TREE_CODE (addr) == ADDR_EXPR)
- addr = TREE_OPERAND (addr, 0);
- return iterative_hash_expr (addr, 0);
+// if (TREE_CODE (addr) == ADDR_EXPR)
+// addr = TREE_OPERAND (addr, 0);
+// return iterative_hash_expr (addr, 0);
+ return 0;
}
/* Htab support. Return true if two tm_memop's are the same. */
inline bool
tm_memop_hasher::equal (const tm_memop *mem1, const tm_memop *mem2)
{
- return operand_equal_p (mem1->addr, mem2->addr, 0);
+ return operand_equal_p2 (mem1->addr, mem2->addr, 4);
}
/* Sets for solving data flow equations in the memory optimization pass. */
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] operand_equal_p checking (PR sanitizer/70683)
2016-04-26 13:02 [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683) Jakub Jelinek
@ 2016-04-26 22:51 ` Jakub Jelinek
2016-04-27 12:41 ` Richard Biener
2016-04-26 23:00 ` [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683, take 2) Jakub Jelinek
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2016-04-26 22:51 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On Tue, Apr 26, 2016 at 03:02:38PM +0200, Jakub Jelinek wrote:
> The debugging hack is too ugly and slows down the compiler (by artificially
> increasing number of collisions), so it is not appropriate, but perhaps we
> can add some internal only use OEP_* flag, pass it to the recursive calls
> of operand_equal_p and if not set and flag_checking, verify
> iterative_hash_expr equality in the outermost call).
Here is the corresponding checking patch. It uncovered two further issues
in the tree.[ch] patch which I'm going to post momentarily.
Both patches together bootstrapped/regtested on x86_64-linux and i686-linux,
ok for trunk?
2016-04-27 Jakub Jelinek <jakub@redhat.com>
PR sanitizer/70683
* tree-core.h (enum operand_equal_flag): Add OEP_NO_HASH_CHECK.
* fold-const.c (operand_equal_p): If flag_checking and
OEP_NO_HASH_CHECK is not set in flag, recurse with OEP_NO_HASH_CHECK
and if it returns non-zero, assert iterative_hash_expr on both
args is the same.
--- gcc/tree-core.h.jj 2016-04-22 18:21:55.000000000 +0200
+++ gcc/tree-core.h 2016-04-26 17:47:19.875753297 +0200
@@ -765,7 +765,9 @@ enum operand_equal_flag {
OEP_ONLY_CONST = 1,
OEP_PURE_SAME = 2,
OEP_MATCH_SIDE_EFFECTS = 4,
- OEP_ADDRESS_OF = 8
+ OEP_ADDRESS_OF = 8,
+ /* Internal within operand_equal_p: */
+ OEP_NO_HASH_CHECK = 16
};
/* Enum and arrays used for tree allocation stats.
--- gcc/fold-const.c.jj 2016-04-22 18:21:32.000000000 +0200
+++ gcc/fold-const.c 2016-04-26 18:30:40.919080701 +0200
@@ -2749,6 +2749,25 @@ combine_comparisons (location_t loc,
int
operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
{
+ /* When checking, verify at the outermost operand_equal_p call that
+ if operand_equal_p returns non-zero then ARG0 and ARG1 has the same
+ hash value. */
+ if (flag_checking && !(flags & OEP_NO_HASH_CHECK))
+ {
+ if (operand_equal_p (arg0, arg1, flags | OEP_NO_HASH_CHECK))
+ {
+ inchash::hash hstate0 (0), hstate1 (0);
+ inchash::add_expr (arg0, hstate0, flags);
+ inchash::add_expr (arg1, hstate1, flags);
+ hashval_t h0 = hstate0.end ();
+ hashval_t h1 = hstate1.end ();
+ gcc_assert (h0 == h1);
+ return 1;
+ }
+ else
+ return 0;
+ }
+
/* If either is ERROR_MARK, they aren't equal. */
if (TREE_CODE (arg0) == ERROR_MARK || TREE_CODE (arg1) == ERROR_MARK
|| TREE_TYPE (arg0) == error_mark_node
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683, take 2)
2016-04-26 13:02 [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683) Jakub Jelinek
2016-04-26 22:51 ` [PATCH] operand_equal_p checking " Jakub Jelinek
@ 2016-04-26 23:00 ` Jakub Jelinek
2016-04-27 7:41 ` Richard Biener
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Jelinek @ 2016-04-26 23:00 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On Tue, Apr 26, 2016 at 03:02:38PM +0200, Jakub Jelinek wrote:
> I've been using the attached hack; without this patch during x86_64-linux
> and i686-linux yes,extra,rtl checking bootstraps there were 66931
> notes (surprisingly only from the ivopts and gimple-ssa-strength-reduction
> hash tables, no others), with the patch there are none.
>
> Ok for trunk?
With the checking patch I've just posted, I've found two additional issues.
One is that the patch treated all tcc_comparison class codes by
canonicalizing them to the lower of the two codes and perhaps swapping the
arguments. That is fine for most of the codes, but not for the commutative
comparisons, because operand_equal_p will return true e.g. for x != y and
y != y. So, we need to treat those as commutative.
And the second issue is in hashing INTEGER_CSTs. E.g. on
builtin-arith-overflow-10.c, operand_equal_p returns non-zero for
DImode 0x8000000000000000 and TImode of the same value, but they weren't
hashing the same, the former has TREE_INT_CST_NUNITS == 1, the latter
== 2 (but both have TREE_INT_CST_EXT_NUNITS == 2).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2016-04-27 Jakub Jelinek <jakub@redhat.com>
PR sanitizer/70683
* tree.h (inchash::add_expr): Add FLAGS argument.
* tree.c (inchash::add_expr): Likewise. If not OEP_ADDRESS_OF,
use STRIP_NOPS first. For INTEGER_CST assert not OEP_ADDRESS_OF.
For REAL_CST and !HONOR_SIGNED_ZEROS (t) hash +/- 0 the same.
Formatting fix. Adjust recursive calls. For tcc_comparison,
if swap_tree_comparison (code) is smaller than code, hash that
and arguments in the other order. Hash CONVERT_EXPR the same
as NOP_EXPR. For OEP_ADDRESS_OF hash MEM_REF with 0 offset
of ADDR_EXPR of decl as the decl itself. Add or remove
OEP_ADDRESS_OF from recursive flags as needed. For
FMA_EXPR, WIDEN_MULT_{PLUS,MINUS}_EXPR hash the first two
operands commutatively and only the third one normally.
For internal CALL_EXPR hash in CALL_EXPR_IFN.
--- gcc/tree.h.jj 2016-04-22 18:21:32.000000000 +0200
+++ gcc/tree.h 2016-04-26 10:59:50.333534452 +0200
@@ -4759,7 +4759,7 @@ extern int simple_cst_equal (const_tree,
namespace inchash
{
-extern void add_expr (const_tree, hash &);
+extern void add_expr (const_tree, hash &, unsigned int = 0);
}
--- gcc/tree.c.jj 2016-04-22 18:21:32.000000000 +0200
+++ gcc/tree.c 2016-04-26 23:00:12.238080960 +0200
@@ -7769,7 +7769,7 @@ namespace inchash
This function is intended to produce the same hash for expressions which
would compare equal using operand_equal_p. */
void
-add_expr (const_tree t, inchash::hash &hstate)
+add_expr (const_tree t, inchash::hash &hstate, unsigned int flags)
{
int i;
enum tree_code code;
@@ -7781,6 +7781,9 @@ add_expr (const_tree t, inchash::hash &h
return;
}
+ if (!(flags & OEP_ADDRESS_OF))
+ STRIP_NOPS (t);
+
code = TREE_CODE (t);
switch (code)
@@ -7791,12 +7794,17 @@ add_expr (const_tree t, inchash::hash &h
hstate.merge_hash (0);
return;
case INTEGER_CST:
- for (i = 0; i < TREE_INT_CST_NUNITS (t); i++)
+ gcc_checking_assert (!(flags & OEP_ADDRESS_OF));
+ for (i = 0; i < TREE_INT_CST_EXT_NUNITS (t); i++)
hstate.add_wide_int (TREE_INT_CST_ELT (t, i));
return;
case REAL_CST:
{
- unsigned int val2 = real_hash (TREE_REAL_CST_PTR (t));
+ unsigned int val2;
+ if (!HONOR_SIGNED_ZEROS (t) && real_zerop (t))
+ val2 = rvc_zero;
+ else
+ val2 = real_hash (TREE_REAL_CST_PTR (t));
hstate.merge_hash (val2);
return;
}
@@ -7807,17 +7815,18 @@ add_expr (const_tree t, inchash::hash &h
return;
}
case STRING_CST:
- hstate.add ((const void *) TREE_STRING_POINTER (t), TREE_STRING_LENGTH (t));
+ hstate.add ((const void *) TREE_STRING_POINTER (t),
+ TREE_STRING_LENGTH (t));
return;
case COMPLEX_CST:
- inchash::add_expr (TREE_REALPART (t), hstate);
- inchash::add_expr (TREE_IMAGPART (t), hstate);
+ inchash::add_expr (TREE_REALPART (t), hstate, flags);
+ inchash::add_expr (TREE_IMAGPART (t), hstate, flags);
return;
case VECTOR_CST:
{
unsigned i;
for (i = 0; i < VECTOR_CST_NELTS (t); ++i)
- inchash::add_expr (VECTOR_CST_ELT (t, i), hstate);
+ inchash::add_expr (VECTOR_CST_ELT (t, i), hstate, flags);
return;
}
case SSA_NAME:
@@ -7831,16 +7840,17 @@ add_expr (const_tree t, inchash::hash &h
/* A list of expressions, for a CALL_EXPR or as the elements of a
VECTOR_CST. */
for (; t; t = TREE_CHAIN (t))
- inchash::add_expr (TREE_VALUE (t), hstate);
+ inchash::add_expr (TREE_VALUE (t), hstate, flags);
return;
case CONSTRUCTOR:
{
unsigned HOST_WIDE_INT idx;
tree field, value;
+ flags &= ~OEP_ADDRESS_OF;
FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (t), idx, field, value)
{
- inchash::add_expr (field, hstate);
- inchash::add_expr (value, hstate);
+ inchash::add_expr (field, hstate, flags);
+ inchash::add_expr (value, hstate, flags);
}
return;
}
@@ -7865,21 +7875,102 @@ add_expr (const_tree t, inchash::hash &h
/* DECL's have a unique ID */
hstate.add_wide_int (DECL_UID (t));
}
+ else if (tclass == tcc_comparison && !commutative_tree_code (code))
+ {
+ /* For comparisons that can be swapped, use the lower
+ tree code. */
+ enum tree_code ccode = swap_tree_comparison (code);
+ if (code < ccode)
+ ccode = code;
+ hstate.add_object (ccode);
+ inchash::add_expr (TREE_OPERAND (t, ccode != code), hstate, flags);
+ inchash::add_expr (TREE_OPERAND (t, ccode == code), hstate, flags);
+ }
+ else if (CONVERT_EXPR_CODE_P (code))
+ {
+ /* NOP_EXPR and CONVERT_EXPR are considered equal by
+ operand_equal_p. */
+ enum tree_code ccode = NOP_EXPR;
+ hstate.add_object (ccode);
+
+ /* Don't hash the type, that can lead to having nodes which
+ compare equal according to operand_equal_p, but which
+ have different hash codes. Make sure to include signedness
+ in the hash computation. */
+ hstate.add_int (TYPE_UNSIGNED (TREE_TYPE (t)));
+ inchash::add_expr (TREE_OPERAND (t, 0), hstate, flags);
+ }
+ /* For OEP_ADDRESS_OF, hash MEM_EXPR[&decl, 0] the same as decl. */
+ else if (code == MEM_REF
+ && (flags & OEP_ADDRESS_OF) != 0
+ && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR
+ && DECL_P (TREE_OPERAND (TREE_OPERAND (t, 0), 0))
+ && integer_zerop (TREE_OPERAND (t, 1)))
+ inchash::add_expr (TREE_OPERAND (TREE_OPERAND (t, 0), 0),
+ hstate, flags);
else
{
gcc_assert (IS_EXPR_CODE_CLASS (tclass));
+ unsigned int sflags = flags;
hstate.add_object (code);
+ switch (code)
+ {
+ case ADDR_EXPR:
+ gcc_checking_assert (!(flags & OEP_ADDRESS_OF));
+ flags |= OEP_ADDRESS_OF;
+ sflags = flags;
+ break;
+
+ case INDIRECT_REF:
+ case MEM_REF:
+ case TARGET_MEM_REF:
+ flags &= ~OEP_ADDRESS_OF;
+ sflags = flags;
+ break;
+
+ case ARRAY_REF:
+ case ARRAY_RANGE_REF:
+ case COMPONENT_REF:
+ case BIT_FIELD_REF:
+ sflags &= ~OEP_ADDRESS_OF;
+ break;
+
+ case COND_EXPR:
+ flags &= ~OEP_ADDRESS_OF;
+ break;
+
+ case FMA_EXPR:
+ case WIDEN_MULT_PLUS_EXPR:
+ case WIDEN_MULT_MINUS_EXPR:
+ {
+ /* The multiplication operands are commutative. */
+ inchash::hash one, two;
+ inchash::add_expr (TREE_OPERAND (t, 0), one, flags);
+ inchash::add_expr (TREE_OPERAND (t, 1), two, flags);
+ hstate.add_commutative (one, two);
+ inchash::add_expr (TREE_OPERAND (t, 2), two, flags);
+ return;
+ }
+
+ case CALL_EXPR:
+ if (CALL_EXPR_FN (t) == NULL_TREE)
+ hstate.add_int (CALL_EXPR_IFN (t));
+ break;
+
+ default:
+ break;
+ }
+
/* Don't hash the type, that can lead to having nodes which
compare equal according to operand_equal_p, but which
have different hash codes. */
- if (CONVERT_EXPR_CODE_P (code)
- || code == NON_LVALUE_EXPR)
+ if (code == NON_LVALUE_EXPR)
{
/* Make sure to include signness in the hash computation. */
hstate.add_int (TYPE_UNSIGNED (TREE_TYPE (t)));
- inchash::add_expr (TREE_OPERAND (t, 0), hstate);
+ inchash::add_expr (TREE_OPERAND (t, 0), hstate, flags);
}
else if (commutative_tree_code (code))
@@ -7889,13 +7980,14 @@ add_expr (const_tree t, inchash::hash &h
and then rehashing based on the order of their independent
hashes. */
inchash::hash one, two;
- inchash::add_expr (TREE_OPERAND (t, 0), one);
- inchash::add_expr (TREE_OPERAND (t, 1), two);
+ inchash::add_expr (TREE_OPERAND (t, 0), one, flags);
+ inchash::add_expr (TREE_OPERAND (t, 1), two, flags);
hstate.add_commutative (one, two);
}
else
for (i = TREE_OPERAND_LENGTH (t) - 1; i >= 0; --i)
- inchash::add_expr (TREE_OPERAND (t, i), hstate);
+ inchash::add_expr (TREE_OPERAND (t, i), hstate,
+ i == 0 ? flags : sflags);
}
return;
}
Jakub
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683, take 2)
2016-04-26 23:00 ` [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683, take 2) Jakub Jelinek
@ 2016-04-27 7:41 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2016-04-27 7:41 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Wed, 27 Apr 2016, Jakub Jelinek wrote:
> On Tue, Apr 26, 2016 at 03:02:38PM +0200, Jakub Jelinek wrote:
> > I've been using the attached hack; without this patch during x86_64-linux
> > and i686-linux yes,extra,rtl checking bootstraps there were 66931
> > notes (surprisingly only from the ivopts and gimple-ssa-strength-reduction
> > hash tables, no others), with the patch there are none.
> >
> > Ok for trunk?
>
> With the checking patch I've just posted, I've found two additional issues.
>
> One is that the patch treated all tcc_comparison class codes by
> canonicalizing them to the lower of the two codes and perhaps swapping the
> arguments. That is fine for most of the codes, but not for the commutative
> comparisons, because operand_equal_p will return true e.g. for x != y and
> y != y. So, we need to treat those as commutative.
>
> And the second issue is in hashing INTEGER_CSTs. E.g. on
> builtin-arith-overflow-10.c, operand_equal_p returns non-zero for
> DImode 0x8000000000000000 and TImode of the same value, but they weren't
> hashing the same, the former has TREE_INT_CST_NUNITS == 1, the latter
> == 2 (but both have TREE_INT_CST_EXT_NUNITS == 2).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Thanks,
Richard.
> 2016-04-27 Jakub Jelinek <jakub@redhat.com>
>
> PR sanitizer/70683
> * tree.h (inchash::add_expr): Add FLAGS argument.
> * tree.c (inchash::add_expr): Likewise. If not OEP_ADDRESS_OF,
> use STRIP_NOPS first. For INTEGER_CST assert not OEP_ADDRESS_OF.
> For REAL_CST and !HONOR_SIGNED_ZEROS (t) hash +/- 0 the same.
> Formatting fix. Adjust recursive calls. For tcc_comparison,
> if swap_tree_comparison (code) is smaller than code, hash that
> and arguments in the other order. Hash CONVERT_EXPR the same
> as NOP_EXPR. For OEP_ADDRESS_OF hash MEM_REF with 0 offset
> of ADDR_EXPR of decl as the decl itself. Add or remove
> OEP_ADDRESS_OF from recursive flags as needed. For
> FMA_EXPR, WIDEN_MULT_{PLUS,MINUS}_EXPR hash the first two
> operands commutatively and only the third one normally.
> For internal CALL_EXPR hash in CALL_EXPR_IFN.
>
> --- gcc/tree.h.jj 2016-04-22 18:21:32.000000000 +0200
> +++ gcc/tree.h 2016-04-26 10:59:50.333534452 +0200
> @@ -4759,7 +4759,7 @@ extern int simple_cst_equal (const_tree,
> namespace inchash
> {
>
> -extern void add_expr (const_tree, hash &);
> +extern void add_expr (const_tree, hash &, unsigned int = 0);
>
> }
>
> --- gcc/tree.c.jj 2016-04-22 18:21:32.000000000 +0200
> +++ gcc/tree.c 2016-04-26 23:00:12.238080960 +0200
> @@ -7769,7 +7769,7 @@ namespace inchash
> This function is intended to produce the same hash for expressions which
> would compare equal using operand_equal_p. */
> void
> -add_expr (const_tree t, inchash::hash &hstate)
> +add_expr (const_tree t, inchash::hash &hstate, unsigned int flags)
> {
> int i;
> enum tree_code code;
> @@ -7781,6 +7781,9 @@ add_expr (const_tree t, inchash::hash &h
> return;
> }
>
> + if (!(flags & OEP_ADDRESS_OF))
> + STRIP_NOPS (t);
> +
> code = TREE_CODE (t);
>
> switch (code)
> @@ -7791,12 +7794,17 @@ add_expr (const_tree t, inchash::hash &h
> hstate.merge_hash (0);
> return;
> case INTEGER_CST:
> - for (i = 0; i < TREE_INT_CST_NUNITS (t); i++)
> + gcc_checking_assert (!(flags & OEP_ADDRESS_OF));
> + for (i = 0; i < TREE_INT_CST_EXT_NUNITS (t); i++)
> hstate.add_wide_int (TREE_INT_CST_ELT (t, i));
> return;
> case REAL_CST:
> {
> - unsigned int val2 = real_hash (TREE_REAL_CST_PTR (t));
> + unsigned int val2;
> + if (!HONOR_SIGNED_ZEROS (t) && real_zerop (t))
> + val2 = rvc_zero;
> + else
> + val2 = real_hash (TREE_REAL_CST_PTR (t));
> hstate.merge_hash (val2);
> return;
> }
> @@ -7807,17 +7815,18 @@ add_expr (const_tree t, inchash::hash &h
> return;
> }
> case STRING_CST:
> - hstate.add ((const void *) TREE_STRING_POINTER (t), TREE_STRING_LENGTH (t));
> + hstate.add ((const void *) TREE_STRING_POINTER (t),
> + TREE_STRING_LENGTH (t));
> return;
> case COMPLEX_CST:
> - inchash::add_expr (TREE_REALPART (t), hstate);
> - inchash::add_expr (TREE_IMAGPART (t), hstate);
> + inchash::add_expr (TREE_REALPART (t), hstate, flags);
> + inchash::add_expr (TREE_IMAGPART (t), hstate, flags);
> return;
> case VECTOR_CST:
> {
> unsigned i;
> for (i = 0; i < VECTOR_CST_NELTS (t); ++i)
> - inchash::add_expr (VECTOR_CST_ELT (t, i), hstate);
> + inchash::add_expr (VECTOR_CST_ELT (t, i), hstate, flags);
> return;
> }
> case SSA_NAME:
> @@ -7831,16 +7840,17 @@ add_expr (const_tree t, inchash::hash &h
> /* A list of expressions, for a CALL_EXPR or as the elements of a
> VECTOR_CST. */
> for (; t; t = TREE_CHAIN (t))
> - inchash::add_expr (TREE_VALUE (t), hstate);
> + inchash::add_expr (TREE_VALUE (t), hstate, flags);
> return;
> case CONSTRUCTOR:
> {
> unsigned HOST_WIDE_INT idx;
> tree field, value;
> + flags &= ~OEP_ADDRESS_OF;
> FOR_EACH_CONSTRUCTOR_ELT (CONSTRUCTOR_ELTS (t), idx, field, value)
> {
> - inchash::add_expr (field, hstate);
> - inchash::add_expr (value, hstate);
> + inchash::add_expr (field, hstate, flags);
> + inchash::add_expr (value, hstate, flags);
> }
> return;
> }
> @@ -7865,21 +7875,102 @@ add_expr (const_tree t, inchash::hash &h
> /* DECL's have a unique ID */
> hstate.add_wide_int (DECL_UID (t));
> }
> + else if (tclass == tcc_comparison && !commutative_tree_code (code))
> + {
> + /* For comparisons that can be swapped, use the lower
> + tree code. */
> + enum tree_code ccode = swap_tree_comparison (code);
> + if (code < ccode)
> + ccode = code;
> + hstate.add_object (ccode);
> + inchash::add_expr (TREE_OPERAND (t, ccode != code), hstate, flags);
> + inchash::add_expr (TREE_OPERAND (t, ccode == code), hstate, flags);
> + }
> + else if (CONVERT_EXPR_CODE_P (code))
> + {
> + /* NOP_EXPR and CONVERT_EXPR are considered equal by
> + operand_equal_p. */
> + enum tree_code ccode = NOP_EXPR;
> + hstate.add_object (ccode);
> +
> + /* Don't hash the type, that can lead to having nodes which
> + compare equal according to operand_equal_p, but which
> + have different hash codes. Make sure to include signedness
> + in the hash computation. */
> + hstate.add_int (TYPE_UNSIGNED (TREE_TYPE (t)));
> + inchash::add_expr (TREE_OPERAND (t, 0), hstate, flags);
> + }
> + /* For OEP_ADDRESS_OF, hash MEM_EXPR[&decl, 0] the same as decl. */
> + else if (code == MEM_REF
> + && (flags & OEP_ADDRESS_OF) != 0
> + && TREE_CODE (TREE_OPERAND (t, 0)) == ADDR_EXPR
> + && DECL_P (TREE_OPERAND (TREE_OPERAND (t, 0), 0))
> + && integer_zerop (TREE_OPERAND (t, 1)))
> + inchash::add_expr (TREE_OPERAND (TREE_OPERAND (t, 0), 0),
> + hstate, flags);
> else
> {
> gcc_assert (IS_EXPR_CODE_CLASS (tclass));
> + unsigned int sflags = flags;
>
> hstate.add_object (code);
>
> + switch (code)
> + {
> + case ADDR_EXPR:
> + gcc_checking_assert (!(flags & OEP_ADDRESS_OF));
> + flags |= OEP_ADDRESS_OF;
> + sflags = flags;
> + break;
> +
> + case INDIRECT_REF:
> + case MEM_REF:
> + case TARGET_MEM_REF:
> + flags &= ~OEP_ADDRESS_OF;
> + sflags = flags;
> + break;
> +
> + case ARRAY_REF:
> + case ARRAY_RANGE_REF:
> + case COMPONENT_REF:
> + case BIT_FIELD_REF:
> + sflags &= ~OEP_ADDRESS_OF;
> + break;
> +
> + case COND_EXPR:
> + flags &= ~OEP_ADDRESS_OF;
> + break;
> +
> + case FMA_EXPR:
> + case WIDEN_MULT_PLUS_EXPR:
> + case WIDEN_MULT_MINUS_EXPR:
> + {
> + /* The multiplication operands are commutative. */
> + inchash::hash one, two;
> + inchash::add_expr (TREE_OPERAND (t, 0), one, flags);
> + inchash::add_expr (TREE_OPERAND (t, 1), two, flags);
> + hstate.add_commutative (one, two);
> + inchash::add_expr (TREE_OPERAND (t, 2), two, flags);
> + return;
> + }
> +
> + case CALL_EXPR:
> + if (CALL_EXPR_FN (t) == NULL_TREE)
> + hstate.add_int (CALL_EXPR_IFN (t));
> + break;
> +
> + default:
> + break;
> + }
> +
> /* Don't hash the type, that can lead to having nodes which
> compare equal according to operand_equal_p, but which
> have different hash codes. */
> - if (CONVERT_EXPR_CODE_P (code)
> - || code == NON_LVALUE_EXPR)
> + if (code == NON_LVALUE_EXPR)
> {
> /* Make sure to include signness in the hash computation. */
> hstate.add_int (TYPE_UNSIGNED (TREE_TYPE (t)));
> - inchash::add_expr (TREE_OPERAND (t, 0), hstate);
> + inchash::add_expr (TREE_OPERAND (t, 0), hstate, flags);
> }
>
> else if (commutative_tree_code (code))
> @@ -7889,13 +7980,14 @@ add_expr (const_tree t, inchash::hash &h
> and then rehashing based on the order of their independent
> hashes. */
> inchash::hash one, two;
> - inchash::add_expr (TREE_OPERAND (t, 0), one);
> - inchash::add_expr (TREE_OPERAND (t, 1), two);
> + inchash::add_expr (TREE_OPERAND (t, 0), one, flags);
> + inchash::add_expr (TREE_OPERAND (t, 1), two, flags);
> hstate.add_commutative (one, two);
> }
> else
> for (i = TREE_OPERAND_LENGTH (t) - 1; i >= 0; --i)
> - inchash::add_expr (TREE_OPERAND (t, i), hstate);
> + inchash::add_expr (TREE_OPERAND (t, i), hstate,
> + i == 0 ? flags : sflags);
> }
> return;
> }
>
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] operand_equal_p checking (PR sanitizer/70683)
2016-04-26 22:51 ` [PATCH] operand_equal_p checking " Jakub Jelinek
@ 2016-04-27 12:41 ` Richard Biener
2016-04-28 8:59 ` Christophe Lyon
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2016-04-27 12:41 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Wed, 27 Apr 2016, Jakub Jelinek wrote:
> On Tue, Apr 26, 2016 at 03:02:38PM +0200, Jakub Jelinek wrote:
> > The debugging hack is too ugly and slows down the compiler (by artificially
> > increasing number of collisions), so it is not appropriate, but perhaps we
> > can add some internal only use OEP_* flag, pass it to the recursive calls
> > of operand_equal_p and if not set and flag_checking, verify
> > iterative_hash_expr equality in the outermost call).
>
> Here is the corresponding checking patch. It uncovered two further issues
> in the tree.[ch] patch which I'm going to post momentarily.
> Both patches together bootstrapped/regtested on x86_64-linux and i686-linux,
> ok for trunk?
Ok.
Thanks,
Richard.
> 2016-04-27 Jakub Jelinek <jakub@redhat.com>
>
> PR sanitizer/70683
> * tree-core.h (enum operand_equal_flag): Add OEP_NO_HASH_CHECK.
> * fold-const.c (operand_equal_p): If flag_checking and
> OEP_NO_HASH_CHECK is not set in flag, recurse with OEP_NO_HASH_CHECK
> and if it returns non-zero, assert iterative_hash_expr on both
> args is the same.
>
> --- gcc/tree-core.h.jj 2016-04-22 18:21:55.000000000 +0200
> +++ gcc/tree-core.h 2016-04-26 17:47:19.875753297 +0200
> @@ -765,7 +765,9 @@ enum operand_equal_flag {
> OEP_ONLY_CONST = 1,
> OEP_PURE_SAME = 2,
> OEP_MATCH_SIDE_EFFECTS = 4,
> - OEP_ADDRESS_OF = 8
> + OEP_ADDRESS_OF = 8,
> + /* Internal within operand_equal_p: */
> + OEP_NO_HASH_CHECK = 16
> };
>
> /* Enum and arrays used for tree allocation stats.
> --- gcc/fold-const.c.jj 2016-04-22 18:21:32.000000000 +0200
> +++ gcc/fold-const.c 2016-04-26 18:30:40.919080701 +0200
> @@ -2749,6 +2749,25 @@ combine_comparisons (location_t loc,
> int
> operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
> {
> + /* When checking, verify at the outermost operand_equal_p call that
> + if operand_equal_p returns non-zero then ARG0 and ARG1 has the same
> + hash value. */
> + if (flag_checking && !(flags & OEP_NO_HASH_CHECK))
> + {
> + if (operand_equal_p (arg0, arg1, flags | OEP_NO_HASH_CHECK))
> + {
> + inchash::hash hstate0 (0), hstate1 (0);
> + inchash::add_expr (arg0, hstate0, flags);
> + inchash::add_expr (arg1, hstate1, flags);
> + hashval_t h0 = hstate0.end ();
> + hashval_t h1 = hstate1.end ();
> + gcc_assert (h0 == h1);
> + return 1;
> + }
> + else
> + return 0;
> + }
> +
> /* If either is ERROR_MARK, they aren't equal. */
> if (TREE_CODE (arg0) == ERROR_MARK || TREE_CODE (arg1) == ERROR_MARK
> || TREE_TYPE (arg0) == error_mark_node
>
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] operand_equal_p checking (PR sanitizer/70683)
2016-04-27 12:41 ` Richard Biener
@ 2016-04-28 8:59 ` Christophe Lyon
0 siblings, 0 replies; 6+ messages in thread
From: Christophe Lyon @ 2016-04-28 8:59 UTC (permalink / raw)
To: Richard Biener; +Cc: Jakub Jelinek, gcc-patches
Hi,
This caused: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70843
On 27 April 2016 at 14:41, Richard Biener <rguenther@suse.de> wrote:
> On Wed, 27 Apr 2016, Jakub Jelinek wrote:
>
>> On Tue, Apr 26, 2016 at 03:02:38PM +0200, Jakub Jelinek wrote:
>> > The debugging hack is too ugly and slows down the compiler (by artificially
>> > increasing number of collisions), so it is not appropriate, but perhaps we
>> > can add some internal only use OEP_* flag, pass it to the recursive calls
>> > of operand_equal_p and if not set and flag_checking, verify
>> > iterative_hash_expr equality in the outermost call).
>>
>> Here is the corresponding checking patch. It uncovered two further issues
>> in the tree.[ch] patch which I'm going to post momentarily.
>> Both patches together bootstrapped/regtested on x86_64-linux and i686-linux,
>> ok for trunk?
>
> Ok.
>
> Thanks,
> Richard.
>
>> 2016-04-27 Jakub Jelinek <jakub@redhat.com>
>>
>> PR sanitizer/70683
>> * tree-core.h (enum operand_equal_flag): Add OEP_NO_HASH_CHECK.
>> * fold-const.c (operand_equal_p): If flag_checking and
>> OEP_NO_HASH_CHECK is not set in flag, recurse with OEP_NO_HASH_CHECK
>> and if it returns non-zero, assert iterative_hash_expr on both
>> args is the same.
>>
>> --- gcc/tree-core.h.jj 2016-04-22 18:21:55.000000000 +0200
>> +++ gcc/tree-core.h 2016-04-26 17:47:19.875753297 +0200
>> @@ -765,7 +765,9 @@ enum operand_equal_flag {
>> OEP_ONLY_CONST = 1,
>> OEP_PURE_SAME = 2,
>> OEP_MATCH_SIDE_EFFECTS = 4,
>> - OEP_ADDRESS_OF = 8
>> + OEP_ADDRESS_OF = 8,
>> + /* Internal within operand_equal_p: */
>> + OEP_NO_HASH_CHECK = 16
>> };
>>
>> /* Enum and arrays used for tree allocation stats.
>> --- gcc/fold-const.c.jj 2016-04-22 18:21:32.000000000 +0200
>> +++ gcc/fold-const.c 2016-04-26 18:30:40.919080701 +0200
>> @@ -2749,6 +2749,25 @@ combine_comparisons (location_t loc,
>> int
>> operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
>> {
>> + /* When checking, verify at the outermost operand_equal_p call that
>> + if operand_equal_p returns non-zero then ARG0 and ARG1 has the same
>> + hash value. */
>> + if (flag_checking && !(flags & OEP_NO_HASH_CHECK))
>> + {
>> + if (operand_equal_p (arg0, arg1, flags | OEP_NO_HASH_CHECK))
>> + {
>> + inchash::hash hstate0 (0), hstate1 (0);
>> + inchash::add_expr (arg0, hstate0, flags);
>> + inchash::add_expr (arg1, hstate1, flags);
>> + hashval_t h0 = hstate0.end ();
>> + hashval_t h1 = hstate1.end ();
>> + gcc_assert (h0 == h1);
>> + return 1;
>> + }
>> + else
>> + return 0;
>> + }
>> +
>> /* If either is ERROR_MARK, they aren't equal. */
>> if (TREE_CODE (arg0) == ERROR_MARK || TREE_CODE (arg1) == ERROR_MARK
>> || TREE_TYPE (arg0) == error_mark_node
>>
>>
>> Jakub
>>
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-04-28 8:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-26 13:02 [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683) Jakub Jelinek
2016-04-26 22:51 ` [PATCH] operand_equal_p checking " Jakub Jelinek
2016-04-27 12:41 ` Richard Biener
2016-04-28 8:59 ` Christophe Lyon
2016-04-26 23:00 ` [PATCH] Fix up inchash::add_expr to match more closely operand_equal_p (PR sanitizer/70683, take 2) Jakub Jelinek
2016-04-27 7:41 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).