| Message ID | 20181016091727.14213-1-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | lra: fix spill_hard_reg_in_range clobber check | expand |
On 10/16/18 3:17 AM, Ilya Leoshkevich wrote: > Bootstrap and regtest are running on x86_64-redhat-linux. > > Insns in FROM..TO range might not be recognized yet, and their > corresponding entries in lra_insn_recog_data[] might be NULLs. In the > code from PR87596 the problematic insn is merely > > (note 148 154 68 7 NOTE_INSN_DELETED) > > however I assume that non-note insns may occur as well. So make sure > they are recognized before touching their lra_insn_recog_data_t. > > gcc/ChangeLog: > > 2018-10-16 Ilya Leoshkevich <iii@linux.ibm.com> > > PR rtl-optimization/87596 > * lra-constraints.c (spill_hard_reg_in_range): Use > lra_get_insn_recog_data () instead of lra_insn_recog_data[] > for instructions in FROM..TO range. > > gcc/testsuite/ChangeLog: > > 2018-10-16 Ilya Leoshkevich <iii@linux.ibm.com> > > PR rtl-optimization/87596 > * gcc.target/i386/pr87596.c: New test. I think this is just papering over the problem. AFAICT for something like a deleted note lra_get_insn_recog_data is going to call lra_set_insn_recog_data. Given that INSN will be a NOTE I think icode will end up being -1 and we'll eventually get into this clause: if (icode < 0) { int nop, nalt; machine_mode operand_mode[MAX_RECOG_OPERANDS]; const char *constraints[MAX_RECOG_OPERANDS]; nop = asm_noperands (PATTERN (insn)); And I don't think we should be trying to extract the PATTERN of a note. I think you need to either filter out the non-insn thingies in spill_hard_reg_in_range, or go with your patch plus beefing up the checks in lra_set_insn_recog_data. jeff
> Am 16.10.2018 um 16:20 schrieb Jeff Law <law@redhat.com>: > > On 10/16/18 3:17 AM, Ilya Leoshkevich wrote: >> Bootstrap and regtest are running on x86_64-redhat-linux. >> >> Insns in FROM..TO range might not be recognized yet, and their >> corresponding entries in lra_insn_recog_data[] might be NULLs. In the >> code from PR87596 the problematic insn is merely >> >> (note 148 154 68 7 NOTE_INSN_DELETED) >> >> however I assume that non-note insns may occur as well. So make sure >> they are recognized before touching their lra_insn_recog_data_t. >> >> gcc/ChangeLog: >> >> 2018-10-16 Ilya Leoshkevich <iii@linux.ibm.com> >> >> PR rtl-optimization/87596 >> * lra-constraints.c (spill_hard_reg_in_range): Use >> lra_get_insn_recog_data () instead of lra_insn_recog_data[] >> for instructions in FROM..TO range. >> >> gcc/testsuite/ChangeLog: >> >> 2018-10-16 Ilya Leoshkevich <iii@linux.ibm.com> >> >> PR rtl-optimization/87596 >> * gcc.target/i386/pr87596.c: New test. > I think this is just papering over the problem. > > AFAICT for something like a deleted note lra_get_insn_recog_data is > going to call lra_set_insn_recog_data. Given that INSN will be a NOTE I > think icode will end up being -1 and we'll eventually get into this clause: > > if (icode < 0) > { > int nop, nalt; > machine_mode operand_mode[MAX_RECOG_OPERANDS]; > const char *constraints[MAX_RECOG_OPERANDS]; > > nop = asm_noperands (PATTERN (insn)); > > And I don't think we should be trying to extract the PATTERN of a note. > > I think you need to either filter out the non-insn thingies in > spill_hard_reg_in_range, or go with your patch plus beefing up the > checks in lra_set_insn_recog_data. > > jeff > Thanks, you’re absolutely right! I re-ran the test with --enable-checking=all, and it crashed in another branch of that if statement: else { insn_extract (insn); /* Also uses PATTERN (). */ data->insn_static_data = insn_static_data I looked at the usage patterns of lra_insn_recog_data[] and lra_get_insn_recog_data (), and noticed that we use the former directly only when the insn in question is taken from insn_bitmap, and the latter in all other cases. Furthermore, lra_get_insn_recog_data () is normally guarded by INSN_P () or something similar. So I plan to keep using lra_get_insn_recog_data (), but with an extra INSN_P () check.
On 10/16/18 10:10 AM, Ilya Leoshkevich wrote: >> Am 16.10.2018 um 16:20 schrieb Jeff Law <law@redhat.com>: >> >> On 10/16/18 3:17 AM, Ilya Leoshkevich wrote: >>> Bootstrap and regtest are running on x86_64-redhat-linux. >>> >>> Insns in FROM..TO range might not be recognized yet, and their >>> corresponding entries in lra_insn_recog_data[] might be NULLs. In the >>> code from PR87596 the problematic insn is merely >>> >>> (note 148 154 68 7 NOTE_INSN_DELETED) >>> >>> however I assume that non-note insns may occur as well. So make sure >>> they are recognized before touching their lra_insn_recog_data_t. >>> >>> gcc/ChangeLog: >>> >>> 2018-10-16 Ilya Leoshkevich <iii@linux.ibm.com> >>> >>> PR rtl-optimization/87596 >>> * lra-constraints.c (spill_hard_reg_in_range): Use >>> lra_get_insn_recog_data () instead of lra_insn_recog_data[] >>> for instructions in FROM..TO range. >>> >>> gcc/testsuite/ChangeLog: >>> >>> 2018-10-16 Ilya Leoshkevich <iii@linux.ibm.com> >>> >>> PR rtl-optimization/87596 >>> * gcc.target/i386/pr87596.c: New test. >> I think this is just papering over the problem. >> >> AFAICT for something like a deleted note lra_get_insn_recog_data is >> going to call lra_set_insn_recog_data. Given that INSN will be a NOTE I >> think icode will end up being -1 and we'll eventually get into this clause: >> >> if (icode < 0) >> { >> int nop, nalt; >> machine_mode operand_mode[MAX_RECOG_OPERANDS]; >> const char *constraints[MAX_RECOG_OPERANDS]; >> >> nop = asm_noperands (PATTERN (insn)); >> >> And I don't think we should be trying to extract the PATTERN of a note. >> >> I think you need to either filter out the non-insn thingies in >> spill_hard_reg_in_range, or go with your patch plus beefing up the >> checks in lra_set_insn_recog_data. >> >> jeff >> > > Thanks, you’re absolutely right! > > I re-ran the test with --enable-checking=all, and it crashed in another > branch of that if statement: > > else > { > insn_extract (insn); /* Also uses PATTERN (). */ > data->insn_static_data = insn_static_data > > I looked at the usage patterns of lra_insn_recog_data[] and > lra_get_insn_recog_data (), and noticed that we use the former directly > only when the insn in question is taken from insn_bitmap, and the latter > in all other cases. Furthermore, lra_get_insn_recog_data () is normally > guarded by INSN_P () or something similar. > > So I plan to keep using lra_get_insn_recog_data (), but with an extra > INSN_P () check. Seems reasonable. Though note that INSN_P will accept DEBUG_INSNs. That may be OK -- those routines have some code for handling DEBUG_INSNs, but I didn't audit those paths carefully. Jeff
> Am 16.10.2018 um 19:35 schrieb Jeff Law <law@redhat.com>: > > On 10/16/18 10:10 AM, Ilya Leoshkevich wrote: >> >> So I plan to keep using lra_get_insn_recog_data (), but with an extra >> INSN_P () check. > Seems reasonable. Though note that INSN_P will accept DEBUG_INSNs. > That may be OK -- those routines have some code for handling > DEBUG_INSNs, but I didn't audit those paths carefully. The logic for handling DEBUG_INSNs in lra_set_insn_recog_data () is fairly short, so they seem to be fully supported: if (DEBUG_INSN_P (insn)) { data->dup_loc = NULL; data->arg_hard_regs = NULL; data->preferred_alternatives = ALL_ALTERNATIVES; if (DEBUG_BIND_INSN_P (insn)) { data->insn_static_data = &debug_bind_static_data; data->operand_loc = XNEWVEC (rtx *, 1); data->operand_loc[0] = &INSN_VAR_LOCATION_LOC (insn); } else if (DEBUG_MARKER_INSN_P (insn)) { data->insn_static_data = &debug_marker_static_data; data->operand_loc = NULL; } return data; } I browsed a random RTL dump and found that some DEBUG_INSNs may refer to hard regs: (debug_insn (var_location:SI dz (reg/v:SI 0))) So I guess I have to use INSN_P (). I will push the v2 once regtest is finished.
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c index 774d1ff3aaa..0d9cac07fb2 100644 --- a/gcc/lra-constraints.c +++ b/gcc/lra-constraints.c @@ -5696,10 +5696,11 @@ spill_hard_reg_in_range (int regno, enum reg_class rclass, rtx_insn *from, rtx_i continue; for (insn = from; insn != NEXT_INSN (to); insn = NEXT_INSN (insn)) { - lra_insn_recog_data_t id = lra_insn_recog_data[uid = INSN_UID (insn)]; + lra_insn_recog_data_t id = lra_get_insn_recog_data (insn); struct lra_static_insn_data *static_id = id->insn_static_data; struct lra_insn_reg *reg; + uid = INSN_UID (insn); if (bitmap_bit_p (&lra_reg_info[hard_regno].insn_bitmap, uid)) break; for (reg = static_id->hard_regs; reg != NULL; reg = reg->next) diff --git a/gcc/testsuite/gcc.target/i386/pr87596.c b/gcc/testsuite/gcc.target/i386/pr87596.c new file mode 100644 index 00000000000..764708b694a --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr87596.c @@ -0,0 +1,16 @@ +/* LRA corner case which triggered a segfault. */ +/* Reduced testcase by Arseny Solokha. */ +/* { dg-do compile { target int128 } } */ +/* { dg-options "-O1 -fschedule-insns -ftrapv -funroll-all-loops -fno-tree-dominator-opts -fno-tree-loop-im" } */ + +void +wh (__int128 *ku) +{ + unsigned int *dp; + + while (*ku < 1) + { + *dp <<= 32; /* { dg-warning "left shift count >= width of type" } */ + ++*ku; + } +}
Bootstrap and regtest are running on x86_64-redhat-linux. Insns in FROM..TO range might not be recognized yet, and their corresponding entries in lra_insn_recog_data[] might be NULLs. In the code from PR87596 the problematic insn is merely (note 148 154 68 7 NOTE_INSN_DELETED) however I assume that non-note insns may occur as well. So make sure they are recognized before touching their lra_insn_recog_data_t. gcc/ChangeLog: 2018-10-16 Ilya Leoshkevich <iii@linux.ibm.com> PR rtl-optimization/87596 * lra-constraints.c (spill_hard_reg_in_range): Use lra_get_insn_recog_data () instead of lra_insn_recog_data[] for instructions in FROM..TO range. gcc/testsuite/ChangeLog: 2018-10-16 Ilya Leoshkevich <iii@linux.ibm.com> PR rtl-optimization/87596 * gcc.target/i386/pr87596.c: New test. --- gcc/lra-constraints.c | 3 ++- gcc/testsuite/gcc.target/i386/pr87596.c | 16 ++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.target/i386/pr87596.c