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

lto linking, missing symbols #24

Open
asavah opened this issue Aug 4, 2018 · 13 comments · May be fixed by #109
Open

lto linking, missing symbols #24

asavah opened this issue Aug 4, 2018 · 13 comments · May be fixed by #109
Labels
enhancement Requests a new feature or improvement. Without "need more information", we agree it's desirable. help wanted The libxcrypt core developers do not plan to work on this themselves but would review a PR.

Comments

@asavah
Copy link

asavah commented Aug 4, 2018

gcc 8.2.0
binutils 2.31.1
glibc 2.28 (with --disable-crypt)

crosscompilation for aarch64, libxcrypt v4.1.1
configure --enable-shared --disable static

If libxcrypt is built with lto in *flags almost all symbols are missing from the library

lto build - bad

ls libcrypt.so.1.1.0 -ah
-rwxr-xr-x 1 asavah asavah 9496 Aug  5 00:46 libcrypt.so.1.1.0

nm -DC libcrypt.so.1.1.0
                 w __cxa_finalize
0000000000000000 A GLIBC_2.17
                 w __gmon_start__
                 w _ITM_deregisterTMCloneTable
                 w _ITM_registerTMCloneTable
0000000000000000 A OW_CRYPT_1.0
0000000000000000 A XCRYPT_2.0

Non lto build (gold linker) - good

ls libcrypt.so.1.1.0 -al
-rwxr-xr-x 1 asavah asavah 146864 Aug  5 00:49 libcrypt.so.1.1.0


nm -DC libcrypt.so.1
                 U __assert_fail
                 U close
000000000000a470 T crypt
000000000000a470 T crypt
0000000000008390 T crypt_gensalt
0000000000008390 T crypt_gensalt
0000000000008390 T crypt_gensalt
0000000000005530 T crypt_gensalt_ra
0000000000005530 T crypt_gensalt_ra
0000000000005530 T crypt_gensalt_ra
00000000000053b0 T crypt_gensalt_rn
00000000000053b0 T crypt_gensalt_rn
00000000000053b0 T crypt_gensalt_rn
0000000000005350 T crypt_r
0000000000005350 T crypt_r
0000000000005250 T crypt_ra
0000000000005250 T crypt_ra
00000000000051c0 T crypt_rn
00000000000051c0 T crypt_rn
                 w __cxa_finalize
000000000000aed0 T encrypt
000000000000aeb0 T encrypt_r
                 U __errno_location
                 U explicit_bzero
000000000000a470 T fcrypt
                 U free
                 U getentropy
                 U getrandom
0000000000000000 A GLIBC_2.17
                 w __gmon_start__
                 w _ITM_deregisterTMCloneTable
                 w _ITM_registerTMCloneTable
                 U malloc
                 U memcmp
                 U memcpy
                 U memset
                 U open
0000000000000000 A OW_CRYPT_1.0
                 U read
                 U realloc
000000000000aec0 T setkey
000000000000aea0 T setkey_r
                 U snprintf
                 U sprintf
                 U strlen
                 U strncmp
                 U strspn
                 U strtoul
                 U syscall
0000000000000000 A XCRYPT_2.0

Is this expected ?

@besser82
Copy link
Owner

besser82 commented Aug 5, 2018

Long term known problem with LTO and LD versioning scripts and GCC [1]. Has been brought up with FUSE [2] last year, too. Clang has some issues with LTO too, but that's a different story.

Maybe this will work with GCC 9…

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48200
[2] libfuse/libfuse#198

@asavah
Copy link
Author

asavah commented Aug 5, 2018

Thanks for the explanation, feel free to close,
maybe it's worth mentioning in the README ?

@besser82
Copy link
Owner

besser82 commented Aug 5, 2018

Maybe we should crash the configure script with a descriptive error, if -flto is detected in the CFLAGS or LDFLAGS@zackw and @ldv-alt What do you think?

@ldv-alt
Copy link
Collaborator

ldv-alt commented Aug 5, 2018

Given that -flto can probably be used along with some other options that mitigate this problem,
I think a more fine-grained approach is needed.

@zackw
Copy link
Collaborator

zackw commented Aug 6, 2018

I looked briefly at what it would take to detect problematic CC/CFLAGS settings within configure, and it doesn't look easy. It is finicky to write autoconf tests involving multiple object files, but I've done that before. The main problem is, for an accurate check we would need to run libtool, but libtool doesn't even exist until config.status runs. If someone could clue me in how to run ltmain.sh from within configure and have it behave as if it were the generated libtool, I could take it the rest of the way, but I don't have time to figure out that part myself.

"make check" does detect the problem, at least. For now I think we should leave this bug open and mention it in the README.

@besser82 besser82 added the documentation Primarily an issue with the documentation. label Aug 24, 2018
zackw added a commit that referenced this issue Aug 29, 2018
 * README.md: mention yescrypt
 * README.md: document required tools for building from a Git
   checkout (issue #23)
 * README.md: document that -flto cannot be used due to
   incompatibility with symbol versioning (issue #24)
 * README.md: use curly quotes and apostrophes; use out-of-line
   hyperlinks for better readability as plain text
 * AUTHORS: say that the sunmd5 hash module is a clean-room
   reimplementation to avoid the CDDL
 * THANKS: move prior contributor credits here from README;
   alphabetize; mention Dmitry Levin, Alec Muffett, and Colin Percival

 * crypt_gensalt.3: document errno codes that can occur when
   obtaining random bytes from the operating system fails
@zackw
Copy link
Collaborator

zackw commented Aug 29, 2018

Documented the limitation in 6b06074. I'm still happy to write a configure check if someone tells me how to run libtool from inside configure.

@zackw zackw closed this as completed Aug 29, 2018
@besser82
Copy link
Owner

besser82 commented Dec 21, 2018

@asavah, @zackw FYI, building with -flto is working with Clang / LLVM >= 7.0.0 now.

Anyways, test-symbols-static and test-symbols-renames are failing, because nm does not support Clang's lto-object format out of the box.

@zackw
Copy link
Collaborator

zackw commented Jan 24, 2019

@besser82 Neat. I wonder what they did to make it work. I'll maybe see if I can fix the tests.

@besser82
Copy link
Owner

Re-opening as GCC 10.2 supports the needed initial bits now.

@besser82 besser82 reopened this Aug 13, 2020
besser82 added a commit that referenced this issue Aug 13, 2020
GCC 10.2 and LLVM/Clang offer initial support for building
libraries, that are using symbol versioning features, with LTO.

To make use of this, the exported versioned symbols need their
visibility attributes declared explicitly.

Fixes #24.
besser82 added a commit that referenced this issue Aug 13, 2020
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, the exported versioned symbols need their
visibility attributes declared explicitly.

Fixes #24.
besser82 added a commit that referenced this issue Aug 13, 2020
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, the exported versioned symbols need their
visibility attributes declared explicitly.

Fixes #24.
besser82 added a commit that referenced this issue Aug 13, 2020
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, the exported versioned symbols need their
visibility attributes declared explicitly.

Fixes #24.
besser82 added a commit that referenced this issue Aug 13, 2020
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, the exported versioned symbols need their
visibility attributes declared explicitly.

Fixes #24.
besser82 added a commit that referenced this issue Aug 13, 2020
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, the exported versioned symbols need their
visibility attributes declared explicitly.

Fixes #24.
besser82 added a commit that referenced this issue Aug 13, 2020
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, the exported versioned symbols need their
visibility attributes declared explicitly.

Fixes #24.
besser82 added a commit that referenced this issue Aug 13, 2020
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, the exported versioned symbols need their
attributes declared explicitly.

Fixes #24.
besser82 added a commit that referenced this issue Aug 13, 2020
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.
besser82 added a commit that referenced this issue Aug 14, 2020
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.
besser82 added a commit that referenced this issue Aug 14, 2020
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.
besser82 added a commit that referenced this issue Aug 14, 2020
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.
besser82 added a commit that referenced this issue Aug 14, 2020
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.
besser82 added a commit that referenced this issue Aug 14, 2020
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.
besser82 added a commit that referenced this issue Aug 14, 2020
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.
@solardiz
Copy link
Collaborator

Here's a different perspective, which might or might not be helpful:

LTO feels primarily like an unjustified risk to me: exposing potential cross-translation-unit instances of undefined behavior, making them actually manifest themselves, sometimes as security vulnerabilities. Do we really want that? What for? Instead of supporting LTO, how about explicitly not supporting it - making attempted builds with LTO fail and not produce output files? Document that libxcrypt must not be built with LTO.

Maybe add a flag to "support" and enable build with LTO for stress-testing only (not production), so that we get a chance to detect and fix some of those instances of UB ourselves before someone possibly reuses the code somewhere with LTO or merges source files. Somehow ensure that build wouldn't be inadvertently usable in production.

BTW, I recall @besser82 also wanted to support build with auto-merging of source files, which would have similar bug-exposing and security implications, and I similarly advised against that except maybe as a stress-test.

I'm no LTO expert, and am not up to date on whether it's somehow possibly required somewhere in the ecosystem now. Please correct me if so.

besser82 added a commit that referenced this issue Aug 15, 2020
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.
besser82 added a commit that referenced this issue Aug 15, 2020
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.
besser82 added a commit that referenced this issue Aug 18, 2020
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.
besser82 added a commit that referenced this issue Aug 18, 2020
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.
besser82 added a commit that referenced this issue Aug 18, 2020
GCC 10 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, 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.
@besser82
Copy link
Owner

Accidentally closed this.

@besser82 besser82 reopened this Mar 31, 2021
@zackw zackw added enhancement Requests a new feature or improvement. Without "need more information", we agree it's desirable. help wanted The libxcrypt core developers do not plan to work on this themselves but would review a PR. and removed documentation Primarily an issue with the documentation. labels Mar 26, 2024
@zackw zackw removed their assignment Mar 26, 2024
@zackw
Copy link
Collaborator

zackw commented Mar 26, 2024

It's not clear to me what still needs to be done here. Off the top of my head, some or all of these may be needed:

  • Verify that, when a compiler that supports __attribute__((symver)) is in use, a shared library build with -flto exposes the same set of versioned symbols, including all compatibility aliases and compatibility-only symbols, as a build without that option, for all values of all configure options that affect what symbols are exposed. Test both GCC and Clang.
  • Find and eradicate all instances of undefined behavior that are promoted from latent to manifest bugs by -flto. The test suite should be thorough enough to catch most of them for valid input; I'm not sure about invalid input. A crude but effective way to determine whether the compiler thinks the code might have UB is to compile with -fsanitize=undefined and then look through the machine code for calls to the UBSan runtime. Watch out for compiler bugs exposed by -flto. Testing on at least two non-x86 CPU architectures will be needed.
  • Document the supported compiler versions for use of -flto. (doc: Update LTO support status in README.md #176 makes a start on this.)
  • Add -flto to the CI test matrix.
  • Document whether we think it is a good idea to use -flto. I am currently of the opinion that it is not a good idea, even after all the above testing, basically because I doubt we can be sure enough that we got all the UB (@solardiz said similar things above: lto linking, missing symbols #24 (comment)) and I've heard a number of horror stories about LTO exposing compiler bugs. I am open to persuasion.

What did I miss?

@zackw
Copy link
Collaborator

zackw commented Mar 26, 2024

I mistakenly thought PR #109 had already been merged. Further discussion should probably take place there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Requests a new feature or improvement. Without "need more information", we agree it's desirable. help wanted The libxcrypt core developers do not plan to work on this themselves but would review a PR.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants