[PATCH 0/1] DTC v1.7.1 Build Error

[PATCH 0/1] DTC v1.7.1 Build Error

[Markus Mayer]

Hi all,

Our automated build environment flagged a new build error introduced by
DTC 1.7.1. Specifically, a compiler option (warning) that was added is
only supported by gcc but not by clang. This results in the build
failing immediately if clang is being used.

clang-18 -I libfdt -I . -DFDT_ASSUME_MASK=0 -DNO_VALGRIND -g -Os -fPIC
  -Werror -Wall -Wpointer-arith -Wcast-qual -Wnested-externs -Wsign-compare
  -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
  -Wsuggest-attribute=format -Wwrite-strings  -DNO_YAML -o srcpos.o -c
  srcpos.c
error: unknown warning option '-Wsuggest-attribute=format'; did you mean
  '-Wproperty-attribute-mismatch'? [-Werror,-Wunknown-warning-option]
make: *** [Makefile:359: srcpos.o] Error 1

The commit in question is [1]

e3cde0613bfd Add -Wsuggest-attribute=format warning, correct warnings thus generated

This commit was not part of v1.7.0 and is part of v1.7.1. The problem
is, clang doesn't like this option, and with -Werror enabled, the build
fails. Meanwhile, clang is perfectly capable of building dtc once the
offending option has been removed.

I am submitting a potential solution to this problem that relies on some
Makefile code taken from the Linux kernel to detect if the compiler
supports a certain option or not. Admittedly, the added code is a bit
wordy, but it should work very well going forward if another such
situation occurs. It is also very concise and easy to read when it is
being used.

Lastly, here is some debug output to show how the value of $(WARNINGS)
is affected by the compiler.

# using gcc
$ make
Makefile:49: *** The warnings are: -Wall -Wpointer-arith -Wcast-qual
  -Wnested-externs -Wsign-compare -Wstrict-prototypes -Wmissing-prototypes
  -Wredundant-decls -Wshadow -Wwrite-strings -Wsuggest-attribute=format.
Stop.

# using clang
$ CC=clang-18 make
Makefile:49: *** The warnings are: -Wall -Wpointer-arith -Wcast-qual
  -Wnested-externs -Wsign-compare -Wstrict-prototypes -Wmissing-prototypes
  -Wredundant-decls -Wshadow -Wwrite-strings .
Stop.

As intended, "-Wsuggest-attribute=format" disappears if clang is being
used, and with this, clang can successfully complete the build.

Please let me know what you think.

Thanks,
-Markus

[1] https://git.kernel.org/pub/scm/utils/dtc/dtc.git/commit/?id=e3cde0613bfd

Markus Mayer (1):
  Makefile: only use compiler flags when supported

 Makefile | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

-- 
2.46.0

[PATCH 1/1] Makefile: only use compiler flags when supported

[Markus Mayer]

We need to check if some compiler flags are supported as not all
compilers may do so. For instance, "-Wsuggest-attribute=format" is
supported by gcc but not by clang.

Using a compiler option that is not supported will most likely cause the
build to fail, since we are building with "-Werror" and the compiler will
issue a warning about the unknown option.

As a result, we only use the option if the compiler understands it.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 Makefile | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index f1f0ab368cd3..febdca5742c5 100644
--- a/Makefile
+++ b/Makefile
@@ -3,6 +3,26 @@
 # Device Tree Compiler
 #
 
+# try-run and cc-option were stolen from the Linux kernel
+TMPOUT = .tmp_$$$$
+# try-run
+# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
+# Exit code chooses option. "$$TMP" serves as a temporary file and is
+# automatically cleaned up.
+try-run = $(shell set -e;		\
+	TMP=$(TMPOUT)/tmp;		\
+	mkdir -p $(TMPOUT);		\
+	trap "rm -rf $(TMPOUT)" EXIT;	\
+	if ($(1)) >/dev/null 2>&1;	\
+	then echo "$(2)";		\
+	else echo "$(3)";		\
+	fi)
+
+# cc-option
+# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
+cc-option = $(call try-run, \
+	$(CC) -Werror $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
+
 #
 # Version information will be constructed in this order:
 # DTC_VERSION release version as MAJOR.MINOR.PATCH
@@ -18,9 +38,12 @@ CONFIG_LOCALVERSION =
 ASSUME_MASK ?= 0
 
 CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
+# Warnings where we need to check they are supported
+OPT_WARNINGS = $(call cc-option,-Wsuggest-attribute=format)
+# Regular warnings
 WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs -Wsign-compare \
 	-Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow \
-	-Wsuggest-attribute=format -Wwrite-strings
+	-Wwrite-strings $(OPT_WARNINGS)
 CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
 
 BISON = bison
-- 
2.46.0

Re: [PATCH 1/1] Makefile: only use compiler flags when supported

[Simon Glass]

Hi Markus,

On Tue, 8 Oct 2024 at 15:42, Markus Mayer <mmayer@broadcom.com> wrote:
>
> We need to check if some compiler flags are supported as not all
> compilers may do so. For instance, "-Wsuggest-attribute=format" is
> supported by gcc but not by clang.
>
> Using a compiler option that is not supported will most likely cause the
> build to fail, since we are building with "-Werror" and the compiler will
> issue a warning about the unknown option.
>
> As a result, we only use the option if the compiler understands it.
>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  Makefile | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index f1f0ab368cd3..febdca5742c5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3,6 +3,26 @@
>  # Device Tree Compiler
>  #
>
> +# try-run and cc-option were stolen from the Linux kernel
> +TMPOUT = .tmp_$$$$
> +# try-run
> +# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
> +# Exit code chooses option. "$$TMP" serves as a temporary file and is
> +# automatically cleaned up.
> +try-run = $(shell set -e;              \
> +       TMP=$(TMPOUT)/tmp;              \
> +       mkdir -p $(TMPOUT);             \
> +       trap "rm -rf $(TMPOUT)" EXIT;   \
> +       if ($(1)) >/dev/null 2>&1;      \
> +       then echo "$(2)";               \
> +       else echo "$(3)";               \
> +       fi)
> +
> +# cc-option
> +# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
> +cc-option = $(call try-run, \
> +       $(CC) -Werror $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
> +
>  #
>  # Version information will be constructed in this order:
>  # DTC_VERSION release version as MAJOR.MINOR.PATCH
> @@ -18,9 +38,12 @@ CONFIG_LOCALVERSION =
>  ASSUME_MASK ?= 0
>
>  CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
> +# Warnings where we need to check they are supported
> +OPT_WARNINGS = $(call cc-option,-Wsuggest-attribute=format)

This is fine, but you should be able to use the existing cc-option in U-Boot

> +# Regular warnings
>  WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs -Wsign-compare \
>         -Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow \
> -       -Wsuggest-attribute=format -Wwrite-strings
> +       -Wwrite-strings $(OPT_WARNINGS)
>  CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
>
>  BISON = bison
> --
> 2.46.0
>
>

Regards,
Simon

Re: [PATCH 1/1] Makefile: only use compiler flags when supported

[Markus Mayer]

Hi Simon,

On Tue, 8 Oct 2024 at 18:57, Simon Glass <sjg@chromium.org> wrote:
>
> > diff --git a/Makefile b/Makefile
> > index f1f0ab368cd3..febdca5742c5 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -3,6 +3,26 @@
> >  # Device Tree Compiler
> >  #
> >
> > +# try-run and cc-option were stolen from the Linux kernel
> > +TMPOUT = .tmp_$$$$
> > +# try-run
> > +# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
> > +# Exit code chooses option. "$$TMP" serves as a temporary file and is
> > +# automatically cleaned up.
> > +try-run = $(shell set -e;              \
> > +       TMP=$(TMPOUT)/tmp;              \
> > +       mkdir -p $(TMPOUT);             \
> > +       trap "rm -rf $(TMPOUT)" EXIT;   \
> > +       if ($(1)) >/dev/null 2>&1;      \
> > +       then echo "$(2)";               \
> > +       else echo "$(3)";               \
> > +       fi)
> > +
> > +# cc-option
> > +# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
> > +cc-option = $(call try-run, \
> > +       $(CC) -Werror $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
> > +
> >  #
> >  # Version information will be constructed in this order:
> >  # DTC_VERSION release version as MAJOR.MINOR.PATCH
> > @@ -18,9 +38,12 @@ CONFIG_LOCALVERSION =
> >  ASSUME_MASK ?= 0
> >
> >  CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
> > +# Warnings where we need to check they are supported
> > +OPT_WARNINGS = $(call cc-option,-Wsuggest-attribute=format)
>
> This is fine, but you should be able to use the existing cc-option in U-Boot

I seem to be missing something... how does U-Boot factor in here?

My use case is to build a standalone version of dtc with clang for
x86. This dtc is meant to run on the build host. U-Boot sources are
not involved.

Thanks,
-Markus

Re: [PATCH 1/1] Makefile: only use compiler flags when supported

[Simon Glass]

Hi Markus,

On Wed, 9 Oct 2024 at 14:43, Markus Mayer <mmayer@broadcom.com> wrote:
>
> Hi Simon,
>
> On Tue, 8 Oct 2024 at 18:57, Simon Glass <sjg@chromium.org> wrote:
> >
> > > diff --git a/Makefile b/Makefile
> > > index f1f0ab368cd3..febdca5742c5 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -3,6 +3,26 @@
> > >  # Device Tree Compiler
> > >  #
> > >
> > > +# try-run and cc-option were stolen from the Linux kernel
> > > +TMPOUT = .tmp_$$$$
> > > +# try-run
> > > +# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
> > > +# Exit code chooses option. "$$TMP" serves as a temporary file and is
> > > +# automatically cleaned up.
> > > +try-run = $(shell set -e;              \
> > > +       TMP=$(TMPOUT)/tmp;              \
> > > +       mkdir -p $(TMPOUT);             \
> > > +       trap "rm -rf $(TMPOUT)" EXIT;   \
> > > +       if ($(1)) >/dev/null 2>&1;      \
> > > +       then echo "$(2)";               \
> > > +       else echo "$(3)";               \
> > > +       fi)
> > > +
> > > +# cc-option
> > > +# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
> > > +cc-option = $(call try-run, \
> > > +       $(CC) -Werror $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
> > > +
> > >  #
> > >  # Version information will be constructed in this order:
> > >  # DTC_VERSION release version as MAJOR.MINOR.PATCH
> > > @@ -18,9 +38,12 @@ CONFIG_LOCALVERSION =
> > >  ASSUME_MASK ?= 0
> > >
> > >  CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
> > > +# Warnings where we need to check they are supported
> > > +OPT_WARNINGS = $(call cc-option,-Wsuggest-attribute=format)
> >
> > This is fine, but you should be able to use the existing cc-option in U-Boot
>
> I seem to be missing something... how does U-Boot factor in here?
>
> My use case is to build a standalone version of dtc with clang for
> x86. This dtc is meant to run on the build host. U-Boot sources are
> not involved.

Sorry, I saw this in the middle of a bunch of U-Boot reviews and
didn't notice :-)

Regards,
Simon

Re: [PATCH 1/1] Makefile: only use compiler flags when supported

[Markus Mayer]

On Wed, 9 Oct 2024 at 14:15, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Markus,
>
> On Wed, 9 Oct 2024 at 14:43, Markus Mayer <mmayer@broadcom.com> wrote:
> >
> > Hi Simon,
[...]
> > > This is fine, but you should be able to use the existing cc-option in U-Boot
> >
> > I seem to be missing something... how does U-Boot factor in here?
> >
> > My use case is to build a standalone version of dtc with clang for
> > x86. This dtc is meant to run on the build host. U-Boot sources are
> > not involved.
>
> Sorry, I saw this in the middle of a bunch of U-Boot reviews and
> didn't notice :-)

*LOL* Gotcha. I am not going crazy. That is a relief.

Regards,
-Markus

Re: [PATCH 1/1] Makefile: only use compiler flags when supported

[David Gibson]

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

On Tue, Oct 08, 2024 at 02:41:32PM -0700, Markus Mayer wrote:
> We need to check if some compiler flags are supported as not all
> compilers may do so. For instance, "-Wsuggest-attribute=format" is
> supported by gcc but not by clang.
> 
> Using a compiler option that is not supported will most likely cause the
> build to fail, since we are building with "-Werror" and the compiler will
> issue a warning about the unknown option.
> 
> As a result, we only use the option if the compiler understands it.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>

Hrm.  So, I can see the use, and the logic looks sound.  On the other
hand, I'm not especially keen to have a tiny little project like dtc
carry around a complex configure & build system with lots of compiler
knowledge.  This can be worked around by explicitly overriding CFLAGS,
and if using clang you're presumably already overriding CC.

This is perhaps another impetus to deprecate the Makefiles in favour
of the meson build system which we already support.  AIUI meson has a
fair bit of built in knowledge about compilers and their capabilities.

> ---
>  Makefile | 25 ++++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index f1f0ab368cd3..febdca5742c5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3,6 +3,26 @@
>  # Device Tree Compiler
>  #
>  
> +# try-run and cc-option were stolen from the Linux kernel
> +TMPOUT = .tmp_$$$$
> +# try-run
> +# Usage: option = $(call try-run, $(CC)...-o "$$TMP",option-ok,otherwise)
> +# Exit code chooses option. "$$TMP" serves as a temporary file and is
> +# automatically cleaned up.
> +try-run = $(shell set -e;		\
> +	TMP=$(TMPOUT)/tmp;		\
> +	mkdir -p $(TMPOUT);		\
> +	trap "rm -rf $(TMPOUT)" EXIT;	\
> +	if ($(1)) >/dev/null 2>&1;	\
> +	then echo "$(2)";		\
> +	else echo "$(3)";		\
> +	fi)
> +
> +# cc-option
> +# Usage: cflags-y += $(call cc-option,-march=winchip-c6,-march=i586)
> +cc-option = $(call try-run, \
> +	$(CC) -Werror $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2))
> +
>  #
>  # Version information will be constructed in this order:
>  # DTC_VERSION release version as MAJOR.MINOR.PATCH
> @@ -18,9 +38,12 @@ CONFIG_LOCALVERSION =
>  ASSUME_MASK ?= 0
>  
>  CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
> +# Warnings where we need to check they are supported
> +OPT_WARNINGS = $(call cc-option,-Wsuggest-attribute=format)
> +# Regular warnings
>  WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs -Wsign-compare \
>  	-Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow \
> -	-Wsuggest-attribute=format -Wwrite-strings
> +	-Wwrite-strings $(OPT_WARNINGS)
>  CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
>  
>  BISON = bison

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

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