-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:
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
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
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.
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
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.
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
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)
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.
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)
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.
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)
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/scri...
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/scri...
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)
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/scri...
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.
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
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
linux-stable-mirror@lists.linaro.org
-
'Luis Ressel' -
David Laight -
Josh Poimboeuf -
Luis Ressel