This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch, AArch64, Obvious] Fix PR64231.
- From: Mike Stump <mikestump at comcast dot net>
- To: Tejas Belagod <tejas dot belagod at arm dot com>
- Cc: "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>, Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>, jakub at gcc dot gnu dot org, Andrew Pinski <pinskia at gmail dot com>
- Date: Fri, 23 Jan 2015 08:48:43 -0800
- Subject: Re: [Patch, AArch64, Obvious] Fix PR64231.
- Authentication-results: sourceware.org; auth=none
- References: <54C22A71 dot 7050603 at arm dot com>
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.