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

RFC: -C export-executable-symbols #2841

Merged

Conversation

MaulingMonkey
Copy link
Contributor

@MaulingMonkey MaulingMonkey commented Dec 29, 2019

Rendered

Add the ability to export symbols from executables, not just dylibs, via a new compiler flag: -C export-executable-symbols.

(Bonus: I've also implemented and used this flag as a proof-of-concept to make sure I am indeed solving my problems with this suggestion, but I'm happy to rewrite or discard that if another approach seems better.)

@retep998
Copy link
Member

How would the user specify this in the normal cargo world without custom rustc flags?

@MaulingMonkey
Copy link
Contributor Author

Specifying a .cargo/config file alongside/above Cargo.toml in the project tree works, but it'd be possible to extend Cargo.toml manifests as well - the most similar looking options all live under [profile.*]. Could be a new option:

[profile.dev]
export-executable-symbols = true

On the other hand, this is a niche enough use case, that just leaving it as a custom rustc flag might be more than enough. For my own use cases, I'm likely to want to apply the setting to an entire workspace once and forget about it - instead of configuring it on a per-profile or per-package basis - so I'm likely to resort to an in-tree .cargo/config regardless.

@tanriol
Copy link

tanriol commented Dec 30, 2019

Would be useful to me too. So far for me on Linux is mostly just works, except that I needed to add a code path that may call the same exported functions from the executable itself because otherwise they were discarded as dead code. Would love to get rid of that hack.

@pnkfelix
Copy link
Member

pnkfelix commented Dec 31, 2019

Its sort of unfortunate that this RFC is written in a fashion where it sounds like it is saying "just do what dylibs do, but on executables", when at least currently, the semantics of dylibs are ... in flux at best.

(That is, to my knowledge, there is not much specification for dylibs themselves beyond "keep rustc working, and maybe try to keep compiler-plugins working for a little while longer." For some examples of the complications involved, see the discussion on rust-lang/rust#37530.)

So let me ask some basic questions, where I would like an answer more specific than "just do what dylibs do":

  • Is this going to export solely symbols for items marked pub ? If so, it might be good to make that point explicit here. (At least, I don't think I saw visibility mentioned anywhere.)

  • Do you need fine grain control over which symbols are exported and which ones aren't?

  • Potentially related to the previous bullet: Why not make this an attribute in the source rather than a compiler-flag? Are there cases where embedding this info into the Rust source itself would be a mistake or an anti-pattern?

    • An attribute could here mean either something like #[export] that you'd put on every item whose symbol you want exported, or it could mean an attribute like #![export_all_symbols] that you'd just write once in the root main.rs file.
    • (Even if you need the behavior to be applied to dependencies in your crate graph, you could still get that semantics via an attribute with a global-effect included in the source code of the executable crate itself.)

@retep998
Copy link
Member

The behavior of which symbols to export should match what we do for cdylib because anything else would be inconsistent and confusing. I'd love if we could improve that behavior but I don't think this RFC is the place to do that.

A crate wide attribute to toggle exporting symbols is a valid alternative and should go in the alternatives section. I am neutral on crate wide attribute vs rustc flag and would be happy with either.

@MaulingMonkey
Copy link
Contributor Author

Is this going to export solely symbols for items marked pub ? If so, it might be good to make that point explicit here. (At least, I don't think I saw visibility mentioned anywhere.)

I'm interested in explicitly annotated items:

  • #[no_mangle] pub extern "..." fn s, which I'm after right now for JNI interop.
  • #[used] #[no_mangle] pub static s have possible uses, although I don't have any immediate need for these.

However, pub here means public to the mod, not necessairly the crate. Exposing these publicly all the way to the crate root, while doable, would be awkward in the likely event that the item names are generated by macros for the purpouse of mangling to some other language's naming standard.

The current behavior seems to be to export anything tagged #[no_mangle] (including main and rust_eh_personality from Rust itself), but otherwise ignoring visibility and ABI entirely.

Do you need fine grain control over which symbols are exported and which ones aren't?

I don't, although I could see some proprietary devs not wanting to export the entire universe. They'd likely have similar concerns with dylibs, however.

Potentially related to the previous bullet: Why not make this an attribute in the source rather than a compiler-flag? Are there cases where embedding this info into the Rust source itself would be a mistake or an anti-pattern?

A source attribute would probably work fine? My only possible concern would be ease of implementation - piping that information up to the linking stage - although I'd be happy to take a stab at it if that seems preferable.

An attribute could here mean either something like #[export] that you'd put on every item whose symbol you want exported, or it could mean an attribute like #![export_all_symbols] that you'd just write once in the root main.rs file.

Finer grained control over exports could be useful to dylibs as well, for those who want fine grained control. E.g. one might have middleware looking to link against a specific known #[no_mangle] symbol for allocation or logging purpouses - yet wish to deter reverse engineering by statically linking in said middleware, and not exporting the symbol dynamically, even when building a dynamic library for Android, while still needing to export the main entry points for Android to call into.

@Lokathor
Copy link
Contributor

Remember that using no_mangle effectively puts the symbol at the crate root because module location within a crate is performed with mangling.

@gilescope
Copy link

Would this allow us to write integration tests against an exe? Currently we can only do it against libs. (I.e. tests in the tests dir). This leads to a lot of exes being changed into libs just for test purposes...

@MaulingMonkey
Copy link
Contributor Author

Would this allow us to write integration tests against an exe? Currently we can only do it against libs. (I.e. tests in the tests dir). This leads to a lot of exes being changed into libs just for test purposes...

Not on its own.

# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

On a technical level, this just involves preventing an early bailout when
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a reference level explanation of the feature, but rather an approach to implementation. For RFC process this is not too useful and is prone to becoming invalid and impossible to understand in the future or in context of alternative implementations.

Instead this section should be exhaustively enumerating interactions with other language features (@pnkfelix mentioned for instance visibility) and describing how the relevant portions (symbols) of produced artifacts change as a result.

Alternatives:

- Unconditionally export symbols from executables instead of introducing a new compiler flag.
- Introduce a crate-level attribute instead of a compiler flag (`#![export_all_symbols]`? `#![export_symbols]`?)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in discussion already, add the alternative: introduce an item-level attribute to only "in-executable export" specific items.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see that as well. I feel like exporting a symbol should be an item-level attribute, and the top-level compiler flag should only be for overrides (like "don't export anything despite those attributes").

# Prior art
[prior-art]: #prior-art

C and C++ compilers can already do this via `__declspec(dllexport)` annotations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dllexport was a part of a previous (closed) RFC #1296 and discussed fairly extensively. Might be worth it to look through that RFC to see if we didn’t miss anything.

@jonas-schievink jonas-schievink added A-ffi FFI related proposals. T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC. labels Mar 7, 2020
@nikomatsakis
Copy link
Contributor

Wearing my lang team hat: I am nominating this PR for discussion by @rust-lang/compiler. During the @rust-lang/lang "Backlog Bonanza" meeting we concluded that we feel like this is more of a "compiler team" matter and that we wouldn't require close supervision. However, we wanted to check in with the compiler team to see if they agreed that this was more of a compiler team matter. =)

Wearing my compiler team hat: I feel like these sorts of flags are more the compiler's area, but I'm also not sure who among us really "owns" these kinds of flags.

@bjorn3
Copy link
Member

bjorn3 commented Oct 22, 2020

Would this allow us to write integration tests against an exe? Currently we can only do it against libs. (I.e. tests in the tests dir). This leads to a lot of exes being changed into libs just for test purposes...

That would require including crate metadata in the executable. This may also be useful for Headcrab to compile rust code against the debugged executable and inject it. for a counter to the print command of gdb and lldb that accepts arbitrary rust code. DWARF debuginfo doesn't provide enough information for arbitrary rust code. Only for the tiny subset of rust that gdb and lldb accept.

@spastorino
Copy link
Member

@rustbot modify labels: -I-nominated

This was discussed during today's compiler team meeting

@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2020

Error: This repository is not enabled to use triagebot.
Add a triagebot.toml in the root of the master branch to enable it.

Please let @rust-lang/release know if you're having trouble with this bot.

@pnkfelix
Copy link
Member

pnkfelix commented Nov 6, 2020

@rfcbot fcp merge

We discussed this a bit in today's T-compiler triage meeting.

There was general agreement that we want to push forward on experimental implementation here. We can revisit the unresolved questions before stabilizing the flag. (But the text does need to be updated with the questions I raised, in the unresolved questions section, so that we don't lose track of them.)

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 6, 2020

Team member @pnkfelix has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Nov 6, 2020
@nikomatsakis
Copy link
Contributor

Wearing my @rust-lang/lang lead hat, I went ahead and checked the boxes for @cramertj, @joshtriplett, @withoutboats, and @scottmcm, since @rust-lang/lang opted out from this RFC and wanted to delegate it to the compiler team. If any of you have concerns, please feel free to raise an objection or ping me privately.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Nov 15, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 15, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Nov 25, 2020
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 25, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@Aaron1011
Copy link
Member

Was this supposed to get merged?

@nikomatsakis
Copy link
Contributor

It was.

@nikomatsakis
Copy link
Contributor

Hmm, I was going to merge this, but I don't seem to have write access. I'm not sure if that's a result from leaving @rust-lang/core or some other thing:

remote: Resolving deltas: 100% (15/15), completed with 4 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: At least 1 approving review is required by reviewers with write access.
To github.com:rust-lang/rfcs.git
 ! [remote rejected]   master -> master (protected branch hook declined)
error: failed to push some refs to 'git@github.com:rust-lang/rfcs.git'

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 13, 2021

I did create a tracking issue rust-lang/rust#84161 and I pushed a merged version to my work.

@Mark-Simulacrum
Copy link
Member

I'll take a look shortly - we've not had a principled story on permissions for the RFCs repo.

@Manishearth
Copy link
Member

I feel like the lang team used to have merge permissions on this repo, but that might have been individually granted.

@Mark-Simulacrum Mark-Simulacrum force-pushed the pr/export-executable-symbols branch from 2f636a3 to 804dbdd Compare April 13, 2021 16:50
@Mark-Simulacrum Mark-Simulacrum merged commit 181c597 into rust-lang:master Apr 13, 2021
@Mark-Simulacrum
Copy link
Member

Oh, I suspect the branch protection means @nikomatsakis you'd need to push to the branch of this PR and then push to master via a merge (after approving the commits here), rather than a direct push to master. I don't think there was actually a permissions problem in the "you don't have write" sense.

@nikomatsakis
Copy link
Contributor

@Mark-Simulacrum oh, I see, yes, that makes sense. I thought it might have to do with branch protection but I didn't get what I was missing. Thanks!

@pickfire

This comment has been minimized.

@tmandry
Copy link
Member

tmandry commented Aug 24, 2022

Finer-grained attributes weren't added to the RFC as an alternative. This is what C/C++ compilers have, so I would definitely expect it to be mentioned in that section, and from #2841 (comment) it looks like this was discussed and considered a useful idea.

Should we update the RFC?

[prior-art]: #prior-art

C and C++ compilers can already do this via `__declspec(dllexport)` annotations.
Most people don't really notice it, for good or for ill.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also -rdynamic in gcc/clang (or --export-dynamic at the linker) which exports all (visible) symbols from the constructed dynamic ELF (I assume it's a no-op when building an .so).

Possibly interesting as prior art since it's a universal modifier (i.e. not per-symbol) which matches the RFC proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ffi FFI related proposals. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-compiler Relevant to the compiler team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.