Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strip utility does not have LTO support, mangles archives #49

Open
InBetweenNames opened this issue Oct 20, 2017 · 22 comments
Open

Strip utility does not have LTO support, mangles archives #49

InBetweenNames opened this issue Oct 20, 2017 · 22 comments

Comments

@InBetweenNames
Copy link
Owner

InBetweenNames commented Oct 20, 2017

This is a bit of a long running issue that unfortunately needs upstream support in order to fix. I'll detail it here for interested parties.

Upstream Gentoo bug: https://bugs.gentoo.org/603594
Upstream binutils bug: https://sourceware.org/bugzilla/show_bug.cgi?id=21479

The strip utility in LTO builds will commonly emit errors like this for static archives:

x86_64-pc-linux-gnu-strip: /dev/shm/portage/sys-libs/zlib-1.2.8-r1/image/usr/lib64/stzjlms5/adler32.o: plugin needed to handle lto object

Originally I didn't think this was a problem since that should effectively make it a no-op. I was wrong, though -- it actually makes the static archive unusable (I believe this has something to do with the index).

The real solution to this will be introducing LTO support into strip. However, a workaround is needed in the meantime. The linked upstream threads suggest a few options

  • Add FEATURES=nostrip (will disable stripping entirely globally)
  • Run ranlib after strip (requires a Portage patch, but it does seem to fix it... I'm unsure of the effects of this on non-LTO systems)
  • Use -ffat-lto-objects or remove -flto(Obviously not ideal)

@pchome has suggested a workaround that we'll be using in the meantime, which is to set the STRIP_MASK variable to disable stripping static archives only (stripping binaries still). This is the best workaround right now until the upstream issues are sorted out. We can close this issue when things are handled properly upstream.

InBetweenNames pushed a commit that referenced this issue Oct 20, 2017
Introduce STRIP_MASK functionality to work around STRIP LTO plugin bug (workaround #49)
@wolfwood
Copy link
Contributor

wolfwood commented May 8, 2018

looks like there is now an llvm-strip tool, might be an interesting platform if development work is required https://www.phoronix.com/scan.php?page=news_item&px=LLVM-Strip

@InBetweenNames
Copy link
Owner Author

Been looking into this again, and it looks like strip works fine on LTO object files as long as they aren't in a static archive. Putting them in a static archive mangles the archive. It's definitely the index. I do have a rudimentary patch to Portage that invokes ranlib as a workaround, but I'm unwilling to commit to it just yet.

@InBetweenNames
Copy link
Owner Author

InBetweenNames commented Aug 31, 2018

I tried llvm-strip, too, but it doesn't support a number of GNU strip options which causes other problems. It also doesn't appear to handle static archives. However, I'd be willing to switch to it if it addresses these problems eventually.

One more thing: objcopy suffers from the same problem in my test configuration:

> objcopy lto1.a lto1.a.modified
objcopy: struufsL/lto1.o: plugin needed to handle lto object
> diff lto1.a lto1.a.modified
Binary files lto1.a and lto1.a.modified differ
> g++ lto2.cpp lto1.a -o bin
> g++ lto2.cpp lto1.a.modified -o bin                                          
/tmp/ccE7vu4Y.o:lto2.cpp:function main: error: undefined reference to 'func1(int, int)'
collect2: error: ld returned 1 exit status
> ranlib lto1.a.modified
> g++ lto2.cpp lto1.a.modified -o bin                                                                                                                                                                          

@pchome
Copy link
Contributor

pchome commented Aug 31, 2018

One more thing: objcopy suffers from the same problem in my test configuration

AFAIK strip is objcopy , or shares it's code.

@InBetweenNames
Copy link
Owner Author

InBetweenNames commented Aug 31, 2018

Based on the binutils sources I think you're right. I found this relevant snippet in archive.c:

if (syms[src_count]->name[0] == '_'
			  && syms[src_count]->name[1] == '_'
			  && strcmp (syms[src_count]->name
				     + (syms[src_count]->name[2] == '_'),
				     "__gnu_lto_slim") == 0)
			_bfd_error_handler
			  (_("%pB: plugin needed to handle lto object"),
			   current);
		      namelen = strlen (syms[src_count]->name);
		      amt = sizeof (char *);
		      map[orl_count].name = (char **) bfd_alloc (arch, amt);

It's not easy to read, and I'm not even sure if this is the offending code just yet, but if I understand correctly it is applying the same logic to non-LTO symbols as it does to LTO symbols, hence the warning. This code appears to be what generates the archive index/header. If I had to guess, this is where the header gets mangled.

@InBetweenNames
Copy link
Owner Author

Okay, after more poking around, it does appear to be directly relevant. It is indeed the index generation code. The problem is it doesn't understand how to build an index for LTO symbols. Curiously, ranlib does. strip is basically objcopy, and ranlib is basically ar s.

@InBetweenNames
Copy link
Owner Author

Need some review: workaround patch in interim for Portage.

From edee2620d3173cd50762a7ebe7b9cb9a2020f0f9 Mon Sep 17 00:00:00 2001
From: Shane Peelar <[email protected]>
Date: Fri, 31 Aug 2018 17:11:21 -0400
Subject: [PATCH] LTO workaround

---
 bin/estrip | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/bin/estrip b/bin/estrip
index 5709b862c..b2216e964 100755
--- a/bin/estrip
+++ b/bin/estrip
@@ -425,6 +425,9 @@ do
                        # retain the debug info in the archive itself.
                        if ! ${FEATURES_splitdebug} || ${RESTRICT_splitdebug} ; then
                                ${STRIP} -g "${x}"
+                               RANLIB=${RANLIB:-"${CHOST}-ranlib"}
+                               ${RANLIB} "${x}"
+                               echo "${RANLIB}: rebuilding index for ${x}"
                        fi
                fi
        elif [[ ${f} == *"SB executable"* || ${f} == *"SB pie executable"* ||
--
2.18.0

InBetweenNames added a commit that referenced this issue Oct 8, 2018
Patch portage to rebuild static archive indexes to fix mangled LTO
symbols.

Address issue #49, Fix #119

Signed-off-by: Shane Peelar <[email protected]>
@zeule
Copy link
Contributor

zeule commented Oct 10, 2018

Could you apply the patch to sys-apps/portage-mgorny too, please?

@InBetweenNames
Copy link
Owner Author

I can see no reason why not! However, I cannot test as I don't use that version of portage. Could you try it yourself and verify it works as described in README.md? If so, I'd be happy to add it in.

@zeule
Copy link
Contributor

zeule commented Oct 10, 2018

What do I need to test exactly?

@InBetweenNames
Copy link
Owner Author

Apply the patch to sys-apps/portage-mgorny, try building zlib and ensure that the fixups are applied at the end of the build sequence, for example. Make sure your STRIP_MASK is unset of course.

@InBetweenNames
Copy link
Owner Author

I think, actually, I could implement this functionality in bashrc.d instead of as a Portage patch. It would largely replicate the existing machinery present in portage for strip, and would run after that command to perform the fixup. Larger than a one line patch, yes, but would also allow both sys-apps/portage and sys-apps/portage-mgorny to take advantage of this functionality.

InBetweenNames added a commit that referenced this issue Oct 27, 2018
LTO patches are now applied directly from lto-overlay,
using functionality replicated directly from portage.
/etc/patches is now left untouched by LTOize, and
a script, 41-lto-patch.sh, is symlinked into your
portage bashrc.d.

You can experiment with your own LTO patches in /etc/patches
as usual.  Please consider submitting them upstream if you
make working ones.

Also, with this change, we no longer need to increment the version
number with LTOize whenever new patches come in.

Address #49

Signed-off-by: Shane Peelar <[email protected]>
@InBetweenNames
Copy link
Owner Author

OK, so investigating further, it looks like it makes more sense to have this functionality in portage for now instead of in bashrc.d. There's a lot of edge cases that are handled in estrip that I don't want to have to maintain separately. For now, we'll leave it as a portage patch.

@InBetweenNames
Copy link
Owner Author

@zeule , I made an upstream issue to address sys-apps/portage-mgorny. It can be found here

@zeule
Copy link
Contributor

zeule commented Oct 29, 2018

Thank you! I did not manage to understand how all this stuff works and how to verify it.

@InBetweenNames
Copy link
Owner Author

No worries. @mgorny accepted the patch upstream, so you should get it soon. It may be accepted into upstream Portage as well thanks to this.

@funghetto
Copy link
Contributor

Is this related to the script installed by portage-bashrc-mv?

@InBetweenNames
Copy link
Owner Author

Well, it's a patch that's installed via an installed bashrc.d script. But it's not a script itself.

@ionenwks
Copy link

There we go, essentially exact same as the patch here minus the lto overlay message.

sys-apps/portage: version bump to 2.3.52
 #603594 - Run RANLIB after stripping static archives to fix LTO

@InBetweenNames
Copy link
Owner Author

InBetweenNames commented Nov 19, 2018

Beautiful. Unless there are any objections, I think we can finally close this issue for good!

@jelinekto
Copy link
Contributor

jelinekto commented Nov 20, 2018

I just noticed some builds spitting out a bunch of aforementioned plugin needed to handle lto object messages, e.g. both python 2 and 3 or binutils-2.31-r1. Now this happens when using either of portage-2.3.52 and portage-mgorny-9999.
Is it something to be worried about?

EDIT: Ah nevermind, my system's a bit of a mess right now, turns out simply re-emerging ltoize fixes the issue. Yippee.

EDIT2: Uh, taking the first edit back, for some reason exactly one build of binutils passed without any plugin needed. Plus I've replicated on a second machine.

@InBetweenNames
Copy link
Owner Author

The warnings will still be thrown up (can't be avoided), but a message should be printed after to inform you that the archive in question had its index rebuilt. In other words, strip mangles it, and then ranlib is run to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants