| Message ID | bb7f757e-efed-a6b3-290f-e77c6201169e@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/29/2016 06:21 PM, Jeff Law wrote: > > On a small number of ports, we only have 2 defined register classes. > NO_REGS and ALL_REGS. Examples would include nvptx and vax. > > So let's look at check_and_process_move from lra-constraints.c: > > sclass = dclass = NO_REGS; > if (REG_P (dreg)) > dclass = get_reg_class (REGNO (dreg)); > if (dclass == ALL_REGS) > /* ALL_REGS is used for new pseudos created by transformations > like reload of SUBREG_REG (see function > simplify_operand_subreg). We don't know their class yet. We > should figure out the class from processing the insn > constraints not in this fast path function. Even if ALL_REGS > were a right class for the pseudo, secondary_... hooks usually > are not define for ALL_REGS. */ > return false; > [ ... ] > /* Set up hard register for a reload pseudo for hook > secondary_reload because some targets just ignore unassigned > pseudos in the hook. */ > if (dclass != NO_REGS && lra_get_regno_hard_regno (REGNO (dreg)) < 0) > { > dregno = REGNO (dreg); > reg_renumber[dregno] = ira_class_hard_regs[dclass][0]; > } > > > The reference to ira_class_hard_regs is flagged by VRP as having an > out-of-bounds array reference. WTF!? Of course it's pretty obvious > once you look at the dumps... > > On most targets dclass in this code will have a VR_VARYING range and as > a result no warning will be issued. But on the ptx and vax ports VRP is > able to give us the range ~[NO_REGS,ALL_REGS] aka ~[0,1] The > out-of-range array index is now obvious. So I tried to look up the rules for enum values and it seems like we can't prove that the code in the if statement is dead. However, can't we at least prove that it is "dead enough" for warning purposes? > Ok for the trunk? Hmm, seems like a deficiency in the warning code TBH, but I guess this patch can't hurt. Maybe open a PR for improving the warning. Bernd
On 11/02/2016 01:20 PM, Bernd Schmidt wrote: > On 10/29/2016 06:21 PM, Jeff Law wrote: >> >> On a small number of ports, we only have 2 defined register classes. >> NO_REGS and ALL_REGS. Examples would include nvptx and vax. >> >> So let's look at check_and_process_move from lra-constraints.c: >> >> sclass = dclass = NO_REGS; >> if (REG_P (dreg)) >> dclass = get_reg_class (REGNO (dreg)); >> if (dclass == ALL_REGS) >> /* ALL_REGS is used for new pseudos created by transformations >> like reload of SUBREG_REG (see function >> simplify_operand_subreg). We don't know their class yet. We >> should figure out the class from processing the insn >> constraints not in this fast path function. Even if ALL_REGS >> were a right class for the pseudo, secondary_... hooks usually >> are not define for ALL_REGS. */ >> return false; >> [ ... ] >> /* Set up hard register for a reload pseudo for hook >> secondary_reload because some targets just ignore unassigned >> pseudos in the hook. */ >> if (dclass != NO_REGS && lra_get_regno_hard_regno (REGNO (dreg)) < 0) >> { >> dregno = REGNO (dreg); >> reg_renumber[dregno] = ira_class_hard_regs[dclass][0]; >> } >> >> >> The reference to ira_class_hard_regs is flagged by VRP as having an >> out-of-bounds array reference. WTF!? Of course it's pretty obvious >> once you look at the dumps... >> >> On most targets dclass in this code will have a VR_VARYING range and as >> a result no warning will be issued. But on the ptx and vax ports VRP is >> able to give us the range ~[NO_REGS,ALL_REGS] aka ~[0,1] The >> out-of-range array index is now obvious. > > So I tried to look up the rules for enum values and it seems like we > can't prove that the code in the if statement is dead. However, can't we > at least prove that it is "dead enough" for warning purposes? For both C & C++ you can shove in a value outside the range of the enum. There is new strongly typed enum in C++11, but I didn't figure converting to those would be well received :-) So for the given code the range really is ~[0,1] and we can overflow the array bounds. > >> Ok for the trunk? > > Hmm, seems like a deficiency in the warning code TBH, but I guess this > patch can't hurt. Maybe open a PR for improving the warning. I guess we could enhance the warning on the assumption that getting a value that is not one of the enumeration constants. That's assuming we've got the right type in the right place. I'll take a look and see before going forward. jeff
On 11/02/2016 01:20 PM, Bernd Schmidt wrote: > On 10/29/2016 06:21 PM, Jeff Law wrote: >> >> On a small number of ports, we only have 2 defined register classes. >> NO_REGS and ALL_REGS. Examples would include nvptx and vax. >> >> So let's look at check_and_process_move from lra-constraints.c: >> >> sclass = dclass = NO_REGS; >> if (REG_P (dreg)) >> dclass = get_reg_class (REGNO (dreg)); >> if (dclass == ALL_REGS) >> /* ALL_REGS is used for new pseudos created by transformations >> like reload of SUBREG_REG (see function >> simplify_operand_subreg). We don't know their class yet. We >> should figure out the class from processing the insn >> constraints not in this fast path function. Even if ALL_REGS >> were a right class for the pseudo, secondary_... hooks usually >> are not define for ALL_REGS. */ >> return false; >> [ ... ] >> /* Set up hard register for a reload pseudo for hook >> secondary_reload because some targets just ignore unassigned >> pseudos in the hook. */ >> if (dclass != NO_REGS && lra_get_regno_hard_regno (REGNO (dreg)) < 0) >> { >> dregno = REGNO (dreg); >> reg_renumber[dregno] = ira_class_hard_regs[dclass][0]; >> } >> >> >> The reference to ira_class_hard_regs is flagged by VRP as having an >> out-of-bounds array reference. WTF!? Of course it's pretty obvious >> once you look at the dumps... >> >> On most targets dclass in this code will have a VR_VARYING range and as >> a result no warning will be issued. But on the ptx and vax ports VRP is >> able to give us the range ~[NO_REGS,ALL_REGS] aka ~[0,1] The >> out-of-range array index is now obvious. > > So I tried to look up the rules for enum values and it seems like we > can't prove that the code in the if statement is dead. However, can't we > at least prove that it is "dead enough" for warning purposes? Thinking more about this, suppressing the warning in the case where we might have an out of bounds enum seems wrong -- that would seem like an important case we would want to catch. (ie, some path shoves an out-of-range value into the enum object which is then used in an array reference). That does make me wonder about if we would want a warning if we identify an assignment to an enum object where the RHS is out of hte range of the enum. It'd probably trigger too many false positives in practice :( > >> Ok for the trunk? > > Hmm, seems like a deficiency in the warning code TBH, but I guess this > patch can't hurt. Maybe open a PR for improving the warning. I'm going to go ahead and install the patch. I'm still [ondering what, if any, BZ to open around improving the existing warning, adding a new one or converting GCC to use safe enums :-) jeff
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c index b592168..f14c86c 100644 --- a/gcc/lra-constraints.c +++ b/gcc/lra-constraints.c @@ -1127,6 +1127,7 @@ check_and_process_move (bool *change_p, bool *sec_mem_p ATTRIBUTE_UNUSED) sclass = dclass = NO_REGS; if (REG_P (dreg)) dclass = get_reg_class (REGNO (dreg)); + gcc_assert (dclass < LIM_REG_CLASSES); if (dclass == ALL_REGS) /* ALL_REGS is used for new pseudos created by transformations like reload of SUBREG_REG (see function @@ -1138,6 +1139,7 @@ check_and_process_move (bool *change_p, bool *sec_mem_p ATTRIBUTE_UNUSED) return false; if (REG_P (sreg)) sclass = get_reg_class (REGNO (sreg)); + gcc_assert (sclass < LIM_REG_CLASSES); if (sclass == ALL_REGS) /* See comments above. */ return false;
On a small number of ports, we only have 2 defined register classes. NO_REGS and ALL_REGS. Examples would include nvptx and vax. So let's look at check_and_process_move from lra-constraints.c: sclass = dclass = NO_REGS; if (REG_P (dreg)) dclass = get_reg_class (REGNO (dreg)); if (dclass == ALL_REGS) /* ALL_REGS is used for new pseudos created by transformations like reload of SUBREG_REG (see function simplify_operand_subreg). We don't know their class yet. We should figure out the class from processing the insn constraints not in this fast path function. Even if ALL_REGS were a right class for the pseudo, secondary_... hooks usually are not define for ALL_REGS. */ return false; [ ... ] /* Set up hard register for a reload pseudo for hook secondary_reload because some targets just ignore unassigned pseudos in the hook. */ if (dclass != NO_REGS && lra_get_regno_hard_regno (REGNO (dreg)) < 0) { dregno = REGNO (dreg); reg_renumber[dregno] = ira_class_hard_regs[dclass][0]; } The reference to ira_class_hard_regs is flagged by VRP as having an out-of-bounds array reference. WTF!? Of course it's pretty obvious once you look at the dumps... On most targets dclass in this code will have a VR_VARYING range and as a result no warning will be issued. But on the ptx and vax ports VRP is able to give us the range ~[NO_REGS,ALL_REGS] aka ~[0,1] The out-of-range array index is now obvious. The fix is trivial, we just assert that dclass is < LIM_REG_CLASSES, similarly for sclass as it triggers a similar warning. Bootstrapped and regression tested on x86_64-linux-gnu. Also verified that the ptx and vax ports will build via config-list.mk. Ok for the trunk? Jeff * lra-constraints.c (check_and_process_move): Constrain the range of DCLASS and SCLASS to avoid false positive out of bounds array index warning.