[PATCHv2] Switch to more standard debuginfo generation

[Laura Abbott]

Once upon a time, the kernel needed a lot of special handling to
generate proper debuginfo as the kernel was ahead in technology. These
days, rpm has improved debuginfo support. The kernel has not kept up
with this and it's forward looking calls are now out of date. Switch to
more standard invocations of debuginfo calls.
---
This is a slightly reduced version of the previous patch. It keeps the existing
behavior by using the standard find-debuginfo.sh call with the proper undefines.
---
 kbuild-AFTER_LINK.patch | 126 ------------------------------------------------
 kernel.spec             |  43 +++++------------
 2 files changed, 12 insertions(+), 157 deletions(-)
 delete mode 100644 kbuild-AFTER_LINK.patch

diff --git a/kbuild-AFTER_LINK.patch b/kbuild-AFTER_LINK.patch
deleted file mode 100644
index ab738c62..00000000
--- a/kbuild-AFTER_LINK.patch
+++ /dev/null
@@ -1,126 +0,0 @@
-From 649d991ca7737dd227f2a1ca4f30247daf6a7b4b Mon Sep 17 00:00:00 2001
-From: Roland McGrath <roland(a)redhat.com&gt;
-Date: Mon, 6 Oct 2008 23:03:03 -0700
-Subject: [PATCH] kbuild: AFTER_LINK
-
-If the make variable AFTER_LINK is set, it is a command line to run
-after each final link.  This includes vmlinux itself and vDSO images.
-
-Bugzilla: N/A
-Upstream-status: ??
-
-Signed-off-by: Roland McGrath <roland(a)redhat.com&gt;
----
- arch/arm64/kernel/vdso/Makefile     | 3 ++-
- arch/powerpc/kernel/vdso32/Makefile | 3 ++-
- arch/powerpc/kernel/vdso64/Makefile | 3 ++-
- arch/s390/kernel/vdso32/Makefile    | 3 ++-
- arch/s390/kernel/vdso64/Makefile    | 3 ++-
- arch/x86/entry/vdso/Makefile        | 5 +++--
- scripts/link-vmlinux.sh             | 4 ++++
- 7 files changed, 17 insertions(+), 7 deletions(-)
-
-diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
-index 62c84f7..f44236a 100644
---- a/arch/arm64/kernel/vdso/Makefile
-+++ b/arch/arm64/kernel/vdso/Makefile
-@@ -54,7 +54,8 @@ $(obj-vdso): %.o: %.S FORCE
- 
- # Actual build commands
- quiet_cmd_vdsold = VDSOL   $@
--      cmd_vdsold = $(CC) $(c_flags) -Wl,-n -Wl,-T $^ -o $@
-+      cmd_vdsold = $(CC) $(c_flags) -Wl,-n -Wl,-T $^ -o $@ \
-+				    $(if $(AFTER_LINK),;$(AFTER_LINK))
- quiet_cmd_vdsoas = VDSOA   $@
-       cmd_vdsoas = $(CC) $(a_flags) -c -o $@ $<
- 
-diff --git a/arch/powerpc/kernel/vdso32/Makefile b/arch/powerpc/kernel/vdso32/Makefile
-index 78a7449..c9592c0 100644
---- a/arch/powerpc/kernel/vdso32/Makefile
-+++ b/arch/powerpc/kernel/vdso32/Makefile
-@@ -44,7 +44,8 @@ $(obj-vdso32): %.o: %.S FORCE
- 
- # actual build commands
- quiet_cmd_vdso32ld = VDSO32L $@
--      cmd_vdso32ld = $(CROSS32CC) $(c_flags) -o $@ -Wl,-T$(filter %.lds,$^) $(filter
%.o,$^)
-+      cmd_vdso32ld = $(CROSS32CC) $(c_flags) -o $@ -Wl,-T$(filter %.lds,$^) $(filter
%.o,$^) \
-+		     $(if $(AFTER_LINK),; $(AFTER_LINK))
- quiet_cmd_vdso32as = VDSO32A $@
-       cmd_vdso32as = $(CROSS32CC) $(a_flags) -c -o $@ $<
- 
-diff --git a/arch/powerpc/kernel/vdso64/Makefile b/arch/powerpc/kernel/vdso64/Makefile
-index 31107bf..96aded3 100644
---- a/arch/powerpc/kernel/vdso64/Makefile
-+++ b/arch/powerpc/kernel/vdso64/Makefile
-@@ -33,7 +33,8 @@ $(obj)/%.so: $(obj)/%.so.dbg FORCE
- 
- # actual build commands
- quiet_cmd_vdso64ld = VDSO64L $@
--      cmd_vdso64ld = $(CC) $(c_flags) -o $@ -Wl,-T$(filter %.lds,$^) $(filter %.o,$^)
-+      cmd_vdso64ld = $(CC) $(c_flags) -o $@ -Wl,-T$(filter %.lds,$^) $(filter %.o,$^) \
-+		    $(if $(AFTER_LINK),; $(AFTER_LINK))
- 
- # install commands for the unstripped file
- quiet_cmd_vdso_install = INSTALL $@
-diff --git a/arch/s390/kernel/vdso32/Makefile b/arch/s390/kernel/vdso32/Makefile
-index 6cc9478..94fb536 100644
---- a/arch/s390/kernel/vdso32/Makefile
-+++ b/arch/s390/kernel/vdso32/Makefile
-@@ -46,7 +46,8 @@ $(obj-vdso32): %.o: %.S
- 
- # actual build commands
- quiet_cmd_vdso32ld = VDSO32L $@
--      cmd_vdso32ld = $(CC) $(c_flags) -Wl,-T $^ -o $@
-+      cmd_vdso32ld = $(CC) $(c_flags) -Wl,-T $^ -o $@ \
-+		     $(if $(AFTER_LINK),; $(AFTER_LINK))
- quiet_cmd_vdso32as = VDSO32A $@
-       cmd_vdso32as = $(CC) $(a_flags) -c -o $@ $<
- 
-diff --git a/arch/s390/kernel/vdso64/Makefile b/arch/s390/kernel/vdso64/Makefile
-index 2d54c18..a0e3e9d 100644
---- a/arch/s390/kernel/vdso64/Makefile
-+++ b/arch/s390/kernel/vdso64/Makefile
-@@ -46,7 +46,8 @@ $(obj-vdso64): %.o: %.S
- 
- # actual build commands
- quiet_cmd_vdso64ld = VDSO64L $@
--      cmd_vdso64ld = $(CC) $(c_flags) -Wl,-T $^ -o $@
-+      cmd_vdso64ld = $(CC) $(c_flags) -Wl,-T $^ -o $@ \
-+		     $(if $(AFTER_LINK),; $(AFTER_LINK))
- quiet_cmd_vdso64as = VDSO64A $@
-       cmd_vdso64as = $(CC) $(a_flags) -c -o $@ $<
- 
-diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
-index d540966..eeb47b6 100644
---- a/arch/x86/entry/vdso/Makefile
-+++ b/arch/x86/entry/vdso/Makefile
-@@ -167,8 +167,9 @@ $(obj)/vdso32.so.dbg: FORCE \
- quiet_cmd_vdso = VDSO    $@
-       cmd_vdso = $(CC) -nostdlib -o $@ \
- 		       $(VDSO_LDFLAGS) $(VDSO_LDFLAGS_$(filter %.lds,$(^F))) \
--		       -Wl,-T,$(filter %.lds,$^) $(filter %.o,$^) && \
--		 sh $(srctree)/$(src)/checkundef.sh '$(NM)' '$@'
-+		       -Wl,-T,$(filter %.lds,$^) $(filter %.o,$^) \
-+		$(if $(AFTER_LINK),; $(AFTER_LINK)) && \
-+		sh $(srctree)/$(src)/checkundef.sh '$(NM)' '$@'
- 
- VDSO_LDFLAGS = -fPIC -shared $(call cc-ldoption, -Wl$(comma)--hash-style=both) \
- 	$(call cc-ldoption, -Wl$(comma)--build-id) -Wl,-Bsymbolic $(LTO_CFLAGS)
-diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
-index f742c65..526eee4 100755
---- a/scripts/link-vmlinux.sh
-+++ b/scripts/link-vmlinux.sh
-@@ -111,6 +111,10 @@ vmlinux_link()
- 			-lutil -lrt -lpthread
- 		rm -f linux
- 	fi
-+	if [ -n "${AFTER_LINK}" ]; then
-+		/usr/lib/rpm/debugedit -b ${RPM_BUILD_DIR} -d /usr/src/debug -i ${2} \
-+			> ${2}.id
-+	fi
- }
- 
- 
--- 
-2.7.4
-
diff --git a/kernel.spec b/kernel.spec
index 78f692e9..97d8d464 100644
--- a/kernel.spec
+++ b/kernel.spec
@@ -395,7 +395,14 @@ BuildRequires: pciutils-devel gettext ncurses-devel
 BuildConflicts: rhbuildsys(DiskFree) < 500Mb
 %if %{with_debuginfo}
 BuildRequires: rpm-build, elfutils
-%define debuginfo_args --strict-build-id -r
+# Most of these should be enabled after more investigation
+%undefine _include_minidebuginfo
+%undefine _find_debuginfo_dwz_opts
+%undefine _unique_build_ids
+%undefine _unique_debug_names
+%undefine _unique_debug_srcs
+%global _find_debuginfo_opts -r
+%global _missing_build_ids_terminate_build 1
 %endif
 
 %if %{signkernel}%{signmodules}
@@ -492,9 +499,6 @@ Source5000: patch-4.%{base_sublevel}-git%{gitrev}.xz
 
 ## Patches needed for building this package
 
-# build tweak for build ID magic, even for -vanilla
-Patch001: kbuild-AFTER_LINK.patch
-
 ## compile fixes
 
 # ongoing complaint, full discussion delayed until ksummit/plumbers
@@ -705,7 +709,7 @@ This package provides debug information for the perf package.
 # symlinks because of the trailing nonmatching alternation and
 # the leading .*, because of find-debuginfo.sh's buggy handling
 # of matching the pattern against the symlinks file.
-%{expand:%%global debuginfo_args %{?debuginfo_args} -p
'.*%%{_bindir}/perf(\.debug)?|.*%%{_libexecdir}/perf-core/.*|.*%%{_libdir}/traceevent/plugins/.*|XXX'
-o perf-debuginfo.list}
+%{expand:%%global _find_debuginfo_opts %{?_find_debuginfo_opts} -p
'.*%%{_bindir}/perf(\.debug)?|.*%%{_libexecdir}/perf-core/.*|.*%%{_libdir}/traceevent/plugins/.*|XXX'
-o perf-debuginfo.list}
 
 %package -n python-perf
 Summary: Python bindings for apps which will manipulate perf events
@@ -726,7 +730,7 @@ AutoReqProv: no
 This package provides debug information for the perf python bindings.
 
 # the python_sitearch macro should already be defined from above
-%{expand:%%global debuginfo_args %{?debuginfo_args} -p
'.*%%{python_sitearch}/perf.so(\.debug)?|XXX' -o python-perf-debuginfo.list}
+%{expand:%%global _find_debuginfo_opts %{?_find_debuginfo_opts} -p
'.*%%{python_sitearch}/perf.so(\.debug)?|XXX' -o python-perf-debuginfo.list}
 
 
 %endif # with_perf
@@ -781,7 +785,7 @@ This package provides debug information for package kernel-tools.
 # symlinks because of the trailing nonmatching alternation and
 # the leading .*, because of find-debuginfo.sh's buggy handling
 # of matching the pattern against the symlinks file.
-%{expand:%%global debuginfo_args %{?debuginfo_args} -p
'.*%%{_bindir}/centrino-decode(\.debug)?|.*%%{_bindir}/powernow-k8-decode(\.debug)?|.*%%{_bindir}/cpupower(\.debug)?|.*%%{_libdir}/libcpupower.*|.*%%{_bindir}/turbostat(\.debug)?|.*%%{_bindir}/x86_energy_perf_policy(\.debug)?|.*%%{_bindir}/tmon(\.debug)?|.*%%{_bindir}/lsgpio(\.debug)?|.*%%{_bindir}/gpio-hammer(\.debug)?|.*%%{_bindir}/gpio-event-mon(\.debug)?|.*%%{_bindir}/iio_event_monitor(\.debug)?|.*%%{_bindir}/iio_generic_buffer(\.debug)?|.*%%{_bindir}/lsiio(\.debug)?|XXX'
-o kernel-tools-debuginfo.list}
+%{expand:%%global _find_debuginfo_opts %{?_find_debuginfo_opts} -p
'.*%%{_bindir}/centrino-decode(\.debug)?|.*%%{_bindir}/powernow-k8-decode(\.debug)?|.*%%{_bindir}/cpupower(\.debug)?|.*%%{_libdir}/libcpupower.*|.*%%{_bindir}/turbostat(\.debug)?|.*%%{_bindir}/x86_energy_perf_policy(\.debug)?|.*%%{_bindir}/tmon(\.debug)?|.*%%{_bindir}/lsgpio(\.debug)?|.*%%{_bindir}/gpio-hammer(\.debug)?|.*%%{_bindir}/gpio-event-mon(\.debug)?|.*%%{_bindir}/iio_event_monitor(\.debug)?|.*%%{_bindir}/iio_generic_buffer(\.debug)?|.*%%{_bindir}/lsiio(\.debug)?|XXX'
-o kernel-tools-debuginfo.list}
 
 %endif # with_tools
 
@@ -801,7 +805,7 @@ AutoReqProv: no\
 %description %{?1:%{1}-}debuginfo\
 This package provides debug information for package %{name}%{?1:-%{1}}.\
 This is required to use SystemTap with %{name}%{?1:-%{1}}-%{KVERREL}.\
-%{expand:%%global debuginfo_args %{?debuginfo_args} -p
'/.*/%%{KVERREL}%{?1:[+]%{1}}/.*|/.*%%{KVERREL}%{?1:\+%{1}}(\.debug)?' -o
debuginfo%{?1}.list}\
+%{expand:%%global _find_debuginfo_opts %{?_find_debuginfo_opts} -p
'/.*/%%{KVERREL}%{?1:[+]%{1}}/.*|/.*%%{KVERREL}%{?1:\+%{1}}(\.debug)?' -o
debuginfo%{?1}.list}\
 %{nil}
 
 #
@@ -1282,18 +1286,6 @@ cd ..
 %define sparse_mflags	C=1
 %endif
 
-%if %{with_debuginfo}
-# This override tweaks the kernel makefiles so that we run debugedit on an
-# object before embedding it.  When we later run find-debuginfo.sh, it will
-# run debugedit again.  The edits it does change the build ID bits embedded
-# in the stripped object, but repeating debugedit is a no-op.  We do it
-# beforehand to get the proper final build ID bits into the embedded image.
-# This affects the vDSO images in vmlinux, and the vmlinux image in bzImage.
-export AFTER_LINK=\
-'sh -xc "/usr/lib/rpm/debugedit -b $$RPM_BUILD_DIR -d /usr/src/debug \
-                                -i $@ > $@.id"'
-%endif
-
 cp_vmlinux()
 {
   eu-strip --remove-comment -o "$2" "$1"
@@ -1505,13 +1497,6 @@ BuildKernel() {
     cp $RPM_BUILD_ROOT/lib/modules/$KernelVer/build/.config
$RPM_BUILD_ROOT/lib/modules/$KernelVer/build/include/config/auto.conf
 
 %if %{with_debuginfo}
-    if test -s vmlinux.id; then
-      cp vmlinux.id $RPM_BUILD_ROOT/lib/modules/$KernelVer/build/vmlinux.id
-    else
-      echo >&2 "*** ERROR *** no vmlinux build ID! ***"
-      exit 1
-    fi
-
     #
     # save the vmlinux file for kernel debugging into the kernel-debuginfo rpm
     #
@@ -1747,10 +1732,6 @@ popd
 
 %if %{with_debuginfo}
 
-%define __debug_install_post \
-  /usr/lib/rpm/find-debuginfo.sh %{debuginfo_args} %{_builddir}/%{?buildsubdir}\
-%{nil}
-
 %ifnarch noarch
 %global __debug_package 1
 %files -f debugfiles.list debuginfo-common-%{_target_cpu}
-- 
2.12.2

[Don Zickus]

On Mon, Apr 10, 2017 at 06:22:06PM -0700, Laura Abbott wrote:
>  Once upon a time, the kernel needed a lot of special handling to
>  generate proper debuginfo as the kernel was ahead in technology. These
>  days, rpm has improved debuginfo support. The kernel has not kept up
>  with this and it's forward looking calls are now out of date. Switch to
>  more standard invocations of debuginfo calls.
>  ---
>  This is a slightly reduced version of the previous patch. It keeps the existing
>  behavior by using the standard find-debuginfo.sh call with the proper undefines. 
Thanks Laura!  Sorry about the previous version.

Acked-by: Don Zickus <dzickus(a)redhat.com&gt;


Mark W.,

Do you have much insight into how the below definitions would interact with
the kernel?

>  @@ -395,7 +395,14 @@ BuildRequires: pciutils-devel gettext
> ncurses-devel
>   BuildConflicts: rhbuildsys(DiskFree) < 500Mb
>   %if %{with_debuginfo}
>   BuildRequires: rpm-build, elfutils
>  -%define debuginfo_args --strict-build-id -r
>  +# Most of these should be enabled after more investigation
>  +%undefine _include_minidebuginfo
>  +%undefine _find_debuginfo_dwz_opts
>  +%undefine _unique_build_ids
>  +%undefine _unique_debug_names
>  +%undefine _unique_debug_srcs
>  +%global _find_debuginfo_opts -r
>  +%global _missing_build_ids_terminate_build 1
>   %endif 
I am only poking you because it seems like you spent a lot of time cleaning
up some of the rpm macros in the rpm pkg.

Cheers,
Don

[Mark Wielaard]

Hi,

On Tue, 2017-04-11 at 11:01 -0400, Don Zickus wrote:
>  Mark W.,
>  
>  Do you have much insight into how the below definitions would interact with
>  the kernel?
>  
>  > @@ -395,7 +395,14 @@ BuildRequires: pciutils-devel gettext ncurses-devel
>  >  BuildConflicts: rhbuildsys(DiskFree) < 500Mb
>  >  %if %{with_debuginfo}
>  >  BuildRequires: rpm-build, elfutils
>  > -%define debuginfo_args --strict-build-id -r 
I am happy you seem able to use the defines instead of having to
override the whole find-debuginfo call. Please let me know if there are
other defines you need/want.

>  > +# Most of these should be enabled after more investigation
>  > +%undefine _include_minidebuginfo 
This prevents adding a .gnu_debugdata section to the binaries which
contains a minisymtab with just the function symbols/addresses. This is
not really relevant for the kernel and modules. If added to
binaries/shared libraries it allows some tools to show function names
for more addresses even without having any debuginfo installed.

>  > +%undefine _find_debuginfo_dwz_opts 
This prevents running dwz (the DWARF compressor) which doesn't work for
the kernel and kernel modules. The problem is that kernel modules are
not normal ET_EXEC or ET_DYN, but look like ET_REL object files. object
files contain relocations between ELF sections that dwz cannot handle.
It would be nice to fix this because kernel modules are ideal for DWARF
deduplication. They all contain the same basic debug type information
that could be shared. eu-strip --reloc-debug-sections (-r, see below)
could help with this, but hasn't been integrated. I'll try to make some
time (no promises!) to look at it, because getting dwz working against
the kernel modules would probably provide a huge saving.

>  > +%undefine _unique_build_ids
>  > +%undefine _unique_debug_names
>  > +%undefine _unique_debug_srcs 
It is somewhat unfortunate these don't work with the kernel build. The
idea is that the package nvra is used to make all of the above unique
between all versions of the package/debuginfo.

If I understand correctly we "uniqueify" too late assuming nothing
depends on the build-id till we start handling the debuginfo. But the
kernel builds seem to already pick out build-ids to store and embeds
some ELF images in other ELF images and then those build-ids shouldn't
be touched anymore. The AFTER_LINK hack was used to counter this, but it
is somewhat fragile/doesn't really seem to interact correctly anymore.

If you don't have _unique_build_ids you should also set:
%global _build_id_links alldebug
to make sure build-ids are only used for the debuginfo packages
(otherwise your main packages stops being parallel installable, which
the kernel needs).

I think _unique_debug_names and _unique_debug_srcs should in theory
still work. But they aren't very useful if the build-ids aren't unique.

>  > +%global _find_debuginfo_opts -r 
This was specifically designed for the kernel. It makes sure relocations
between debug sections are removed in ET_REL files. Which normally would
not be correct if those ET_REL object files would be linked together
again. But kernel modules are not real/normal object files. It should
save a couple of 100MBs when installing the debuginfo for the .ko files.
(As said above we really should figure out a way to combine this with
dwz processing so we can safe even more space.)

>  > +%global _missing_build_ids_terminate_build 1 
Yes please. It should show if any ELF file rpm processes doesn't have a
build-id, which would be a bug in the toolchain.

>  I am only poking you because it seems like you spent a lot of time
> cleaning
>  up some of the rpm macros in the rpm pkg. 
Thanks. I am really interested in this. I did see one of the earlier
patches, but missed that I had been CCed on the followups. My email
setup is a bit of a mess currently. My employer recently decided on
short notice that my engineering group didn't need normal email, removed
all our filters and forced us to adopt a "email provider" that
"helpfully" removes any emails it believes you have already received and
that hides everything behind a proprietary web frontend. Sigh. So I had
to switch email addresses. Sorry.

Cheers,

Mark

[Laura Abbott]

On 04/12/2017 03:55 AM, Mark Wielaard wrote:
>  Hi,
>  
>  On Tue, 2017-04-11 at 11:01 -0400, Don Zickus wrote:
> > Mark W.,
> >
> > Do you have much insight into how the below definitions would interact with
> > the kernel?
> >
> >> @@ -395,7 +395,14 @@ BuildRequires: pciutils-devel gettext ncurses-devel
> >>  BuildConflicts: rhbuildsys(DiskFree) < 500Mb
> >>  %if %{with_debuginfo}
> >>  BuildRequires: rpm-build, elfutils
> >> -%define debuginfo_args --strict-build-id -r
>  
>  I am happy you seem able to use the defines instead of having to
>  override the whole find-debuginfo call. Please let me know if there are
>  other defines you need/want.
>   
I brought this up in the previous version but a macro for the
default arguments to --ver-rel and --unique-debug-arch would
be useful for packages like kernel that do filtering with
-p. My previous solution required knowing what the arguments
are to filter.

> >> +# Most of these should be enabled after more investigation
> >> +%undefine _include_minidebuginfo
>  
>  This prevents adding a .gnu_debugdata section to the binaries which
>  contains a minisymtab with just the function symbols/addresses. This is
>  not really relevant for the kernel and modules. If added to
>  binaries/shared libraries it allows some tools to show function names
>  for more addresses even without having any debuginfo installed.
>   
This was explicitly causing problems for me when I was working.
I think it was because it expected to find dynamic symbols
and there were none.

> >> +%undefine _find_debuginfo_dwz_opts
>  
>  This prevents running dwz (the DWARF compressor) which doesn't work for
>  the kernel and kernel modules. The problem is that kernel modules are
>  not normal ET_EXEC or ET_DYN, but look like ET_REL object files. object
>  files contain relocations between ELF sections that dwz cannot handle.
>  It would be nice to fix this because kernel modules are ideal for DWARF
>  deduplication. They all contain the same basic debug type information
>  that could be shared. eu-strip --reloc-debug-sections (-r, see below)
>  could help with this, but hasn't been integrated. I'll try to make some
>  time (no promises!) to look at it, because getting dwz working against
>  the kernel modules would probably provide a huge saving.
>  
> >> +%undefine _unique_build_ids
> >> +%undefine _unique_debug_names
> >> +%undefine _unique_debug_srcs
>  
>  It is somewhat unfortunate these don't work with the kernel build. The
>  idea is that the package nvra is used to make all of the above unique
>  between all versions of the package/debuginfo.
>   
These were working from what I could see, there was just some
discussion about why we didn't have it for everything so I went
with a smaller change first.

>  If I understand correctly we "uniqueify" too late assuming
> nothing
>  depends on the build-id till we start handling the debuginfo. But the
>  kernel builds seem to already pick out build-ids to store and embeds
>  some ELF images in other ELF images and then those build-ids shouldn't
>  be touched anymore. The AFTER_LINK hack was used to counter this, but it
>  is somewhat fragile/doesn't really seem to interact correctly anymore.
>   
Thanks for the history. I'm a bit concerned we shouldn't actually
be dropping the AFTER_LINK call even if everything seems to be
generated okay. Do you have a suggestion of something more specific
I can check besides throwing selected modules into gdb?

I'm really tempted to just throw this patch in and see what happens
but I also don't want to completely break anything unless I have
to.

>  If you don't have _unique_build_ids you should also set:
>  %global _build_id_links alldebug
>  to make sure build-ids are only used for the debuginfo packages
>  (otherwise your main packages stops being parallel installable, which
>  the kernel needs).
>   
Yes, that's still there after you suggested it before.

>  I think _unique_debug_names and _unique_debug_srcs should in theory
>  still work. But they aren't very useful if the build-ids aren't unique.
>  
> >> +%global _find_debuginfo_opts -r
>  
>  This was specifically designed for the kernel. It makes sure relocations
>  between debug sections are removed in ET_REL files. Which normally would
>  not be correct if those ET_REL object files would be linked together
>  again. But kernel modules are not real/normal object files. It should
>  save a couple of 100MBs when installing the debuginfo for the .ko files.
>  (As said above we really should figure out a way to combine this with
>  dwz processing so we can safe even more space.)
>  
> >> +%global _missing_build_ids_terminate_build 1
>  
>  Yes please. It should show if any ELF file rpm processes doesn't have a
>  build-id, which would be a bug in the toolchain.
>  
> > I am only poking you because it seems like you spent a lot of time cleaning
> > up some of the rpm macros in the rpm pkg.
>  
>  Thanks. I am really interested in this. I did see one of the earlier
>  patches, but missed that I had been CCed on the followups. My email
>  setup is a bit of a mess currently. My employer recently decided on
>  short notice that my engineering group didn't need normal email, removed
>  all our filters and forced us to adopt a "email provider" that
>  "helpfully" removes any emails it believes you have already received and
>  that hides everything behind a proprietary web frontend. Sigh. So I had
>  to switch email addresses. Sorry. 
No problem. Thanks for replying.

>  
>  Cheers,
>  
>  Mark
>  _______________________________________________
>  kernel mailing list -- kernel(a)lists.fedoraproject.org
>  To unsubscribe send an email to kernel-leave(a)lists.fedoraproject.org
>

[Mark Wielaard]

On Wed, 2017-04-12 at 10:20 -0700, Laura Abbott wrote:
>  On 04/12/2017 03:55 AM, Mark Wielaard wrote:
>  > I am happy you seem able to use the defines instead of having to
>  > override the whole find-debuginfo call. Please let me know if there are
>  > other defines you need/want.
>  > 
>  
>  I brought this up in the previous version but a macro for the
>  default arguments to --ver-rel and --unique-debug-arch would
>  be useful for packages like kernel that do filtering with
>  -p. My previous solution required knowing what the arguments
>  are to filter. 
Aha. I see what you are doing. You are creating separate debuginfo file
lists for the separate subpackages. And with unique debug name it gets
hard to predict which .debug file names to match.

The good news is that we are working on doing that automagically from
rpmbuild (and then you also get a separate debugsources package). But I
have to see if it will match your specific split.

For now I would indeed recommend to turn off _unique_debug_names and
_unique_debug_srcs to not have to rely on the exact ver-rel-arch triple
it will generate.

>  >>> +# Most of these should be enabled after more
> investigation
>  >>> +%undefine _include_minidebuginfo
>  > 
>  > This prevents adding a .gnu_debugdata section to the binaries which
>  > contains a minisymtab with just the function symbols/addresses. This is
>  > not really relevant for the kernel and modules. If added to
>  > binaries/shared libraries it allows some tools to show function names
>  > for more addresses even without having any debuginfo installed.
>  > 
>  This was explicitly causing problems for me when I was working.
>  I think it was because it expected to find dynamic symbols
>  and there were none. 
I'll do a kernel build with it enabled and see if I can figure out why
it is complaining. The support is a little crude with some fairly
generic sed matches that I can certainly believe might fail in various
corner cases.

It isn't super important I think, so just leave it off for now.

>  > If I understand correctly we "uniqueify" too late
> assuming nothing
>  > depends on the build-id till we start handling the debuginfo. But the
>  > kernel builds seem to already pick out build-ids to store and embeds
>  > some ELF images in other ELF images and then those build-ids shouldn't
>  > be touched anymore. The AFTER_LINK hack was used to counter this, but it
>  > is somewhat fragile/doesn't really seem to interact correctly anymore.
>  > 
>  
>  Thanks for the history. I'm a bit concerned we shouldn't actually
>  be dropping the AFTER_LINK call even if everything seems to be
>  generated okay. Do you have a suggestion of something more specific
>  I can check besides throwing selected modules into gdb? 
One tricky one is the build-id of the vdso. The vdso is inserted in the
process by the kernel from the kernel image, the debugger can find the
matching vdso.debug by checking the build-id (and following the
symlink). You could check if the version found in a running process is
the same as the one in the vdso.debug file on disk.

$ eu-unstrip -n -p $$ | grep vdso
0x7ffe4a89a000+0x2000 50d1ccf000162361f74a9d7c2ecea856f7881f07@0x7ffe4a89a340 .
/usr/lib/debug/usr/lib/modules/3.10.0-514.10.2.el7.x86_64/vdso/vdso.so.debug [vdso:
29852]

$ eu-readelf -n /usr/lib/debug/lib/modules/`uname -r`/vdso/vdso.so.debug | grep
"Build ID"
    Build ID: 50d1ccf000162361f74a9d7c2ecea856f7881f07

The above matches, but that is on RHEL. I guess I really should be
running Fedora :)

To be absolutely sure I think you need an explicit way to tell not to
touch build-ids ever. Currently it is hardcoded to always check and
update them. There is no way to say that you only want to check they are
there, but never to update them because you know what you are doing. I
think it is this way because originally we couldn't rely on the
toolchain to generate the build-ids correctly, so we would always use
the debuginfo to calculate one. But now with
_missing_build_ids_terminate_build you can rely on them I think (well
not, if we want to uniqueify them against nvra build, but we are not
doing that for the kernel for now). I'll propose some update upstream so
you can set _do_not_update_build_ids or something.

The only issue with that is that because it is currently hard-coded to
always update, that would only work for rawhide. Which would make the
spec file fedora version specific. Is that a problem?

Cheers,

Mark

[Laura Abbott]

On 04/12/2017 01:16 PM, Mark Wielaard wrote:
>  On Wed, 2017-04-12 at 10:20 -0700, Laura Abbott wrote:
> > On 04/12/2017 03:55 AM, Mark Wielaard wrote:
> >> I am happy you seem able to use the defines instead of having to
> >> override the whole find-debuginfo call. Please let me know if there are
> >> other defines you need/want.
> >>
> >
> > I brought this up in the previous version but a macro for the
> > default arguments to --ver-rel and --unique-debug-arch would
> > be useful for packages like kernel that do filtering with
> > -p. My previous solution required knowing what the arguments
> > are to filter.
>  
>  Aha. I see what you are doing. You are creating separate debuginfo file
>  lists for the separate subpackages. And with unique debug name it gets
>  hard to predict which .debug file names to match.
>  
>  The good news is that we are working on doing that automagically from
>  rpmbuild (and then you also get a separate debugsources package). But I
>  have to see if it will match your specific split.
>  
>  For now I would indeed recommend to turn off _unique_debug_names and
>  _unique_debug_srcs to not have to rely on the exact ver-rel-arch triple
>  it will generate.
>  
> >>>> +# Most of these should be enabled after more investigation
> >>>> +%undefine _include_minidebuginfo
> >>
> >> This prevents adding a .gnu_debugdata section to the binaries which
> >> contains a minisymtab with just the function symbols/addresses. This is
> >> not really relevant for the kernel and modules. If added to
> >> binaries/shared libraries it allows some tools to show function names
> >> for more addresses even without having any debuginfo installed.
> >>
> > This was explicitly causing problems for me when I was working.
> > I think it was because it expected to find dynamic symbols
> > and there were none.
>  
>  I'll do a kernel build with it enabled and see if I can figure out why
>  it is complaining. The support is a little crude with some fairly
>  generic sed matches that I can certainly believe might fail in various
>  corner cases.
>  
>  It isn't super important I think, so just leave it off for now.
>  
> >> If I understand correctly we "uniqueify" too late assuming nothing
> >> depends on the build-id till we start handling the debuginfo. But the
> >> kernel builds seem to already pick out build-ids to store and embeds
> >> some ELF images in other ELF images and then those build-ids shouldn't
> >> be touched anymore. The AFTER_LINK hack was used to counter this, but it
> >> is somewhat fragile/doesn't really seem to interact correctly anymore.
> >>
> >
> > Thanks for the history. I'm a bit concerned we shouldn't actually
> > be dropping the AFTER_LINK call even if everything seems to be
> > generated okay. Do you have a suggestion of something more specific
> > I can check besides throwing selected modules into gdb?
>  
>  One tricky one is the build-id of the vdso. The vdso is inserted in the
>  process by the kernel from the kernel image, the debugger can find the
>  matching vdso.debug by checking the build-id (and following the
>  symlink). You could check if the version found in a running process is
>  the same as the one in the vdso.debug file on disk.
>  
>  $ eu-unstrip -n -p $$ | grep vdso
>  0x7ffe4a89a000+0x2000 50d1ccf000162361f74a9d7c2ecea856f7881f07@0x7ffe4a89a340 .
> /usr/lib/debug/usr/lib/modules/3.10.0-514.10.2.el7.x86_64/vdso/vdso.so.debug [vdso:
> 29852]
>  
>  $ eu-readelf -n /usr/lib/debug/lib/modules/`uname -r`/vdso/vdso.so.debug | grep
> "Build ID"
>      Build ID: 50d1ccf000162361f74a9d7c2ecea856f7881f07
>  
>  The above matches, but that is on RHEL. I guess I really should be
>  running Fedora :)
>   
Yeah it doesn't match with my patches, so much for my optimism :(

>  To be absolutely sure I think you need an explicit way to tell not
> to
>  touch build-ids ever. Currently it is hardcoded to always check and
>  update them. There is no way to say that you only want to check they are
>  there, but never to update them because you know what you are doing. I
>  think it is this way because originally we couldn't rely on the
>  toolchain to generate the build-ids correctly, so we would always use
>  the debuginfo to calculate one. But now with
>  _missing_build_ids_terminate_build you can rely on them I think (well
>  not, if we want to uniqueify them against nvra build, but we are not
>  doing that for the kernel for now). I'll propose some update upstream so
>  you can set _do_not_update_build_ids or something. 
Sounds good.

>  
>  The only issue with that is that because it is currently hard-coded to
>  always update, that would only work for rawhide. Which would make the
>  spec file fedora version specific. Is that a problem? 
I don't think so as long as everything follows the usual branching
policy.

I'm going to look at a v3 that keeps the AFTER_LINK but still moves
to a more default invocation of find-debuginfo.sh 

>  
>  Cheers,
>  
>  Mark
>   
Thanks,
Laura

[Mark Wielaard]

On Thu, Apr 13, 2017 at 06:59:38PM -0700, Laura Abbott wrote:
>  >> This was explicitly causing problems for me when I was
> working.
>  >> I think it was because it expected to find dynamic symbols
>  >> and there were none.
>  > 
>  > I'll do a kernel build with it enabled and see if I can figure out why
>  > it is complaining. The support is a little crude with some fairly
>  > generic sed matches that I can certainly believe might fail in various
>  > corner cases.
>  > 
>  > It isn't super important I think, so just leave it off for now. 
Found the issue and posted a patch upstream:
http://lists.rpm.org/pipermail/rpm-maint/2017-April/005441.html
That makes sure the minisymtab support skips kernel modules, which
is what generated the errors.

>  > One tricky one is the build-id of the vdso. The vdso is inserted
> in the
>  > process by the kernel from the kernel image, the debugger can find the
>  > matching vdso.debug by checking the build-id (and following the
>  > symlink). You could check if the version found in a running process is
>  > the same as the one in the vdso.debug file on disk.
>  > 
>  > $ eu-unstrip -n -p $$ | grep vdso
>  > 0x7ffe4a89a000+0x2000 50d1ccf000162361f74a9d7c2ecea856f7881f07@0x7ffe4a89a340 .
> /usr/lib/debug/usr/lib/modules/3.10.0-514.10.2.el7.x86_64/vdso/vdso.so.debug [vdso:
> 29852]
>  > 
>  > $ eu-readelf -n /usr/lib/debug/lib/modules/`uname -r`/vdso/vdso.so.debug | grep
> "Build ID"
>  >     Build ID: 50d1ccf000162361f74a9d7c2ecea856f7881f07
>  > 
>  > The above matches, but that is on RHEL. I guess I really should be
>  > running Fedora :)
>  > 
>  
>  Yeah it doesn't match with my patches, so much for my optimism :( 
You need rpmbuild/debugedit to NOT touch build-ids ever. I wrote a
patch for that so when a package sets %global _no_recompute_build_ids 1
it will not do that. Patch also submitted upstream:
http://lists.rpm.org/pipermail/rpm-maint/2017-April/005442.html

With that the above does match for me.

When upstream accepts both patches I will add them to the fedora rpm.
If you are interested in testing with them you can find a scratch build
here: https://koji.fedoraproject.org/koji/taskinfo?taskID=19029567

>  I'm going to look at a v3 that keeps the AFTER_LINK but still
> moves
>  to a more default invocation of find-debuginfo.sh  
I think it would be really nice if we could drop the AFTER_LINK patch
it looks really intrusive and messy. I think we can drop it with the
above two patches to rpm. Then there are only 2 issues left as far as
I can see:

1) The kbuild-AFTER_LINK.patch generated a file that contained vmlinux
   buildid in $RPM_BUILD_ROOT/lib/modules/$KernelVer/build/vmlinux.id
   With the _no_recompute_build_ids setting that should be easy to add
   back with some simple scripting (systemtap seems to check that file).
   eu-readelf -n vmlinux | grep "Build ID" | awk '{print $NF}' >
vmlinux.id

2) The build-id symlinks for the kernel modules are not there.
   But... This is already broken in rawhide. I "fixed" rpmbuild to
   generate those after the file list was generated instead of through
   find-debuginfo.sh. This is too late for the kernel because it will
   xz compress all .ko files and rpmbuild doesn't know it should handle
   .ko.xz files too. Surprisingly this is already partly broken in f25.
   The build-id symlinks are there, but they point to non-existing files
   because after the symlink was created to the .ko file the file gets
   compressed to .ko.xz. I am slightly surprised this hasn't broken
   more things like systemtap which should rely on the build-ids. But it
   might fall back to finding the modules by name.

   Anyway, this is something that was broken without the AFTER_LINK patch
   removal. So it needs a separate fix. I think I know a trick. Instead
   of just compressing each .ko file with xz we should add a script that
   at the same time generates the symlinks. I'll try to come up with a
   patch for that.

Cheers,

Mark

[Mark Wielaard]

On Sun, Apr 16, 2017 at 08:04:18PM +0200, Mark Wielaard wrote:
>     Anyway, this is something that was broken without the AFTER_LINK
> patch
>     removal. So it needs a separate fix. I think I know a trick. Instead
>     of just compressing each .ko file with xz we should add a script that
>     at the same time generates the symlinks. I'll try to come up with a
>     patch for that. 
Haha, I said that before realizing the intricate ways the files lists
for core, modules, extra arch variants are generated. It seems it isn't
as simple as I thought to augment those lists with the correct build-id
symlinks. It looks like it would be easier, and nicer, to make rpmbuild
do that by somehow teaching it to look into the .ko.xz files. The downside
to that is that we then have to uncompress them again in rpm. Maybe we
can do some automagic compression at the same time? So looks like I don't
have a very simple solution yet.

BTW. Random observation. The kernel.spec does:
  find $RPM_BUILD_ROOT/lib/modules/ -type f -name '*.ko' | xargs xz
Might it be efficient to do some of that in parallel?
  find $RPM_BUILD_ROOT/lib/modules/ -type f -name '*.ko' \
    | xargs -n16 -P$(getconf _NPROCESSORS_ONLN) xz -T1
This assumes the .ko files are too small to parallel compress themselves.

Cheers,

Mark

[Mark Wielaard]

On Sun, 2017-04-16 at 20:04 +0200, Mark Wielaard wrote:
>  Found the issue and posted a patch upstream:
>  http://lists.rpm.org/pipermail/rpm-maint/2017-April/005441.html
>  That makes sure the minisymtab support skips kernel modules, which
>  is what generated the errors.
>  [...]
>  You need rpmbuild/debugedit to NOT touch build-ids ever. I wrote a
>  patch for that so when a package sets %global _no_recompute_build_ids 1
>  it will not do that. Patch also submitted upstream:
>  http://lists.rpm.org/pipermail/rpm-maint/2017-April/005442.html
>  
>  With that the above does match for me.
>  
>  When upstream accepts both patches I will add them to the fedora rpm.
>  If you are interested in testing with them you can find a scratch build
>  here: https://koji.fedoraproject.org/koji/taskinfo?taskID=19029567 
Both patches were accepted upstream and I added them to the fedora
rawhide rpm package (rpm-4.13.0.1-19.fc27).

Cheers,

Mark

[Laura Abbott]

On 04/19/2017 04:22 AM, Mark Wielaard wrote:
>  On Sun, 2017-04-16 at 20:04 +0200, Mark Wielaard wrote:
> > Found the issue and posted a patch upstream:
> > http://lists.rpm.org/pipermail/rpm-maint/2017-April/005441.html
> > That makes sure the minisymtab support skips kernel modules, which
> > is what generated the errors.
> > [...]
> > You need rpmbuild/debugedit to NOT touch build-ids ever. I wrote a
> > patch for that so when a package sets %global _no_recompute_build_ids 1
> > it will not do that. Patch also submitted upstream:
> > http://lists.rpm.org/pipermail/rpm-maint/2017-April/005442.html
> >
> > With that the above does match for me.
> >
> > When upstream accepts both patches I will add them to the fedora rpm.
> > If you are interested in testing with them you can find a scratch build
> > here: https://koji.fedoraproject.org/koji/taskinfo?taskID=19029567
>  
>  Both patches were accepted upstream and I added them to the fedora
>  rawhide rpm package (rpm-4.13.0.1-19.fc27).
>  
>  Cheers,
>  
>  Mark 
Thanks! I did a test with your scratch build and the results look good.
I'll send a v3 based on that.

Thanks,
Laura

[Don Zickus]

On Wed, Apr 12, 2017 at 12:55:34PM +0200, Mark Wielaard wrote:
>  Hi,
>  
>  On Tue, 2017-04-11 at 11:01 -0400, Don Zickus wrote:
>  > Mark W.,
>  > 
>  > Do you have much insight into how the below definitions would interact with
>  > the kernel?
>  > 
>  > > @@ -395,7 +395,14 @@ BuildRequires: pciutils-devel gettext ncurses-devel
>  > >  BuildConflicts: rhbuildsys(DiskFree) < 500Mb
>  > >  %if %{with_debuginfo}
>  > >  BuildRequires: rpm-build, elfutils
>  > > -%define debuginfo_args --strict-build-id -r
>  
>  I am happy you seem able to use the defines instead of having to
>  override the whole find-debuginfo call. Please let me know if there are
>  other defines you need/want. 
Hi Mark,

Thanks for responding.  Laura already provided some good feedback.
I was curious if some of this would correctly for our non-kernel package
stuff we build.  I might be reading Laura's patch wrong, but I think some of
the options applied to the kernel _and_ the userspace applications too.


Also, I was trying to understand Laura's previous patch with the -p changes.
It looked like the output of the application's debuginfo binaries did not
match with other programs.  I was curious if you had some thoughts on that
patch (it was split out of this one; you would have to see the original
patch to understand).

Cheers,
Don

> 
> > > +# Most of these should be enabled after more investigation
> > > +%undefine _include_minidebuginfo
> 
> This prevents adding a .gnu_debugdata section to the binaries which
> contains a minisymtab with just the function symbols/addresses. This is
> not really relevant for the kernel and modules. If added to
> binaries/shared libraries it allows some tools to show function names
> for more addresses even without having any debuginfo installed.
> 
> > > +%undefine _find_debuginfo_dwz_opts
> 
> This prevents running dwz (the DWARF compressor) which doesn't work for
> the kernel and kernel modules. The problem is that kernel modules are
> not normal ET_EXEC or ET_DYN, but look like ET_REL object files. object
> files contain relocations between ELF sections that dwz cannot handle.
> It would be nice to fix this because kernel modules are ideal for DWARF
> deduplication. They all contain the same basic debug type information
> that could be shared. eu-strip --reloc-debug-sections (-r, see below)
> could help with this, but hasn't been integrated. I'll try to make some
> time (no promises!) to look at it, because getting dwz working against
> the kernel modules would probably provide a huge saving.
> 
> > > +%undefine _unique_build_ids
> > > +%undefine _unique_debug_names
> > > +%undefine _unique_debug_srcs
> 
> It is somewhat unfortunate these don't work with the kernel build. The
> idea is that the package nvra is used to make all of the above unique
> between all versions of the package/debuginfo.
> 
> If I understand correctly we "uniqueify" too late assuming nothing
> depends on the build-id till we start handling the debuginfo. But the
> kernel builds seem to already pick out build-ids to store and embeds
> some ELF images in other ELF images and then those build-ids shouldn't
> be touched anymore. The AFTER_LINK hack was used to counter this, but it
> is somewhat fragile/doesn't really seem to interact correctly anymore.
> 
> If you don't have _unique_build_ids you should also set:
> %global _build_id_links alldebug
> to make sure build-ids are only used for the debuginfo packages
> (otherwise your main packages stops being parallel installable, which
> the kernel needs).
> 
> I think _unique_debug_names and _unique_debug_srcs should in theory
> still work. But they aren't very useful if the build-ids aren't unique.
> 
> > > +%global _find_debuginfo_opts -r
> 
> This was specifically designed for the kernel. It makes sure relocations
> between debug sections are removed in ET_REL files. Which normally would
> not be correct if those ET_REL object files would be linked together
> again. But kernel modules are not real/normal object files. It should
> save a couple of 100MBs when installing the debuginfo for the .ko files.
> (As said above we really should figure out a way to combine this with
> dwz processing so we can safe even more space.)
> 
> > > +%global _missing_build_ids_terminate_build 1
> 
> Yes please. It should show if any ELF file rpm processes doesn't have a
> build-id, which would be a bug in the toolchain.
> 
> > I am only poking you because it seems like you spent a lot of time cleaning
> > up some of the rpm macros in the rpm pkg.
> 
> Thanks. I am really interested in this. I did see one of the earlier
> patches, but missed that I had been CCed on the followups. My email
> setup is a bit of a mess currently. My employer recently decided on
> short notice that my engineering group didn't need normal email, removed
> all our filters and forced us to adopt a "email provider" that
> "helpfully" removes any emails it believes you have already received and
> that hides everything behind a proprietary web frontend. Sigh. So I had
> to switch email addresses. Sorry.
> 
> Cheers,
> 
> Mark
> _______________________________________________
> kernel mailing list -- kernel(a)lists.fedoraproject.org
> To unsubscribe send an email to kernel-leave(a)lists.fedoraproject.org

[Mark Wielaard]

Hi Don,

On Wed, 2017-04-12 at 13:45 -0400, Don Zickus wrote:
>  Thanks for responding.  Laura already provided some good feedback.
>  I was curious if some of this would correctly for our non-kernel package
>  stuff we build.  I might be reading Laura's patch wrong, but I think some of
>  the options applied to the kernel _and_ the userspace applications too. 
Yes, it would be nice if the user space sub-packages didn't use any of
the "special settings". But at least the splitting of the sub-debuginfo
package needs special handling for now (hopefully not needed in the
future, see below). But the settings should really hurt either. The user
space packages just wouldn't have have all the features they could have.
But the mini symtab and dwz compression for those small tools are IMHO
not major features.

>  Also, I was trying to understand Laura's previous patch with the
> -p changes.
>  It looked like the output of the application's debuginfo binaries did not
>  match with other programs.  I was curious if you had some thoughts on that
>  patch (it was split out of this one; you would have to see the original
>  patch to understand). 
Yes, that is for the subpackage debuginfo splitting. We are working on
that in a more generic way for all packages, so hopefully that will all
become easier in the future:
https://fedoraproject.org/wiki/Changes/SubpackageAndSourceDebuginfo
(Yes, that was originally proposed for Fedora 26, obviously we missed
that...)

Cheers,

Mark