On Sat, 26 Nov 2016, Martin Sebor wrote:
> On 11/25/2016 10:20 AM, Jakub Jelinek wrote:
> > Hi!
> >
> > Here is an attempt to fix a couple of bugs in gimple-ssa-sprintf.c.
> > First of all, it assumes size_t is always the same as uintmax_t, which
> > is not necessarily the case.
> > Second, it uses static tree {,u}intmax_type_node; variables for caching
> > those types, but doesn't register them with GC; but their computation
> > is quite cheap, so I think it isn't worth wasting a GC root for those,
> > especially if we compute it only in the very rare case when somebody
> > uses the j modifier.
> > Third, the code assumes that ptrdiff_t is the signed type for size_t.
> > E.g. vms is one port where that isn't true, ptrdiff_t can be 64-bit,
> > while size_t is 32-bit.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> It looks fine to me. Thanks for fixing these corner cases!
>
> Martin
>
> PS As the comment above the build_intmax_type_node function
> mentions, its body was copied from lto/lto-lang.c. It would be
> useful not to have to duplicate the code in the middle-end and
> instead provide a shared definition of each of the nodes so that
> they could be used everywhere. Ditto for ptrdiff_type_node.
> It would make it less likely for patches to break when someone
> forgets to bootstrap and test Ada, for instance, or where the
> use of the type isn't exercised by bootstrap or the test suite
> due to gaps in coverage.
ptrdiff_type_node has been fixed by Pratamesh. For the rest of the
global nodes in lto-lang.c they probably should be moved to
the common tree nodes. The C ABI types (used for builtins)
should be provided by the middle-end.
Richard.
> >
> > 2016-11-25 Jakub Jelinek <ja...@redhat.com>
> >
> > * gimple-ssa-sprintf.c (build_intmax_type_nodes): Look at
> > UINTMAX_TYPE rather than SIZE_TYPE. Add gcc_unreachable if
> > intmax_t couldn't be determined.
> > (format_integer): Make {,u}intmax_type_node no longer static,
> > initialize them only when needed. For z and t use
> > signed_or_unsigned_type_for instead of assuming size_t and
> > ptrdiff_t have the same precision.
> >
> > --- gcc/gimple-ssa-sprintf.c.jj 2016-11-25 09:49:47.000000000 +0100
> > +++ gcc/gimple-ssa-sprintf.c 2016-11-25 10:26:58.763114194 +0100
> > @@ -733,23 +733,23 @@ format_percent (const conversion_spec &,
> > }
> >
> >
> > -/* Ugh. Compute intmax_type_node and uintmax_type_node the same way
> > - lto/lto-lang.c does it. This should be available in tree.h. */
> > +/* Compute intmax_type_node and uintmax_type_node similarly to how
> > + tree.c builds size_type_node. */
> >
> > static void
> > build_intmax_type_nodes (tree *pintmax, tree *puintmax)
> > {
> > - if (strcmp (SIZE_TYPE, "unsigned int") == 0)
> > + if (strcmp (UINTMAX_TYPE, "unsigned int") == 0)
> > {
> > *pintmax = integer_type_node;
> > *puintmax = unsigned_type_node;
> > }
> > - else if (strcmp (SIZE_TYPE, "long unsigned int") == 0)
> > + else if (strcmp (UINTMAX_TYPE, "long unsigned int") == 0)
> > {
> > *pintmax = long_integer_type_node;
> > *puintmax = long_unsigned_type_node;
> > }
> > - else if (strcmp (SIZE_TYPE, "long long unsigned int") == 0)
> > + else if (strcmp (UINTMAX_TYPE, "long long unsigned int") == 0)
> > {
> > *pintmax = long_long_integer_type_node;
> > *puintmax = long_long_unsigned_type_node;
> > @@ -762,12 +762,14 @@ build_intmax_type_nodes (tree *pintmax,
> > char name[50];
> > sprintf (name, "__int%d unsigned", int_n_data[i].bitsize);
> >
> > - if (strcmp (name, SIZE_TYPE) == 0)
> > + if (strcmp (name, UINTMAX_TYPE) == 0)
> > {
> > *pintmax = int_n_trees[i].signed_type;
> > *puintmax = int_n_trees[i].unsigned_type;
> > + return;
> > }
> > }
> > + gcc_unreachable ();
> > }
> > }
> >
> > @@ -851,15 +853,8 @@ format_pointer (const conversion_spec &s
> > static fmtresult
> > format_integer (const conversion_spec &spec, tree arg)
> > {
> > - /* These are available as macros in the C and C++ front ends but,
> > - sadly, not here. */
> > - static tree intmax_type_node;
> > - static tree uintmax_type_node;
> > -
> > - /* Initialize the intmax nodes above the first time through here. */
> > - if (!intmax_type_node)
> > - build_intmax_type_nodes (&intmax_type_node, &uintmax_type_node);
> > -
> > + tree intmax_type_node;
> > + tree uintmax_type_node;
> > /* Set WIDTH and PRECISION to either the values in the format
> > specification or to zero. */
> > int width = spec.have_width ? spec.width : 0;
> > @@ -909,19 +904,20 @@ format_integer (const conversion_spec &s
> > break;
> >
> > case FMT_LEN_z:
> > - dirtype = sign ? ptrdiff_type_node : size_type_node;
> > + dirtype = signed_or_unsigned_type_for (!sign, size_type_node);
> > break;
> >
> > case FMT_LEN_t:
> > - dirtype = sign ? ptrdiff_type_node : size_type_node;
> > + dirtype = signed_or_unsigned_type_for (!sign, ptrdiff_type_node);
> > break;
> >
> > case FMT_LEN_j:
> > + build_intmax_type_nodes (&intmax_type_node, &uintmax_type_node);
> > dirtype = sign ? intmax_type_node : uintmax_type_node;
> > break;
> >
> > default:
> > - return fmtresult ();
> > + return fmtresult ();
> > }
> >
> > /* The type of the argument to the directive, either deduced from
> >
> > Jakub
> >
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
