| Message ID | 20210427131654.GA1179226@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Fix UB in the compiler [PR100200] | expand |
Jakub Jelinek <jakub@redhat.com> writes: > Hi! > > The following patch fixes UBs in the compiler when negativing > a CONST_INT containing HOST_WIDE_INT_MIN. I've changed the spots where > there wasn't an obvious earlier condition check or predicate that > would fail for such CONST_INTs. > > Bootstrapped/regtested on aarch64-linux, ok for trunk? > > 2021-04-27 Jakub Jelinek <jakub@redhat.com> > > PR target/100200 > * config/aarch64/predicates.md (aarch64_sub_immediate, > aarch64_plus_immediate): Use -UINTVAL instead of -INTVAL. > * config/aarch64/aarch64.md (casesi, rotl<mode>3): Likewise. > * config/aarch64/aarch64.c (aarch64_print_operand, > aarch64_split_atomic_op, aarch64_expand_subvti): Likewise. OK, thanks. Richard > --- gcc/config/aarch64/predicates.md.jj 2021-03-08 23:40:33.852448350 +0100 > +++ gcc/config/aarch64/predicates.md 2021-04-26 16:35:59.810535063 +0200 > @@ -121,12 +121,12 @@ (define_predicate "aarch64_sve_cnt_immed > > (define_predicate "aarch64_sub_immediate" > (and (match_code "const_int") > - (match_test "aarch64_uimm12_shift (-INTVAL (op))"))) > + (match_test "aarch64_uimm12_shift (-UINTVAL (op))"))) > > (define_predicate "aarch64_plus_immediate" > (and (match_code "const_int") > (ior (match_test "aarch64_uimm12_shift (INTVAL (op))") > - (match_test "aarch64_uimm12_shift (-INTVAL (op))")))) > + (match_test "aarch64_uimm12_shift (-UINTVAL (op))")))) > > (define_predicate "aarch64_plus_operand" > (ior (match_operand 0 "register_operand") > --- gcc/config/aarch64/aarch64.md.jj 2021-04-16 13:44:02.586682325 +0200 > +++ gcc/config/aarch64/aarch64.md 2021-04-26 16:40:02.202777429 +0200 > @@ -747,7 +747,8 @@ (define_expand "casesi" > constant can be represented in SImode, this is important > for the corner case where operand[1] is INT_MIN. */ > > - operands[1] = GEN_INT (trunc_int_for_mode (-INTVAL (operands[1]), SImode)); > + operands[1] > + = GEN_INT (trunc_int_for_mode (-UINTVAL (operands[1]), SImode)); > > if (!(*insn_data[CODE_FOR_addsi3].operand[2].predicate) > (operands[1], SImode)) > @@ -5008,7 +5009,7 @@ (define_expand "rotl<mode>3" > /* (SZ - cnt) % SZ == -cnt % SZ */ > if (CONST_INT_P (operands[2])) > { > - operands[2] = GEN_INT ((-INTVAL (operands[2])) > + operands[2] = GEN_INT ((-UINTVAL (operands[2])) > & (GET_MODE_BITSIZE (<MODE>mode) - 1)); > if (operands[2] == const0_rtx) > { > --- gcc/config/aarch64/aarch64.c.jj 2021-04-16 20:49:23.602579852 +0200 > +++ gcc/config/aarch64/aarch64.c 2021-04-26 16:40:46.835269671 +0200 > @@ -10778,7 +10778,7 @@ aarch64_print_operand (FILE *f, rtx x, i > } > > if (GET_MODE_CLASS (GET_MODE (x)) == MODE_VECTOR_INT) > - asm_fprintf (f, "%wd", -INTVAL (elt)); > + asm_fprintf (f, "%wd", -UINTVAL (elt)); > else if (GET_MODE_CLASS (GET_MODE (x)) == MODE_VECTOR_FLOAT > && aarch64_print_vector_float_operand (f, x, true)) > ; > @@ -21598,7 +21598,7 @@ aarch64_split_atomic_op (enum rtx_code c > case MINUS: > if (CONST_INT_P (value)) > { > - value = GEN_INT (-INTVAL (value)); > + value = GEN_INT (-UINTVAL (value)); > code = PLUS; > } > /* Fall through. */ > @@ -23514,7 +23514,7 @@ aarch64_expand_subvti (rtx op0, rtx low_ > { > if (aarch64_plus_immediate (low_in2, DImode)) > emit_insn (gen_subdi3_compare1_imm (low_dest, low_in1, low_in2, > - GEN_INT (-INTVAL (low_in2)))); > + GEN_INT (-UINTVAL (low_in2)))); > else > { > low_in2 = force_reg (DImode, low_in2); > > Jakub
On 27/04/2021 14:16, Jakub Jelinek via Gcc-patches wrote: > Hi! > > The following patch fixes UBs in the compiler when negativing > a CONST_INT containing HOST_WIDE_INT_MIN. I've changed the spots where > there wasn't an obvious earlier condition check or predicate that > would fail for such CONST_INTs. > > Bootstrapped/regtested on aarch64-linux, ok for trunk? > > 2021-04-27 Jakub Jelinek <jakub@redhat.com> > > PR target/100200 > * config/aarch64/predicates.md (aarch64_sub_immediate, > aarch64_plus_immediate): Use -UINTVAL instead of -INTVAL. > * config/aarch64/aarch64.md (casesi, rotl<mode>3): Likewise. > * config/aarch64/aarch64.c (aarch64_print_operand, > aarch64_split_atomic_op, aarch64_expand_subvti): Likewise. > > --- gcc/config/aarch64/predicates.md.jj 2021-03-08 23:40:33.852448350 +0100 > +++ gcc/config/aarch64/predicates.md 2021-04-26 16:35:59.810535063 +0200 > @@ -121,12 +121,12 @@ (define_predicate "aarch64_sve_cnt_immed > > (define_predicate "aarch64_sub_immediate" > (and (match_code "const_int") > - (match_test "aarch64_uimm12_shift (-INTVAL (op))"))) > + (match_test "aarch64_uimm12_shift (-UINTVAL (op))"))) > > (define_predicate "aarch64_plus_immediate" > (and (match_code "const_int") > (ior (match_test "aarch64_uimm12_shift (INTVAL (op))") > - (match_test "aarch64_uimm12_shift (-INTVAL (op))")))) > + (match_test "aarch64_uimm12_shift (-UINTVAL (op))")))) > > (define_predicate "aarch64_plus_operand" > (ior (match_operand 0 "register_operand") > --- gcc/config/aarch64/aarch64.md.jj 2021-04-16 13:44:02.586682325 +0200 > +++ gcc/config/aarch64/aarch64.md 2021-04-26 16:40:02.202777429 +0200 > @@ -747,7 +747,8 @@ (define_expand "casesi" > constant can be represented in SImode, this is important > for the corner case where operand[1] is INT_MIN. */ > > - operands[1] = GEN_INT (trunc_int_for_mode (-INTVAL (operands[1]), SImode)); > + operands[1] > + = GEN_INT (trunc_int_for_mode (-UINTVAL (operands[1]), SImode)); > > if (!(*insn_data[CODE_FOR_addsi3].operand[2].predicate) > (operands[1], SImode)) > @@ -5008,7 +5009,7 @@ (define_expand "rotl<mode>3" > /* (SZ - cnt) % SZ == -cnt % SZ */ > if (CONST_INT_P (operands[2])) > { > - operands[2] = GEN_INT ((-INTVAL (operands[2])) > + operands[2] = GEN_INT ((-UINTVAL (operands[2])) > & (GET_MODE_BITSIZE (<MODE>mode) - 1)); > if (operands[2] == const0_rtx) > { > --- gcc/config/aarch64/aarch64.c.jj 2021-04-16 20:49:23.602579852 +0200 > +++ gcc/config/aarch64/aarch64.c 2021-04-26 16:40:46.835269671 +0200 > @@ -10778,7 +10778,7 @@ aarch64_print_operand (FILE *f, rtx x, i > } > > if (GET_MODE_CLASS (GET_MODE (x)) == MODE_VECTOR_INT) > - asm_fprintf (f, "%wd", -INTVAL (elt)); > + asm_fprintf (f, "%wd", -UINTVAL (elt)); This one looks wrong as you're now passing a UHWI to asm_fprintf when the format expects HWI. I think you need a cast back to HWI. R. > else if (GET_MODE_CLASS (GET_MODE (x)) == MODE_VECTOR_FLOAT > && aarch64_print_vector_float_operand (f, x, true)) > ; > @@ -21598,7 +21598,7 @@ aarch64_split_atomic_op (enum rtx_code c > case MINUS: > if (CONST_INT_P (value)) > { > - value = GEN_INT (-INTVAL (value)); > + value = GEN_INT (-UINTVAL (value)); > code = PLUS; > } > /* Fall through. */ > @@ -23514,7 +23514,7 @@ aarch64_expand_subvti (rtx op0, rtx low_ > { > if (aarch64_plus_immediate (low_in2, DImode)) > emit_insn (gen_subdi3_compare1_imm (low_dest, low_in1, low_in2, > - GEN_INT (-INTVAL (low_in2)))); > + GEN_INT (-UINTVAL (low_in2)))); > else > { > low_in2 = force_reg (DImode, low_in2); > > Jakub >
On Tue, Apr 27, 2021 at 03:20:47PM +0100, Richard Earnshaw via Gcc-patches wrote: > > --- gcc/config/aarch64/aarch64.c.jj 2021-04-16 20:49:23.602579852 +0200 > > +++ gcc/config/aarch64/aarch64.c 2021-04-26 16:40:46.835269671 +0200 > > @@ -10778,7 +10778,7 @@ aarch64_print_operand (FILE *f, rtx x, i > > } > > if (GET_MODE_CLASS (GET_MODE (x)) == MODE_VECTOR_INT) > > - asm_fprintf (f, "%wd", -INTVAL (elt)); > > + asm_fprintf (f, "%wd", -UINTVAL (elt)); > > This one looks wrong as you're now passing a UHWI to asm_fprintf when the > format expects HWI. I think you need a cast back to HWI. I'm not aware of any compiler that would care, but it is true that at least C99 says on va_arg: "or if type is not compatible with the type of the actual next argument (as promoted according to the default argument promotions), the behavior is undefined, except for the following cases: — one type is a signed integer type, the other type is the corresponding unsigned integer type, and the value is representable in both types; — one type is pointer to void and the other is a pointer to a character type." So for pedantic reasons I should add the cast. Note, -Wformat* doesn't warn on this. 2021-04-27 Jakub Jelinek <jakub@redhat.com> PR target/100200 * config/aarch64/aarch64.c (aarch64_print_operand): Cast -UINTVAL back to HOST_WIDE_INT. --- gcc/config/aarch64/aarch64.c.jj 2021-04-27 15:46:13.477346613 +0200 +++ gcc/config/aarch64/aarch64.c 2021-04-27 16:50:27.335821271 +0200 @@ -10778,7 +10778,7 @@ aarch64_print_operand (FILE *f, rtx x, i } if (GET_MODE_CLASS (GET_MODE (x)) == MODE_VECTOR_INT) - asm_fprintf (f, "%wd", -UINTVAL (elt)); + asm_fprintf (f, "%wd", (HOST_WIDE_INT) -UINTVAL (elt)); else if (GET_MODE_CLASS (GET_MODE (x)) == MODE_VECTOR_FLOAT && aarch64_print_vector_float_operand (f, x, true)) ; Jakub
--- gcc/config/aarch64/predicates.md.jj 2021-03-08 23:40:33.852448350 +0100 +++ gcc/config/aarch64/predicates.md 2021-04-26 16:35:59.810535063 +0200 @@ -121,12 +121,12 @@ (define_predicate "aarch64_sve_cnt_immed (define_predicate "aarch64_sub_immediate" (and (match_code "const_int") - (match_test "aarch64_uimm12_shift (-INTVAL (op))"))) + (match_test "aarch64_uimm12_shift (-UINTVAL (op))"))) (define_predicate "aarch64_plus_immediate" (and (match_code "const_int") (ior (match_test "aarch64_uimm12_shift (INTVAL (op))") - (match_test "aarch64_uimm12_shift (-INTVAL (op))")))) + (match_test "aarch64_uimm12_shift (-UINTVAL (op))")))) (define_predicate "aarch64_plus_operand" (ior (match_operand 0 "register_operand") --- gcc/config/aarch64/aarch64.md.jj 2021-04-16 13:44:02.586682325 +0200 +++ gcc/config/aarch64/aarch64.md 2021-04-26 16:40:02.202777429 +0200 @@ -747,7 +747,8 @@ (define_expand "casesi" constant can be represented in SImode, this is important for the corner case where operand[1] is INT_MIN. */ - operands[1] = GEN_INT (trunc_int_for_mode (-INTVAL (operands[1]), SImode)); + operands[1] + = GEN_INT (trunc_int_for_mode (-UINTVAL (operands[1]), SImode)); if (!(*insn_data[CODE_FOR_addsi3].operand[2].predicate) (operands[1], SImode)) @@ -5008,7 +5009,7 @@ (define_expand "rotl<mode>3" /* (SZ - cnt) % SZ == -cnt % SZ */ if (CONST_INT_P (operands[2])) { - operands[2] = GEN_INT ((-INTVAL (operands[2])) + operands[2] = GEN_INT ((-UINTVAL (operands[2])) & (GET_MODE_BITSIZE (<MODE>mode) - 1)); if (operands[2] == const0_rtx) { --- gcc/config/aarch64/aarch64.c.jj 2021-04-16 20:49:23.602579852 +0200 +++ gcc/config/aarch64/aarch64.c 2021-04-26 16:40:46.835269671 +0200 @@ -10778,7 +10778,7 @@ aarch64_print_operand (FILE *f, rtx x, i } if (GET_MODE_CLASS (GET_MODE (x)) == MODE_VECTOR_INT) - asm_fprintf (f, "%wd", -INTVAL (elt)); + asm_fprintf (f, "%wd", -UINTVAL (elt)); else if (GET_MODE_CLASS (GET_MODE (x)) == MODE_VECTOR_FLOAT && aarch64_print_vector_float_operand (f, x, true)) ; @@ -21598,7 +21598,7 @@ aarch64_split_atomic_op (enum rtx_code c case MINUS: if (CONST_INT_P (value)) { - value = GEN_INT (-INTVAL (value)); + value = GEN_INT (-UINTVAL (value)); code = PLUS; } /* Fall through. */ @@ -23514,7 +23514,7 @@ aarch64_expand_subvti (rtx op0, rtx low_ { if (aarch64_plus_immediate (low_in2, DImode)) emit_insn (gen_subdi3_compare1_imm (low_dest, low_in1, low_in2, - GEN_INT (-INTVAL (low_in2)))); + GEN_INT (-UINTVAL (low_in2)))); else { low_in2 = force_reg (DImode, low_in2);