Re: [PATCH] Improve building with LTO

[Mark Wielaard <mark@klomp.org>]

Hi Alexander,

First my apologies, your last commit had "Alexander Miller via
Elfutils-devel <elfutils-devel@sourceware.org>" as author. That is the
stupid mailinglist doing From mangling, bad mailinglist. But also bad
me for not catching that. We probably need a git hook to reject pushes
with such bogus authors. Could you add a From: Alexander Miller <
alex.miller@gmx.de> in the body (I believe git send-email --from=...
will do that for you). That way git am does the right thing.

On Sun, 2021-02-14 at 23:57 +0100, Alexander Miller via Elfutils-devel wrote:
> Here's my attempt to use gcc-10's new symver attribute to avoid global
> asm statements that cause trouble with LTO. That requires converting
> from triple @ syntax to double @ syntax. To satisfy those picky linkers,
> the unversioned name of the affected symbols has to be changed.
> The NEW_VERSION macros have to be moved before the function definitions.

This is really exciting! I had more or less given up on this. I
couldn't make it work because of the triple @ syntax not being
supported. But it looks we don't need that after all.

> Also tried to improve the situation for older gcc versions. Although
> gcc-5 added a no_reorder attribute that's supposed to help here, it
> doesn't work reliably (but improves the situation substantially).

So you rewrite the asm statements to not use @@@ just @ and @@, that
way they match the symver attribute usage. Smart, that way they are as
similar as possible.

> I've tested the patch with different compilers (gcc-10, gcc-9, gcc-6,
> clang-11), linkers (bfd, gold, lld), 32/64 bit (x86), with/without LTO.
> Of course you need to cheat a bit to build elfutils with clang, and
> lld can't be used with every combination. The workaround for older gcc
> was enough for gcc-9 and 32bit gcc-6, but 64bit gcc-6 builds needed
> -flto-partition=none, so this seems to depend on the version and options
> used.

Wow, you really went above and beyond. IMHO it would have been fine to
simply say that you need GCC10 and a recent binutils ld to get LTO
working. Supporting LTO with gcc-6 is really nice, but people stuck on
such an old compiler should probably first look at upgrading that than
trying to play with LTO. We should probably look at making an LTO build
part of the buildbot.

> Symbol versioning worked as expected in all cases, at least the list
> of dynamic symbols of the libs looked good to me.
> 
> The test suite seems brittle, though. It fails on 32bit builds, with
> gold or lld, and with lto builds using clang (unknown object format)
> or gcc-6 (debug info not found). But that's not related to this patch.
> For 64bit builds with gcc-{9,10} and bfd, the test suite succeeds even
> with lto enabled.

Do the 32bit builds with gcc10/binutils ld have a clean test suite?

> Additional notes:
> * The asm names for the compat versions seem unnecessary, but I've
>   kept them.
> * Commenting out old versions in the .map file may not be needed.
>   It's mostly a leftover from an earlier attempt, but I didn't want to
>   re-run all test and I actually prefer it like this, so I left it in.

I would like to make sure it isn't needed. Having to comment out old
versions is a bit of a pain. In my own attempts I actually tried it the
other way around. Keep the earliest version in the version script and
don't add (remove) new versions (those would come from the symver).
That is slightly easier for maintenance, so you don't need to remove
older versions, just add the new (.symver) version in the source.

> * See commit message below.
> 
> ------------------------ 8< ------------------------
> Use symver attribute for symbol versioning instead of .symver
> assembler directive when available. Convert to use double @ syntax
> for default version in all cases (required when using the attribute).
> 
> Add the attributes externally_visible, no_reorder if available when
> using assembler directives to improve the situation for < gcc-10.
> This is not 100% reliable, though; -flto-partition=none may still be
> needed in some cases.

How does the __COUNTER__ magic work?

> Note that -Wno-error=stack-usage= is still needed to build with LTO.

Yeah that is a pity. The issue is in just a few files, but with LTO
those get combined with all the others causing issues. I believe the
warnings are only in the tools now. It would be really good if we could
get rid of the stack-usage warnings in: readelf.c, nm.c, size.c,
strip.c, elflint.c, findtextrel.c, elfcmp.c, objdump.c, ranlib.c, ar.c,
unstrip.c. But that is for another day.

I have to stare at the marcos a bit to make sure I really understand
what is going on. But this looks really good.

Thanks,

Mark