This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix various undefined behaviors in GCC discovered by -fsanitize=undefined (PR other/59545)
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Jakub Jelinek <jakub at redhat dot com>
- Cc: Richard Biener <rguenther at suse dot de>, Tom Tromey <tromey at redhat dot com>, Andrew Haley <aph at redhat dot com>, Vladimir Makarov <vmakarov at redhat dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 19 Dec 2013 21:49:36 +0100
- Subject: Re: [PATCH] Fix various undefined behaviors in GCC discovered by -fsanitize=undefined (PR other/59545)
- Authentication-results: sourceware.org; auth=none
- References: <20131219204238 dot GB892 at tucnak dot redhat dot com>
On Thu, Dec 19, 2013 at 9:42 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> This patch is an attempt to fix various signed integer overflows,
> invalid shifts and loads of uninialized bool value.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Thanks,
Richard.
> This is solely from gcc build, make check has other issues too, but I'd
> prefer to do it incrementally, because otherwise there are way too many
> errors everywhere.
>
> There are also two issues I've left unfixed, Andrew/Tom and Vlad, can you
> please have a look?
>
> The first one is in java/boehm.c:
>
> java/boehm.c:
> /* First word in object corresponds to most significant byte of
> bitmap.
>
> In the case of a multiple-word record, we set pointer
> bits for all words in the record. This is conservative, but the
> size_words != 1 case is impossible in regular java code. */
> for (i = 0; i < size_words; ++i)
> *mask = (*mask).set_bit (ubit - count - i - 1);
>
> (gdb) p ubit
> $1 = 64
> (gdb) p count
> $2 = 67
> (gdb) p i
> $3 = 0
>
> *mask is double_int, so set_bit has only 0 to HOST_BITS_PER_DOUBLE_INT - 1
> valid arguments, but in this case ubit - count - i - 1 is e.g. -4
> (but as the value is unsigned, it is just very large number). I have no
> idea what this code is meant to do, Andrew/Tom, could you please fix this
> up?
>
> ira-color.c:
> if (index < 0)
> continue;
> cost = conflict_costs [i] * mult / div;
> if (cost == 0)
> continue;
>
> ../../gcc/ira-color.c:1508:29: runtime error: signed integer overflow: -65535000 * 1000 cannot be represented in type 'int'
> ../../gcc/ira-color.c:1508:29: runtime error: signed integer overflow: -65535000 * 61 cannot be represented in type 'int'
> ../../gcc/ira-color.c:1508:29: runtime error: signed integer overflow: -71760825 * 976 cannot be represented in type 'int'
> ../../gcc/ira-color.c:1508:29: runtime error: signed integer overflow: -7659400 * 394 cannot be represented in type 'int'
>
> (hundreds of similar messages). I have no idea if negative and so large
> conflict_costs are valid, whether overflow is ok (then perhaps it should be
> unsigned rather than int multiplication) etc. Vlad, can you please check it
> out?
>
> 2013-12-19 Jakub Jelinek <jakub@redhat.com>
>
> PR other/59545
> * genattrtab.c (struct attr_hash): Change hashcode type to unsigned.
> (attr_hash_add_rtx, attr_hash_add_string): Change hashcode parameter
> to unsigned.
> (attr_rtx_1): Change hashcode variable to unsigned.
> (attr_string): Likewise. Perform first multiplication in unsigned
> type.
> * ifcvt.c (noce_try_store_flag_constants): Avoid signed integer
> overflows.
> * double-int.c (neg_double): Likewise.
> * stor-layout.c (set_min_and_max_values_for_integral_type): Likewise.
> * combine.c (force_to_mode): Likewise.
> * postreload.c (move2add_use_add2_insn, move2add_use_add3_insn,
> reload_cse_move2add, move2add_note_store): Likewise.
> * simplify-rtx.c (simplify_const_unary_operation,
> simplify_const_binary_operation): Likewise.
> * ipa-split.c (find_split_points): Initialize first.can_split
> and first.non_ssa_vars.
> * gengtype-state.c (read_state_files_list): Fix up check.
> * genautomata.c (reserv_sets_hash_value): Use portable rotation
> idiom.
> java/
> * class.c (hashUtf8String): Compute hash in unsigned type.
> * javaop.h (WORD_TO_INT): Avoid signed integer overflow.
>
> --- gcc/genattrtab.c.jj 2013-11-19 21:56:29.000000000 +0100
> +++ gcc/genattrtab.c 2013-12-19 16:32:23.758030495 +0100
> @@ -320,7 +320,7 @@ static FILE *attr_file, *dfa_file, *late
> struct attr_hash
> {
> struct attr_hash *next; /* Next structure in the bucket. */
> - int hashcode; /* Hash code of this rtx or string. */
> + unsigned int hashcode; /* Hash code of this rtx or string. */
> union
> {
> char *str; /* The string (negative hash codes) */
> @@ -345,7 +345,7 @@ static struct attr_hash *attr_hash_table
> /* Add an entry to the hash table for RTL with hash code HASHCODE. */
>
> static void
> -attr_hash_add_rtx (int hashcode, rtx rtl)
> +attr_hash_add_rtx (unsigned int hashcode, rtx rtl)
> {
> struct attr_hash *h;
>
> @@ -359,7 +359,7 @@ attr_hash_add_rtx (int hashcode, rtx rtl
> /* Add an entry to the hash table for STRING with hash code HASHCODE. */
>
> static void
> -attr_hash_add_string (int hashcode, char *str)
> +attr_hash_add_string (unsigned int hashcode, char *str)
> {
> struct attr_hash *h;
>
> @@ -384,7 +384,7 @@ static rtx
> attr_rtx_1 (enum rtx_code code, va_list p)
> {
> rtx rt_val = NULL_RTX;/* RTX to return to caller... */
> - int hashcode;
> + unsigned int hashcode;
> struct attr_hash *h;
> struct obstack *old_obstack = rtl_obstack;
>
> @@ -612,15 +612,15 @@ static char *
> attr_string (const char *str, int len)
> {
> struct attr_hash *h;
> - int hashcode;
> + unsigned int hashcode;
> int i;
> char *new_str;
>
> /* Compute the hash code. */
> - hashcode = (len + 1) * 613 + (unsigned) str[0];
> + hashcode = (len + 1) * 613U + (unsigned) str[0];
> for (i = 1; i < len; i += 2)
> hashcode = ((hashcode * 613) + (unsigned) str[i]);
> - if (hashcode < 0)
> + if ((int) hashcode < 0)
> hashcode = -hashcode;
>
> /* Search the table for the string. */
> --- gcc/ifcvt.c.jj 2013-12-19 09:03:11.000000000 +0100
> +++ gcc/ifcvt.c 2013-12-19 16:20:32.849650410 +0100
> @@ -1112,12 +1112,13 @@ noce_try_store_flag_constants (struct no
> ifalse = INTVAL (if_info->a);
> itrue = INTVAL (if_info->b);
>
> + diff = (unsigned HOST_WIDE_INT) itrue - ifalse;
> /* Make sure we can represent the difference between the two values. */
> - if ((itrue - ifalse > 0)
> + if ((diff > 0)
> != ((ifalse < 0) != (itrue < 0) ? ifalse < 0 : ifalse < itrue))
> return FALSE;
>
> - diff = trunc_int_for_mode (itrue - ifalse, mode);
> + diff = trunc_int_for_mode (diff, mode);
>
> can_reverse = (reversed_comparison_code (if_info->cond, if_info->jump)
> != UNKNOWN);
> @@ -1148,7 +1149,7 @@ noce_try_store_flag_constants (struct no
> if (reversep)
> {
> tmp = itrue; itrue = ifalse; ifalse = tmp;
> - diff = trunc_int_for_mode (-diff, mode);
> + diff = trunc_int_for_mode (-(unsigned HOST_WIDE_INT) diff, mode);
> }
>
> start_sequence ();
> --- gcc/double-int.c.jj 2013-11-12 11:31:22.000000000 +0100
> +++ gcc/double-int.c 2013-12-19 16:36:43.023408200 +0100
> @@ -138,7 +138,7 @@ neg_double (unsigned HOST_WIDE_INT l1, H
> if (l1 == 0)
> {
> *lv = 0;
> - *hv = - h1;
> + *hv = - (unsigned HOST_WIDE_INT) h1;
> return (*hv & h1) < 0;
> }
> else
> --- gcc/stor-layout.c.jj 2013-12-02 14:33:34.000000000 +0100
> +++ gcc/stor-layout.c 2013-12-19 17:07:32.379722616 +0100
> @@ -2521,7 +2521,7 @@ set_min_and_max_values_for_integral_type
> max_value
> = build_int_cst_wide (type, precision - HOST_BITS_PER_WIDE_INT >= 0
> ? -1
> - : ((HOST_WIDE_INT) 1 << precision) - 1,
> + : (HOST_WIDE_INT_1U << precision) - 1,
> precision - HOST_BITS_PER_WIDE_INT > 0
> ? ((unsigned HOST_WIDE_INT) ~0
> >> (HOST_BITS_PER_WIDE_INT
> @@ -2534,7 +2534,7 @@ set_min_and_max_values_for_integral_type
> = build_int_cst_wide (type,
> (precision - HOST_BITS_PER_WIDE_INT > 0
> ? 0
> - : (HOST_WIDE_INT) (-1) << (precision - 1)),
> + : HOST_WIDE_INT_M1U << (precision - 1)),
> (((HOST_WIDE_INT) (-1)
> << (precision - HOST_BITS_PER_WIDE_INT - 1 > 0
> ? precision - HOST_BITS_PER_WIDE_INT - 1
> --- gcc/gengtype-state.c.jj 2013-11-12 11:31:10.000000000 +0100
> +++ gcc/gengtype-state.c 2013-12-19 15:04:53.000000000 +0100
> @@ -2651,7 +2651,7 @@ read_state_files_list (void)
> "expecting file in !fileslist of state file");
> };
> t0 = peek_state_token (0);
> - if (!state_token_kind (t0) == STOK_RIGHTPAR)
> + if (state_token_kind (t0) != STOK_RIGHTPAR)
> fatal_reading_state (t0, "missing ) for !fileslist in state file");
> next_state_tokens (1);
> }
> --- gcc/ipa-split.c.jj 2013-12-18 17:32:59.000000000 +0100
> +++ gcc/ipa-split.c 2013-12-19 15:30:14.000000000 +0100
> @@ -950,7 +950,9 @@ find_split_points (int overall_time, int
> first.earliest = INT_MAX;
> first.set_ssa_names = 0;
> first.used_ssa_names = 0;
> + first.non_ssa_vars = 0;
> first.bbs_visited = 0;
> + first.can_split = false;
> stack.safe_push (first);
> ENTRY_BLOCK_PTR_FOR_FN (cfun)->aux = (void *)(intptr_t)-1;
>
> --- gcc/java/class.c.jj 2013-11-22 21:03:05.000000000 +0100
> +++ gcc/java/class.c 2013-12-19 15:40:12.699793621 +0100
> @@ -920,7 +920,7 @@ hashUtf8String (const char *str, int len
> {
> const unsigned char* ptr = (const unsigned char*) str;
> const unsigned char *limit = ptr + len;
> - int32 hash = 0;
> + uint32 hash = 0;
> for (; ptr < limit;)
> {
> int ch = UTF8_GET (ptr, limit);
> --- gcc/java/javaop.h.jj 2013-01-11 09:02:30.000000000 +0100
> +++ gcc/java/javaop.h 2013-12-19 15:54:46.963380710 +0100
> @@ -154,7 +154,7 @@ WORD_TO_INT(jword w)
> {
> jint n = w & 0xffffffff; /* Mask lower 32 bits. */
> n ^= (jint)1 << 31;
> - n -= (jint)1 << 31; /* Sign extend lower 32 bits to upper. */
> + n -= (uint32)1 << 31; /* Sign extend lower 32 bits to upper. */
> return n;
> }
>
> --- gcc/combine.c.jj 2013-12-10 08:52:13.000000000 +0100
> +++ gcc/combine.c 2013-12-19 17:14:05.121706321 +0100
> @@ -8200,9 +8200,7 @@ force_to_mode (rtx x, enum machine_mode
> /* If X is (minus C Y) where C's least set bit is larger than any bit
> in the mask, then we may replace with (neg Y). */
> if (CONST_INT_P (XEXP (x, 0))
> - && (((unsigned HOST_WIDE_INT) (INTVAL (XEXP (x, 0))
> - & -INTVAL (XEXP (x, 0))))
> - > mask))
> + && ((UINTVAL (XEXP (x, 0)) & -UINTVAL (XEXP (x, 0))) > mask))
> {
> x = simplify_gen_unary (NEG, GET_MODE (x), XEXP (x, 1),
> GET_MODE (x));
> --- gcc/genautomata.c.jj 2013-11-22 13:15:54.000000000 +0100
> +++ gcc/genautomata.c 2013-12-19 16:33:17.882757724 +0100
> @@ -3494,7 +3494,7 @@ reserv_sets_hash_value (reserv_sets_t re
> {
> reservs_num--;
> hash_value += ((*reserv_ptr >> i)
> - | (*reserv_ptr << (sizeof (set_el_t) * CHAR_BIT - i)));
> + | (*reserv_ptr << ((sizeof (set_el_t) * CHAR_BIT) & -i)));
> i++;
> if (i == sizeof (set_el_t) * CHAR_BIT)
> i = 0;
> --- gcc/postreload.c.jj 2013-12-10 08:52:06.000000000 +0100
> +++ gcc/postreload.c 2013-12-19 16:59:18.592251929 +0100
> @@ -1766,7 +1766,7 @@ move2add_use_add2_insn (rtx reg, rtx sym
> rtx pat = PATTERN (insn);
> rtx src = SET_SRC (pat);
> int regno = REGNO (reg);
> - rtx new_src = gen_int_mode (INTVAL (off) - reg_offset[regno],
> + rtx new_src = gen_int_mode (UINTVAL (off) - reg_offset[regno],
> GET_MODE (reg));
> bool speed = optimize_bb_for_speed_p (BLOCK_FOR_INSN (insn));
> bool changed = false;
> @@ -1866,7 +1866,7 @@ move2add_use_add3_insn (rtx reg, rtx sym
> && reg_symbol_ref[i] != NULL_RTX
> && rtx_equal_p (sym, reg_symbol_ref[i]))
> {
> - rtx new_src = gen_int_mode (INTVAL (off) - reg_offset[i],
> + rtx new_src = gen_int_mode (UINTVAL (off) - reg_offset[i],
> GET_MODE (reg));
> /* (set (reg) (plus (reg) (const_int 0))) is not canonical;
> use (set (reg) (reg)) instead.
> @@ -1901,7 +1901,7 @@ move2add_use_add3_insn (rtx reg, rtx sym
> tem = gen_rtx_REG (GET_MODE (reg), min_regno);
> if (i != min_regno)
> {
> - rtx new_src = gen_int_mode (INTVAL (off) - reg_offset[min_regno],
> + rtx new_src = gen_int_mode (UINTVAL (off) - reg_offset[min_regno],
> GET_MODE (reg));
> tem = gen_rtx_PLUS (GET_MODE (reg), tem, new_src);
> }
> @@ -2010,7 +2010,7 @@ reload_cse_move2add (rtx first)
> && CONST_INT_P (XEXP (SET_SRC (set), 1)))
> {
> rtx src3 = XEXP (SET_SRC (set), 1);
> - HOST_WIDE_INT added_offset = INTVAL (src3);
> + unsigned HOST_WIDE_INT added_offset = UINTVAL (src3);
> HOST_WIDE_INT base_offset = reg_offset[REGNO (src)];
> HOST_WIDE_INT regno_offset = reg_offset[regno];
> rtx new_src =
> @@ -2224,7 +2224,7 @@ move2add_note_store (rtx dst, const_rtx
> {
> rtx src = SET_SRC (set);
> rtx base_reg;
> - HOST_WIDE_INT offset;
> + unsigned HOST_WIDE_INT offset;
> int base_regno;
>
> switch (GET_CODE (src))
> @@ -2235,7 +2235,7 @@ move2add_note_store (rtx dst, const_rtx
> base_reg = XEXP (src, 0);
>
> if (CONST_INT_P (XEXP (src, 1)))
> - offset = INTVAL (XEXP (src, 1));
> + offset = UINTVAL (XEXP (src, 1));
> else if (REG_P (XEXP (src, 1))
> && move2add_valid_value_p (REGNO (XEXP (src, 1)), mode))
> {
> --- gcc/simplify-rtx.c.jj 2013-12-11 10:11:06.000000000 +0100
> +++ gcc/simplify-rtx.c 2013-12-19 17:03:04.749092616 +0100
> @@ -1647,7 +1647,7 @@ simplify_const_unary_operation (enum rtx
> break;
>
> case NEG:
> - val = - arg0;
> + val = - (unsigned HOST_WIDE_INT) arg0;
> break;
>
> case ABS:
> @@ -4117,15 +4117,15 @@ simplify_const_binary_operation (enum rt
> switch (code)
> {
> case PLUS:
> - val = arg0s + arg1s;
> + val = (unsigned HOST_WIDE_INT) arg0s + arg1s;
> break;
>
> case MINUS:
> - val = arg0s - arg1s;
> + val = (unsigned HOST_WIDE_INT) arg0s - arg1s;
> break;
>
> case MULT:
> - val = arg0s * arg1s;
> + val = (unsigned HOST_WIDE_INT) arg0s * arg1s;
> break;
>
> case DIV:
>
> Jakub