Bug 1789099 - redhat-rpm-config should remove LTO bytecode sections/symbols from installed .o/.a files
Summary: redhat-rpm-config should remove LTO bytecode sections/symbols from installed ...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: redhat-rpm-config
Version: 32
Hardware: All
OS: All
high
high
Target Milestone: ---
Assignee: Florian Festi
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1770880
TreeView+ depends on / blocked
 
Reported: 2020-01-08 18:11 UTC by Jeff Law
Modified: 2020-07-08 16:15 UTC (History)
14 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-07-08 16:15:23 UTC
Type: Bug


Attachments (Terms of Use)
Changes to redhat-rpm-config to implement stripping LTO sections/symbols (1.94 KB, patch)
2020-01-08 18:11 UTC, Jeff Law
no flags Details | Diff

Description Jeff Law 2020-01-08 18:11:24 UTC
Created attachment 1650804 [details]
Changes to redhat-rpm-config to implement stripping LTO sections/symbols

Description of problem:
GCC's LTO implementation emits bytecodes into .o files it generates. These bytecodes are _not_ considered stable from one release of GCC to the next.

Therefore we need to strip the LTO bytecodes out of any .o (and .a) file that gets installed into the buildroot.

This has been an issue for years as packages could individually enable LTO and (unknowingly) end up shipping a .o/.a file with the embedded LTO bytecodes.  However, with the potential introduction of LTO by default in Fedora 32 this issue has become important to address now.

The attached patch adds a new "brp-strip-lto" which is modeled after brp-strip and openSUSE's changes to strip LTO sections/symbols.  It only looks at installed .o/.a files and only strips the LTO bits (.gnu.lto, .gnu.debuglto, __gnu_lto_v1).


Version-Release number of selected component (if applicable):


How reproducible:
Not easily.  I have manually hacked up a package to install .o files with LTO bytecodes and verified that LTO bytecodes show up in the .o's within resultant .rpm file.

I then installed the updated redhat-rpm-config into my mock chroot and rebuilt the package and verified there are not LTO bytecodes in the .o's within the resultant .rpm file.



Additional info:
This can (and should) go into redhat-rpm-config now, independently of other LTO enablement work.

Comment 1 Igor Raits 2020-01-08 18:14:02 UTC
Please submit pull request on src.fedoraproject.org/rpms/redhat-rpm-config

Comment 2 Jason Tibbitts 2020-01-08 18:25:25 UTC
I can't really speak for how much sense this makes, but a couple of general comments:

Most of the existing brp-strip* scripts will at least call file to ensure that they're stripping the right types of files, not just anything ending with a particular extension.

We already have a number of scripts which traverse the entire buildroot (most which grep out /usr/lib/debug and call file and do something with the result).    One of them is even parallelized (brp-strip-static-archive) but all are subtly different.  (They all look like line noise, though.).  I wonder if there's an opportunity for standardization and optimization here.

Comment 3 Florian Weimer 2020-01-08 18:29:51 UTC
(In reply to Jason Tibbitts from comment #2)
> We already have a number of scripts which traverse the entire buildroot
> (most which grep out /usr/lib/debug and call file and do something with the
> result).    One of them is even parallelized (brp-strip-static-archive) but
> all are subtly different.  (They all look like line noise, though.).  I
> wonder if there's an opportunity for standardization and optimization here.

There absolutely is.  We added eu-elfclassify to elfutils for exactly this purpose.

Comment 4 Jeff Law 2020-01-08 18:37:40 UTC
Re: c#1 -- pull request submitted

Re: c#2 -- Well, it's certainly important.  We must strip the LTO sections/symbols from any .o/.a files that get installed during a package build because the content of those sections is not stable across GCC releases.  That's why openSUSE did the exact same thing ~6 months ago when they introduced LTO into openSUSE.  As a GCC developer for ~30 years at this point, I would not consider green-lighting LTO for Fedora without having proper stripping of LTO sections/symbols in place.

I certainly could call "file" to avoid trying to strip something that isn't a suitable .o (and to find anything that contained LTO sections, but wasn't named .o/.a)  That wouldn't be a big deal -- it just seemed a bit error prone to make sure all extraneous output from "file" gets removed for this kind of corner case.

I pondered handling this within one of the existing scripts, but felt it was ultimately cleaner to have it handled by its own script.  Given these are just iterating over the install tree it didn't seem that costly.  But if folks want this integrated into one of the other brp-* scripts, I'm certainly willing to do that.

Comment 5 Mohan Boddu 2020-01-27 12:23:40 UTC
We are ready to run the mass rebuild, but it seems like the changes to redhat-rpm-config are not merged.

Whats the status on it?

Comment 6 Ben Cotton 2020-01-27 14:23:59 UTC
Changing needinfo to ffesti, the main admin of the redhat-rpm-config package. There are several outstanding pull requests: https://src.fedoraproject.org/rpms/redhat-rpm-config/pull-requests

Comment 7 Ben Cotton 2020-02-11 17:25:08 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 32 development cycle.
Changing version to 32.

Comment 8 Jerry James 2020-02-17 22:26:50 UTC
Comment 2 is important.  This change broke the sagemath build: https://koji.fedoraproject.org/koji/buildinfo?buildID=1462675

I canceled the build, but you can see the problem in the x86_64 portion.  Here are some relevant lines from the log:

+ /usr/lib/rpm/redhat/brp-strip-lto /usr/bin/strip
/usr/bin/strip: /builddir/build/BUILDROOT/sagemath-8.9-3.fc33.x86_64/usr/src/debug/sagemath-8.9-3.fc33.x86_64/build/pkgs/rubiks/src/dik/trans/perm4.a: file format not recognized
/usr/bin/strip: /builddir/build/BUILDROOT/sagemath-8.9-3.fc33.x86_64/usr/src/debug/sagemath-8.9-3.fc33.x86_64/build/pkgs/rubiks/src/dik/trans/sperm.a: file format not recognized
[many more omitted]

These files are not static archives, even though they have the .a extension.  This is going to affect other packages of mine as well.  A lot of the gap-pkg-* packages have files named foo.a, foo.b, foo.c, etc., and guess what?  The .a files are not static archives and the .c files are not C code.

Please fix the script to invoke file as requested.  It may be a "corner case", but it affects existing Fedora packages, and therefore must be addressed.

Comment 9 Jeff Law 2020-02-17 23:56:14 UTC
ACK.  I'll make sure it gets taken care of.

Comment 10 Igor Raits 2020-02-18 01:33:41 UTC
There's nothing ffesti can do, PRs are waiting for creator to react on comments.

Comment 11 Jeff Law 2020-02-19 19:53:07 UTC
I think we just want to use 'file' to filter out everything except archives and relocatables.  Something like this:

# Strip ELF binaries
find "$RPM_BUILD_ROOT" -type f -name '*.[ao]' \! -regex "$RPM_BUILD_ROOT/*usr/lib/debug.*" -exec file \{\} \; | \
  egrep '(current ar achive|ELF.*relocatable)' | awk -F: '{print $1}' | \
  xargs -r -P$NCPUS -n32 sh -c "$STRIP -p -R .gnu.lto_* -R .gnu.debuglto_* -N __gnu_lto_v1 \"\$@\"" ARG0

It's untested.  Once I'm sure it works, I'll update the PR.

Comment 12 Jeff Law 2020-02-20 20:55:41 UTC
FTR.  Florian recommended using eu-elfclassify rather than file + awk as the filter.  AFAICT elfutils is part of the basic buildroot, so that seemed quite reasonable (after all that is precisely what eu-elfclassify is meant to do).

My preference would be to handle this via a pull-request, but given it's got Jerry stuck as well as the glibc team, I went ahead and committed the change and started builds.  Everything should be golden once the buildroot regenerates.

Comment 13 Toke Høiland-Jørgensen 2020-04-06 13:14:46 UTC
The brp-strip-lto script also barfs on files containing BPF bytecode, as those included with xdp-tools. See:

https://koji.fedoraproject.org/koji/taskinfo?taskID=43061034

+ /usr/lib/rpm/redhat/brp-strip-lto /usr/bin/strip
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_blk_udp.o'
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_blk_tcp.o'
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_blk_ip.o'
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_blk_eth.o'
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_blk_all.o'
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_wht_udp.o'
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_wht_tcp.o'
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_wht_ip.o'
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_wht_eth.o'
/usr/bin/strip: Unable to recognise the format of the input file `/builddir/build/BUILDROOT/xdp-tools-0.0.2-2.fc33.x86_64/usr/lib64/bpf/xdpfilt_wht_all.o'

I guess this could be fixed by excluding the BPF elf architecture when filtering the .o files?

Comment 14 Jeff Law 2020-04-13 19:12:50 UTC
In general, if it's an ELF relocatable, then it can potentially have LTO bytecode regardless of the underlying target architecture, so filtering on architecture is generally the wrong approach.

Could you pass along one of those .o files so that I can examine it with eu-elfclassify?

Comment 15 Toke Høiland-Jørgensen 2020-04-14 07:21:07 UTC
Well, BPF does tend to be special; we generally need to pass a whole different set of CFLAGS to clang when building BPF files.

I added a workaround to the xdp-tools .spec file, so you can grab the bytecode files from the latest build: https://koji.fedoraproject.org/koji/buildinfo?buildID=1490400

(everything in $LIB/bpf)

Comment 16 Jeff Law 2020-04-16 23:08:56 UTC
Even if you pass a different set of flags when building them, we really want to be absolutely sure there's never any LTO bytecodes or symbols in there.

What's probably going on here is strip wasn't built with BPF support and is thus complaining.  So ISTM we really need to get a binutils bug filed.

Comment 17 Toke Høiland-Jørgensen 2020-04-17 10:52:21 UTC
Yeah, I think you're right - I get the same error from the regular 'strip' invocation now that I fixed the permissions of the installed bytecode files. I'll file a bug against binutils...

Comment 18 Toke Høiland-Jørgensen 2020-04-17 12:51:51 UTC
Opened https://bugzilla.redhat.com/show_bug.cgi?id=1825193

Comment 19 Michael Catanzaro 2020-07-07 12:41:07 UTC
Can this be closed?

Comment 20 Jeff Law 2020-07-08 16:15:23 UTC
Yes, these changes have been committed and this BZ can be closed.


Note You need to log in before you can comment on or make changes to this bug.