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

Prep for upstream llvm change to main mangling. NFC #17099

Merged
merged 1 commit into from Jun 1, 2022
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented May 29, 2022

This change should allow https://reviews.llvm.org/D75277 to land
without breaking emscripten.

In the long run I hope to remove all of this in favor of calling
_start from JS, but that can happen later, and could be user-visible
if folks are calling main themselves.

src/settings_internal.js Show resolved Hide resolved
emscripten.py Show resolved Hide resolved
emcc.py Outdated Show resolved Hide resolved
This change should allow https://reviews.llvm.org/D75277 to land
without breaking emscripten.

In the long run I hope to remove all of this in favor of calling
`_start` from JS, but that can happen later, and could be user-visible
if folks are calling `main` themselves.
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Would it be simpler perhaps to create a function main() that is just a shim to call the new main_argc_argv? if we can do that it would avoid all the JS and python changes in this PR.

@@ -1741,6 +1741,9 @@ def phase_linker_setup(options, state, newargs, user_settings):
# Note the exports the user requested
building.user_requested_exports.update(settings.EXPORTED_FUNCTIONS)

if '_main' in settings.EXPORTED_FUNCTIONS:
settings.EXPORT_IF_DEFINED.append('__main_argc_argv')
Copy link
Member

Choose a reason for hiding this comment

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

A comment here would be good. This stuff is adding a bunch of complexity and comments can at least help the reader with that. The other locations at least use the MANGLED_MAIN setting that the reader can then look up, but this location doesn't even have that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A lot of the complexity where will only last for the transition period, because we need to support both the old and new way that main is exported.

I'm hoping we can simplify this again once https://reviews.llvm.org/D75277 lands.

I don't think a wrapper will work here because __main_argc_argv is the mangled name that clang will start producing for the C main. (This is done using the same mangling mechanism used for C++ symbols in clang.. so __main_argc_argv is main once https://reviews.llvm.org/D75277 lands).

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

I see, thanks, sgtm

@sbc100 sbc100 merged commit 80856b3 into main Jun 1, 2022
@sbc100 sbc100 deleted the main_mangled branch June 1, 2022 15:46
sbc100 added a commit that referenced this pull request Jun 1, 2022
That PR landed before I managed to push these two minor updates.
sbc100 added a commit that referenced this pull request Jun 1, 2022
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.

None yet

2 participants