[PATCH 0/2] build: Fixes for memory-debugging builds

[PATCH 0/2] build: Fixes for memory-debugging builds

[Patrick Steinhardt]

Hi,

this series fixes two issues I've found after investigating the build
failures for LUKS2:

    - Including <config-util.h> in base64.h is the wrong thing to do as
      it's also used in the luks2 module, which shouldn't include the
      util header at all.

    - Ironically, explicitly disabling memory debugging enables memory
      debugging while it would've been disabled if just passing no
      option at all.

These patches fix both issues so that GRUB now builds correctly with and
without memory debugging on my system.

Patrick


Patrick Steinhardt (2):
  build: Fix option to explicitly disable memory debugging
  gnulib: Fix build of base64 when compiling with memory debugging

 configure.ac                                  |  8 +++++---
 grub-core/lib/gnulib-patches/fix-base64.patch | 14 ++++++--------
 2 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH 1/2] build: Fix option to explicitly disable memory debugging

[Patrick Steinhardt]

The memory management system supports a debug mode that can be enabled
at build time by passing "--enable-mm-debug" to the configure script.
Passing the option will cause us define MM_DEBUG as expected, but in
fact the reverse option "--disable-mm-deubg" will do the exact same
thing and also set up the define. This currently causes the build of
"lib/gnulib/base64.c" to fail as it tries to use `grub_debug_malloc()`
and `grub_debug_free()` even though both symbols aren't defined.

Seemingly, `AC_ARG_ENABLE()` will always execute the third argument if
either the positive or negative option was passed. Let's thus fix the
issue by moving the call to`AC_DEFINE()` into an explicit `if test
$xenable_mm_debug` block, similar to how other defines work.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 configure.ac | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index f19489418..9eeec2d76 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1448,9 +1448,11 @@ LIBS="$tmp_LIBS"
 # Memory manager debugging.
 AC_ARG_ENABLE([mm-debug],
 	      AS_HELP_STRING([--enable-mm-debug],
-                             [include memory manager debugging]),
-              [AC_DEFINE([MM_DEBUG], [1],
-                         [Define to 1 if you enable memory manager debugging.])])
+                             [include memory manager debugging]))
+if test x$enable_mm_debug = xyes; then
+    AC_DEFINE([MM_DEBUG], [1],
+            [Define to 1 if you enable memory manager debugging.])
+fi
 
 AC_ARG_ENABLE([cache-stats],
 	      AS_HELP_STRING([--enable-cache-stats],
-- 
2.25.1

Re: [PATCH 1/2] build: Fix option to explicitly disable memory debugging

[Daniel Kiper]

On Sat, Mar 07, 2020 at 05:29:08PM +0100, Patrick Steinhardt wrote:
> The memory management system supports a debug mode that can be enabled
> at build time by passing "--enable-mm-debug" to the configure script.
> Passing the option will cause us define MM_DEBUG as expected, but in
> fact the reverse option "--disable-mm-deubg" will do the exact same
> thing and also set up the define. This currently causes the build of
> "lib/gnulib/base64.c" to fail as it tries to use `grub_debug_malloc()`
> and `grub_debug_free()` even though both symbols aren't defined.
>
> Seemingly, `AC_ARG_ENABLE()` will always execute the third argument if
> either the positive or negative option was passed. Let's thus fix the
> issue by moving the call to`AC_DEFINE()` into an explicit `if test
> $xenable_mm_debug` block, similar to how other defines work.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

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

Daniel

Re: [PATCH 1/2] build: Fix option to explicitly disable memory debugging

[Paul Menzel]

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

Dear Patrick,


On 2020-03-07 17:29, Patrick Steinhardt wrote:
> The memory management system supports a debug mode that can be enabled
> at build time by passing "--enable-mm-debug" to the configure script.
> Passing the option will cause us define MM_DEBUG as expected, but in
> fact the reverse option "--disable-mm-deubg" will do the exact same

s/deubg/debug/

> thing and also set up the define. This currently causes the build of
> "lib/gnulib/base64.c" to fail as it tries to use `grub_debug_malloc()`
> and `grub_debug_free()` even though both symbols aren't defined.
> 
> Seemingly, `AC_ARG_ENABLE()` will always execute the third argument if
> either the positive or negative option was passed. Let's thus fix the
> issue by moving the call to`AC_DEFINE()` into an explicit `if test
> $xenable_mm_debug` block, similar to how other defines work.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  configure.ac | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index f19489418..9eeec2d76 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1448,9 +1448,11 @@ LIBS="$tmp_LIBS"
>  # Memory manager debugging.
>  AC_ARG_ENABLE([mm-debug],
>  	      AS_HELP_STRING([--enable-mm-debug],
> -                             [include memory manager debugging]),
> -              [AC_DEFINE([MM_DEBUG], [1],
> -                         [Define to 1 if you enable memory manager debugging.])])
> +                             [include memory manager debugging]))
> +if test x$enable_mm_debug = xyes; then
> +    AC_DEFINE([MM_DEBUG], [1],
> +            [Define to 1 if you enable memory manager debugging.])
> +fi
>  
>  AC_ARG_ENABLE([cache-stats],
>  	      AS_HELP_STRING([--enable-cache-stats],

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

Re: [PATCH 1/2] build: Fix option to explicitly disable memory debugging

[Patrick Steinhardt]

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

On Mon, Mar 09, 2020 at 12:18:41PM +0100, Paul Menzel wrote:
> Dear Patrick,
> 
> 
> On 2020-03-07 17:29, Patrick Steinhardt wrote:
> > The memory management system supports a debug mode that can be enabled
> > at build time by passing "--enable-mm-debug" to the configure script.
> > Passing the option will cause us define MM_DEBUG as expected, but in
> > fact the reverse option "--disable-mm-deubg" will do the exact same
> 
> s/deubg/debug/

Thanks for pointing this out! Daniel, you want me to send out a v2 of
this to fix the typo or will you fix it up locally?

Patrick

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

Re: [PATCH 1/2] build: Fix option to explicitly disable memory debugging

[Daniel Kiper]

On Mon, Mar 09, 2020 at 01:22:34PM +0100, Patrick Steinhardt wrote:
> On Mon, Mar 09, 2020 at 12:18:41PM +0100, Paul Menzel wrote:
> > Dear Patrick,
> >
> > On 2020-03-07 17:29, Patrick Steinhardt wrote:
> > > The memory management system supports a debug mode that can be enabled
> > > at build time by passing "--enable-mm-debug" to the configure script.
> > > Passing the option will cause us define MM_DEBUG as expected, but in
> > > fact the reverse option "--disable-mm-deubg" will do the exact same
> >
> > s/deubg/debug/
>
> Thanks for pointing this out! Daniel, you want me to send out a v2 of
> this to fix the typo or will you fix it up locally?

I will fix it locally...

Daniel

[PATCH 2/2] gnulib: Fix build of base64 when compiling with memory debugging

[Patrick Steinhardt]

When building GRUB with memory management debugging enabled, then the
build fails because of `grub_debug_malloc()` and `grub_debug_free()`
being undefined in the luks2 module. The cause is that we patch
"base64.h" to unconditionaly include "config-util.h", which shouldn't be
included for modules at all. As a result, `MM_DEBUG` is defined when
building the module, causing it to use the debug memory allocation
functions. As these are not built into modules, we end up with a linker
error.

Fix the issue by removing the <config-util.h> include altogether. The
sole reason it was included was for the `_GL_ATTRIBUTE_CONST` macro,
which we can simply define as empty in case it's not set.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/lib/gnulib-patches/fix-base64.patch | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/grub-core/lib/gnulib-patches/fix-base64.patch b/grub-core/lib/gnulib-patches/fix-base64.patch
index e075b6fab..985db1279 100644
--- a/grub-core/lib/gnulib-patches/fix-base64.patch
+++ b/grub-core/lib/gnulib-patches/fix-base64.patch
@@ -1,14 +1,8 @@
 diff --git a/lib/base64.h b/lib/base64.h
-index 9cd0183b8..a2aaa2d4a 100644
+index 9cd0183b8..185a2afa1 100644
 --- a/lib/base64.h
 +++ b/lib/base64.h
-@@ -18,11 +18,16 @@
- #ifndef BASE64_H
- # define BASE64_H
- 
-+/* Get _GL_ATTRIBUTE_CONST */
-+# include <config-util.h>
-+
+@@ -21,8 +21,14 @@
  /* Get size_t. */
  # include <stddef.h>
  
@@ -17,6 +11,10 @@ index 9cd0183b8..a2aaa2d4a 100644
 +#ifndef GRUB_POSIX_BOOL_DEFINED
 +typedef enum { false = 0, true = 1 } bool;
 +#define GRUB_POSIX_BOOL_DEFINED 1
++#endif
++
++#ifndef _GL_ATTRIBUTE_CONST
++# define _GL_ATTRIBUTE_CONST /* empty */
 +#endif
  
  # ifdef __cplusplus
-- 
2.25.1

Re: [PATCH 2/2] gnulib: Fix build of base64 when compiling with memory debugging

[Daniel Kiper]

On Sat, Mar 07, 2020 at 05:29:09PM +0100, Patrick Steinhardt wrote:
> When building GRUB with memory management debugging enabled, then the
> build fails because of `grub_debug_malloc()` and `grub_debug_free()`
> being undefined in the luks2 module. The cause is that we patch
> "base64.h" to unconditionaly include "config-util.h", which shouldn't be
> included for modules at all. As a result, `MM_DEBUG` is defined when
> building the module, causing it to use the debug memory allocation
> functions. As these are not built into modules, we end up with a linker
> error.
>
> Fix the issue by removing the <config-util.h> include altogether. The
> sole reason it was included was for the `_GL_ATTRIBUTE_CONST` macro,
> which we can simply define as empty in case it's not set.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/lib/gnulib-patches/fix-base64.patch | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/grub-core/lib/gnulib-patches/fix-base64.patch b/grub-core/lib/gnulib-patches/fix-base64.patch
> index e075b6fab..985db1279 100644
> --- a/grub-core/lib/gnulib-patches/fix-base64.patch
> +++ b/grub-core/lib/gnulib-patches/fix-base64.patch
> @@ -1,14 +1,8 @@
>  diff --git a/lib/base64.h b/lib/base64.h
> -index 9cd0183b8..a2aaa2d4a 100644
> +index 9cd0183b8..185a2afa1 100644
>  --- a/lib/base64.h
>  +++ b/lib/base64.h
> -@@ -18,11 +18,16 @@
> - #ifndef BASE64_H
> - # define BASE64_H

Hmmm... It seems to me that you should not drop this...

> -
> -+/* Get _GL_ATTRIBUTE_CONST */
> -+# include <config-util.h>
> -+
> +@@ -21,8 +21,14 @@
>   /* Get size_t. */
>   # include <stddef.h>
>
> @@ -17,6 +11,10 @@ index 9cd0183b8..a2aaa2d4a 100644
>  +#ifndef GRUB_POSIX_BOOL_DEFINED
>  +typedef enum { false = 0, true = 1 } bool;
>  +#define GRUB_POSIX_BOOL_DEFINED 1
> ++#endif
> ++
> ++#ifndef _GL_ATTRIBUTE_CONST
> ++# define _GL_ATTRIBUTE_CONST /* empty */
>  +#endif
>
>   # ifdef __cplusplus
> --
> 2.25.1

Daniel

Re: [PATCH 2/2] gnulib: Fix build of base64 when compiling with memory debugging

[Patrick Steinhardt]

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

On Mon, Mar 09, 2020 at 12:19:15PM +0100, Daniel Kiper wrote:
> On Sat, Mar 07, 2020 at 05:29:09PM +0100, Patrick Steinhardt wrote:
> > When building GRUB with memory management debugging enabled, then the
> > build fails because of `grub_debug_malloc()` and `grub_debug_free()`
> > being undefined in the luks2 module. The cause is that we patch
> > "base64.h" to unconditionaly include "config-util.h", which shouldn't be
> > included for modules at all. As a result, `MM_DEBUG` is defined when
> > building the module, causing it to use the debug memory allocation
> > functions. As these are not built into modules, we end up with a linker
> > error.
> >
> > Fix the issue by removing the <config-util.h> include altogether. The
> > sole reason it was included was for the `_GL_ATTRIBUTE_CONST` macro,
> > which we can simply define as empty in case it's not set.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  grub-core/lib/gnulib-patches/fix-base64.patch | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/grub-core/lib/gnulib-patches/fix-base64.patch b/grub-core/lib/gnulib-patches/fix-base64.patch
> > index e075b6fab..985db1279 100644
> > --- a/grub-core/lib/gnulib-patches/fix-base64.patch
> > +++ b/grub-core/lib/gnulib-patches/fix-base64.patch
> > @@ -1,14 +1,8 @@
> >  diff --git a/lib/base64.h b/lib/base64.h
> > -index 9cd0183b8..a2aaa2d4a 100644
> > +index 9cd0183b8..185a2afa1 100644
> >  --- a/lib/base64.h
> >  +++ b/lib/base64.h
> > -@@ -18,11 +18,16 @@
> > - #ifndef BASE64_H
> > - # define BASE64_H
> 
> Hmmm... It seems to me that you should not drop this...

Note that this is a diff of a patch. So all that's getting dropped is
the patch that added the #include. So after applying this, "base64.h"
doesn't get touched by fix-base64.patch at all anymore.

Patrick

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

Re: [PATCH 2/2] gnulib: Fix build of base64 when compiling with memory debugging

[Daniel Kiper]

On Mon, Mar 09, 2020 at 01:01:51PM +0100, Patrick Steinhardt wrote:
> On Mon, Mar 09, 2020 at 12:19:15PM +0100, Daniel Kiper wrote:
> > On Sat, Mar 07, 2020 at 05:29:09PM +0100, Patrick Steinhardt wrote:
> > > When building GRUB with memory management debugging enabled, then the
> > > build fails because of `grub_debug_malloc()` and `grub_debug_free()`
> > > being undefined in the luks2 module. The cause is that we patch
> > > "base64.h" to unconditionaly include "config-util.h", which shouldn't be
> > > included for modules at all. As a result, `MM_DEBUG` is defined when
> > > building the module, causing it to use the debug memory allocation
> > > functions. As these are not built into modules, we end up with a linker
> > > error.
> > >
> > > Fix the issue by removing the <config-util.h> include altogether. The
> > > sole reason it was included was for the `_GL_ATTRIBUTE_CONST` macro,
> > > which we can simply define as empty in case it's not set.
> > >
> > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > ---
> > >  grub-core/lib/gnulib-patches/fix-base64.patch | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/grub-core/lib/gnulib-patches/fix-base64.patch b/grub-core/lib/gnulib-patches/fix-base64.patch
> > > index e075b6fab..985db1279 100644
> > > --- a/grub-core/lib/gnulib-patches/fix-base64.patch
> > > +++ b/grub-core/lib/gnulib-patches/fix-base64.patch
> > > @@ -1,14 +1,8 @@
> > >  diff --git a/lib/base64.h b/lib/base64.h
> > > -index 9cd0183b8..a2aaa2d4a 100644
> > > +index 9cd0183b8..185a2afa1 100644
> > >  --- a/lib/base64.h
> > >  +++ b/lib/base64.h
> > > -@@ -18,11 +18,16 @@
> > > - #ifndef BASE64_H
> > > - # define BASE64_H
> >
> > Hmmm... It seems to me that you should not drop this...
>
> Note that this is a diff of a patch. So all that's getting dropped is
> the patch that added the #include. So after applying this, "base64.h"
> doesn't get touched by fix-base64.patch at all anymore.

Ahhh... Right, these are reference lines. So, sorry for the confusion...

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

Daniel