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