| Message ID | 54C22A71.7050603@arm.com |
|---|---|
| State | New |
| Headers | show |
On Jan 23, 2015, at 3:03 AM, Tejas Belagod <tejas.belagod@arm.com> wrote:
> This is an almost obvious patch to fix PR64231 as discovered by A. Pinksi and as proposed by Jakub.
Kinda crappy code. The macro to use here should take the number of bits as an int, and wether the constant is signed or not.
FITS (x, 32, UNSIGNED)
would be nicer. Complexity of your patch, 16, complexity of the above, 4. This is 4x better. Additionally, since more than one port and more than 1 place in the compiler does this, would be nice to refactor them all to be as nice.
I’m sore on this topic, as I’ve seen failing code that used the style in your patch that I had to fix in GNU ld. It was as annoying to find and fix as this PR.
Now what are the odds that someone doesn’t understand how to use IN_RANGE and does this same exact problem again, multiplied by the number of times IN_RANGE appears in gcc multiplied by the time it took for everyone to track down and talk about this bug? Compare than number to 0. In my version, it should be impossible to ever misuse, leading to a bug rate of near 0. The biggest issue with mine, one will need to add overloads for tree, rtx, int, long, maxint_t as the code is found to not compile. These, while annoying, are much easier to resolve than the bug in the PR.
On Fri, Jan 23, 2015 at 08:48:43AM -0800, Mike Stump wrote: > On Jan 23, 2015, at 3:03 AM, Tejas Belagod <tejas.belagod@arm.com> wrote: > > This is an almost obvious patch to fix PR64231 as discovered by A. Pinksi and as proposed by Jakub. > > Kinda crappy code. The macro to use here should take the number of bits as an int, and wether the constant is signed or not. > > FITS (x, 32, UNSIGNED) Except that the test is testing something different. First of all, it is closer to FITS (x, 33, SIGNED), but the details are different, the test as is (except for the bug on 32-bit hosts) INTVAL (x) is in not in [ -4GB + 33, 4GB - 32 ] interval (inclusive). Why it isn't exactly "not in [ -4GB, 4GB - 1 ]" or similar. Jakub
On 23/01/15 17:15, Jakub Jelinek wrote: > On Fri, Jan 23, 2015 at 08:48:43AM -0800, Mike Stump wrote: >> On Jan 23, 2015, at 3:03 AM, Tejas Belagod <tejas.belagod@arm.com> wrote: >>> This is an almost obvious patch to fix PR64231 as discovered by A. Pinksi and as proposed by Jakub. >> >> Kinda crappy code. The macro to use here should take the number of bits as an int, and wether the constant is signed or not. >> >> FITS (x, 32, UNSIGNED) > > Except that the test is testing something different. > First of all, it is closer to FITS (x, 33, SIGNED), but > the details are different, the test as is (except for the bug on 32-bit > hosts) INTVAL (x) is in not in [ -4GB + 33, 4GB - 32 ] interval (inclusive). > Why it isn't exactly "not in [ -4GB, 4GB - 1 ]" or similar. > The value of the offset itself is a heuristic as we cannot accurately say what the distance of a symbol from the ADRP will be at compile time. So it doesn't really matter what the exact value of range is in this case. Thanks, Tejas.
On Fri, Jan 23, 2015 at 11:03:13AM +0000, Tejas Belagod wrote: > > Hi, > > This is an almost obvious patch to fix PR64231 as discovered by A. Pinksi > and as proposed by Jakub. > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64231 > > Regressions happy. OK to commit? This is ok for trunk. We have a real bug that we need to fix, if we have some more useful macro in the future, this can be rewritten to use that macro together with the many other spots that would be changed for it as well. But blocking the fix for it doesn't sound right to me. > 2015-01-23 Tejas Belagod <tejas.belagod@arm.com> > Andrew Pinski <pinskia@gcc.gnu.org> > Jakub Jelinek <jakub@gcc.gnu.org> > > PR target/64231 > * config/aarch64/aarch64.c (aarch64_classify_symbol): Fix large > integer typing for small model. Use IN_RANGE. Jakub
On 30/01/15 13:25, Jakub Jelinek wrote: > On Fri, Jan 23, 2015 at 11:03:13AM +0000, Tejas Belagod wrote: >> >> Hi, >> >> This is an almost obvious patch to fix PR64231 as discovered by A. Pinksi >> and as proposed by Jakub. >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64231 >> >> Regressions happy. OK to commit? > > This is ok for trunk. We have a real bug that we need to fix, if we have > some more useful macro in the future, this can be rewritten to use that > macro together with the many other spots that would be changed for it as > well. But blocking the fix for it doesn't sound right to me. > Thanks. Committed as r220348. Thanks, Tejas. >> 2015-01-23 Tejas Belagod <tejas.belagod@arm.com> >> Andrew Pinski <pinskia@gcc.gnu.org> >> Jakub Jelinek <jakub@gcc.gnu.org> >> >> PR target/64231 >> * config/aarch64/aarch64.c (aarch64_classify_symbol): Fix large >> integer typing for small model. Use IN_RANGE. > > Jakub >
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index dd49fcd..b790bc6 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -7083,8 +7083,8 @@ aarch64_classify_symbol (rtx x, rtx offset, /* Same reasoning as the tiny code model, but the offset cap here is 4G. */ if (SYMBOL_REF_WEAK (x) - || INTVAL (offset) < (HOST_WIDE_INT) -4294967263 - || INTVAL (offset) > (HOST_WIDE_INT) 4294967264) + || !IN_RANGE (INTVAL (offset), HOST_WIDE_INT_C (-4294967263), + HOST_WIDE_INT_C (4294967264))) return SYMBOL_FORCE_TO_MEM; return SYMBOL_SMALL_ABSOLUTE;