On Sat, Jul 17, 2021 at 2:31 PM Segher Boessenkool
<seg...@kernel.crashing.org> wrote:
>
> Hi!
>
> On Fri, Jul 16, 2021 at 11:35:25AM -0700, apinski--- via Gcc-patches wrote:
> > --- a/gcc/c-family/c-common.c
> > +++ b/gcc/c-family/c-common.c
> > @@ -5799,7 +5799,7 @@ parse_optimize_options (tree args, bool attr_p)
> >
> > if (TREE_CODE (value) == INTEGER_CST)
> > {
> > - char buffer[20];
> > + char buffer[HOST_BITS_PER_LONG / 3 + 4];
> > sprintf (buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
> > vec_safe_push (optimize_args, ggc_strdup (buffer));
> > }
>
> This calculation is correct, assuming "value" is non-negative. You can
> easily avoid all of that though:
Actually it is still correct even for negative values because we are
adding 4 rather than 3 to make sure there is enough room for the sign
character.
So it takes 11 max characters for a signed 32bit integer
(ceil(log(2^31)) + 1) and add the "-O" part and the null character
gives us 14. 32/3 is 10, and then add 4 gives us 14. This is the
exact amount.
So it takes 20 max characters for a signed 64bit integer
(ceil(log(2^63)) + 1) and add the "-O" part and the null character
gives us 23. 64/3 is 21, and then add 4 gives us 25. There are 2
extra bytes used which is ok.
Thanks,
Andrew
>
> int n = snprintf (0, 0, "-O%ld", (long) TREE_INT_CST_LOW (value));
> char buffer[n + 1];
> sprintf (buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
> vec_safe_push (optimize_args, ggc_strdup (buffer));
>
> This creates a variable length allocation on the stack though. You can
> do better (for some value of "better") by using the known longest value:
> (again assuming non-negative):
>
> int n = snprintf (0, 0, "-O%ld", LONG_MAX);
> char buffer[n + 1];
> sprintf (buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
> vec_safe_push (optimize_args, ggc_strdup (buffer));
>
> This does not call snprintf, GCC can evaluate that at compile time.
> This pattern works well in portable code.
>
> But we can do better still:
>
> char *buffer;
> asprintf (&buffer, "-O%ld", (long) TREE_INT_CST_LOW (value));
> vec_safe_push (optimize_args, ggc_strdup (buffer));
> free (buffer);
>
> (asprintf is in libiberty already). And we could avoid the ggc_strdup
> if we made a variant of asprintf that marks the GGC itself (the aprintf
> implementation already factors calculating the length needed, it will
> be easy to do this).
>
>
> Segher
