[PATCH v3 0/5] Various build and doc fixes

[PATCH v3 0/5] Various build and doc fixes

[Daniel Kiper]

Hey,

As in subject... Please review...

Daniel

 INSTALL      | 47 +++++++++++++++++++++++++++++++----------------
 autogen.sh   |  2 +-
 configure.ac | 19 ++++++++++++++-----
 3 files changed, 46 insertions(+), 22 deletions(-)

Daniel Kiper (5):
      configure: Drop unneeded TARGET_CFLAGS expansion
      configure: Set gnu99 C language standard by default
      INSTALL/configure: Update install doc and configure comment
      INSTALL: Update configure example
      autogen: Replace -iname with -ipath in find command

[PATCH v3 1/5] configure: Drop unneeded TARGET_CFLAGS expansion

[Daniel Kiper]

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 configure.ac | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 88c0adbae..b2576b013 100644
--- a/configure.ac
+++ b/configure.ac
@@ -77,7 +77,7 @@ grub_TRANSFORM([grub-file])
 
 # Optimization flag.  Allow user to override.
 if test "x$TARGET_CFLAGS" = x; then
-  TARGET_CFLAGS="$TARGET_CFLAGS -Os"
+  TARGET_CFLAGS=-Os
 fi
 
 # Default HOST_CPPFLAGS
-- 
2.11.0

[PATCH v3 2/5] configure: Set gnu99 C language standard by default

[Daniel Kiper]

Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
const qualifiers) introduced "restrict" keyword into some functions
definitions. This keyword was introduced in C99 standard. However, some
compilers by default may use C89 or something different. This behavior
leads to the breakage during builds when c89 or gnu89 is in force. So,
let's set gnu99 C language standard for all compilers by default. This
way a bit random build issue will be fixed and the GRUB source will be
build consistently regardless of type and version of the compiler.

It was decided to use gnu99 C language standard because it fixes the
issue mentioned above and also provides some useful extensions which are
used here and there in the GRUB source. Potentially we can use gnu11
too. However, this may reduce pool of older compilers which can be used
to build the GRUB. So, let's live with gnu99 until we discover that we
strongly require a feature from newer C standard.

The user is still able to override C language standard using relevant
*_CFLAGS variables.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v3 - suggestions/fixes:
   - add a comment before the change in configure.ac
     (suggested by Javier Martinez Canillas and Leif Lindholm),
   - improve commit message
     (suggested by Javier Martinez Canillas and Leif Lindholm).

v2 - suggestions/fixes:
   - unconditionally enforce gnu99 C language standard
     (suggested by Leif Lindholm).
---
 configure.ac | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/configure.ac b/configure.ac
index b2576b013..3eda9a5a2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -80,6 +80,12 @@ if test "x$TARGET_CFLAGS" = x; then
   TARGET_CFLAGS=-Os
 fi
 
+# Enable support for "restrict" keyword and other
+# features from gnu99 C language standard.
+BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
+HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
+TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"
+
 # Default HOST_CPPFLAGS
 HOST_CPPFLAGS="$HOST_CPPFLAGS -Wall -W"
 HOST_CPPFLAGS="$HOST_CPPFLAGS -DGRUB_UTIL=1"
-- 
2.11.0

[PATCH v3 3/5] INSTALL/configure: Update install doc and configure comment

[Daniel Kiper]

..to reflect the GRUB build reality in them.

Additionally, fix text formatting a bit.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
v3 - suggestions/fixes:
   - improve wording of changed text
     (suggested by Leif Lindholm),
   - drop configure example change from this patch.
---
 INSTALL      | 27 +++++++++++++++++----------
 configure.ac | 11 +++++++----
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/INSTALL b/INSTALL
index 8acb40902..5cea313af 100644
--- a/INSTALL
+++ b/INSTALL
@@ -182,20 +182,23 @@ corresponding platform are not needed for the platform in question.
 
   - For host
     1. --host= to autoconf name of host.
-    2. CC= for gcc able to compile for host
-    3. HOST_CFLAGS= for C options for host.
-    4. HOST_CPPFLAGS= for C preprocessor options for host.
-    5. HOST_LDFLAGS= for linker options for host.
-    6. PKG_CONFIG= for pkg-config for host (optional).
-    7. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
-    8. Libfuse if any must be in standard linker folders (-lfuse) (optional).
-    9. Libzfs if any must be in standard linker folders (-lzfs) (optional).
-    10. Liblzma if any must be in standard linker folders (-llzma) (optional).
+    2. CC= for gcc able to compile for host.
+    3. CFLAGS= for C options for host.
+    4. HOST_CC= for gcc able to compile for host.
+    5. HOST_CFLAGS= for C options for host.
+    6. HOST_CPPFLAGS= for C preprocessor options for host.
+    7. HOST_LDFLAGS= for linker options for host.
+    8. PKG_CONFIG= for pkg-config for host (optional).
+    9. Libdevmapper if any must be in standard linker folders (-ldevmapper) (optional).
+    10. Libfuse if any must be in standard linker folders (-lfuse) (optional).
+    11. Libzfs if any must be in standard linker folders (-lzfs) (optional).
+    12. Liblzma if any must be in standard linker folders (-llzma) (optional).
+    Note: The HOST_* variables override not prefixed variables.
 
   - For target
     1. --target= to autoconf cpu name of target.
     2. --with-platform to choose firmware.
-    3. TARGET_CC= for gcc able to compile for target
+    3. TARGET_CC= for gcc able to compile for target.
     4. TARGET_CFLAGS= for C options for target.
     5. TARGET_CPPFLAGS= for C preprocessor options for target.
     6. TARGET_CCASFLAGS= for assembler options for target.
@@ -204,6 +207,10 @@ corresponding platform are not needed for the platform in question.
     9. TARGET_STRIP= for strip for target.
     10. TARGET_NM= for nm for target.
     11. TARGET_RANLIB= for ranlib for target.
+    Note: If the TARGET_* variables are not specified then they will default
+          to be the same as the host variables. If host variables are not
+          specified then the TARGET_* variables will default to be the same
+          as not prefixed variables.
 
   - Additionally for emu, for host and target.
     1. SDL is looked for in standard linker directories (-lSDL) (optional)
diff --git a/configure.ac b/configure.ac
index 3eda9a5a2..7c10a4db7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -26,10 +26,13 @@ dnl This is necessary because the target type in autoconf does not
 dnl describe such a system very well.
 dnl
 dnl The current strategy is to use variables with no prefix (such as
-dnl CC, CFLAGS, etc.) for the host type, variables with prefix "BUILD_"
-dnl (such as BUILD_CC, BUILD_CFLAGS, etc.) for the build type and variables
-dnl with the prefix "TARGET_" (such as TARGET_CC, TARGET_CFLAGS, etc.) are
-dnl used for the target type. See INSTALL for full list of variables.
+dnl CC, CFLAGS, etc.) for the host and target type, variables with
+dnl prefix "BUILD_" (such as BUILD_CC, BUILD_CFLAGS, etc.) for the
+dnl build type, variables with prefix "HOST_" (such as HOST_CC,
+dnl HOST_CFLAGS, etc.) for the host type and variables with the prefix
+dnl "TARGET_" (such as TARGET_CC, TARGET_CFLAGS, etc.) are used for
+dnl the target type. See INSTALL for full list of variables and
+dnl description of the relationships between them.
 
 AC_INIT([GRUB],[2.05],[bug-grub@gnu.org])
 
-- 
2.11.0

[PATCH v3 4/5] INSTALL: Update configure example

[Daniel Kiper]

..to make it more relevant.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 INSTALL | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/INSTALL b/INSTALL
index 5cea313af..dedf236a8 100644
--- a/INSTALL
+++ b/INSTALL
@@ -160,12 +160,20 @@ For this example the configure line might look like (more details below)
 (some options are optional and included here for completeness but some rarely
 used options are omitted):
 
-./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu
-CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config
---target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc
-TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6"
-TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip"
-TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
+  ./configure --host=x86_64-linux-gnu --target=arm-linux-gnueabihf \
+    --with-platform=efi BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config \
+    HOST_CC=x86_64-linux-gnu-gcc HOST_CFLAGS='-g -O2' \
+    PKG_CONFIG=x86_64-linux-gnu-pkg-config TARGET_CC=arm-linux-gnueabihf-gcc \
+    TARGET_CFLAGS='-Os -march=armv8.3-a' TARGET_CCASFLAGS='-march=armv8.3-a' \
+    TARGET_OBJCOPY=arm-linux-gnueabihf-objcopy \
+    TARGET_STRIP=arm-linux-gnueabihf-strip TARGET_NM=arm-linux-gnueabihf-nm \
+    TARGET_RANLIB=arm-linux-gnueabihf-ranlib LEX=flex
+
+Normally, for building a GRUB on amd64 with tools to run on amd64 to
+generate images to run on ARM, using your Linux distribution's
+packaged cross compiler, the following would suffice:
+
+  ./configure --target=arm-linux-gnueabihf --with-platform=efi
 
 You need to use following options to specify tools and platforms. For minimum
 version look at prerequisites. All tools not mentioned in this section under
-- 
2.11.0

[PATCH v3 5/5] autogen: Replace -iname with -ipath in find command

[Daniel Kiper]

..because -iname cannot be used to match paths.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 autogen.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/autogen.sh b/autogen.sh
index ef43270fc..31b0ced7e 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -13,7 +13,7 @@ fi
 export LC_COLLATE=C
 unset LC_ALL
 
-find . -iname '*.[ch]' ! -ipath './grub-core/lib/libgcrypt-grub/*' ! -ipath './build-aux/*' ! -ipath './grub-core/lib/libgcrypt/src/misc.c' ! -ipath './grub-core/lib/libgcrypt/src/global.c' ! -ipath './grub-core/lib/libgcrypt/src/secmem.c'  ! -ipath './util/grub-gen-widthspec.c' ! -ipath './util/grub-gen-asciih.c' ! -ipath './gnulib/*' ! -iname './grub-core/lib/gnulib/*' |sort > po/POTFILES.in
+find . -iname '*.[ch]' ! -ipath './grub-core/lib/libgcrypt-grub/*' ! -ipath './build-aux/*' ! -ipath './grub-core/lib/libgcrypt/src/misc.c' ! -ipath './grub-core/lib/libgcrypt/src/global.c' ! -ipath './grub-core/lib/libgcrypt/src/secmem.c'  ! -ipath './util/grub-gen-widthspec.c' ! -ipath './util/grub-gen-asciih.c' ! -ipath './gnulib/*' ! -ipath './grub-core/lib/gnulib/*' |sort > po/POTFILES.in
 find util -iname '*.in' ! -name Makefile.in  |sort > po/POTFILES-shell.in
 
 echo "Importing unicode..."
-- 
2.11.0

Re: [PATCH v3 0/5] Various build and doc fixes

[Leif Lindholm]

On Wed, May 13, 2020 at 14:02:46 +0200, Daniel Kiper wrote:
> Hey,
> 
> As in subject... Please review...

Hi Daniel,

This looks good to me.
For the series:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>


> Daniel
> 
>  INSTALL      | 47 +++++++++++++++++++++++++++++++----------------
>  autogen.sh   |  2 +-
>  configure.ac | 19 ++++++++++++++-----
>  3 files changed, 46 insertions(+), 22 deletions(-)
> 
> Daniel Kiper (5):
>       configure: Drop unneeded TARGET_CFLAGS expansion
>       configure: Set gnu99 C language standard by default
>       INSTALL/configure: Update install doc and configure comment
>       INSTALL: Update configure example
>       autogen: Replace -iname with -ipath in find command
> 

Re: [PATCH v3 5/5] autogen: Replace -iname with -ipath in find command

[Daniel Axtens]

Daniel Kiper <daniel.kiper@oracle.com> writes:

> ..because -iname cannot be used to match paths.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  autogen.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/autogen.sh b/autogen.sh
> index ef43270fc..31b0ced7e 100755
> --- a/autogen.sh
> +++ b/autogen.sh
> @@ -13,7 +13,7 @@ fi
>  export LC_COLLATE=C
>  unset LC_ALL
>  
> -find . -iname '*.[ch]' ! -ipath './grub-core/lib/libgcrypt-grub/*' ! -ipath './build-aux/*' ! -ipath './grub-core/lib/libgcrypt/src/misc.c' ! -ipath './grub-core/lib/libgcrypt/src/global.c' ! -ipath './grub-core/lib/libgcrypt/src/secmem.c'  ! -ipath './util/grub-gen-widthspec.c' ! -ipath './util/grub-gen-asciih.c' ! -ipath './gnulib/*' ! -iname './grub-core/lib/gnulib/*' |sort > po/POTFILES.in
> +find . -iname '*.[ch]' ! -ipath './grub-core/lib/libgcrypt-grub/*' ! -ipath './build-aux/*' ! -ipath './grub-core/lib/libgcrypt/src/misc.c' ! -ipath './grub-core/lib/libgcrypt/src/global.c' ! -ipath './grub-core/lib/libgcrypt/src/secmem.c'  ! -ipath './util/grub-gen-widthspec.c' ! -ipath './util/grub-gen-asciih.c' ! -ipath './gnulib/*' ! -ipath './grub-core/lib/gnulib/*' |sort > po/POTFILES.in
>  find util -iname '*.in' ! -name Makefile.in  |sort > po/POTFILES-shell.in

Fantastic, that had been bugging me every time I saw it.

Reviewed-by: Daniel Axtens <dja@axtens.net>

Regards,
Daniel

>  
>  echo "Importing unicode..."
> -- 
> 2.11.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v3 2/5] configure: Set gnu99 C language standard by default

[Daniel Axtens]

Hi Daniel,

> Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
> const qualifiers) introduced "restrict" keyword into some functions
> definitions. This keyword was introduced in C99 standard. However, some
> compilers by default may use C89 or something different. This behavior
> leads to the breakage during builds when c89 or gnu89 is in force. So,
> let's set gnu99 C language standard for all compilers by default. This
> way a bit random build issue will be fixed and the GRUB source will be
> build consistently regardless of type and version of the compiler.
>
> It was decided to use gnu99 C language standard because it fixes the
> issue mentioned above and also provides some useful extensions which are
> used here and there in the GRUB source. Potentially we can use gnu11
> too. However, this may reduce pool of older compilers which can be used
> to build the GRUB. So, let's live with gnu99 until we discover that we
> strongly require a feature from newer C standard.

I tried building this with CC=clang just to see if it didn't like gnu99
for some reason.

It does not, but for different reasons to what I expected:

clang -DHAVE_CONFIG_H -I.  -Wall -W -DGRUB_UTIL=1 -D_FILE_OFFSET_BITS=64 -I./include -DGRUB_FILE=\"grub-core/lib/json/json.c\" -I. -I. -I. -I. -I./include -I./include -I./grub-core/lib/libgcrypt-grub/src/  -I./grub-core/lib/gnulib -I./grub-core/lib/gnulib -I./grub-core/lib/json -D_FILE_OFFSET_BITS=64 -std=gnu99  -Wall -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces -Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type -Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs -Wunknown-pragmas -Wunused -Wunused-function -Wunused-label -Wunused-parameter -Wunused-value  -Wunused-variable -Wwrite-strings -Wnested-externs -Wstrict-prototypes -Wcast-align  -Wextra -Wattributes -Wendif-labels -Winit-self -Wint-to-pointer-cast -Winvalid-pch -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla -Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros -Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs -Wmissing-prototypes -Wmissing-declarations -Wformat=2 -Werror  -Wno-undef -Wno-sign-compare -Wno-unused -Wno-unused-parameter -Wno-redundant-decls -Wno-unreachable-code -Wno-conversion  -MT grub-core/lib/json/libgrubkern_a-json.o -MD -MP -MF grub-core/lib/json/.deps-util/libgrubkern_a-json.Tpo -c -o grub-core/lib/json/libgrubkern_a-json.o `test -f 'grub-core/lib/json/json.c' || echo './'`grub-core/lib/json/json.c
In file included from grub-core/lib/json/json.c:24:
./grub-core/lib/json/json.h:39:24: error: redefinition of typedef 'jsmntok_t' is a C11 feature [-Werror,-Wtypedef-redefinition]
typedef struct jsmntok jsmntok_t;
                       ^
./grub-core/lib/json/jsmn.h:77:3: note: previous definition is here
} jsmntok_t;
  ^
1 error generated.


clang --version
clang version 9.0.0-2 (tags/RELEASE_900/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

Without this patch, clang-9 fails on the use of nested functions in
commit cb2f15c54489 ("normal/main: Search for specific config files for
netboot") - I'll send a fix for that shortly. If I revert that as a
temporary measure, clang will build.

I notice from ./configure that it already does some form of testing for
restrict:

checking for C/C++ restrict keyword... __restrict

I'm not sure what brings that test in to configure (gnulib?), and
accessing it is a little bit tricky because we use AC_CONFIG_FILES for
config.h rather than AC_CONFIG_HEADER, but that could be a general
purpose solution.

Regards,
Daniel

> The user is still able to override C language standard using relevant
> *_CFLAGS variables.
>
> Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
> v3 - suggestions/fixes:
>    - add a comment before the change in configure.ac
>      (suggested by Javier Martinez Canillas and Leif Lindholm),
>    - improve commit message
>      (suggested by Javier Martinez Canillas and Leif Lindholm).
>
> v2 - suggestions/fixes:
>    - unconditionally enforce gnu99 C language standard
>      (suggested by Leif Lindholm).
> ---
>  configure.ac | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/configure.ac b/configure.ac
> index b2576b013..3eda9a5a2 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -80,6 +80,12 @@ if test "x$TARGET_CFLAGS" = x; then
>    TARGET_CFLAGS=-Os
>  fi
>  
> +# Enable support for "restrict" keyword and other
> +# features from gnu99 C language standard.
> +BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
> +HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
> +TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"
> +
>  # Default HOST_CPPFLAGS
>  HOST_CPPFLAGS="$HOST_CPPFLAGS -Wall -W"
>  HOST_CPPFLAGS="$HOST_CPPFLAGS -DGRUB_UTIL=1"
> -- 
> 2.11.0
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v3 0/5] Various build and doc fixes

[Daniel Kiper]

On Wed, May 13, 2020 at 01:49:59PM +0100, Leif Lindholm wrote:
> On Wed, May 13, 2020 at 14:02:46 +0200, Daniel Kiper wrote:
> > Hey,
> >
> > As in subject... Please review...
>
> Hi Daniel,
>
> This looks good to me.

Great!

> For the series:
> Reviewed-by: Leif Lindholm <leif@nuviainc.com>

Thanks for review. It looks that patch #2 breaks builds with clang.
So, I will do not push it this time. I will wait for a fix.

Daniel

Re: [PATCH v3 2/5] configure: Set gnu99 C language standard by default

[Daniel Kiper]

Hi Daniel,

Adding Patrick...

On Fri, May 15, 2020 at 11:56:56AM +1000, Daniel Axtens wrote:
> Hi Daniel,
>
> > Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
> > const qualifiers) introduced "restrict" keyword into some functions
> > definitions. This keyword was introduced in C99 standard. However, some
> > compilers by default may use C89 or something different. This behavior
> > leads to the breakage during builds when c89 or gnu89 is in force. So,
> > let's set gnu99 C language standard for all compilers by default. This
> > way a bit random build issue will be fixed and the GRUB source will be
> > build consistently regardless of type and version of the compiler.
> >
> > It was decided to use gnu99 C language standard because it fixes the
> > issue mentioned above and also provides some useful extensions which are
> > used here and there in the GRUB source. Potentially we can use gnu11
> > too. However, this may reduce pool of older compilers which can be used
> > to build the GRUB. So, let's live with gnu99 until we discover that we
> > strongly require a feature from newer C standard.
>
> I tried building this with CC=clang just to see if it didn't like gnu99
> for some reason.

Thank you for testing this...

> It does not,

Great!

> but for different reasons to what I expected:
>
> clang -DHAVE_CONFIG_H -I.  -Wall -W -DGRUB_UTIL=1 -D_FILE_OFFSET_BITS=64 -I./include -DGRUB_FILE=\"grub-core/lib/json/json.c\" -I. -I. -I. -I. -I./include -I./include -I./grub-core/lib/libgcrypt-grub/src/  -I./grub-core/lib/gnulib -I./grub-core/lib/gnulib -I./grub-core/lib/json -D_FILE_OFFSET_BITS=64 -std=gnu99  -Wall -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces -Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type -Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs -Wunknown-pragmas -Wunused -Wunused-function -Wunused-label -Wunused-parameter -Wunused-value  -Wunused-variable -Wwrite-strings -Wnested-externs -Wstrict-prototypes -Wcast-align  -Wextra -Wattributes -Wendif-labels -Winit-self -Wint-to-pointer-cast -Winvalid-pch -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla -Wpointer-to-int-cast -Wstrict-aliasing -Wvariadic-macros -Wvolatile-register-var -Wpointer-sign -Wmissing-include-dirs -Wmissing-prototypes -Wmissing-declarations -Wformat=2 -Werror  -Wno-undef -Wno-sign-compare -Wno-unused -Wno-unused-parameter -Wno-redundant-decls -Wno-unreachable-code -Wno-conversion  -MT grub-core/lib/json/libgrubkern_a-json.o -MD -MP -MF grub-core/lib/json/.deps-util/libgrubkern_a-json.Tpo -c -o grub-core/lib/json/libgrubkern_a-json.o `test -f 'grub-core/lib/json/json.c' || echo './'`grub-core/lib/json/json.c
> In file included from grub-core/lib/json/json.c:24:
> ./grub-core/lib/json/json.h:39:24: error: redefinition of typedef 'jsmntok_t' is a C11 feature [-Werror,-Wtypedef-redefinition]
> typedef struct jsmntok jsmntok_t;
>                        ^
> ./grub-core/lib/json/jsmn.h:77:3: note: previous definition is here
> } jsmntok_t;
>   ^
> 1 error generated.

Patrick, could you take a look at this?

> clang --version
> clang version 9.0.0-2 (tags/RELEASE_900/final)
> Target: x86_64-pc-linux-gnu
> Thread model: posix
> InstalledDir: /usr/bin
>
> Without this patch, clang-9 fails on the use of nested functions in
> commit cb2f15c54489 ("normal/main: Search for specific config files for
> netboot") - I'll send a fix for that shortly. If I revert that as a
> temporary measure, clang will build.

Javier, could you take a look at this too?

> I notice from ./configure that it already does some form of testing for
> restrict:
>
> checking for C/C++ restrict keyword... __restrict
>
> I'm not sure what brings that test in to configure (gnulib?), and
> accessing it is a little bit tricky because we use AC_CONFIG_FILES for
> config.h rather than AC_CONFIG_HEADER, but that could be a general
> purpose solution.

Yeah, I saw that too. However, I think we have to enforce C standard in
one way or another. So, I would not bother much if we chose gnu99.

Daniel

Re: [PATCH v3 2/5] configure: Set gnu99 C language standard by default

[Javier Martinez Canillas]

Hello Daniel,

On 5/18/20 5:49 PM, Daniel Kiper wrote:

[snip]

>>
>> Without this patch, clang-9 fails on the use of nested functions in
>> commit cb2f15c54489 ("normal/main: Search for specific config files for
>> netboot") - I'll send a fix for that shortly. If I revert that as a
>> temporary measure, clang will build.
> 
> Javier, could you take a look at this too?
>

Sure, I'll take a look to this.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v3 2/5] configure: Set gnu99 C language standard by default

[Daniel Axtens]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Hello Daniel,
>
> On 5/18/20 5:49 PM, Daniel Kiper wrote:
>
> [snip]
>
>>>
>>> Without this patch, clang-9 fails on the use of nested functions in
>>> commit cb2f15c54489 ("normal/main: Search for specific config files for
>>> netboot") - I'll send a fix for that shortly. If I revert that as a
>>> temporary measure, clang will build.
>> 
>> Javier, could you take a look at this too?

Javier,

I think this is as simple as un-nesting the function, but you know the
code base better than me, so I'll leave it to you unless you
particularly want me to handle it.

Regards,
Daniel

>>
>
> Sure, I'll take a look to this.
>
> Best regards,
> -- 
> Javier Martinez Canillas
> Software Engineer - Desktop Hardware Enablement
> Red Hat

Re: [PATCH v3 2/5] configure: Set gnu99 C language standard by default

[Javier Martinez Canillas]

Hello Daniel,

On 5/19/20 3:47 AM, Daniel Axtens wrote:
> Javier Martinez Canillas <javierm@redhat.com> writes:
> 
>> Hello Daniel,
>>
>> On 5/18/20 5:49 PM, Daniel Kiper wrote:
>>
>> [snip]
>>
>>>>
>>>> Without this patch, clang-9 fails on the use of nested functions in
>>>> commit cb2f15c54489 ("normal/main: Search for specific config files for
>>>> netboot") - I'll send a fix for that shortly. If I revert that as a
>>>> temporary measure, clang will build.
>>>
>>> Javier, could you take a look at this too?
> 
> Javier,
> 
> I think this is as simple as un-nesting the function, but you know the
> code base better than me, so I'll leave it to you unless you
> particularly want me to handle it.
>

Yes, that should be enough. I'll post a patch but I don't have a clang
build environment so I would appreciate if you can test it.

> Regards,
> Daniel
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH v3 2/5] configure: Set gnu99 C language standard by default

[Daniel Axtens]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Hello Daniel,
>
> On 5/19/20 3:47 AM, Daniel Axtens wrote:
>> Javier Martinez Canillas <javierm@redhat.com> writes:
>> 
>>> Hello Daniel,
>>>
>>> On 5/18/20 5:49 PM, Daniel Kiper wrote:
>>>
>>> [snip]
>>>
>>>>>
>>>>> Without this patch, clang-9 fails on the use of nested functions in
>>>>> commit cb2f15c54489 ("normal/main: Search for specific config files for
>>>>> netboot") - I'll send a fix for that shortly. If I revert that as a
>>>>> temporary measure, clang will build.
>>>>
>>>> Javier, could you take a look at this too?
>> 
>> Javier,
>> 
>> I think this is as simple as un-nesting the function, but you know the
>> code base better than me, so I'll leave it to you unless you
>> particularly want me to handle it.
>>
>
> Yes, that should be enough. I'll post a patch but I don't have a clang
> build environment so I would appreciate if you can test it.

Will do.

>
>> Regards,
>> Daniel
>> 
>
> Best regards,
> -- 
> Javier Martinez Canillas
> Software Engineer - Desktop Hardware Enablement
> Red Hat

[PATCH] json: Remove invalid typedef redefinition

[Patrick Steinhardt]

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

The C standard does not allow for typedef redefinitions, even if they
map to the same underlying type. In order to avoid including the
"jsmn.h" in "json.h" and thus exposing jsmn's internals, we have exactly
such a forward-declaring typedef in "json.h". If enforcing the GNU99 C
standard, clang may generate a warning about this non-standard
construct.

Fix the issue by using a simple `struct jsmntok` forward declaration
instead of using a typedef.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---

Sorry for the late response, didn't notice at first that this directly
impacts code I wrote. I don't have Clang available on my computer and
GCC seems to lack the equivalent option for this, so I wasn't able to
reproduce the warning. Below patch should fix the issue, though, as we
simply avoid using a typedef for the forward declaration.

 grub-core/lib/json/json.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
index d9f99454d..2e2b0bd07 100644
--- a/grub-core/lib/json/json.h
+++ b/grub-core/lib/json/json.h
@@ -36,13 +36,14 @@ enum grub_json_type
 };
 typedef enum grub_json_type grub_json_type_t;
 
-typedef struct jsmntok jsmntok_t;
+/* Forward-declaration to avoid including "jsmn.h" */
+struct jsmntok;
 
 struct grub_json
 {
-  jsmntok_t   *tokens;
-  char	      *string;
-  grub_size_t idx;
+  struct jsmntok *tokens;
+  char		 *string;
+  grub_size_t	 idx;
 };
 typedef struct grub_json grub_json_t;
 
-- 
2.26.2


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

Re: [PATCH] json: Remove invalid typedef redefinition

[Daniel Kiper]

On Wed, May 27, 2020 at 11:18:24AM +0200, Patrick Steinhardt wrote:
> The C standard does not allow for typedef redefinitions, even if they
> map to the same underlying type. In order to avoid including the
> "jsmn.h" in "json.h" and thus exposing jsmn's internals, we have exactly
> such a forward-declaring typedef in "json.h". If enforcing the GNU99 C
> standard, clang may generate a warning about this non-standard
> construct.
>
> Fix the issue by using a simple `struct jsmntok` forward declaration
> instead of using a typedef.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

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

Daniel A., could you check this patch?

> ---
>
> Sorry for the late response, didn't notice at first that this directly
> impacts code I wrote. I don't have Clang available on my computer and
> GCC seems to lack the equivalent option for this, so I wasn't able to
> reproduce the warning. Below patch should fix the issue, though, as we
> simply avoid using a typedef for the forward declaration.

Not a problem. Thanks a lot for fixing this issue...

Daniel

Re: [PATCH] json: Remove invalid typedef redefinition

[Daniel Axtens]

Hi Patrick,

> The C standard does not allow for typedef redefinitions, even if they
> map to the same underlying type. In order to avoid including the
> "jsmn.h" in "json.h" and thus exposing jsmn's internals, we have exactly
> such a forward-declaring typedef in "json.h". If enforcing the GNU99 C
> standard, clang may generate a warning about this non-standard
> construct.
>
> Fix the issue by using a simple `struct jsmntok` forward declaration
> instead of using a typedef.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Tested-by: Daniel Axtens <dja@axtens.net> # clang build test for emu target only

With this patch, clang can compile master again. Thanks Patrick!

Regards,
Daniel

> ---
>
> Sorry for the late response, didn't notice at first that this directly
> impacts code I wrote. I don't have Clang available on my computer and
> GCC seems to lack the equivalent option for this, so I wasn't able to
> reproduce the warning. Below patch should fix the issue, though, as we
> simply avoid using a typedef for the forward declaration.
>
>  grub-core/lib/json/json.h | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/lib/json/json.h b/grub-core/lib/json/json.h
> index d9f99454d..2e2b0bd07 100644
> --- a/grub-core/lib/json/json.h
> +++ b/grub-core/lib/json/json.h
> @@ -36,13 +36,14 @@ enum grub_json_type
>  };
>  typedef enum grub_json_type grub_json_type_t;
>  
> -typedef struct jsmntok jsmntok_t;
> +/* Forward-declaration to avoid including "jsmn.h" */
> +struct jsmntok;
>  
>  struct grub_json
>  {
> -  jsmntok_t   *tokens;
> -  char	      *string;
> -  grub_size_t idx;
> +  struct jsmntok *tokens;
> +  char		 *string;
> +  grub_size_t	 idx;
>  };
>  typedef struct grub_json grub_json_t;
>  
> -- 
> 2.26.2