Re: [PATCH] Improve building with LTO

[Alexander Miller <alex.miller@gmx.de>]

[Re-sending mail because the list has been omitted by mistake.]

On Wed, 17 Feb 2021 21:22:15 +0100
Mark Wielaard <mark@klomp.org> wrote:

> 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.

Yes, I wanted symbol versioning to work the same way in both cases.

> > 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.

Of course gcc-6 is really old (I just happened to have it still available).
But getting LTO to work with gcc-9 would be nice. That worked in my tests,
but I'm not sure if it's reliable. And it's only a small addition (and I
didn't add anything more for gcc-6).

But most importantly I didn't want to paint myself into a corner and change
symbol versioning in a way that will only ever work with one particular
toolchain.

> > 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?

Only the 64bit builds with bfd passed. For all 32bit builds (64bit system,
but built with -m32) the test case "run-backtrace-native-biarch.sh" fails:
|
| 0x556a53cef000  0x556a53cf4000  .../tests/backtrace-child-biarch
| 0x7fadd8118000  0x7fadd828d000  /lib64/libc-2.32.so
| 0x7fadd8298000  0x7fadd82b4000  /lib64/libpthread-2.32.so
| 0x7fadd82f0000  0x7fadd831d000  /lib64/ld-2.32.so
| 0x7ffdf13fc000  0x7ffdf13fd000  [vdso: 6783]
| TID 6783:
| .../tests/backtrace: dwfl_thread_getframes: no error
| backtrace: linux-pid-attach.c:326: pid_set_initial_registers: Assertion `pid_arg->tid_attached == 0' failed.
| ./test-subr.sh: line 84:  6782 Aborted                 LD_LIBRARY_PATH="${built_library_path}${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH" $VALGRIND_CMD "$@"
| backtrace-child-biarch: no main
| FAIL run-backtrace-native-biarch.sh (exit status: 1)

But that isn't related to my patch. It was the same before.

> > * 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.

I've run a few tests and it works without that change. I'll skip that
in the next version of the patch.

(Gold has trouble when a an object defines an unversioned symbol and
the version script contains the old version; that isn't the case in my
solution because the "new" functions get renamed.)

> How does the __COUNTER__ magic work?

It doesn't.

Sorry. I'm adding two levels of indirection to make it work. I didn't
notice since it's not needed at the moment. In the future, somebody
might want to add multiple old versions for a symbol, and then they
need different names.

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

Don't hesitate to ask if something isn't clear.

I'll post an updated patch later.


Cheers,

Alex