[PATCH v2 0/3] Fix building with clang

[PATCH v2 0/3] Fix building with clang

[Darren Kenny]

The abiltiy to build with clang was broken in the last release after the
upgrade of gnulib, but it would also appear to have been broken too with
versions of clang prior to 8.0.0.

There were two main issues:

- The use of __builtin_trap in the abort() macro.

  This builtin doesn't exist for clang builds

  After some discussion between Daniel and Vladimir, it was requested that I
  should revert some past changes in this area, and re-introduce the use of
  grub_abort().

- The is some use of variable length arrays (vla) in gnulib's code, and when
  an attempt was made to resolve this in gnulib itself, I was informed that we
  shouldn't be building gnulib with -Werror.

  Rather than totally disabling -Werror, it seemed better to just limit it for
  the specific case that is causing problems, i.e. vla.

- Attempts to build clang with versions prior to 8.0.0 are also failing due to
  the use of the previously introduced safematch function usage. So we're also
  bumping the minimum version of clang in the INSTALL file and safemath.h
  where the test is done for the requisite version.

Thanks,

Darren.

v1 -> v2
--------
- Update with changes to INSTALL and safemath.h after testing various clang
  versions from 3.8 and up.

Darren Kenny (3):
  gnulib: Provide abort() implementation for gnulib
  configure: Fix building with clang
  build: Update to reflect minimum clang version 8.0

 INSTALL                           |  2 +-
 config.h.in                       | 10 ----------
 configure.ac                      |  4 ++++
 grub-core/kern/compiler-rt.c      |  9 ---------
 grub-core/kern/misc.c             |  2 +-
 grub-core/lib/posix_wrap/stdlib.h |  6 ++++++
 include/grub/misc.h               |  5 +----
 include/grub/safemath.h           |  6 +++---
 8 files changed, 16 insertions(+), 28 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] gnulib: Provide abort() implementation for gnulib

[Darren Kenny]

The recent gnulib updates require an implemention of abort(), but the
current macro provided by changeset:

 cd37d3d3916c gnulib: Drop no-abort.patch

to config.h.in does not work with the clang compiler since it doesn't
provide a __builtin_trap implementation, so this element of the
changeset needs to be reverted, and replaced.

After some discussion with Vladimir 'phcoder' Serbinenko and Daniel
Kiper it was suggested to bring back in the change from the changeset:

 db7337a3d353 "* grub-core/gnulib/regcomp.c (regerror): ..."

Which implements abort() as an inline call to grub_abort(), but since
that was made static by changeset:

 a8f15bceeafe "* grub-core/kern/misc.c (grub_abort): Make static"

it is also necessary to revert the specific part that makes it a static
function too.

Another implementation of abort() was found in
grub-core/kern/compiler-rt.c which needs to also be removed to be
consistent.

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
 config.h.in                       | 10 ----------
 grub-core/kern/compiler-rt.c      |  9 ---------
 grub-core/kern/misc.c             |  2 +-
 grub-core/lib/posix_wrap/stdlib.h |  6 ++++++
 include/grub/misc.h               |  5 +----
 5 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/config.h.in b/config.h.in
index 01dcbbfc82f0..4d1e50eba79c 100644
--- a/config.h.in
+++ b/config.h.in
@@ -137,16 +137,6 @@ typedef __UINT_FAST32_TYPE__ uint_fast32_t;
     void *								\
     reallocarray (void *ptr, unsigned int nmemb, unsigned int size);
 #    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.
- */
-#    define abort __builtin_trap
 #  endif /* !_GL_INLINE_HEADER_BEGIN */
 
 /* gnulib doesn't build cleanly with older compilers. */
diff --git a/grub-core/kern/compiler-rt.c b/grub-core/kern/compiler-rt.c
index 8948fdf77278..8051552e3a1a 100644
--- a/grub-core/kern/compiler-rt.c
+++ b/grub-core/kern/compiler-rt.c
@@ -195,15 +195,6 @@ __ctzsi2 (grub_uint32_t x)
 #endif
 
 
-#if defined (__clang__) && !defined(GRUB_EMBED_DECOMPRESSOR)
-/* clang emits references to abort().  */
-void __attribute__ ((noreturn))
-abort (void)
-{
-  grub_fatal ("compiler abort");
-}
-#endif
-
 #if (defined (__MINGW32__) || defined (__CYGWIN__))
 void __register_frame_info (void)
 {
diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
index 6c0221cc336d..dfae4f9d7897 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/grub-core/lib/posix_wrap/stdlib.h b/grub-core/lib/posix_wrap/stdlib.h
index 148e9d94bde0..f5279756abef 100644
--- a/grub-core/lib/posix_wrap/stdlib.h
+++ b/grub-core/lib/posix_wrap/stdlib.h
@@ -58,4 +58,10 @@ abs (int c)
   return (c >= 0) ? c : -c;
 }
 
+static inline void __attribute__ ((noreturn))
+abort (void)
+{
+  grub_abort ();
+}
+
 #endif
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 892ac6a42dda..ddac3aae8bcc 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -385,6 +385,7 @@ 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_exit) (void) __attribute__ ((noreturn));
+void EXPORT_FUNC(grub_abort) (void) __attribute__ ((noreturn));
 grub_uint64_t EXPORT_FUNC(grub_divmod64) (grub_uint64_t n,
 					  grub_uint64_t d,
 					  grub_uint64_t *r);
@@ -454,10 +455,6 @@ void EXPORT_FUNC(grub_reboot) (void) __attribute__ ((noreturn));
 void grub_reboot (void) __attribute__ ((noreturn));
 #endif
 
-#if defined (__clang__) && !defined (GRUB_UTIL)
-void __attribute__ ((noreturn)) EXPORT_FUNC (abort) (void);
-#endif
-
 #ifdef GRUB_MACHINE_PCBIOS
 /* Halt the system, using APM if possible. If NO_APM is true, don't
  * use APM even if it is available.  */
-- 
2.31.1

[PATCH v2 2/3] configure: Fix building with clang

[Darren Kenny]

Building the current code with clang and the latest gnulib fails due to
the use of a variable-length-array (vla) warning, which turns in to an
error due to the presence of the -Werror during the build.

The gnulib team stated that their code should not be built with -Werror.

At present, the only way to do this is for the complete code-base, by
using the --disable-werror option to configure.

Rather than doing this, and failing to gain any benefit that it
provides, instead, if building with clang, this patch makes it possible
to specifically not error on vlas, while retaining the -Werror
functionality otherwise.

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
 configure.ac | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure.ac b/configure.ac
index 1348b06a985a..93626b7982d4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1939,6 +1939,10 @@ AC_ARG_ENABLE([werror],
 if test x"$enable_werror" != xno ; then
   TARGET_CFLAGS="$TARGET_CFLAGS -Werror"
   HOST_CFLAGS="$HOST_CFLAGS -Werror"
+  if test "x$grub_cv_cc_target_clang" = xyes; then
+    TARGET_CFLAGS="$TARGET_CFLAGS -Wno-error=vla"
+    HOST_CFLAGS="$HOST_CFLAGS -Wno-error=vla"
+  fi
 fi
 
 TARGET_CPP="$TARGET_CC -E"
-- 
2.31.1

[PATCH v2 3/3] build: Update to reflect minimum clang version 8.0

[Darren Kenny]

After doing some validation with clang from versions 3.8 and up, the
builds prior to version 8.0.0 fail due to the use of safemath functions
at link time.

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>

fix

Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
---
 INSTALL                 | 2 +-
 include/grub/safemath.h | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/INSTALL b/INSTALL
index 7bca64f69881..620dcceb4814 100644
--- a/INSTALL
+++ b/INSTALL
@@ -16,7 +16,7 @@ you don't have any of them, please obtain and install them before
 configuring the GRUB.
 
 * GCC 5.1.0 or later
-  Experimental support for clang 3.8.0 or later (results in much bigger binaries)
+  Experimental support for clang 8.0.0 or later (results in much bigger binaries)
   for i386, x86_64, arm (including thumb), arm64, mips(el), powerpc, sparc64
 * GNU Make
 * GNU Bison 2.3 or later
diff --git a/include/grub/safemath.h b/include/grub/safemath.h
index c17b89bba17b..c2e338263107 100644
--- a/include/grub/safemath.h
+++ b/include/grub/safemath.h
@@ -23,15 +23,15 @@
 
 #include <grub/compiler.h>
 
-/* These appear in gcc 5.1 and clang 3.8. */
-#if GNUC_PREREQ(5, 1) || CLANG_PREREQ(3, 8)
+/* These appear in gcc 5.1 and clang 8.0 */
+#if GNUC_PREREQ(5, 1) || CLANG_PREREQ(8, 0)
 
 #define grub_add(a, b, res)	__builtin_add_overflow(a, b, res)
 #define grub_sub(a, b, res)	__builtin_sub_overflow(a, b, res)
 #define grub_mul(a, b, res)	__builtin_mul_overflow(a, b, res)
 
 #else
-#error gcc 5.1 or newer or clang 3.8 or newer is required
+#error gcc 5.1 or newer or clang 8.0 or newer is required
 #endif
 
 #endif /* GRUB_SAFEMATH_H */
-- 
2.31.1

Re: [PATCH v2 0/3] Fix building with clang

[Daniel Kiper]

On Fri, Oct 21, 2022 at 01:32:58PM +0000, Darren Kenny wrote:
> The abiltiy to build with clang was broken in the last release after the
> upgrade of gnulib, but it would also appear to have been broken too with
> versions of clang prior to 8.0.0.
>
> There were two main issues:
>
> - The use of __builtin_trap in the abort() macro.
>
>   This builtin doesn't exist for clang builds
>
>   After some discussion between Daniel and Vladimir, it was requested that I
>   should revert some past changes in this area, and re-introduce the use of
>   grub_abort().
>
> - The is some use of variable length arrays (vla) in gnulib's code, and when
>   an attempt was made to resolve this in gnulib itself, I was informed that we
>   shouldn't be building gnulib with -Werror.
>
>   Rather than totally disabling -Werror, it seemed better to just limit it for
>   the specific case that is causing problems, i.e. vla.
>
> - Attempts to build clang with versions prior to 8.0.0 are also failing due to
>   the use of the previously introduced safematch function usage. So we're also
>   bumping the minimum version of clang in the INSTALL file and safemath.h
>   where the test is done for the requisite version.
>
> Thanks,
>
> Darren.
>
> v1 -> v2
> --------
> - Update with changes to INSTALL and safemath.h after testing various clang
>   versions from 3.8 and up.
>
> Darren Kenny (3):
>   gnulib: Provide abort() implementation for gnulib
>   configure: Fix building with clang
>   build: Update to reflect minimum clang version 8.0

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com> for all patches...

Thank you for fixing all these issues!

Daniel