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 IGNORE_MISSING_MAIN setting and --no-entry argument #10827

Merged
merged 1 commit into from
Apr 3, 2020
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Apr 2, 2020

This setting, wen disabled, forces they developer to be explicit
about the presence of absence of main their program. It is enabled
by default but disabled in STRICT mode.

When disabled, if you want to build a program without an main function
one must either build with --no-entry or pass a list of
EXPORTED_FUNCTIONS the doesn't include _main.

The reason for adding the extra --no-entry method is that this
is what wasm-ld recommends in its corresponding error message. Its
also a lot easier to type than -s EXPORTED_FUNCTIONS=[].

This change is a step towards enabling better error reporting from lld
by enabling the removal of the --allow-undefined option. See #10826

Partia fix for: #9640

@sbc100 sbc100 requested a review from kripken April 2, 2020 01:28
@sbc100 sbc100 force-pushed the no_entry branch 4 times, most recently from 92a2cea to 6a9aa23 Compare April 2, 2020 04:30
@sbc100 sbc100 changed the title Add AUTO_DETECT_MAIN setting and --no-entry argument Add IGNORE_MISSING_MAIN setting and --no-entry argument Apr 2, 2020
@kripken
Copy link
Member

kripken commented Apr 3, 2020

This setting, when enabled, forces they developer to be explicit
about the presence of absence of main their program. It is disabled
by default but enabled in STRICT mode.

This setting == IGNORE_MISSING_MAIN? Then if it's enabled, we would ignore a missing main, which is the opposite of being explicit about the presence of main? Or am I looking at this wrong?

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 3, 2020

Sorry, I renamed the setting and it inverted it meaning.

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.

This seems reasonable, but how bad would it be to not add this option, and just change to this behavior? It would be a breaking change for libraries, but that's a smaller amount of users, and it would be an easy update/fix for them, with a clear error?

tools/shared.py Outdated
if not use_start_function:
cmd += ['--no-entry']
has_main = '_main' in Settings.EXPORTED_FUNCTIONS
Copy link
Member

Choose a reason for hiding this comment

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

instead of has, how about exports?

src/settings.js Outdated
// * AUTO_JS_LIBRARIES is disabled.
// * AUTO_ARCHIVE_INDEXES is disabled.
// [compile+link]
var STRICT = 0;

// Allow program to link with or without `main` symbol.
// If this is disabled then one must provide a `main` symbol or expclictly
Copy link
Member

Choose a reason for hiding this comment

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

typo in explicitly

emscripten.py Show resolved Hide resolved
@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 3, 2020

The idea with having it behind and option is that is that we can introduce it gradually and back it out if people object. Since its technically a breaking change.

It also allows me to enable it unconditionally in places like STRICT and with LLD_REPORT_UNDEFINED which it turns out needs this feature.

I'd love to get rid of the option ASAP and just have it be the default. In fact I think I can make this an internal option so no user should ever be exposed to it.

@kripken
Copy link
Member

kripken commented Apr 3, 2020

I see, thanks, sgtm. And lgtm with those comments.

This settings, when enabled, forces the developer to be explicit
about the presence or absence of `main` their program.  It is disabled
by default but enabled in STRICT mode.

In this mode, if you want to build a program without a `main` function
you must either build with `--no-entry` or pass a list of
EXPORTED_FUNCTIONS the doesn't include `_main`.

The reason for adding the extra `--no-entry` option is that this
is what `wasm-ld` recommends in its corresponding error message.  Its
also a lot easier to type than `-s EXPORTED_FUNCTIONS=[]`.

This change is a step towards enabling better error reporting from lld
by enabling the removal of the `--allow-undefined` option.
@sbc100 sbc100 merged commit 05f6384 into master Apr 3, 2020
@sbc100 sbc100 deleted the no_entry branch April 29, 2020 01:26
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.

2 participants