Xen devel - Jun 2013 - [PATCH] xen: correct definition of uintptr_t on 32-bit platforms.

Currently uintptr_t is unsigned long for both 32- and 64- bit platforms but
__PRIPTR_PREFIX is just "" on 32-bit leading to:

    error: format ''%u'' expects argument of type
''unsigned int'', but argument 2 has type ''long
unsigned int'' [-Werror=format]

It was a bit of a tossup between changing __PREPTR_PREFIX and changing the
definition of uintptr_t. I went with the latter since it is consistent with
glibc.

Tested with:
    printk("uintptr %"PRIuPTR"\n",
(uintptr_t)a_convenient_local_var);
in a conveniently built place on unstable (x86_64, arm32, arm64) and Xen 4.2
(x86_32p & x86_64).

This error will be exposed by the fixes for XSA55 and therefore will need to
be backported along with those.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: ian.jackson@eu.citrix.com
Cc: keir@xen.org
Cc: jbeulich@suse.com
---
 xen/include/xen/types.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
index 8596ded..641e1d1 100644
--- a/xen/include/xen/types.h
+++ b/xen/include/xen/types.h
@@ -57,6 +57,10 @@ typedef __u32 __be32;
 typedef __u64 __le64;
 typedef __u64 __be64;
 
+#if BITS_PER_LONG == 64
 typedef unsigned long uintptr_t;
+#else
+typedef unsigned int uintptr_t;
+#endif
 
 #endif /* __TYPES_H__ */
-- 
1.7.2.5

>>> On 04.06.13 at 15:16, Ian Campbell <ian.campbell@citrix.com>
wrote:
> Currently uintptr_t is unsigned long for both 32- and 64- bit platforms but
> __PRIPTR_PREFIX is just "" on 32-bit leading to:
> 
>     error: format ''%u'' expects argument of type
''unsigned int'', but argument
> 2 has type ''long unsigned int'' [-Werror=format]
> 
> It was a bit of a tossup between changing __PREPTR_PREFIX and changing the
> definition of uintptr_t. I went with the latter since it is consistent with
> glibc.
Actually I would much prefer the format string prefix to be changed
- like Linux, we''re assuming all over the place that pointer and longs
are equivalent in size, so creating an association between pointers
and ints is much more likely to cause problems than adjusting the
format string prefix.

Jan
> Tested with:
>     printk("uintptr %"PRIuPTR"\n",
(uintptr_t)a_convenient_local_var);
> in a conveniently built place on unstable (x86_64, arm32, arm64) and Xen
4.2
> (x86_32p & x86_64).
> 
> This error will be exposed by the fixes for XSA55 and therefore will need
to
> be backported along with those.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: ian.jackson@eu.citrix.com 
> Cc: keir@xen.org 
> Cc: jbeulich@suse.com 
> ---
>  xen/include/xen/types.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index 8596ded..641e1d1 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -57,6 +57,10 @@ typedef __u32 __be32;
>  typedef __u64 __le64;
>  typedef __u64 __be64;
>  
> +#if BITS_PER_LONG == 64
>  typedef unsigned long uintptr_t;
> +#else
> +typedef unsigned int uintptr_t;
> +#endif
>  
>  #endif /* __TYPES_H__ */
> -- 
> 1.7.2.5

On Tue, 2013-06-04 at 14:52 +0100, Jan Beulich wrote:
> >>> On 04.06.13 at 15:16, Ian Campbell
<ian.campbell@citrix.com> wrote:
> > Currently uintptr_t is unsigned long for both 32- and 64- bit
platforms but
> > __PRIPTR_PREFIX is just "" on 32-bit leading to:
> > 
> >     error: format ''%u'' expects argument of type
''unsigned int'', but argument
> > 2 has type ''long unsigned int'' [-Werror=format]
> > 
> > It was a bit of a tossup between changing __PREPTR_PREFIX and changing
the
> > definition of uintptr_t. I went with the latter since it is consistent
with
> > glibc.
> 
> Actually I would much prefer the format string prefix to be changed
> - like Linux, we''re assuming all over the place that pointer and
longs
> are equivalent in size, so creating an association between pointers
> and ints is much more likely to cause problems than adjusting the
> format string prefix.
Can do that if you prefer.

Would this just push the problem into the code which is compiled for
both user and hypervisor space (e.g. libelf), uintptr_t would then be
different in the two environments. I haven''t tried it so I
don''t know
what the practical impact is.

>>> On 04.06.13 at 16:00, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Tue, 2013-06-04 at 14:52 +0100, Jan Beulich wrote:
>> >>> On 04.06.13 at 15:16, Ian Campbell
<ian.campbell@citrix.com> wrote:
>> > Currently uintptr_t is unsigned long for both 32- and 64- bit
platforms but
>> > __PRIPTR_PREFIX is just "" on 32-bit leading to:
>> > 
>> >     error: format ''%u'' expects argument of type
''unsigned int'', but
> argument 
>> > 2 has type ''long unsigned int'' [-Werror=format]
>> > 
>> > It was a bit of a tossup between changing __PREPTR_PREFIX and
changing the
>> > definition of uintptr_t. I went with the latter since it is
consistent with
>> > glibc.
>> 
>> Actually I would much prefer the format string prefix to be changed
>> - like Linux, we''re assuming all over the place that pointer
and longs
>> are equivalent in size, so creating an association between pointers
>> and ints is much more likely to cause problems than adjusting the
>> format string prefix.
> 
> Can do that if you prefer.
> 
> Would this just push the problem into the code which is compiled for
> both user and hypervisor space (e.g. libelf), uintptr_t would then be
> different in the two environments. I haven''t tried it so I
don''t know
> what the practical impact is.
In user environment the print format should be taken from the
standard C headers just as the uintptr_t definition ought to be.
No risk of collision if done consistently.

Jan

On Tue, 2013-06-04 at 15:36 +0100, Jan Beulich wrote:
> >>> On 04.06.13 at 16:00, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > On Tue, 2013-06-04 at 14:52 +0100, Jan Beulich wrote:
> >> >>> On 04.06.13 at 15:16, Ian Campbell
<ian.campbell@citrix.com> wrote:
> >> > Currently uintptr_t is unsigned long for both 32- and 64- bit
platforms but
> >> > __PRIPTR_PREFIX is just "" on 32-bit leading to:
> >> > 
> >> >     error: format ''%u'' expects argument of
type ''unsigned int'', but
> > argument 
> >> > 2 has type ''long unsigned int''
[-Werror=format]
> >> > 
> >> > It was a bit of a tossup between changing __PREPTR_PREFIX and
changing the
> >> > definition of uintptr_t. I went with the latter since it is
consistent with
> >> > glibc.
> >> 
> >> Actually I would much prefer the format string prefix to be
changed
> >> - like Linux, we''re assuming all over the place that
pointer and longs
> >> are equivalent in size, so creating an association between
pointers
> >> and ints is much more likely to cause problems than adjusting the
> >> format string prefix.
> > 
> > Can do that if you prefer.
> > 
> > Would this just push the problem into the code which is compiled for
> > both user and hypervisor space (e.g. libelf), uintptr_t would then be
> > different in the two environments. I haven''t tried it so I
don''t know
> > what the practical impact is.
> 
> In user environment the print format should be taken from the
> standard C headers just as the uintptr_t definition ought to be.
> No risk of collision if done consistently.
Yes, absolutely.

I was thinking more of "assignment from incompatible... " type errors,
where fixing one case would break the other...

Ian.

>>> On 04.06.13 at 16:42, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Tue, 2013-06-04 at 15:36 +0100, Jan Beulich wrote:
>> >>> On 04.06.13 at 16:00, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>> > On Tue, 2013-06-04 at 14:52 +0100, Jan Beulich wrote:
>> >> >>> On 04.06.13 at 15:16, Ian Campbell
<ian.campbell@citrix.com> wrote:
>> >> > Currently uintptr_t is unsigned long for both 32- and 64-
bit platforms
> but
>> >> > __PRIPTR_PREFIX is just "" on 32-bit leading
to:
>> >> > 
>> >> >     error: format ''%u'' expects argument
of type ''unsigned int'', but
>> > argument 
>> >> > 2 has type ''long unsigned int''
[-Werror=format]
>> >> > 
>> >> > It was a bit of a tossup between changing __PREPTR_PREFIX
and changing the
>> >> > definition of uintptr_t. I went with the latter since it
is consistent
> with
>> >> > glibc.
>> >> 
>> >> Actually I would much prefer the format string prefix to be
changed
>> >> - like Linux, we''re assuming all over the place that
pointer and longs
>> >> are equivalent in size, so creating an association between
pointers
>> >> and ints is much more likely to cause problems than adjusting
the
>> >> format string prefix.
>> > 
>> > Can do that if you prefer.
>> > 
>> > Would this just push the problem into the code which is compiled
for
>> > both user and hypervisor space (e.g. libelf), uintptr_t would then
be
>> > different in the two environments. I haven''t tried it so
I don''t know
>> > what the practical impact is.
>> 
>> In user environment the print format should be taken from the
>> standard C headers just as the uintptr_t definition ought to be.
>> No risk of collision if done consistently.
> 
> Yes, absolutely.
> 
> I was thinking more of "assignment from incompatible... " type
errors,
> where fixing one case would break the other...
Ah, okay - that risk exists no matter what type we pick, as it''s
not just Linux our user mode stuff gets built on.

Jan

On Tue, 2013-06-04 at 14:16 +0100, Ian Campbell wrote:
> Currently uintptr_t is unsigned long for both 32- and 64- bit platforms but
> __PRIPTR_PREFIX is just "" on 32-bit leading to:
> 
>     error: format ''%u'' expects argument of type
''unsigned int'', but argument 2 has type ''long
unsigned int'' [-Werror=format]
> 
> It was a bit of a tossup between changing __PREPTR_PREFIX and changing the
> definition of uintptr_t. I went with the latter since it is consistent with
> glibc.
> 
> Tested with:
>     printk("uintptr %"PRIuPTR"\n",
(uintptr_t)a_convenient_local_var);
> in a conveniently built place on unstable (x86_64, arm32, arm64) and Xen
4.2
> (x86_32p & x86_64).
> 
> This error will be exposed by the fixes for XSA55 and therefore will need
to
> be backported along with those.
After discussing with Ian J we decided that rather than add yet more
complexity / dependencies to XSA-55 it would be better for him to carry
his existing workaround (#define ELF_PRIPTR, or some similar name) and
to fix the issue with uintptr_t vs. PRIuPTR after XSA-55 was done.

This is 4.4 material IMHO.

Ian.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: ian.jackson@eu.citrix.com
> Cc: keir@xen.org
> Cc: jbeulich@suse.com
> ---
>  xen/include/xen/types.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index 8596ded..641e1d1 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -57,6 +57,10 @@ typedef __u32 __be32;
>  typedef __u64 __le64;
>  typedef __u64 __be64;
>  
> +#if BITS_PER_LONG == 64
>  typedef unsigned long uintptr_t;
> +#else
> +typedef unsigned int uintptr_t;
> +#endif
>  
>  #endif /* __TYPES_H__ */