Open besser82 opened 4 years ago
Merging #109 into develop will decrease coverage by
0.13%
. The diff coverage isn/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%) |
:arrow_down: |
lib/crypt-des.c | 91.13% <0.00%> (-0.79%) |
:arrow_down: |
lib/crypt.c | 95.16% <0.00%> (+0.11%) |
:arrow_up: |
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 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.
@zackw Tested and works with GCC 10.2, LTO enabled: https://koji.fedoraproject.org/koji/buildinfo?buildID=1593132
@zackw Would you mind to have another look here?
Before merging this, would anyone please comment on the concerns I brought up in https://github.com/besser82/libxcrypt/issues/24#issuecomment-674139907
Also, just to verify, the xcrypt_symbol
annotations are no longer necessary with this version of the patch?
Also, just to verify, the
xcrypt_symbol
annotations are no longer necessary with this version of the patch?
Yes, that's correct.
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.
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.
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.
Here is an overview of what got changed by this pull request:
Issues
======
- Added 1
See the complete overview on Codacy
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 :-(
@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…
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.