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

Add a feature + cfg attribute to exclude GNU extension functions #1116

Closed
shravanrn opened this issue Sep 1, 2019 · 23 comments
Closed

Add a feature + cfg attribute to exclude GNU extension functions #1116

shravanrn opened this issue Sep 1, 2019 · 23 comments

Comments

@shravanrn
Copy link

Hi,
I was wondering if it would be possible to add a feature flag and the appropriate cfg calls to separate POSIX functions from the GNU specific extensions (usable when _GNU_SOURCE is defined).

My use case is as follows. I am using some packages (lucet) that have a dependency on the nix packages. While I'm using these packages in a Linux system, we are linking against libraries without the GNU extension functions... Thus the presence of calls to process_vm_writev, process_vm_readv and setns is causing problems. Additionally, it seems that the nix wrappers for these functions cannot be eliminated by dead code removal, and so persist in the final library, resulting in missing symbol errors.

Currently, it seems that these functions are protected only with #[cfg(any(target_os = "linux"))]. Thus the easiest workaround that comes to mind is to pretend that target_os is not linux, but this feels hacky at best... I was wondering if it would be possible to add a new feature flag which we can set at compile time, that would exclude these GNU specific functions...

(also, I apologize if I got some details wrong... I'm still quite new to rust :) )

Please let me know... If required, I am also happy to contribute a patch for the same...

@asomers
Copy link
Member

asomers commented Sep 1, 2019

What C library are you using? musl? newlib? something else? The correct solution would be to guard such functions with a #[cfg(target_env = ...)]

@shravanrn
Copy link
Author

Thanks a bunch for the quick response! Oh, that's interesting... I just read up about target_env, that sounds like the right solution, and I believe should be sufficient for my needs. Let me know if this fix is something this will be addressed by you/one of the other nix devs, or something I should be contributing a patch for.

I am following up about the exact libc we use (I'm not overly familiar with that part of our build system and so am waiting to hear back from others about this. I will post here as soon as I find out)

@asomers
Copy link
Member

asomers commented Sep 1, 2019

Please submit a PR. Nix thrives on the contributions of our users.

@shravanrn
Copy link
Author

Ah, so apparently we do use glibc, its just that we don't allow binaries that make calls to GNU_SOURCE functions... This actually means, that the target_env solution may not be enough to exclude the GNU specific functions.

While it would be ideal to add #[cfg(target_env = "gnu")] to functions like setns with and then exclude it via a suitable target triple, this doesn't quite work for us... The target triple we are forced to use is x86_64-unknown-linux-gnu, although in spirit what want is something like x86_64-unknown-linux-unknown (even though this doesn't exist)

Question - Would you be willing to accept a PR if I add both #[cfg(target_env = "gnu")] and a feature flag, so that we can control whether GNU functions are included through a build parameter?

shravanrn added a commit to shravanrn/nix that referenced this issue Sep 1, 2019
… GNU function wrappers are included more selectively
shravanrn added a commit to shravanrn/nix that referenced this issue Sep 1, 2019
@asomers
Copy link
Member

asomers commented Sep 1, 2019

I still don't understand exactly what your problem is. If the C library is missing a function that Nix wraps, that shouldn't be a problem. It happens all the time, in fact, because libc and Nix define functions newly added on recent OS versions, but still compile and test on older versions. I just checked, and it wasn't a problem to add a bogus extern "C" definition to Nix. It's only a problem if I try to call that function from a test. Can you show me exactly what error you're getting?

@shravanrn
Copy link
Author

shravanrn commented Sep 1, 2019

Ah, so sorry for the confusion... I think I finally got to the bottom of this. I believe you are completely right... I just did the same test of a bogus extern C and wrapping it. This results in a few undefined symbols in the resulting binary. While this is not an error by itself, we have additional build checks in our project that mark any missing symbols as an error causing build breaks on our side...

So definitely my bad with the earlier description... I think the amended description looks something like this - We are linking against a version of glibc without the GNU extension functions. Thus the presence of calls to process_vm_writev, process_vm_readv and setns results in undefined symbols in the binary. Part of our build infrastructure makes any missing symbols a build failure, which means including nix in our source causes our build infrastructure to complain.

Please let me know if this makes sense...

With regards to use of nix in our codebase, I think this sort of leaves us in the same place as before... I'm wondering if it would be reasonable to accept a feature flag to exclude such gnu source functions? This would allow us to use upstream nix as is, instead of a having to maintain a fork with this one extra patch.

@asomers
Copy link
Member

asomers commented Sep 1, 2019

No, we wouldn't accept such a patch. When using Rust with FFI, missing symbols are the norm. You need to fix your build infrastructure.

@shravanrn
Copy link
Author

Alright, thanks for the info.

@froydnj
Copy link

froydnj commented Sep 1, 2019

Hi, I'd like to ask that this bug be reopened, possibly with the title modified.

No, we wouldn't accept such a patch. When using Rust with FFI, missing symbols are the norm. You need to fix your build infrastructure.

The issue runs thusly: the Firefox build infrastructure builds against an older version of glibc to ensure the broadest possible compatibility for official Firefox releases (GLIBC_2.12 symbol version). Symbols like process_vm_readv, which are the ones being modified by the proposed patch, come with a symbol version of GLIBC_2.15 and are therefore not available to link against. So when one transitively depends on nix-rust, even if you don't call process_vm_readv and friends, you still have to wind up linking against a glibc that supports those functions so your code will actually build. I suspect you don't see this on your CI because your older versions of glibc (I can't tell what comes with Ubuntu trusty, but I suspect it has GLIBC_2.15 symbol versions) are still newer than what we use.

Just because the target_env is gnu, that does not imply that all of the functions in the most recent revision of glibc are necessarily available.

You might say that lto = true should be set in Cargo.toml. And that works (I think) for release profiles, but it's not true by default in dev profiles and changing the dev profile to do LTO by default is not acceptable.

@asomers
Copy link
Member

asomers commented Sep 1, 2019

But as I explained earlier, I can add complete bogus extern "C" function definitions to Nix, and even call them from Rust functions, without a problem. The regular Rust tools have no problem with missing symbols. What are you doing differently? Even if we were to remove (or disable) those functions in Nix, they would still be defined in libc, which you must link against too.

@froydnj
Copy link

froydnj commented Sep 1, 2019

But as I explained earlier, I can add complete bogus extern "C" function definitions to Nix, and even call them from Rust functions, without a problem. The regular Rust tools have no problem with missing symbols. What are you doing differently?

Possibly compiling with -Wl,-z,defs, which requires all symbols to resolve to something at link time.

Even if we were to remove (or disable) those functions in Nix, they would still be defined in libc, which you must link against too.

They are not defined in the libc we are linking against, as I explained earlier.

@asomers
Copy link
Member

asomers commented Sep 1, 2019

But as I explained earlier, I can add complete bogus extern "C" function definitions to Nix, and even call them from Rust functions, without a problem. The regular Rust tools have no problem with missing symbols. What are you doing differently?

Possibly compiling with -Wl,-z,defs, which requires all symbols to resolve to something at link time.

Even if we were to remove (or disable) those functions in Nix, they would still be defined in libc, which you must link against too.

They are not defined in the libc we are linking against, as I explained earlier.

Sorry; there are two different things named "libc". I meant they would still be defined in rust-lang/libc, which you must link against too.

@shravanrn
Copy link
Author

So a missing extern C function doesn't cause issues with respect to missing symbols... But the nix wrapper that calls such a function does result in missing symbols in the binary... Hence rust/libc is not an issue here

@froydnj
Copy link

froydnj commented Sep 1, 2019

It's possible that we don't have a new enough version of rust-lang/libc to contain process_vm_readv and friends? Assuming that rust-lang/libc's version just makes the syscall directly, rather than just being a wrapper for the glibc version.

@asomers
Copy link
Member

asomers commented Sep 1, 2019

@froydnj libc never makes syscalls directly; it always calls the system's C library (except when the symbol in question is a macro, in which case rust-lang/libc must reimplement it). And you most certainly have a sufficiently recent version of libc, because Nix would pull it in.

I understand your problem, but your proposed solution is far too narrow. C libraries add new symbols all the time. Even POSIX adds new functions, and glibc doesn't adopt them all at once. If Nix were to adopt your feature flag, it would probably only work for a narrow range of glibc versions.

If Nix were a C library, it would solve this problem with something like autoconf and a bunch of #ifdefs. But Nix is a Rust crate and Rust uses FFI. FFI is great for cross-compiling, but it makes autoconf-like functionality nearly impossible. Your problem isn't specific to the Nix crate; it could happen anywhere. Normal Rust users simply don't use -z defs. Have you considered asking your question on https://internals.rust-lang.org/ ?

@froydnj
Copy link

froydnj commented Sep 1, 2019

I understand your problem, but your proposed solution is far too narrow. C libraries add new symbols all the time. Even POSIX adds new functions, and glibc doesn't adopt them all at once. If Nix were to adopt your feature flag, it would probably only work for a narrow range of glibc versions.

I agree that the proposed solution in the patches upthread is far too narrow. A more widely applicable solution would be for nix and maybe rust-lang/libc to expose features corresponding to glibc symbol versions. Would that be an acceptable patch?

Normal Rust users simply don't use -z defs.

Our position is that we'd like our software to run on the widest range of machines as is feasible. We'd prefer to be told about problems with that goal at build time rather than after our users download the software. -Wl,-z,defs is a reasonable contribution towards that goal. Please don't dismiss it as "abnormal Rust" or that we are somehow using Rust incorrectly.

Have you considered asking your question on https://internals.rust-lang.org/ ?

To what end?

@asomers
Copy link
Member

asomers commented Sep 1, 2019

I understand your problem, but your proposed solution is far too narrow. C libraries add new symbols all the time. Even POSIX adds new functions, and glibc doesn't adopt them all at once. If Nix were to adopt your feature flag, it would probably only work for a narrow range of glibc versions.

I agree that the proposed solution in the patches upthread is far too narrow. A more widely applicable solution would be for nix and maybe rust-lang/libc to expose features corresponding to glibc symbol versions. Would that be an acceptable patch?

Are you talking about ELF symbol versioning, or something that glibc does with the preprocessor? I didn't think that glibc made use of ELF symbol versioning.

Normal Rust users simply don't use -z defs.

Our position is that we'd like our software to run on the widest range of machines as is feasible. We'd prefer to be told about problems with that goal at build time rather than after our users download the software. -Wl,-z,defs is a reasonable contribution towards that goal. Please don't dismiss it as "abnormal Rust" or that we are somehow using Rust incorrectly.

Have you considered asking your question on https://internals.rust-lang.org/ ?

To what end?

To see if anybody else has run into the same problem. Nix is far from the only crate that calls native C functions, and you may not be the only person concerned with missing symbols.

@froydnj
Copy link

froydnj commented Sep 1, 2019

Are you talking about ELF symbol versioning, or something that glibc does with the preprocessor? I didn't think that glibc made use of ELF symbol versioning.

I am talking about ELF symbol versioning. glibc is the motivating example for porting over SunOS/Solaris's symbol versioning feature, cf https://www.akkadia.org/drepper/symbol-versioning.

@asomers
Copy link
Member

asomers commented Sep 1, 2019

I didn't think that glibc used it. FreeBSD does, and that causes other problems with rust-lang/libc. Do you have an idea for how libc (or other FFI projects) could use ELF symbol versioning more intelligently?

@froydnj
Copy link

froydnj commented Sep 1, 2019

I didn't think that glibc used it. FreeBSD does, and that causes other problems with rust-lang/libc. Do you have an idea for how libc (or other FFI projects) could use ELF symbol versioning more intelligently?

I didn't realize that it was already a problem for rust-lang/libc on FreeBSD! Have there been any proposals in this space? The idea I referenced upthread was:

[features]
glibc_2_23_or_later = ["glibc_2_23", "glibc_2_22_or_later"]
glibc_2_22_or_later = ["glibc_2_22", "glibc_2_21_or_later"]
...

and then annotating each symbol that needs it with the appropriate feature check ("glibc_2_22", etc.). That's a lot of clutter, though.

@asomers
Copy link
Member

asomers commented Sep 2, 2019

No, that's not ELF symbol versioning. ELF symbol versioning is how a library can contain multiple versions of the same symbol. For example, fstat@FBSD_1.0 and fstat@FBSD_1.5. Binaries compiled for FreeBSD 11 or older will link to the former. Binaries compiled for FreeBSD 12 or later will link to the latter, even though the source code is exactly the same. It works well for C programs, but it's problematic with Rust.

@froydnj
Copy link

froydnj commented Sep 2, 2019

glibc uses exactly the same mechanism as FreeBSD, as the paper above suggests. You have, for process_vm_readv, which is a versioned symbol:

froydnj@hawkeye:~/src$ readelf -sW /lib/x86_64-linux-gnu/libc-2.23.so |grep process_vm
   491: 0000000000108040    36 FUNC    GLOBAL DEFAULT   13 process_vm_writev@@GLIBC_2.15
  1811: 0000000000108010    36 FUNC    GLOBAL DEFAULT   13 process_vm_readv@@GLIBC_2.15

And glibc also has multiple versions of the same symbol, using memcpy as an example:

froydnj@hawkeye:~/src$ readelf -sW /lib/x86_64-linux-gnu/libc-2.23.so |grep GLIBC |grep memcpy
   335: 00000000000ac890     9 FUNC    WEAK   DEFAULT   13 wmemcpy@@GLIBC_2.2.5
  1014: 00000000001179d0    25 FUNC    GLOBAL DEFAULT   13 __wmemcpy_chk@@GLIBC_2.4
  1132: 00000000000943f0   106 IFUNC   GLOBAL DEFAULT   13 memcpy@@GLIBC_2.14
  1134: 000000000008f14b    87 IFUNC   GLOBAL DEFAULT   13 memcpy@GLIBC_2.2.5
  1637: 0000000000116210   104 IFUNC   GLOBAL DEFAULT   13 __memcpy_chk@@GLIBC_2.3.4

memcpy comes in two different flavors.

As for supporting multiple versions of the same symbol, I'm not sure what a good solution looks like. I can see a crate with features for each of fstat@FBSD_1.0 and fstat@FBSD_1.5, and you have to pick one that gets exposed as fstat when you compile the crate. Each one is available explicitly through some sort of mangled name. Client crates could probably work out some scheme for depending on the same library compiled with different features, but that might get messy pretty quickly. Maybe it's OK if it's messy, as I wouldn't expect people to rely on multiple versions of the same symbol very often?

@asomers
Copy link
Member

asomers commented Sep 2, 2019

I don't think there's any reason for a Rust crate to export multiple versions of the same symbol, unless it's a crate that's normally built as a shared library. The problem is that a Rust crate may need to link to a C crate that does export multiple versions, and right now there's no way to select which one to use except "always use the oldest supported symbol version". Here's a longer discussion.
rust-lang/libc#570

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants