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

Symbols in optimized async programs are often not useful #65978

Open
cbiffle opened this issue Oct 30, 2019 · 12 comments
Open

Symbols in optimized async programs are often not useful #65978

cbiffle opened this issue Oct 30, 2019 · 12 comments
Labels
A-async-await Area: Async & Await A-codegen Area: Code generation A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cbiffle
Copy link
Contributor

cbiffle commented Oct 30, 2019

(As observed on rustc 1.40.0-nightly (4a8c5b20c 2019-10-23) targeting thumbv7em-none-eabihf; CC @tmandry)

I'm making my first aggressive use of async fn in an application. It's a deeply-embedded performance-sensitive application, and I wind up inspecting the disassembly output a lot (using objdump).

This is complicated by the fact that basically all of my functions are named poll_with_tls_context. (Some of them aren't -- some of them are named after future combinators.)

For example, here is my function called poll_with_tls_context calling another one, also named poll_with_tls_context:

; This is an ARMv-7M Thumb-2 listing.
080003b8 <core::future::poll_with_tls_context>:
 80003b8:       b570            push    {r4, r5, r6, lr}
 80003ba:       4604            mov     r4, r0
   ; irrelevant setup omitted...
 80003f4:       f000 fa3c       bl      8000870 <core::future::poll_with_tls_context> ; note different addr
 80003f8:       2101            movs    r1, #1
 80003fa:       2800            cmp     r0, #0
  ; ...and so on

(The observant reader will note poll_with_tls_context does not appear in libcore. That's correct -- I've hacked async in a #[no_std] environment. I'm pretty sure the hack is not the problem.)

I understand why this is happening: poll_with_tls_context is an implementation detail of the current lowering of async fn, and it is being specialized to the future type it's given, hence many such functions. But I also don't think it's ideal.

(For what it's worth, I can change the situation by forcing poll_with_tls_context to inline, though this produces unacceptable code bloat in my application (and this option isn't available for people who aren't open to using a patched libstd). By default, poll_with_tls_context doesn't inline, but get_task_context does, which seems like the right result for size/speed.)

I am compiling at opt-level = 3 with an override for debug = true in my release profile.

@Mark-Simulacrum
Copy link
Member

It seems like we might want the lowering to emit into debug info (or symbols? not sure if we can use symbols here, i.e., due to duplicates) in such a way that preserves the name of the original function. That seems like something we want, in general.

@Alexendoo Alexendoo added A-codegen Area: Code generation O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 30, 2019
@tmandry
Copy link
Member

tmandry commented Oct 30, 2019

I think debuginfo should already include line-by-line information of code that was inlined. But when looking at objdump or a backtrace, it might not show up. We have to pick one function name which is going to be shown for every stack frame (or symbol).

In this case, there's one monomorphization per call site. Maybe we can add an attribute to poll_with_tls_context which makes the compiler emit a symbol name which includes the caller (sort of like it was inlined), but also append the name of the poll_with_tls_context function to prevent collisions.

@cbiffle
Copy link
Contributor Author

cbiffle commented Oct 31, 2019

If one were to change which symbol is recorded around poll_with_tls_context, it is the callee, not the caller, that's of interest -- the identity of the Future type it's been handed.

@tmandry
Copy link
Member

tmandry commented Oct 31, 2019

Ah, that's right. (I was confused by the fact that #[inline(always)] on poll_with_tls_context happens to do what you want -- it's the secondary effect of that, which causes its callee to be pulled out into its own function -- that you want.)

In that case it's less clear to me how to solve this. poll_with_tls_context can (and does) call multiple functions, and >1 of them can be inlined into it. How would we decide which one has "naming rights" on the symbol?

From the outside, it would look like a caller got inlined into its callee, rather than the other way around.

We could make some annotation that says "when a bunch of functions are inlined into a symbol, don't use the symbol name of this function," and the compiler can pick the name of the first inlined function which doesn't contain this annotation, if any.

Another way is to gather all the functions inlined into one and pick the "most specific function name," i.e. the function that is a candidate for inlining the least number of times. This might work, but it might also have odd unpredictable effects.

I'm not sure how easy these would be to implement.

@tmandry tmandry added the A-async-await Area: Async & Await label Dec 2, 2019
@nikomatsakis nikomatsakis added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Dec 3, 2019
@nikomatsakis
Copy link
Contributor

Marking as triaged. @Centril notes that #66398 is relevant.

@Aaron1011
Copy link
Member

I think this is partially fixed by the new symbol mangling scheme (-Zsymbol-mangling-version=v0), which includes the instantiated parameters in the symbol name. In this case, each monomorphization of poll_with_tls_context shoud include the specific future type from the generic parameter F.

@csmoe
Copy link
Member

csmoe commented May 17, 2020

Close since tls was removed from internal async, feel free to reopen with more concerns.
cc @tmandry

@csmoe csmoe closed this as completed May 17, 2020
@tmandry
Copy link
Member

tmandry commented May 18, 2020

Perhaps someone should compile a program and look at the symbols that come out. It's possible that poll_with_tls_context got replaced with something equally generic.

@tmandry
Copy link
Member

tmandry commented May 26, 2020

Reopening until we confirm that the backtrace is more helpful now.

@tmandry tmandry reopened this May 26, 2020
@cbiffle
Copy link
Contributor Author

cbiffle commented Jun 5, 2020

Hi! I've updated the codebase to a post-resume-arguments toolchain, and I can confirm that the symbols got replaced by something equally generic. :-)

Now most functions in my program are named <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll. (Some of them are still named after combinators, which is also not useful.)

Here's a snippet from the nm output.

08001048 t <futures_util::future::poll_fn::PollFn<F> as core::future::future::Future>::poll
0800107e t <futures_util::future::poll_fn::PollFn<F> as core::future::future::Future>::poll
080010a0 t <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
0800111c t <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
08001224 t <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
080012b0 t <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
08001698 t <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
08004304 t <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
0800437c t <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
08004670 t <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
080046f0 t <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll

...and so forth.

In my ideal world, each of these functions would be named something that links them back to the async fn from which they sprang. Currently I'm inferring it using the DWARF info's file-line information, but that's manual and still error prone due to inlining.

@tmandry tmandry added P-medium Medium priority A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) and removed O-Arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state labels Jun 10, 2020
@crlf0710 crlf0710 added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jun 11, 2020
@wesleywiser
Copy link
Member

This is caused by the legacy (but still default) symbol mangling rustc uses. If you compile with -Csymbol-mangling-version=v0, you should get much more detailed symbol names.

Using an example from the reqwest crate, I see a lot of symbols like the following when compiling with the default settings:

...
<core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
...

and when building with RUSTFLAGS="-Csymbol-mangling-version=v0":

...
<core::future::from_generator::GenFuture<form::main::{closure#0}> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<<hyper::client::conn::SendRequest<reqwest::async_impl::body::ImplStream>>::when_ready::{closure#0}> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<<tokio::net::tcp::socket::TcpSocket>::connect::{closure#0}> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<<tokio::net::tcp::stream::TcpStream>::connect_mio::{closure#0}> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<<reqwest::connect::Connector>::connect_via_proxy::{closure#0}> as core::future::future::Future>::poll
<core::future::from_generator::GenFuture<<reqwest::connect::Connector>::connect_with_maybe_proxy::{closure#0}> as core::future::future::Future>::poll
...

@Swatinem
Copy link
Contributor

#104321 removed the GenFuture shim, and also closed #74779 in the process.

I’m wondering if there is anything more here to do, or this issue can be closed?
I opened #104830 as a separate issue that symbol mangling does not show async fn differently from {closure#0}. I tried to take a stab at that in #104333, but would need some mentoring to make progress there, especially as that would require changes to the symbol mangling format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-codegen Area: Code generation A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-enhancement Category: An issue proposing an enhancement or a PR with one. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants