[PATCH] gnulib: Handle aborts in gnulib more gracefully

[PATCH] gnulib: Handle aborts in gnulib more gracefully

[Glenn Washburn]

Gnulib does not define abort(), but expects it to be defined, usually by a
libc implementation. However, GRUB has no libc and does not have an abort()
function. Until recently GRUB had patched out abort() in gnulib, effectively
making it a noop. This changed with a recent update of the version of gnulib
GRUB uses to be a compiler defined trap. While this is fine for user space
code where the kernel can be expected to cleanup after a process when this
happens, the firmware may not be good at doing this.

GRUB does have a grub_abort(), with the same function signature that can be
used instead. So instead define gnulib's abort() implementation to be
grub_abort(). This provides consistency of behavior regardless of whether
the abort happens in gnulib code or in GRUB code.

Because we want to avoid patching gnulib again and gnulib expects that
abort(), now grub_abort(), is defined externally, we must provide a
prototype in the config.h. This is complicated by the fact that config.h
is included in many GRUB compilation units as well as grub/misc.h which
already declares grub_abort(). A macro, GNULIB, is provided only to
compilation units using gnulib code, so that the grub_abort() prototype
is only included in those compilation units where it is lacking.

Also, export grub_abort() so that it can now be used within modules.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 conf/Makefile.common  |  2 +-
 config.h.in           | 17 ++++++++++-------
 grub-core/kern/misc.c |  2 +-
 include/grub/misc.h   |  1 +
 4 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/conf/Makefile.common b/conf/Makefile.common
index b343a038ee..67996b710c 100644
--- a/conf/Makefile.common
+++ b/conf/Makefile.common
@@ -65,7 +65,7 @@ grubconfdir = $(sysconfdir)/grub.d
 platformdir = $(pkglibdir)/$(target_cpu)-$(platform)
 starfielddir = $(pkgdatadir)/themes/starfield
 
-CFLAGS_GNULIB = -Wno-undef -Wno-sign-compare -Wno-unused -Wno-unused-parameter -Wno-redundant-decls -Wno-unreachable-code -Wno-conversion
+CFLAGS_GNULIB = -Wno-undef -Wno-sign-compare -Wno-unused -Wno-unused-parameter -Wno-redundant-decls -Wno-unreachable-code -Wno-conversion -DGNULIB=1
 CPPFLAGS_GNULIB = -I$(top_builddir)/grub-core/lib/gnulib -I$(top_srcdir)/grub-core/lib/gnulib
 
 CFLAGS_POSIX = -fno-builtin
diff --git a/config.h.in b/config.h.in
index 01dcbbfc82..5719b28ebe 100644
--- a/config.h.in
+++ b/config.h.in
@@ -139,14 +139,17 @@ typedef __UINT_FAST32_TYPE__ uint_fast32_t;
 #    define _GL_INLINE_HEADER_END   _Pragma ("GCC diagnostic pop")
 
 /*
- * We don't have an abort() for gnulib to call in regexp.  Because gnulib is
- * built as a separate object that is then linked with, it doesn't have any
- * of our headers or functions around to use - so we unfortunately can't
- * print anything.  Builds of emu include the system's stdlib, which includes
- * a prototype for abort(), so leave this as a macro that doesn't take
- * arguments.
+ * We don't have an abort() for gnulib's functions to call (eg. in regexp), but
+ * one needs to be provided and grub_abort() is the equivalent. Gnulib does not
+ * provide a prototype for abort either, so one must be provided. However, this
+ * duplicates the prototype in grub/misc.h for GRUB compilation units, which
+ * causes a failure to compile. So only include the prototype if this is in a
+ * gnulib compilation unit.
  */
-#    define abort __builtin_trap
+#    ifdef GNULIB
+#      define abort grub_abort
+void __attribute__ ((noreturn)) abort (void);
+#    endif
 #  endif /* !_GL_INLINE_HEADER_BEGIN */
 
 /* gnulib doesn't build cleanly with older compilers. */
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 6c0221cc33..dfae4f9d78 100644
--- a/grub-core/kern/misc.c
+++ b/grub-core/kern/misc.c
@@ -1249,7 +1249,7 @@ grub_printf_fmt_check (const char *fmt, const char *fmt_expected)
 
 
 /* Abort GRUB. This function does not return.  */
-static void __attribute__ ((noreturn))
+void __attribute__ ((noreturn))
 grub_abort (void)
 {
   grub_printf ("\nAborted.");
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 7d2b551969..776dbf8af0 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -353,6 +353,7 @@ int EXPORT_FUNC(grub_vsnprintf) (char *str, grub_size_t n, const char *fmt,
 char *EXPORT_FUNC(grub_xasprintf) (const char *fmt, ...)
      __attribute__ ((format (GNU_PRINTF, 1, 2))) WARN_UNUSED_RESULT;
 char *EXPORT_FUNC(grub_xvasprintf) (const char *fmt, va_list args) WARN_UNUSED_RESULT;
+void EXPORT_FUNC(grub_abort) (void) __attribute__ ((noreturn));
 void EXPORT_FUNC(grub_exit) (void) __attribute__ ((noreturn));
 grub_uint64_t EXPORT_FUNC(grub_divmod64) (grub_uint64_t n,
 					  grub_uint64_t d,
-- 
2.27.0

Re: [PATCH] gnulib: Handle aborts in gnulib more gracefully

[Robbie Harwood]

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

Glenn Washburn <development@efficientek.com> writes:

> Gnulib does not define abort(), but expects it to be defined, usually by a
> libc implementation. However, GRUB has no libc and does not have an abort()
> function. Until recently GRUB had patched out abort() in gnulib, effectively
> making it a noop. This changed with a recent update of the version of gnulib
> GRUB uses to be a compiler defined trap. While this is fine for user space
> code where the kernel can be expected to cleanup after a process when this
> happens, the firmware may not be good at doing this.
>
> GRUB does have a grub_abort(), with the same function signature that can be
> used instead. So instead define gnulib's abort() implementation to be
> grub_abort(). This provides consistency of behavior regardless of whether
> the abort happens in gnulib code or in GRUB code.
>
> Because we want to avoid patching gnulib again and gnulib expects that
> abort(), now grub_abort(), is defined externally, we must provide a
> prototype in the config.h. This is complicated by the fact that config.h
> is included in many GRUB compilation units as well as grub/misc.h which
> already declares grub_abort(). A macro, GNULIB, is provided only to
> compilation units using gnulib code, so that the grub_abort() prototype
> is only included in those compilation units where it is lacking.

This approach is nice.

> Also, export grub_abort() so that it can now be used within modules.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Robbie Harwood <rharwood@redhat.com>

Be well,
--Robbie

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH] gnulib: Handle aborts in gnulib more gracefully

[Daniel Kiper]

Adding Robbie and Vladimir...

On Thu, Mar 24, 2022 at 07:12:22PM -0500, Glenn Washburn wrote:
> Gnulib does not define abort(), but expects it to be defined, usually by a
> libc implementation. However, GRUB has no libc and does not have an abort()
> function. Until recently GRUB had patched out abort() in gnulib, effectively
> making it a noop. This changed with a recent update of the version of gnulib
> GRUB uses to be a compiler defined trap. While this is fine for user space
> code where the kernel can be expected to cleanup after a process when this
> happens, the firmware may not be good at doing this.
>
> GRUB does have a grub_abort(), with the same function signature that can be
> used instead. So instead define gnulib's abort() implementation to be
> grub_abort(). This provides consistency of behavior regardless of whether
> the abort happens in gnulib code or in GRUB code.
>
> Because we want to avoid patching gnulib again and gnulib expects that
> abort(), now grub_abort(), is defined externally, we must provide a
> prototype in the config.h. This is complicated by the fact that config.h
> is included in many GRUB compilation units as well as grub/misc.h which
> already declares grub_abort(). A macro, GNULIB, is provided only to
> compilation units using gnulib code, so that the grub_abort() prototype
> is only included in those compilation units where it is lacking.
>
> Also, export grub_abort() so that it can now be used within modules.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  conf/Makefile.common  |  2 +-
>  config.h.in           | 17 ++++++++++-------
>  grub-core/kern/misc.c |  2 +-
>  include/grub/misc.h   |  1 +
>  4 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/conf/Makefile.common b/conf/Makefile.common
> index b343a038ee..67996b710c 100644
> --- a/conf/Makefile.common
> +++ b/conf/Makefile.common
> @@ -65,7 +65,7 @@ grubconfdir = $(sysconfdir)/grub.d
>  platformdir = $(pkglibdir)/$(target_cpu)-$(platform)
>  starfielddir = $(pkgdatadir)/themes/starfield
>
> -CFLAGS_GNULIB = -Wno-undef -Wno-sign-compare -Wno-unused -Wno-unused-parameter -Wno-redundant-decls -Wno-unreachable-code -Wno-conversion
> +CFLAGS_GNULIB = -Wno-undef -Wno-sign-compare -Wno-unused -Wno-unused-parameter -Wno-redundant-decls -Wno-unreachable-code -Wno-conversion -DGNULIB=1
>  CPPFLAGS_GNULIB = -I$(top_builddir)/grub-core/lib/gnulib -I$(top_srcdir)/grub-core/lib/gnulib
>
>  CFLAGS_POSIX = -fno-builtin
> diff --git a/config.h.in b/config.h.in
> index 01dcbbfc82..5719b28ebe 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -139,14 +139,17 @@ typedef __UINT_FAST32_TYPE__ uint_fast32_t;
>  #    define _GL_INLINE_HEADER_END   _Pragma ("GCC diagnostic pop")
>
>  /*
> - * We don't have an abort() for gnulib to call in regexp.  Because gnulib is
> - * built as a separate object that is then linked with, it doesn't have any
> - * of our headers or functions around to use - so we unfortunately can't
> - * print anything.  Builds of emu include the system's stdlib, which includes
> - * a prototype for abort(), so leave this as a macro that doesn't take
> - * arguments.
> + * We don't have an abort() for gnulib's functions to call (eg. in regexp), but
> + * one needs to be provided and grub_abort() is the equivalent. Gnulib does not
> + * provide a prototype for abort either, so one must be provided. However, this
> + * duplicates the prototype in grub/misc.h for GRUB compilation units, which
> + * causes a failure to compile. So only include the prototype if this is in a
> + * gnulib compilation unit.
>   */
> -#    define abort __builtin_trap
> +#    ifdef GNULIB
> +#      define abort grub_abort
> +void __attribute__ ((noreturn)) abort (void);
> +#    endif

Please take a look at the commit db7337a3d (grub-core/lib/posix_wrap/stdlib.h
(abort): Removed). It looks we have more natural mechanism for this kind of
functions. Though I am not sure why it was removed. Vladimir?

>  #  endif /* !_GL_INLINE_HEADER_BEGIN */
>
>  /* gnulib doesn't build cleanly with older compilers. */
> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> index 6c0221cc33..dfae4f9d78 100644
> --- a/grub-core/kern/misc.c
> +++ b/grub-core/kern/misc.c
> @@ -1249,7 +1249,7 @@ grub_printf_fmt_check (const char *fmt, const char *fmt_expected)
>
>
>  /* Abort GRUB. This function does not return.  */
> -static void __attribute__ ((noreturn))
> +void __attribute__ ((noreturn))
>  grub_abort (void)
>  {
>    grub_printf ("\nAborted.");
> diff --git a/include/grub/misc.h b/include/grub/misc.h
> index 7d2b551969..776dbf8af0 100644
> --- a/include/grub/misc.h
> +++ b/include/grub/misc.h
> @@ -353,6 +353,7 @@ int EXPORT_FUNC(grub_vsnprintf) (char *str, grub_size_t n, const char *fmt,
>  char *EXPORT_FUNC(grub_xasprintf) (const char *fmt, ...)
>       __attribute__ ((format (GNU_PRINTF, 1, 2))) WARN_UNUSED_RESULT;
>  char *EXPORT_FUNC(grub_xvasprintf) (const char *fmt, va_list args) WARN_UNUSED_RESULT;
> +void EXPORT_FUNC(grub_abort) (void) __attribute__ ((noreturn));

I am OK with these changes.

Daniel