Skip to content

Stop running --inline-main #11995

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

Merged
merged 5 commits into from
Aug 23, 2020
Merged

Stop running --inline-main #11995

merged 5 commits into from
Aug 23, 2020

Conversation

kripken
Copy link
Member

@kripken kripken commented Aug 21, 2020

LLVM adding __original_main can be confusing for users, as it shows up in
stack traces, and they don't expect it. So we added a pass to inline that into
main, and we ran it on all builds, debug and release. However, this has some
downsides:

  • As a comment that this PR removes shows, we actually stopped doing
    this in debug builds because it can mess with DWARF. So the place we need
    it most is no longer relevant anyhow, and it wasn't actually helping us with
    debugging - @dschuff unless I'm missing something?
  • It is more work and complexity during link, which we are trying to reduce,
    see Reduce modifications done in wasm-emscripten-finalize WebAssembly/binaryen#3043
  • There are some planned changes to these functions and _start and all
    that anyhow (see discussion on the PR) so this will become irrelevant soon.

This does not affect behavior, unless you look at the wasm internals. But a noticeable
effect is that if you use ASYNCIFY_ADD or ASYNCIFY_ONLY then you may need
to add __original_main to there (since you are doing manual fine-tuning of
the list of functions, which depends on the wasm's internals) - see the test change
here.

Note that this should not matter in -O2+ anyhow as normal inlining generally removes
__original_main.

@kripken kripken requested a review from sbc100 August 21, 2020 23:07
@dschuff
Copy link
Member

dschuff commented Aug 21, 2020

I think you meant for your link to be WebAssembly/binaryen#3043

Do we have a bug or doc for the planned changes to _start? I know we talked about them but can't remember the details.

@dschuff
Copy link
Member

dschuff commented Aug 21, 2020

and i guess as a followup question, is _original_main still part of the plan going forward?

@kripken
Copy link
Member Author

kripken commented Aug 21, 2020

I think you meant for your link to be WebAssembly/binaryen#3043

Oh, yes... thanks!

Do we have a bug or doc for the planned changes to _start? I know we talked about them but can't remember the details.

I also just remember talking to Sam about wanting to reduce differences in emscripten vs. what LLVM already emits, in general.

@dschuff
Copy link
Member

dschuff commented Aug 22, 2020

Do we have a bug or doc for the planned changes to _start? I know we talked about them but can't remember the details.

I also just remember talking to Sam about wanting to reduce differences in emscripten vs. what LLVM already emits, in general.

OK, just wondering because that's a long and complex explanation in the release notes... just wondering if that situation was final or if we'd be changing it again. If we're changing it again, then it might be worth mentioning that it's a transitory state.

@sbc100
Copy link
Collaborator

sbc100 commented Aug 22, 2020

There are two different followup changes happening with main and the entry point in general.

  1. Switching llvm to use the new name mangling approach for main that WASI is using. This removing __original_main but adds new complications where main is aliased or mangled.

  2. Move to using _start in emscripten so that the actual name of the main function becomes less relevant.

I suggest we try to do these at the same time to avoid too much churn.

I also wonder if that ChangeLog entry is TMI for most readers. It guess this implementation detail can leak out so we don't have a choice but if you can think of way to simplify it or summarize it in a more digestible way I think that might be useful.

Otherwise lgtm.

@sbc100
Copy link
Collaborator

sbc100 commented Aug 22, 2020

Actually having re-read the ChangeLog it looks fine to me.

@kripken kripken merged commit e41d9e3 into master Aug 23, 2020
@kripken kripken deleted the noinlinemain branch August 23, 2020 21:32
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

Successfully merging this pull request may close these issues.

3 participants