| Message ID | orr34k3e5e.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
On 01/02/2017 10:29 PM, Alexandre Oliva wrote: > This patch fixes a false-positive warning in df-scan, at bootstrap-O3 > failed, and enables GCC to optimize out the code that leads to the > warning. > > df_ref_create_structure was inlined into the else part of > df_ref_record. Due to the condition of the corresponding if, In the > else part, VRP deduced unsigned regno >= FIRST_PSEUDO_REGISTER. > > In df_ref_create_structure, there's another regno variable, > initialized with the same expression and value as the caller's. GCC > can tell as much, but this regno variable is signed. It is used, > shifted right, to index a hard regset bit array within a path that > tests that this signed regno < FIRST_PSEUDO_REGISTER. > > GCC warned about the possible out-of-range indexing into the hard > regset array. It shouldn't, after all, the same regno can't possibly > be both < FIRST_PSEUDO_REGISTER and >= FIRST_PSEUDO_REGISTER, can it? > > Well, the optimizers correctly decide it could, if it was a negative > int that, when converted to unsigned, became larger than > FIRST_PSEUDO_REGISTER. But GCC doesn't know regno can't be negative, > so the test could not be optimize out. What's more, given the > constraints, VRP correctly concluded the hard regset array would > always be indexed by a value way outside the array index range. > > This patch changes the inlined regno to unsigned, like the caller's, > so that we can now tell the conditions can't both hold, so we optimize > out the path containing the would-be out-of-range array indexing. > > Regstrapped on x86_64-linux-gnu and i686-linux-gnu. OK to install? > > for gcc/ChangeLog > > * df-scan.c (df_ref_create_structure): Make regno unsigned, > to match the caller. What if REGNO is 2147483648 (assume 32 bit host). That will get us into the else block in df_ref_record as it's >= FIRST_PSEUDO_REGISTER. In df_ref_create_structure, we use the same expression to compute REGNO, but this time it's interpreted as a signed integer, so -2147483648. That gets us into the path where we call TEST_HARD_REG_BIT and thus the oob array index. Right? The patch is OK. It does highlight the desire to pick the right type and consistently use it. jeff
On Tue, Jan 03, 2017 at 11:08:05AM -0700, Jeff Law wrote: > What if REGNO is 2147483648 (assume 32 bit host). That will get us into the > else block in df_ref_record as it's >= FIRST_PSEUDO_REGISTER. > > In df_ref_create_structure, we use the same expression to compute REGNO, but > this time it's interpreted as a signed integer, so -2147483648. That gets us > into the path where we call TEST_HARD_REG_BIT and thus the oob array index. > > Right? > > The patch is OK. It does highlight the desire to pick the right type and > consistently use it. Note I think we require --disable-werror for the not so common bootstrap configurations, only normal bootstrap and profiledbootstrap are supposed to be --enable-werror free. At least that is my understanding of the general preference, there are too many bootstrap options and too many different sets of false positives. Not talking about this particular patch, just a general comment. Jakub
On 01/03/2017 11:11 AM, Jakub Jelinek wrote: > On Tue, Jan 03, 2017 at 11:08:05AM -0700, Jeff Law wrote: >> What if REGNO is 2147483648 (assume 32 bit host). That will get us into the >> else block in df_ref_record as it's >= FIRST_PSEUDO_REGISTER. >> >> In df_ref_create_structure, we use the same expression to compute REGNO, but >> this time it's interpreted as a signed integer, so -2147483648. That gets us >> into the path where we call TEST_HARD_REG_BIT and thus the oob array index. >> >> Right? >> >> The patch is OK. It does highlight the desire to pick the right type and >> consistently use it. > > Note I think we require --disable-werror for the not so common bootstrap > configurations, only normal bootstrap and profiledbootstrap are supposed to > be --enable-werror free. At least that is my understanding of the general > preference, there are too many bootstrap options and too many different sets > of false positives. Not talking about this particular patch, just a general > comment. Agreed. I was on the fence WRT these patches for that exact reason. Jeff
On Jan 3, 2017, Jeff Law <law@redhat.com> wrote: > What if REGNO is 2147483648 (assume 32 bit host). That will get us > into the else block in df_ref_record as it's >= FIRST_PSEUDO_REGISTER. > In df_ref_create_structure, we use the same expression to compute > REGNO, but this time it's interpreted as a signed integer, so > -2147483648. That gets us into the path where we call > TEST_HARD_REG_BIT and thus the oob array index. > Right? Yup, that's exactly how VRP goes about in concluding that there is something to warn about. If we get a pseudo count overflow, I guess we'll have bigger problems than this one, but VRP doesn't know it ;-)
diff --git a/gcc/df-scan.c b/gcc/df-scan.c index e6b55b5..ee8282f 100644 --- a/gcc/df-scan.c +++ b/gcc/df-scan.c @@ -2483,7 +2483,7 @@ df_ref_create_structure (enum df_ref_class cl, int ref_flags) { df_ref this_ref = NULL; - int regno = REGNO (GET_CODE (reg) == SUBREG ? SUBREG_REG (reg) : reg); + unsigned int regno = REGNO (GET_CODE (reg) == SUBREG ? SUBREG_REG (reg) : reg); struct df_scan_problem_data *problem_data = (struct df_scan_problem_data *) df_scan->problem_data;