Add the needed bits for compiling with link-time optimization.

[besser82]

GCC 10.2 and LLVM/Clang 10 offer initial support for building libraries, that are using symbol versioning features, with LTO.

To make use of this with GCC 10.2, the exported versioned symbols need to be declared explicitly with __attribute__((symver (...))).

LLVM/Clang 10 supports symbol versioning with LTO out of the box without any changes needed.

Fixes #24.

[codecov]

Codecov Report

Merging #109 into develop will decrease coverage by 0.13%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #109      +/-   ##
===========================================
- Coverage    94.48%   94.34%   -0.14%     
===========================================
  Files           32       32              
  Lines         3750     3764      +14     
===========================================
+ Hits          3543     3551       +8     
- Misses         207      213       +6     

Impacted Files	Coverage Δ
lib/crypt-yescrypt.c	82.00% <0.00%> (-6.64%)	⬇️
lib/crypt-des.c	91.13% <0.00%> (-0.79%)	⬇️
lib/crypt.c	95.16% <0.00%> (+0.11%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 328563b...2e443c5. Read the comment docs.

[zackw]

Is this really enough to make it work? I was under the impression we needed __attribute__ ((symver)) as well (see GCC bug 48200).

Why is it necessary to use -flto-partition=none?

I was under the impression one ought to set AR, CC, NM, RANLIB, and CFLAGS as arguments to configure, e.g.

./configure CC="gcc" AR="gcc-ar" NM="gcc-nm" RANLIB="gcc-ranlib" CFLAGS="-g -O2 -flto -flto-partition=none" ...

The change to use ${NM-nm} in test/symbols-*.sh is approved independent of the rest of the patch, please go ahead and push it separately.

[besser82]

@zackw I've fixed your concerns in the rebased commits.

Is this really enough to make it work? I was under the impression we needed __attribute__ ((symver)) as well (see GCC bug 48200).

For LLVM/Clang it simply works that way. For GCC 10.2 one either needs to implement __attribute__ ((symver)) for symbol versioning, or pass -flto -flto-partition=none to skip automatic partitioning of the symbol table, resolved by the linker, which would drop all exported versioned symbols not used by the library itself.

Why is it necessary to use -flto-partition=none?

This is necessary, if one does NOT use __attribute__ ((symver)) with GCC and LTO enabled; see above.
Anyways, my rebased commits use the new symver attribute with GCC 10.2. LLVM/Clang doesn't support it nor needs it.

[besser82]

@zackw Tested and works with GCC 10.2, LTO enabled: https://koji.fedoraproject.org/koji/buildinfo?buildID=1593132

[besser82]

@zackw Would you mind to have another look here?

[solardiz]

Before merging this, would anyone please comment on the concerns I brought up in #24 (comment)

[zackw]

I have to say I share @solardiz's concerns about exposing latent UB. I would be okay with merging the changes to crypt-port.h, documenting that an LTO-using build is now possible but experimental and discouraged for production use, and not changing libxcrypt.spec.rpkg yet. [never mind that last bit, I thought it was the spec for what actually goes into Fedora]

If we made the configure script detect LTO and force --disable-obsolete-api when it did, do you think that would be sufficient to "ensure that build wouldn't be inadvertently usable in production", @solardiz ?

[zackw]

Also, just to verify, the xcrypt_symbol annotations are no longer necessary with this version of the patch?

[besser82]

Also, just to verify, the xcrypt_symbol annotations are no longer necessary with this version of the patch?

Yes, that's correct.

[zackw]

Can we get a CI build with all hashes enabled, LTO on, and ASan+UBSan also on? That would be a quick way to get some more confidence that the code is UB-sound.

[besser82]

Can we get a CI build with all hashes enabled, LTO on, and ASan+UBSan also on? That would be a quick way to get some more confidence that the code is UB-sound.

I will check if that's possible. We LLVM / Clang >= 7 on Travis for this. Or is this possible with GCC as well? We can use Fedora >= 32 to do it, if GCC supports that as well.

[solardiz]

If we made the configure script detect LTO and force --disable-obsolete-api when it did, do you think that would be sufficient to "ensure that build wouldn't be inadvertently usable in production", @solardiz ?

I think that would be weird - mixing unrelated things together, and excluding the obsolete APIs from our own LTO stress-testing. If we do use LTO for stress-testing, let's use the opportunity fully.

[besser82]

Here is an overview of what got changed by this pull request:

Issues
======
- Added 1
           

See the complete overview on Codacy

[zackw]

Every time you force-push this branch, I get multiple emails from Codacy about the missing syntax-highlighting annotation in the README. It's making it hard for me to tell what actually changed :-(

[besser82]

@zackw For some funny reason I can even reproduce the errors shown on TravisCI with GCC 10.2 + ASan,UBSan without LTO enabled… :S

The other funny thing is: If I replace crypt_r with crypt_rn in test/special-char-salt.c the tests succeeds…