[PATCH] Check for the appropriate condition in types.h

[PATCH] Check for the appropriate condition in types.h

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 675 bytes --]

Currently <grub/types.h> verifies that sizeof(void*)==sizeof(long) and
then proceeds to define most fixed-length types based on sizeof(void*).
While simplification seems a good idea on paper, it is always good
practice to check specifically for what we want to know. In particular,
the assumption that long can hold a pointer will go down when (if)
mingw64 support is merged, because Win64 follows the LLP64 model instead
of the LP64 *nix model.

Given that currently both values are, by precondition, the same, this
change is guaranteed not to create any problems, while it might avoid
them in the future.

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #1.2: checkforlong.patch --]
[-- Type: text/x-patch, Size: 1046 bytes --]

diff --git a/include/grub/types.h b/include/grub/types.h
index 8e2ad15..50b253a 100644
--- a/include/grub/types.h
+++ b/include/grub/types.h
@@ -62,7 +62,7 @@
 typedef signed char		grub_int8_t;
 typedef short			grub_int16_t;
 typedef int			grub_int32_t;
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#if GRUB_CPU_SIZEOF_LONG == 8
 typedef long			grub_int64_t;
 #else
 typedef long long		grub_int64_t;
@@ -71,7 +71,7 @@ typedef long long		grub_int64_t;
 typedef unsigned char		grub_uint8_t;
 typedef unsigned short		grub_uint16_t;
 typedef unsigned		grub_uint32_t;
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#if GRUB_CPU_SIZEOF_LONG == 8
 typedef unsigned long		grub_uint64_t;
 #else
 typedef unsigned long long	grub_uint64_t;
@@ -100,7 +100,7 @@ typedef grub_uint32_t	grub_size_t;
 typedef grub_int32_t	grub_ssize_t;
 #endif
 
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#if GRUB_CPU_SIZEOF_LONG == 8
 # define GRUB_ULONG_MAX 18446744073709551615UL
 # define GRUB_LONG_MAX 9223372036854775807L
 # define GRUB_LONG_MIN (-9223372036854775807L - 1)

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] Check for the appropriate condition in types.h

[Pavel Roskin]

On Thu, 2009-07-23 at 13:10 +0200, Javier Martín wrote:
> Currently <grub/types.h> verifies that sizeof(void*)==sizeof(long) and
> then proceeds to define most fixed-length types based on sizeof(void*).
> While simplification seems a good idea on paper, it is always good
> practice to check specifically for what we want to know. In particular,
> the assumption that long can hold a pointer will go down when (if)
> mingw64 support is merged, because Win64 follows the LLP64 model instead
> of the LP64 *nix model.
> 
> Given that currently both values are, by precondition, the same, this
> change is guaranteed not to create any problems, while it might avoid
> them in the future.

The patch is good.  Please include the ChangeLog entry.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Check for the appropriate condition in types.h

[Javier Martín]

[-- Attachment #1.1: Type: text/plain, Size: 2679 bytes --]

El jue, 23-07-2009 a las 10:37 -0400, Pavel Roskin escribió:
> On Thu, 2009-07-23 at 13:10 +0200, Javier Martín wrote:
> > Currently <grub/types.h> verifies that sizeof(void*)==sizeof(long) and
> > then proceeds to define most fixed-length types based on sizeof(void*).
> > While simplification seems a good idea on paper, it is always good
> > practice to check specifically for what we want to know. In particular,
> > the assumption that long can hold a pointer will go down when (if)
> > mingw64 support is merged, because Win64 follows the LLP64 model instead
> > of the LP64 *nix model.
> > 
> > Given that currently both values are, by precondition, the same, this
> > change is guaranteed not to create any problems, while it might avoid
> > them in the future.
> 
> The patch is good.  Please include the ChangeLog entry.
> 
Thanks. I have though of an additional change: if it seems acceptable,
apply the "new" version of the patch that I attach with this message. If
it is not, just use the old version in the original post and I'll send
the new change as another patch.

The additional change is the refactoring of the UINT_TO_PTR macro. Given
that we have a grub_addr_t type fulfilling a role similar to the
standard uintptr_t (an integral type "long enough to hold a pointer and
back"), there is no need for the conditional definition of UINT_TO_PTR
as either casting to a 32 or 64-bit type: grub_addr_t is defined exactly
like that.

Furthermore, I have added a PTR_TO_UINT macro as a means to perform
safer* "hard" pointer arithmetic that frequently comes up in low-level
code without having to think of the types to cast to and back. This way,
this random code: (in acpi.c)
	target = (grub_uint8_t *) ((((long) target - 1) | 0xf) + 1);
Would become:
	target = UINT_TO_PTR(((PTR_TO_UINT(target) - 1) | 0xf) + 1);
Which is safe* as long as grub_addr_t is properly defined. Thus, this
new macro will allow coders to gather scattered typing decisions and
centralize them to types.h. The process would be just: PTR_TO_UINT ->
perform weird arithmetic -> UINT_TO_PTR.


* safer in the sense of not risking any truncations or "undefined
behavior" if the addr_t types change, of course, not for the actual
arithmetic being performed


Regarding the ChangeLog entry, I always have problems with them. What
about this one?:
2009-07-23 Javier Martin <lordhabbit@gmail.com>
	* include/grub/types.h (GRUB_CPU_SIZEOF_VOID_P): substitute for
	  GRUB_CPU_SIZEOF_LONG where appropriate
	  (UINT_TO_PTR): move outside wordsize conditionals
	  (PTR_TO_UINT): new macro

-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #1.2: checkforlong.1.patch --]
[-- Type: text/x-patch, Size: 1634 bytes --]

diff --git a/include/grub/types.h b/include/grub/types.h
index 8e2ad15..72f1288 100644
--- a/include/grub/types.h
+++ b/include/grub/types.h
@@ -62,7 +62,7 @@
 typedef signed char		grub_int8_t;
 typedef short			grub_int16_t;
 typedef int			grub_int32_t;
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#if GRUB_CPU_SIZEOF_LONG == 8
 typedef long			grub_int64_t;
 #else
 typedef long long		grub_int64_t;
@@ -71,7 +71,7 @@ typedef long long		grub_int64_t;
 typedef unsigned char		grub_uint8_t;
 typedef unsigned short		grub_uint16_t;
 typedef unsigned		grub_uint32_t;
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#if GRUB_CPU_SIZEOF_LONG == 8
 typedef unsigned long		grub_uint64_t;
 #else
 typedef unsigned long long	grub_uint64_t;
@@ -100,7 +100,7 @@ typedef grub_uint32_t	grub_size_t;
 typedef grub_int32_t	grub_ssize_t;
 #endif
 
-#if GRUB_CPU_SIZEOF_VOID_P == 8
+#if GRUB_CPU_SIZEOF_LONG == 8
 # define GRUB_ULONG_MAX 18446744073709551615UL
 # define GRUB_LONG_MAX 9223372036854775807L
 # define GRUB_LONG_MIN (-9223372036854775807L - 1)
@@ -110,12 +110,12 @@ typedef grub_int32_t	grub_ssize_t;
 # define GRUB_LONG_MIN (-2147483647L - 1)
 #endif
 
+#define UINT_TO_PTR(x) ((void*)(grub_addr_t)(x))
+#define PTR_TO_UINT(x) ((grub_addr_t)(x))
 #if GRUB_CPU_SIZEOF_VOID_P == 4
-#define UINT_TO_PTR(x) ((void*)(grub_uint32_t)(x))
 #define PTR_TO_UINT64(x) ((grub_uint64_t)(grub_uint32_t)(x))
 #define PTR_TO_UINT32(x) ((grub_uint32_t)(x))
 #else
-#define UINT_TO_PTR(x) ((void*)(grub_uint64_t)(x))
 #define PTR_TO_UINT64(x) ((grub_uint64_t)(x))
 #define PTR_TO_UINT32(x) ((grub_uint32_t)(grub_uint64_t)(x))
 #endif

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] Check for the appropriate condition in types.h

[Vladimir 'phcoder' Serbinenko]

> Thanks. I have though of an additional change: if it seems acceptable,
> apply the "new" version of the patch that I attach with this message. If
> it is not, just use the old version in the original post and I'll send
> the new change as another patch.
>
> The additional change is the refactoring of the UINT_TO_PTR macro. Given
> that we have a grub_addr_t type fulfilling a role similar to the
> standard uintptr_t (an integral type "long enough to hold a pointer and
> back"), there is no need for the conditional definition of UINT_TO_PTR
> as either casting to a 32 or 64-bit type: grub_addr_t is defined exactly
> like that.
I'm fine with that
>
> Furthermore, I have added a PTR_TO_UINT macro as a means to perform
> safer* "hard" pointer arithmetic that frequently comes up in low-level
> code without having to think of the types to cast to and back. This way,
> this random code: (in acpi.c)
>        target = (grub_uint8_t *) ((((long) target - 1) | 0xf) + 1);
> Would become:
>        target = UINT_TO_PTR(((PTR_TO_UINT(target) - 1) | 0xf) + 1);
Actualy I should have used ALIGN_UP here

>
> Regarding the ChangeLog entry, I always have problems with them. What
> about this one?:
> 2009-07-23 Javier Martin <lordhabbit@gmail.com>
>        * include/grub/types.h (GRUB_CPU_SIZEOF_VOID_P): substitute for
This would suggest that you modify GRUB_CPU_SIZEOF_VOID_P macro which
isn't the case. I would prefer something like:
[GRUB_CPU_SIZEOF_VOID_P == 8]: Changed to ...
[GRUB_CPU_SIZEOF_LONG == 8]: ... this.
>          GRUB_CPU_SIZEOF_LONG where appropriate
>          (UINT_TO_PTR): move outside wordsize conditionals

>          (PTR_TO_UINT): new macro

>
> --
> -- Lazy, Oblivious, Recurrent Disaster -- Habbit
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Check for the appropriate condition in types.h

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 1080 bytes --]

El jue, 23-07-2009 a las 17:40 +0200, Vladimir 'phcoder' Serbinenko
escribió:
> Actualy I should have used ALIGN_UP here
It's just a random sample of code, brought up by a search for (long).
I'm pretty sure more examples abound.

> > Regarding the ChangeLog entry, I always have problems with them. What
> > about this one?:
> > 2009-07-23 Javier Martin <lordhabbit@gmail.com>
> >        * include/grub/types.h (GRUB_CPU_SIZEOF_VOID_P): substitute for
> This would suggest that you modify GRUB_CPU_SIZEOF_VOID_P macro which
> isn't the case. I would prefer something like:
> [GRUB_CPU_SIZEOF_VOID_P == 8]: Changed to ...
> [GRUB_CPU_SIZEOF_LONG == 8]: ... this.
Ok, let's adopt this form instead. The proposed ChangeLog would now be:

2009-07-23 Javier Martin <lordhabbit@gmail.com>
        * include/grub/types.h
	  [GRUB_CPU_SIZEOF_VOID_P == 8]: Changed to ...
	  [GRUB_CPU_SIZEOF_LONG == 8]: ...this
          (UINT_TO_PTR): move outside wordsize conditionals
          (PTR_TO_UINT): new macro


-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] Check for the appropriate condition in types.h

[Pavel Roskin]

On Thu, 2009-07-23 at 20:11 +0200, Javier Martín wrote:
> El jue, 23-07-2009 a las 17:40 +0200, Vladimir 'phcoder' Serbinenko
> escribió:
> > Actualy I should have used ALIGN_UP here
> It's just a random sample of code, brought up by a search for (long).
> I'm pretty sure more examples abound.
> 
> > > Regarding the ChangeLog entry, I always have problems with them. What
> > > about this one?:
> > > 2009-07-23 Javier Martin <lordhabbit@gmail.com>
> > >        * include/grub/types.h (GRUB_CPU_SIZEOF_VOID_P): substitute for
> > This would suggest that you modify GRUB_CPU_SIZEOF_VOID_P macro which
> > isn't the case. I would prefer something like:
> > [GRUB_CPU_SIZEOF_VOID_P == 8]: Changed to ...
> > [GRUB_CPU_SIZEOF_LONG == 8]: ... this.
> Ok, let's adopt this form instead. The proposed ChangeLog would now be:

From the GNU Coding Standards:

"C programs often contain compile-time `#if' conditionals.  Many changes
are conditional; sometimes you add a new definition which is entirely
contained in a conditional.  It is very useful to indicate in the
change log the conditions for which the change applies.  Our convention
for indicating conditional changes is to use square brackets around the
name of the condition."

It means that the square brackets are used if the changes only affect
the code under the condition specified in brackets.  This is not what is
happening here.

>           (UINT_TO_PTR): move outside wordsize conditionals
>           (PTR_TO_UINT): new macro

We should remove PTR_TO_UINT32 and PTR_TO_UINT64 with PTR_TO_UINT
everywhere.  I've checked that it doesn't introduce any warnings on any
platform.

There should be no need to combine a cast to a scalar type with a cast
to change the type precision.  By eliminating the second cast, we make
it possible for the compiler to catch precision loss.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Check for the appropriate condition in types.h

[Vladimir 'phcoder' Serbinenko]

>> > [GRUB_CPU_SIZEOF_VOID_P == 8]: Changed to ...
>> > [GRUB_CPU_SIZEOF_LONG == 8]: ... this.
>> Ok, let's adopt this form instead. The proposed ChangeLog would now be:
>
> >From the GNU Coding Standards:
>
> "C programs often contain compile-time `#if' conditionals.  Many changes
> are conditional; sometimes you add a new definition which is entirely
> contained in a conditional.  It is very useful to indicate in the
> change log the conditions for which the change applies.  Our convention
> for indicating conditional changes is to use square brackets around the
> name of the condition."
>
> It means that the square brackets are used if the changes only affect
> the code under the condition specified in brackets.  This is not what is
> happening here.
>
This is exactly what happens here: we change only what happens in
conditionals [GRUB_CPU_SIZEOF_VOID_P == 8] and [GRUB_CPU_SIZEOF_LONG
== 8]
>>           (UINT_TO_PTR): move outside wordsize conditionals
>>           (PTR_TO_UINT): new macro
>
> We should remove PTR_TO_UINT32 and PTR_TO_UINT64 with PTR_TO_UINT
> everywhere.  I've checked that it doesn't introduce any warnings on any
> platform.
>
Sometimes PTR_TOUINT32 with precision loss is exactly what the coder
wants. A typical example is booting 32-bit OS on 64-bit platform. This
is the case of booting linux on efi64. Then the code has to ensure
that the target is below 4GiB (see my mm propositions for more on how
to ensure it). But I'm ok with requirement of additional explicit cast
in such cases as
(grub_uint32_t) PTR_TO_UINT (x)
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH] Check for the appropriate condition in types.h

[Pavel Roskin]

On Thu, 2009-07-23 at 22:30 +0200, Vladimir 'phcoder' Serbinenko wrote:
> >> > [GRUB_CPU_SIZEOF_VOID_P == 8]: Changed to ...
> >> > [GRUB_CPU_SIZEOF_LONG == 8]: ... this.
> >> Ok, let's adopt this form instead. The proposed ChangeLog would now be:
> >
> > >From the GNU Coding Standards:
> >
> > "C programs often contain compile-time `#if' conditionals.  Many changes
> > are conditional; sometimes you add a new definition which is entirely
> > contained in a conditional.  It is very useful to indicate in the
> > change log the conditions for which the change applies.  Our convention
> > for indicating conditional changes is to use square brackets around the
> > name of the condition."
> >
> > It means that the square brackets are used if the changes only affect
> > the code under the condition specified in brackets.  This is not what is
> > happening here.
> >
> This is exactly what happens here: we change only what happens in
> conditionals [GRUB_CPU_SIZEOF_VOID_P == 8] and [GRUB_CPU_SIZEOF_LONG
> == 8]

I'm not interested to discuss the right interpretation of the coding
standards.

Frankly, I'm not a fan of keeping ChangeLog, as it's too formal and it's
a point of contention for parallel development.  I prefer the Linux
kernel style of descriptions.

But since we are updating ChangeLog, let's keep it readable.

> >>           (UINT_TO_PTR): move outside wordsize conditionals
> >>           (PTR_TO_UINT): new macro
> >
> > We should remove PTR_TO_UINT32 and PTR_TO_UINT64 with PTR_TO_UINT
> > everywhere.  I've checked that it doesn't introduce any warnings on any
> > platform.
> >
> Sometimes PTR_TOUINT32 with precision loss is exactly what the coder
> wants. A typical example is booting 32-bit OS on 64-bit platform. This
> is the case of booting linux on efi64. Then the code has to ensure
> that the target is below 4GiB (see my mm propositions for more on how
> to ensure it). But I'm ok with requirement of additional explicit cast
> in such cases as
> (grub_uint32_t) PTR_TO_UINT (x)

That would be much better that PTR_TO_UINT32, as it makes the cast
explicit in the code that should ensure that the cast is valid.

We can find such cases by compiling with -Wconversion and finding the
newly appearing warnings after PTR_TO_UINT32 and PTR_TO_UINT64 are
replaced with PTR_TO_UINT.

-- 
Regards,
Pavel Roskin

Re: [PATCH] Check for the appropriate condition in types.h

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 3496 bytes --]

El jue, 23-07-2009 a las 16:57 -0400, Pavel Roskin escribió:
> On Thu, 2009-07-23 at 22:30 +0200, Vladimir 'phcoder' Serbinenko wrote:
> > >> > [GRUB_CPU_SIZEOF_VOID_P == 8]: Changed to ...
> > >> > [GRUB_CPU_SIZEOF_LONG == 8]: ... this.
> > >> Ok, let's adopt this form instead. The proposed ChangeLog would now be:
> > >
> > > >From the GNU Coding Standards:
> > >
> > > "C programs often contain compile-time `#if' conditionals.  Many changes
> > > are conditional; sometimes you add a new definition which is entirely
> > > contained in a conditional.  It is very useful to indicate in the
> > > change log the conditions for which the change applies.  Our convention
> > > for indicating conditional changes is to use square brackets around the
> > > name of the condition."
> > >
> > > It means that the square brackets are used if the changes only affect
> > > the code under the condition specified in brackets.  This is not what is
> > > happening here.
> > >
> > This is exactly what happens here: we change only what happens in
> > conditionals [GRUB_CPU_SIZEOF_VOID_P == 8] and [GRUB_CPU_SIZEOF_LONG
> > == 8]
> 
> I'm not interested to discuss the right interpretation of the coding
> standards.
> 
> Frankly, I'm not a fan of keeping ChangeLog, as it's too formal and it's
> a point of contention for parallel development.  I prefer the Linux
> kernel style of descriptions.
> 
> But since we are updating ChangeLog, let's keep it readable.
So, how would this precise change be specified? I don't mean to seem
anxious, but I wouldn't want such a simple patch to be ensnared by what
I personally consider a Byzantine argument. I propose the following
wording, but we need someone more familiar with the FSF "bureaucracy" to
review it: O Robert, we summon Thou ;)

2009-07-26 Javier Martin <lordhabbit@gmail.com>
        * include/grub/types.h (GRUB_CPU_SIZEOF_VOID_P == 8): Change...
          (GRUB_CPU_SIZEOF_LONG == 8) ... into this where appropriate
          (UINT_TO_PTR): move outside wordsize conditionals
          (PTR_TO_UINT): new macro


> 
> > >>           (UINT_TO_PTR): move outside wordsize conditionals
> > >>           (PTR_TO_UINT): new macro
> > >
> > > We should remove PTR_TO_UINT32 and PTR_TO_UINT64 with PTR_TO_UINT
> > > everywhere.  I've checked that it doesn't introduce any warnings on any
> > > platform.
> > >
> > Sometimes PTR_TOUINT32 with precision loss is exactly what the coder
> > wants. A typical example is booting 32-bit OS on 64-bit platform. This
> > is the case of booting linux on efi64. Then the code has to ensure
> > that the target is below 4GiB (see my mm propositions for more on how
> > to ensure it). But I'm ok with requirement of additional explicit cast
> > in such cases as
> > (grub_uint32_t) PTR_TO_UINT (x)
> 
> That would be much better that PTR_TO_UINT32, as it makes the cast
> explicit in the code that should ensure that the cast is valid.
> 
> We can find such cases by compiling with -Wconversion and finding the
> newly appearing warnings after PTR_TO_UINT32 and PTR_TO_UINT64 are
> replaced with PTR_TO_UINT.
> 
I also agree that PTR_TO_UINTnn should not exist: the eventual precision
loss should be shown in the actual source using/abusing it. After this
patch is merged, I could send in another one replacing all occurrences
of those two macros by PTR_TO_UINT with the right casts.


-- 
-- Lazy, Oblivious, Recurrent Disaster -- Habbit

[-- Attachment #2: Esto es una parte de mensaje firmado digitalmente --]
[-- Type: application/pgp-signature, Size: 835 bytes --]