[PATCH] objtool: Don't use -Werror

[PATCH] objtool: Don't use -Werror

[Luis Ressel]

-Werror can be handy for development, but enabling it for production
builds is a bad idea -- other compilers might produce unexpected
warnings, or #included library headers might trigger warnings.

In my case, libelf's (not elfutil's!) headers trigger several -Wundef
warnings. This wasn't a problem before 056d28d135bc, since gcc doesn't
emit warnings for system headers, but now that there's a
-I/usr/include/libelf/ in the gcc command line, those warnings appear
and break the build.

CC'ing stable@ since 056d28d135bc has also been included in stable
versions.

Fixes: 056d28d135bc ("objtool: Query pkg-config for libelf location")
Signed-off-by: Luis Ressel <aranea@aixah.de>
Cc: stable@vger.kernel.org
---
 tools/objtool/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
index 53f8be0f4a1f..ad2c11a881db 100644
--- a/tools/objtool/Makefile
+++ b/tools/objtool/Makefile
@@ -34,7 +34,7 @@ INCLUDES := -I$(srctree)/tools/include \
 	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
 	    -I$(srctree)/tools/objtool/arch/$(ARCH)/include
 WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
-CFLAGS   += -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
+CFLAGS   += $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
 LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
 
 # Allow old libelf to be used:
-- 
2.21.0

Re: [PATCH] objtool: Don't use -Werror

[Josh Poimboeuf]

On Fri, Apr 05, 2019 at 01:01:50PM +0200, Luis Ressel wrote:
> -Werror can be handy for development, but enabling it for production
> builds is a bad idea -- other compilers might produce unexpected
> warnings, or #included library headers might trigger warnings.
> 
> In my case, libelf's (not elfutil's!) headers trigger several -Wundef
> warnings. This wasn't a problem before 056d28d135bc, since gcc doesn't
> emit warnings for system headers, but now that there's a
> -I/usr/include/libelf/ in the gcc command line, those warnings appear
> and break the build.

Hi Luis,

What version of libelf are you using?  AFAIK, the non-elfutils version
of libelf is buggy and has been unmaintained for 10 years.

> CC'ing stable@ since 056d28d135bc has also been included in stable
> versions.
> 
> Fixes: 056d28d135bc ("objtool: Query pkg-config for libelf location")
> Signed-off-by: Luis Ressel <aranea@aixah.de>
> Cc: stable@vger.kernel.org
> ---
>  tools/objtool/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/objtool/Makefile b/tools/objtool/Makefile
> index 53f8be0f4a1f..ad2c11a881db 100644
> --- a/tools/objtool/Makefile
> +++ b/tools/objtool/Makefile
> @@ -34,7 +34,7 @@ INCLUDES := -I$(srctree)/tools/include \
>  	    -I$(srctree)/tools/arch/$(HOSTARCH)/include/uapi \
>  	    -I$(srctree)/tools/objtool/arch/$(ARCH)/include
>  WARNINGS := $(EXTRA_WARNINGS) -Wno-switch-default -Wno-switch-enum -Wno-packed
> -CFLAGS   += -Werror $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
> +CFLAGS   += $(WARNINGS) $(KBUILD_HOSTCFLAGS) -g $(INCLUDES) $(LIBELF_FLAGS)
>  LDFLAGS  += $(LIBELF_LIBS) $(LIBSUBCMD) $(KBUILD_HOSTLDFLAGS)
>  
>  # Allow old libelf to be used:
> -- 
> 2.21.0
> 

-- 
Josh

Re: [PATCH] objtool: Don't use -Werror

[Luis Ressel]

On Fri, Apr 05, 2019 at 07:39:09AM -0500, Josh Poimboeuf wrote:
> What version of libelf are you using?  AFAIK, the non-elfutils version
> of libelf is buggy and has been unmaintained for 10 years.

I'm using libelf 0.8.13, which is indeed 10y old, abandoned and rather
suboptimal (elfutils OTOH is nonportable, and I haven't gotten around
yet to fixing its incompatibilities with the musl libc).

I can accept that you might not be interested in fixing issues related
to libelf, but using -Werror is a more general problem which just
happens to be triggered by my particular setup.

Regards,
Luis

Re: [PATCH] objtool: Don't use -Werror

[Josh Poimboeuf]

On Fri, Apr 05, 2019 at 04:24:43PM +0200, Luis Ressel wrote:
> On Fri, Apr 05, 2019 at 07:39:09AM -0500, Josh Poimboeuf wrote:
> > What version of libelf are you using?  AFAIK, the non-elfutils version
> > of libelf is buggy and has been unmaintained for 10 years.
> 
> I'm using libelf 0.8.13, which is indeed 10y old, abandoned and rather
> suboptimal (elfutils OTOH is nonportable, and I haven't gotten around
> yet to fixing its incompatibilities with the musl libc).

If you can't use the elfutils version, I'd recommend just disabling all
the features which rely on objtool.  Because some of the libelf-related
bugs I've seen are pretty bad, and we rely on objtool for some pretty
fundamental things like ORC oops stack traces and live patching.

In fact I think we probably need a patch to fail the build if the
10-year-old libelf is used, as I've gotten several bug reports there and
the answer is always the same ("don't use old libelf").

> I can accept that you might not be interested in fixing issues related
> to libelf, but using -Werror is a more general problem which just
> happens to be triggered by my particular setup.

Hm, I would actually argue the reverse.  Warnings are generally bad and
-Werror is useful for ensuring that we don't have any.  For warnings
that don't provide value, we just disable those individual warnings.

-- 
Josh

Re: [PATCH] objtool: Don't use -Werror

[Luis Ressel]

On Fri, Apr 05, 2019 at 09:39:26AM -0500, Josh Poimboeuf wrote:

> Hm, I would actually argue the reverse.  Warnings are generally bad and
> -Werror is useful for ensuring that we don't have any.  For warnings
> that don't provide value, we just disable those individual warnings.

Sure, during development it's an excellent idea to investigate compiler
warnings, and -Werror can be useful for that. But the Linux kernel is
built by countless users in wildly varying environments, and it's almost
a given that someone will use a compiler that'll complain about a valid
part of your code whose style it considers bad.

As an example, the warning that's breaking the build for me is -Wundef
complaining about several "#if UNDEFINED_IDENTIFIER" constructs in the
libelf headers. (I agree with gcc in considering this bad style, but
it's perfectly valid C, and there probably wasn't a warning about it
back when this header was written.)

Regards,
Luis

Re: [PATCH] objtool: Don't use -Werror

[Josh Poimboeuf]

On Fri, Apr 05, 2019 at 06:05:50PM +0200, Luis Ressel wrote:
> On Fri, Apr 05, 2019 at 09:39:26AM -0500, Josh Poimboeuf wrote:
> 
> > Hm, I would actually argue the reverse.  Warnings are generally bad and
> > -Werror is useful for ensuring that we don't have any.  For warnings
> > that don't provide value, we just disable those individual warnings.
> 
> Sure, during development it's an excellent idea to investigate compiler
> warnings, and -Werror can be useful for that. But the Linux kernel is
> built by countless users in wildly varying environments, and it's almost
> a given that someone will use a compiler that'll complain about a valid
> part of your code whose style it considers bad.

Maybe so, but objtool only supports two compilers: GCC and clang.  And
only one version of libelf ;-)

> As an example, the warning that's breaking the build for me is -Wundef
> complaining about several "#if UNDEFINED_IDENTIFIER" constructs in the
> libelf headers. (I agree with gcc in considering this bad style, but
> it's perfectly valid C, and there probably wasn't a warning about it
> back when this header was written.)

I consider that a good thing, because I *want* the build to be broken
when somebody uses a bad version of libelf.  A patch to produce a more
useful error message (e.g., "bad version of libelf") would be welcome of
course.

If/when we get to the point where there's a valid use case for warnings
in the objtool build, I would consider this patch.  But we don't seem to
be there yet.

-- 
Josh

Re: [PATCH] objtool: Don't use -Werror

[Luis Ressel]

On Fri, Apr 05, 2019 at 11:15:41AM -0500, Josh Poimboeuf wrote:
> I consider that a good thing, because I *want* the build to be broken
> when somebody uses a bad version of libelf.  A patch to produce a more
> useful error message (e.g., "bad version of libelf") would be welcome of
> course.

Do that if you must, but please do it properly (i.e. check the libelf
version and error out explicitly with a suitable message, instead of
relying on -Werror), and don't backport the change to stable kernels.

The patch which I referenced in my commit message suddently broke the
build for me between 4.19.32 and .33; I consider that a regression which
ought to be fixed.

Regards,
Luis

RE: [PATCH] objtool: Don't use -Werror

[David Laight]

From: Luis Ressel
> Sent: 05 April 2019 17:06
> On Fri, Apr 05, 2019 at 09:39:26AM -0500, Josh Poimboeuf wrote:
> 
> > Hm, I would actually argue the reverse.  Warnings are generally bad and
> > -Werror is useful for ensuring that we don't have any.  For warnings
> > that don't provide value, we just disable those individual warnings.
> 
> Sure, during development it's an excellent idea to investigate compiler
> warnings, and -Werror can be useful for that. But the Linux kernel is
> built by countless users in wildly varying environments, and it's almost
> a given that someone will use a compiler that'll complain about a valid
> part of your code whose style it considers bad.
> 
> As an example, the warning that's breaking the build for me is -Wundef
> complaining about several "#if UNDEFINED_IDENTIFIER" constructs in the
> libelf headers. (I agree with gcc in considering this bad style, but
> it's perfectly valid C, and there probably wasn't a warning about it
> back when this header was written.)

In which case you should be looking at a way of removing -Wundef
not removing -Werror.

FWIW I had to update libelf.so from version 0.153 to 0.165 in
order for the amd64 orc unwinder code in objtool to not generate
corrupt output files.
That is an Ubuntu 13.04 system - nothing like 10 years old.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] objtool: Don't use -Werror

[Josh Poimboeuf]

On Fri, Apr 05, 2019 at 04:16:14PM +0000, David Laight wrote:
> From: Luis Ressel
> > Sent: 05 April 2019 17:06
> > On Fri, Apr 05, 2019 at 09:39:26AM -0500, Josh Poimboeuf wrote:
> > 
> > > Hm, I would actually argue the reverse.  Warnings are generally bad and
> > > -Werror is useful for ensuring that we don't have any.  For warnings
> > > that don't provide value, we just disable those individual warnings.
> > 
> > Sure, during development it's an excellent idea to investigate compiler
> > warnings, and -Werror can be useful for that. But the Linux kernel is
> > built by countless users in wildly varying environments, and it's almost
> > a given that someone will use a compiler that'll complain about a valid
> > part of your code whose style it considers bad.
> > 
> > As an example, the warning that's breaking the build for me is -Wundef
> > complaining about several "#if UNDEFINED_IDENTIFIER" constructs in the
> > libelf headers. (I agree with gcc in considering this bad style, but
> > it's perfectly valid C, and there probably wasn't a warning about it
> > back when this header was written.)
> 
> In which case you should be looking at a way of removing -Wundef
> not removing -Werror.

Agreed.

> FWIW I had to update libelf.so from version 0.153 to 0.165 in
> order for the amd64 orc unwinder code in objtool to not generate
> corrupt output files.
> That is an Ubuntu 13.04 system - nothing like 10 years old.

Ah.  It would be great if we had a patch to fail the build for old
versions of elfutils-libelf as well.

-- 
Josh

RE: [PATCH] objtool: Don't use -Werror

[David Laight]

From: Josh Poimboeuf
> Sent: 05 April 2019 17:21
..
> > FWIW I had to update libelf.so from version 0.153 to 0.165 in
> > order for the amd64 orc unwinder code in objtool to not generate
> > corrupt output files.
> > That is an Ubuntu 13.04 system - nothing like 10 years old.
> 
> Ah.  It would be great if we had a patch to fail the build for old
> versions of elfutils-libelf as well.

I think the check I added for that specific error got committed.
Look at the defintion of cmd_objtool in scripts/Makefile.build.

Although I seem to have a local change to the definition of
cmd_modversions_c.
It might be that the version of a change I made that got committed
is missing an extra $ when processing the saved output from 'objdump -h'.
It used to just pipe into grep - so lost the error result from objdump.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] objtool: Don't use -Werror

[Josh Poimboeuf]

On Fri, Apr 05, 2019 at 04:35:50PM +0000, David Laight wrote:
> From: Josh Poimboeuf
> > Sent: 05 April 2019 17:21
> ..
> > > FWIW I had to update libelf.so from version 0.153 to 0.165 in
> > > order for the amd64 orc unwinder code in objtool to not generate
> > > corrupt output files.
> > > That is an Ubuntu 13.04 system - nothing like 10 years old.
> > 
> > Ah.  It would be great if we had a patch to fail the build for old
> > versions of elfutils-libelf as well.
> 
> I think the check I added for that specific error got committed.
> Look at the defintion of cmd_objtool in scripts/Makefile.build.
> 
> Although I seem to have a local change to the definition of
> cmd_modversions_c.
> It might be that the version of a change I made that got committed
> is missing an extra $ when processing the saved output from 'objdump -h'.
> It used to just pipe into grep - so lost the error result from objdump.

Hm, I don't see that in cmd_objtool, or any commits from you in
scripts/Makefile.build.

-- 
Josh

RE: [PATCH] objtool: Don't use -Werror

[David Laight]

> Hm, I don't see that in cmd_objtool, or any commits from you in
> scripts/Makefile.build.

Not sure I remember actually committing them.
Maybe 'git diff' isn't doing what I expect :-)
I hate git.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] objtool: Don't use -Werror

[Josh Poimboeuf]

On Fri, Apr 05, 2019 at 05:17:15PM +0000, David Laight wrote:
> > Hm, I don't see that in cmd_objtool, or any commits from you in
> > scripts/Makefile.build.
> 
> Not sure I remember actually committing them.
> Maybe 'git diff' isn't doing what I expect :-)
> I hate git.

Do you see it here?

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.build

-- 
Josh

RE: [PATCH] objtool: Don't use -Werror

[David Laight]

From: Josh Poimboeuf
> Sent: 05 April 2019 18:23
> On Fri, Apr 05, 2019 at 05:17:15PM +0000, David Laight wrote:
> > > Hm, I don't see that in cmd_objtool, or any commits from you in
> > > scripts/Makefile.build.
> >
> > Not sure I remember actually committing them.
> > Maybe 'git diff' isn't doing what I expect :-)
> > I hate git.
> 
> Do you see it here?
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.build

No, I must have had to tell git to apply the change locally.
I can't see how to get git to diff against the 'real' head.
Anyway a 'random' number of ^ after HEAD gave me:

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index e7889f4..a2f7abe 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -193,7 +193,9 @@ else
 cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $<

 cmd_modversions_c =                                                            \
-       if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then             \
+       if ! obj_headers=$$($(OBJDUMP) -h $(@D)/.tmp_$(@F)); then               \
+               false;                                                          \
+       elif echo "$$obj_headers" | grep -q __ksymtab; then                     \
                $(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))  \
                    > $(@D)/.tmp_$(@F:.o=.ver);                                 \
                                                                                \
@@ -277,7 +279,14 @@ endif
 # 'OBJECT_FILES_NON_STANDARD_foo.o := 'n': override directory skip for a file
 cmd_objtool = $(if $(patsubst y%,, \
        $(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \
-       $(__objtool_obj) $(objtool_args) "$(objtool_o)";)
+       $(__objtool_obj) $(objtool_args) "$(objtool_o)" && { \
+               $(OBJDUMP) -h "$(objtool_o)" >/dev/null 2>&1 || { \
+                       echo >&2 "******* objtool generated the invalid file $(objtool_o)"; \
+                       echo >&2 "******* Update libelf.so or disable the ORC unwinder"; \
+                       /bin/false; \
+               }; \
+       };)
+
 objtool_obj = $(if $(patsubst y%,, \
        $(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \
        $(__objtool_obj))

Do you want a formal patch?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] objtool: Don't use -Werror

[Josh Poimboeuf]

On Mon, Apr 08, 2019 at 09:20:50AM +0000, David Laight wrote:
> From: Josh Poimboeuf
> > Sent: 05 April 2019 18:23
> > On Fri, Apr 05, 2019 at 05:17:15PM +0000, David Laight wrote:
> > > > Hm, I don't see that in cmd_objtool, or any commits from you in
> > > > scripts/Makefile.build.
> > >
> > > Not sure I remember actually committing them.
> > > Maybe 'git diff' isn't doing what I expect :-)
> > > I hate git.
> > 
> > Do you see it here?
> > 
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/Makefile.build
> 
> No, I must have had to tell git to apply the change locally.
> I can't see how to get git to diff against the 'real' head.
> Anyway a 'random' number of ^ after HEAD gave me:
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index e7889f4..a2f7abe 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -193,7 +193,9 @@ else
>  cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $<
> 
>  cmd_modversions_c =                                                            \
> -       if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then             \
> +       if ! obj_headers=$$($(OBJDUMP) -h $(@D)/.tmp_$(@F)); then               \
> +               false;                                                          \
> +       elif echo "$$obj_headers" | grep -q __ksymtab; then                     \
>                 $(call cmd_gensymtypes_c,$(KBUILD_SYMTYPES),$(@:.o=.symtypes))  \
>                     > $(@D)/.tmp_$(@F:.o=.ver);                                 \
>                                                                                 \
> @@ -277,7 +279,14 @@ endif
>  # 'OBJECT_FILES_NON_STANDARD_foo.o := 'n': override directory skip for a file
>  cmd_objtool = $(if $(patsubst y%,, \
>         $(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \
> -       $(__objtool_obj) $(objtool_args) "$(objtool_o)";)
> +       $(__objtool_obj) $(objtool_args) "$(objtool_o)" && { \
> +               $(OBJDUMP) -h "$(objtool_o)" >/dev/null 2>&1 || { \
> +                       echo >&2 "******* objtool generated the invalid file $(objtool_o)"; \
> +                       echo >&2 "******* Update libelf.so or disable the ORC unwinder"; \
> +                       /bin/false; \
> +               }; \
> +       };)
> +
>  objtool_obj = $(if $(patsubst y%,, \
>         $(OBJECT_FILES_NON_STANDARD_$(basetarget).o)$(OBJECT_FILES_NON_STANDARD)n), \
>         $(__objtool_obj))
> 
> Do you want a formal patch?

Hm, I wonder if there's a way for objtool itself to detect it's
producing a bad file.  That would be a bit cleaner.

-- 
Josh

Re: [PATCH] objtool: Don't use -Werror

['Luis Ressel']

On Fri, Apr 05, 2019 at 04:16:14PM +0000, David Laight wrote:

> In which case you should be looking at a way of removing -Wundef
> not removing -Werror.

No, my whole point is that this blacklisting approach doesn't work
outside a controlled dev environment, because different
compilers/compiler versions produce wildy different warnings.

I'm aware of several Linux distros which remove -Werror from build
systems whereever it is encountered. It's a well-known fact among
distributors that enabling -Werror for production builds is a bad idea.

Cheers,
Luis

Re: [PATCH] objtool: Don't use -Werror

['Luis Ressel']

On Fri, Apr 05, 2019 at 06:26:27PM +0200, 'Luis Ressel' wrote:
> On Fri, Apr 05, 2019 at 04:16:14PM +0000, David Laight wrote:
> 
> > In which case you should be looking at a way of removing -Wundef
> > not removing -Werror.
> 
> No, my whole point is that this blacklisting approach doesn't work
> outside a controlled dev environment, because different
> compilers/compiler versions produce wildy different warnings.

Besides, warnings about purely stylistic issues shouldn't break
compilation for end users, even though you probably want to see them as
a dev, so disabling these warning categories completely doesn't work
particularily well.

Regards,
Luis