[PATCH] efi: Change grub_efi_boolean_t from char to enum

[PATCH] efi: Change grub_efi_boolean_t from char to enum

[Glenn Washburn]

This allows using GRUB_EFI_TRUE and GRUB_EFI_FALSE with proper type and
value checking. The UEFI 2.10 specification, in section 2.3.1, table 2.3,
says the size of the boolean is 1 byte and may only contain the values 0 or
1. In order to have the enum be 1-byte in size instead of the default
int-sized, add the packed attribute to the enum.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 include/grub/efi/api.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 5934db1c992b..be7c128dfb42 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -552,7 +552,13 @@ enum grub_efi_reset_type
 typedef enum grub_efi_reset_type grub_efi_reset_type_t;
 
 /* Types.  */
-typedef char grub_efi_boolean_t;
+enum GRUB_PACKED grub_efi_boolean
+  {
+    GRUB_EFI_FALSE,
+    GRUB_EFI_TRUE
+  };
+typedef enum grub_efi_boolean grub_efi_boolean_t;
+
 #if GRUB_CPU_SIZEOF_VOID_P == 8
 typedef grub_int64_t grub_efi_intn_t;
 typedef grub_uint64_t grub_efi_uintn_t;
-- 
2.34.1


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum

[Daniel Kiper]

On Mon, Aug 14, 2023 at 04:38:40PM -0500, Glenn Washburn wrote:
> This allows using GRUB_EFI_TRUE and GRUB_EFI_FALSE with proper type and
> value checking. The UEFI 2.10 specification, in section 2.3.1, table 2.3,
> says the size of the boolean is 1 byte and may only contain the values 0 or
> 1. In order to have the enum be 1-byte in size instead of the default
> int-sized, add the packed attribute to the enum.

Hmmm... Could you point out us an official doc saying "packed" attribute
does that?

And I hope you tested that change because we use grub_efi_boolean_t type
quite often...

> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  include/grub/efi/api.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index 5934db1c992b..be7c128dfb42 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -552,7 +552,13 @@ enum grub_efi_reset_type
>  typedef enum grub_efi_reset_type grub_efi_reset_type_t;
>
>  /* Types.  */
> -typedef char grub_efi_boolean_t;
> +enum GRUB_PACKED grub_efi_boolean
> +  {
> +    GRUB_EFI_FALSE,

I would explicitly do

  GRUB_EFI_FALSE = 0,

> +    GRUB_EFI_TRUE
> +  };

I would move GRUB_PACKED here to be inline with its other uses.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum

[Vladimir 'phcoder' Serbinenko]

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

I see little value in using enum here and too much compiler variance about
how it's handled. Compiler is pretty much allowed to choose any type for
enum. If we go this route at all we should at very least do a compile time
assert. Ideally I would keep it as-is. C unlike C++ does nothing to enforce
enum. Feel free to put GRUB_EFI_TRUE and FALSE into unnamed enum

Le lun. 14 août 2023, 23:39, Glenn Washburn <development@efficientek.com> a
écrit :

> This allows using GRUB_EFI_TRUE and GRUB_EFI_FALSE with proper type and
> value checking. The UEFI 2.10 specification, in section 2.3.1, table 2.3,
> says the size of the boolean is 1 byte and may only contain the values 0 or
> 1. In order to have the enum be 1-byte in size instead of the default
> int-sized, add the packed attribute to the enum.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  include/grub/efi/api.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index 5934db1c992b..be7c128dfb42 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -552,7 +552,13 @@ enum grub_efi_reset_type
>  typedef enum grub_efi_reset_type grub_efi_reset_type_t;
>
>  /* Types.  */
> -typedef char grub_efi_boolean_t;
> +enum GRUB_PACKED grub_efi_boolean
> +  {
> +    GRUB_EFI_FALSE,
> +    GRUB_EFI_TRUE
> +  };
> +typedef enum grub_efi_boolean grub_efi_boolean_t;
> +
>  #if GRUB_CPU_SIZEOF_VOID_P == 8
>  typedef grub_int64_t grub_efi_intn_t;
>  typedef grub_uint64_t grub_efi_uintn_t;
> --
> 2.34.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2424 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum

[Glenn Washburn]

On Thu, 31 Aug 2023 20:05:37 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, Aug 14, 2023 at 04:38:40PM -0500, Glenn Washburn wrote:
> > This allows using GRUB_EFI_TRUE and GRUB_EFI_FALSE with proper type and
> > value checking. The UEFI 2.10 specification, in section 2.3.1, table 2.3,
> > says the size of the boolean is 1 byte and may only contain the values 0 or
> > 1. In order to have the enum be 1-byte in size instead of the default
> > int-sized, add the packed attribute to the enum.
> 
> Hmmm... Could you point out us an official doc saying "packed" attribute
> does that?

This has been around since at least gcc 3.3, but this is link for a
more current gcc[1] (search for "packed").

> And I hope you tested that change because we use grub_efi_boolean_t type
> quite often...

I've been using this change for a couple months now with no issues.
Also, no tests are failing because of this change, although this might
not be a great recommendation. I want to do some more testing, just to
be sure.

> 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  include/grub/efi/api.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> > index 5934db1c992b..be7c128dfb42 100644
> > --- a/include/grub/efi/api.h
> > +++ b/include/grub/efi/api.h
> > @@ -552,7 +552,13 @@ enum grub_efi_reset_type
> >  typedef enum grub_efi_reset_type grub_efi_reset_type_t;
> >
> >  /* Types.  */
> > -typedef char grub_efi_boolean_t;
> > +enum GRUB_PACKED grub_efi_boolean
> > +  {
> > +    GRUB_EFI_FALSE,
> 
> I would explicitly do
> 
>   GRUB_EFI_FALSE = 0,

Good idea.

> 
> > +    GRUB_EFI_TRUE
> > +  };
> 
> I would move GRUB_PACKED here to be inline with its other uses.

The following from the GCC manual has me questioning why we do this:
"You can also place [attributes] just past the closing curly brace of
the definition, but this is less preferred because logically the type
should be fully defined at the closing brace."[2] I'll change this in
the next iteration for consistency, but perhaps we should reconsider
the policy?

Glenn

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html
[2] https://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum

[Daniel Kiper]

On Thu, Aug 31, 2023 at 11:58:42PM -0500, Glenn Washburn wrote:
> On Thu, 31 Aug 2023 20:05:37 +0200
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Mon, Aug 14, 2023 at 04:38:40PM -0500, Glenn Washburn wrote:
> > > This allows using GRUB_EFI_TRUE and GRUB_EFI_FALSE with proper type and
> > > value checking. The UEFI 2.10 specification, in section 2.3.1, table 2.3,
> > > says the size of the boolean is 1 byte and may only contain the values 0 or
> > > 1. In order to have the enum be 1-byte in size instead of the default
> > > int-sized, add the packed attribute to the enum.
> >
> > Hmmm... Could you point out us an official doc saying "packed" attribute
> > does that?
>
> This has been around since at least gcc 3.3, but this is link for a
> more current gcc[1] (search for "packed").

Does it work on clang? I, as Valdimir, am afraid we are entering into
shakey grounds...

> > And I hope you tested that change because we use grub_efi_boolean_t type
> > quite often...
>
> I've been using this change for a couple months now with no issues.
> Also, no tests are failing because of this change, although this might
> not be a great recommendation. I want to do some more testing, just to
> be sure.
>
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  include/grub/efi/api.h | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> > > index 5934db1c992b..be7c128dfb42 100644
> > > --- a/include/grub/efi/api.h
> > > +++ b/include/grub/efi/api.h
> > > @@ -552,7 +552,13 @@ enum grub_efi_reset_type
> > >  typedef enum grub_efi_reset_type grub_efi_reset_type_t;
> > >
> > >  /* Types.  */
> > > -typedef char grub_efi_boolean_t;
> > > +enum GRUB_PACKED grub_efi_boolean
> > > +  {
> > > +    GRUB_EFI_FALSE,
> >
> > I would explicitly do
> >
> >   GRUB_EFI_FALSE = 0,
>
> Good idea.
>
> >
> > > +    GRUB_EFI_TRUE
> > > +  };
> >
> > I would move GRUB_PACKED here to be inline with its other uses.
>
> The following from the GCC manual has me questioning why we do this:
> "You can also place [attributes] just past the closing curly brace of
> the definition, but this is less preferred because logically the type
> should be fully defined at the closing brace."[2] I'll change this in
> the next iteration for consistency, but perhaps we should reconsider
> the policy?

I am OK with doing this cleanup but after the release...

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum

[Glenn Washburn]

On Thu, 31 Aug 2023 20:36:43 +0200
"Vladimir 'phcoder' Serbinenko" <phcoder@gmail.com> wrote:

> I see little value in using enum here and too much compiler variance about
> how it's handled. 

Are you specifically talking about GCC, or the diversity of compilers
that GRUB might be compiled with?

> Compiler is pretty much allowed to choose any type for enum.

If you're talking about GCC, then with the packed attribute this
statement seems false[1].

> If we go this route at all we should at very least do a compile time
> assert.

Sounds like a good idea.

> Ideally I would keep it as-is. C unlike C++ does nothing to enforce
> enum. 

Yes, this is unfortunate. I made this change because I think it looks
nicer. I was thinking that the compiler might do obvious checking to
fail if say a literal value outside the enum range is passed as a
function argument that is an enum type. But you're right it doesn't do
that. Using the enum type then obscures the fact that the boolean type
should be 8 bits as specified by the spec. So typedef'ing as a char,
as is currently done, seems to be a clearer implementation.

I was also hoping that this change would provide a little more detail
for backtraces in GDB, but I've yet to check this.

> Feel free to put GRUB_EFI_TRUE and FALSE into unnamed enum

Yes, I do like this instead of the macros. Now I'm questioning if its
worth the change though.

[1] https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html

> Le lun. 14 août 2023, 23:39, Glenn Washburn <development@efficientek.com> a
> écrit :
> 
> > This allows using GRUB_EFI_TRUE and GRUB_EFI_FALSE with proper type and
> > value checking. The UEFI 2.10 specification, in section 2.3.1, table 2.3,
> > says the size of the boolean is 1 byte and may only contain the values 0 or
> > 1. In order to have the enum be 1-byte in size instead of the default
> > int-sized, add the packed attribute to the enum.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  include/grub/efi/api.h | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> > index 5934db1c992b..be7c128dfb42 100644
> > --- a/include/grub/efi/api.h
> > +++ b/include/grub/efi/api.h
> > @@ -552,7 +552,13 @@ enum grub_efi_reset_type
> >  typedef enum grub_efi_reset_type grub_efi_reset_type_t;
> >
> >  /* Types.  */
> > -typedef char grub_efi_boolean_t;
> > +enum GRUB_PACKED grub_efi_boolean
> > +  {
> > +    GRUB_EFI_FALSE,
> > +    GRUB_EFI_TRUE
> > +  };
> > +typedef enum grub_efi_boolean grub_efi_boolean_t;
> > +
> >  #if GRUB_CPU_SIZEOF_VOID_P == 8
> >  typedef grub_int64_t grub_efi_intn_t;
> >  typedef grub_uint64_t grub_efi_uintn_t;
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
> >

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum

[Glenn Washburn]

On Sat, 2 Sep 2023 20:45:34 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Thu, Aug 31, 2023 at 11:58:42PM -0500, Glenn Washburn wrote:
> > On Thu, 31 Aug 2023 20:05:37 +0200
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> >
> > > On Mon, Aug 14, 2023 at 04:38:40PM -0500, Glenn Washburn wrote:
> > > > This allows using GRUB_EFI_TRUE and GRUB_EFI_FALSE with proper type and
> > > > value checking. The UEFI 2.10 specification, in section 2.3.1, table 2.3,
> > > > says the size of the boolean is 1 byte and may only contain the values 0 or
> > > > 1. In order to have the enum be 1-byte in size instead of the default
> > > > int-sized, add the packed attribute to the enum.
> > >
> > > Hmmm... Could you point out us an official doc saying "packed" attribute
> > > does that?
> >
> > This has been around since at least gcc 3.3, but this is link for a
> > more current gcc[1] (search for "packed").
> 
> Does it work on clang? I, as Valdimir, am afraid we are entering into
> shakey grounds...

I didn't realize that clang was the concern. No I've not tested with
clang and not setup to test it right now. So I'll table this for now.

> > > And I hope you tested that change because we use grub_efi_boolean_t type
> > > quite often...
> >
> > I've been using this change for a couple months now with no issues.
> > Also, no tests are failing because of this change, although this might
> > not be a great recommendation. I want to do some more testing, just to
> > be sure.
> >
> > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > ---
> > > >  include/grub/efi/api.h | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> > > > index 5934db1c992b..be7c128dfb42 100644
> > > > --- a/include/grub/efi/api.h
> > > > +++ b/include/grub/efi/api.h
> > > > @@ -552,7 +552,13 @@ enum grub_efi_reset_type
> > > >  typedef enum grub_efi_reset_type grub_efi_reset_type_t;
> > > >
> > > >  /* Types.  */
> > > > -typedef char grub_efi_boolean_t;
> > > > +enum GRUB_PACKED grub_efi_boolean
> > > > +  {
> > > > +    GRUB_EFI_FALSE,
> > >
> > > I would explicitly do
> > >
> > >   GRUB_EFI_FALSE = 0,
> >
> > Good idea.
> >
> > >
> > > > +    GRUB_EFI_TRUE
> > > > +  };
> > >
> > > I would move GRUB_PACKED here to be inline with its other uses.
> >
> > The following from the GCC manual has me questioning why we do this:
> > "You can also place [attributes] just past the closing curly brace of
> > the definition, but this is less preferred because logically the type
> > should be fully defined at the closing brace."[2] I'll change this in
> > the next iteration for consistency, but perhaps we should reconsider
> > the policy?
> 
> I am OK with doing this cleanup but after the release...

Sounds good. I think its pretty low priority.

Glenn

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel