* [PATCH] efi: Change grub_efi_boolean_t from char to enum @ 2023-08-14 21:38 Glenn Washburn 2023-08-31 18:05 ` Daniel Kiper 2023-08-31 18:36 ` Vladimir 'phcoder' Serbinenko 0 siblings, 2 replies; 7+ messages in thread From: Glenn Washburn @ 2023-08-14 21:38 UTC (permalink / raw) To: grub-devel, Daniel Kiper, Ard Biesheuvel; +Cc: 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum 2023-08-14 21:38 [PATCH] efi: Change grub_efi_boolean_t from char to enum Glenn Washburn @ 2023-08-31 18:05 ` Daniel Kiper 2023-09-01 4:58 ` Glenn Washburn 2023-08-31 18:36 ` Vladimir 'phcoder' Serbinenko 1 sibling, 1 reply; 7+ messages in thread From: Daniel Kiper @ 2023-08-31 18:05 UTC (permalink / raw) To: Glenn Washburn; +Cc: grub-devel, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum 2023-08-31 18:05 ` Daniel Kiper @ 2023-09-01 4:58 ` Glenn Washburn 2023-09-02 18:45 ` Daniel Kiper 0 siblings, 1 reply; 7+ messages in thread From: Glenn Washburn @ 2023-09-01 4:58 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum 2023-09-01 4:58 ` Glenn Washburn @ 2023-09-02 18:45 ` Daniel Kiper 2023-09-04 19:34 ` Glenn Washburn 0 siblings, 1 reply; 7+ messages in thread From: Daniel Kiper @ 2023-09-02 18:45 UTC (permalink / raw) To: Glenn Washburn; +Cc: grub-devel, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum 2023-09-02 18:45 ` Daniel Kiper @ 2023-09-04 19:34 ` Glenn Washburn 0 siblings, 0 replies; 7+ messages in thread From: Glenn Washburn @ 2023-09-04 19:34 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel, Ard Biesheuvel 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum 2023-08-14 21:38 [PATCH] efi: Change grub_efi_boolean_t from char to enum Glenn Washburn 2023-08-31 18:05 ` Daniel Kiper @ 2023-08-31 18:36 ` Vladimir 'phcoder' Serbinenko 2023-09-04 19:30 ` Glenn Washburn 1 sibling, 1 reply; 7+ messages in thread From: Vladimir 'phcoder' Serbinenko @ 2023-08-31 18:36 UTC (permalink / raw) To: The development of GNU GRUB [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] efi: Change grub_efi_boolean_t from char to enum 2023-08-31 18:36 ` Vladimir 'phcoder' Serbinenko @ 2023-09-04 19:30 ` Glenn Washburn 0 siblings, 0 replies; 7+ messages in thread From: Glenn Washburn @ 2023-09-04 19:30 UTC (permalink / raw) To: Vladimir 'phcoder' Serbinenko; +Cc: The development of GNU GRUB 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-09-04 19:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-14 21:38 [PATCH] efi: Change grub_efi_boolean_t from char to enum Glenn Washburn 2023-08-31 18:05 ` Daniel Kiper 2023-09-01 4:58 ` Glenn Washburn 2023-09-02 18:45 ` Daniel Kiper 2023-09-04 19:34 ` Glenn Washburn 2023-08-31 18:36 ` Vladimir 'phcoder' Serbinenko 2023-09-04 19:30 ` Glenn Washburn
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.