Re: Possible GCC optimization bug

Anubis 1101 <[email protected]> writes:

> That does appear to "fix" it, but it doesn't tell me what was actually
> wrong.  It just corrects the bad addressing and additionally prints
> out "runtime error: index 10 out of bounds for type 'char
> [10]'"... which, yea, I know that. That's half of why I'm here.  If
> there's something specific I'm doing that's causing it, that's what
> I'd like to know. As far as I can tell, there's no flaw in my code
> itself.

It doesn't fix it.  It demonstrates why your code is incorrect (at least
in this instance).

GCC deduced that the "10 < 10" condition, as you named it, holds true,
because GCC deduced that the former 10 comes from a variable that is
used to access a loop of length 10.  I didn't track down what variable
this is, so let's call it X for posterity.

The language rules state that, for an array access expression a[i] where
'a' is an array of length N, 'i' must be such that 0 <= i < N.

Here, 'a' is 'data' and 'i' is X on lines 166 and 177 (fn:1).  As a
result, GCC is allowed to assume that X < N.  As N here was 10, X < 10,
leading to the 10 < 10 condition being "true".

This is properly fixed by not allowing the loops in length and size
overrun the array; something like this:

  while (i < Capacity && data[i] != '\0'){i++;}

Indeed, this fixes the UBSan warnings (note that the capacity check must
come first so that data[i] is not executed when i=Capacity).

C and C++ are high-level programming languages.  What you did here is
assume that term immediately follows data, and data[Capacity] is the
same as term.  But, the language does not actually specify this, so you
can't do that.  The ABI does specify this.  But you're writing C++
abstract machine code, so that doesn't matter.  data[Capacity] is, in
fact, not a valid expression.

The bug here is that GCC didn't catch and report this in a warning.
There's supposed to be a warning for this case.  Not sure why it didn't
trigger.  But definitely not incorrect codegen (and I can't debug the
warning at this exact moment, sorry).

I recommend not separating 'term' out.  Reserve Capacity+1, and write
logic that ensures there's always a terminator.

Have a lovely day!

[fn:1] Note that X here is not the 'i' used inside those loops.  It is
       whatever was downstream from the 'i' returned from those
       functions.  That's why I'm giving it the name X.
-- 
Arsen Arsenović

-----BEGIN PGP SIGNATURE-----

iQECBAEWCgCqFiEE/uKz0RP8AKMWLWBhUsKUMB6ixJMFAmmODaQbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwyXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25z
Lm9wZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRGRUUyQjNEMTEzRkMwMEEzMTYyRDYw
NjE1MkMyOTQzMDFFQTJDNDkzEBxhcnNlbkBhYXJzZW4ubWUACgkQUsKUMB6ixJN0
owEAypncA5nBHpT0++rkPAeF+fn4i9gyPjjP7c8XOy/Jn7IA/18nfkuG8szKTHvh
nKIRTpsEtIEL8Oql21q/nPCNGfEE
=wqC2
-----END PGP SIGNATURE-----