Re: [PATCH] Fix a couple of issues in gimple-ssa-sprintf.c
On Fri, 25 Nov 2016, 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?
Ok.
Richard.
> 2016-11-25 Jakub Jelinek
>
> * 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.0 +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
> static fmtresult
> format_integer (const conversion_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 (_type_node, _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
>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 (_type_node, _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
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
Re: [PATCH] Fix a couple of issues in gimple-ssa-sprintf.c
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
> >
> > * 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.0 +0100
> > +++ gcc/gimple-ssa-sprintf.c2016-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
> > static fmtresult
> > format_integer (const conversion_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 (_type_node, _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
> >break;
> >
> > case FMT_LEN_z:
> > - dirtype = sign ? ptrdiff_type_node : size_type_node;
> > + dirtype = signed_or_unsigned_type_for (!sign, size_type_node);
> >break;
> >
>
Re: [PATCH] Fix a couple of issues in gimple-ssa-sprintf.c
On Sat, Nov 26, 2016 at 11:17:09AM -0700, Martin Sebor wrote: > 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. Well, every unnecessary or very rarely used global tree means typically another GC root or global variable that needs to be handled on every garbage collection. So the question is if it is worth it. As for sharing the code, I've actually started writing such an inline function, but then realized that every of the spots that use such a technique does something slightly different. Jakub
Re: [PATCH] Fix a couple of issues in gimple-ssa-sprintf.c
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.
2016-11-25 Jakub Jelinek
* 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.0 +0100
+++ gcc/gimple-ssa-sprintf.c2016-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
static fmtresult
format_integer (const conversion_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 (_type_node, _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
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 (_type_node, _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
[PATCH] Fix a couple of issues in gimple-ssa-sprintf.c
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?
2016-11-25 Jakub Jelinek
* 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.0 +0100
+++ gcc/gimple-ssa-sprintf.c2016-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
static fmtresult
format_integer (const conversion_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 (_type_node, _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
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 (_type_node, _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
