Skip to content

Conversation

@juj
Copy link
Collaborator

@juj juj commented Feb 1, 2020

Various fixes when running core test suite with MINIMAL_RUNTIME=1 enabled.

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.

Nice work! Anything not commented on looks fine (so almost all of it).

@juj juj force-pushed the more_minimal_runtime_code_building branch 2 times, most recently from 573cecb to d5fbcb1 Compare February 4, 2020 17:12
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.

lgtm with those final comments.


if shared.Settings.MINIMAL_RUNTIME or 'MINIMAL_RUNTIME=1' in settings_changes or 'MINIMAL_RUNTIME=2' in settings_changes:
# Remove the default exported functions 'malloc', 'free', etc. those should only be linked in if used
shared.Settings.DEFAULT_LIBRARY_FUNCS_TO_INCLUDE = []
Copy link
Member

Choose a reason for hiding this comment

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

how about erroring if DEFAULT_LIBRARY_FUNCS_TO_INCLUDE is in settings_changes? (That is, error if the user tries to add stuff which are otherwise removed silently here.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The intent is to enable user to pass stuff explicitly from command line. I.e. if user wants to do -s DEFAULT_LIBRARY_FUNCS_TO_INCLUDE =[malloc] in MINIMAL_RUNTIME, then we should include malloc.

src/library.js Outdated
abort('stack overflow')
},

_get_executable_name: function() {
Copy link
Member

Choose a reason for hiding this comment

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

how about $getExecutableName as this is a pure JS helper, not an implementation of a C function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is intended to be internal, compare to e.g. __handle_stack_overflow above? For other functions like this we have usually prepended underscores (in html5.h _get_canvas_element_size, In WebGL _heapAccessShiftForWebGLHeap) - though we do have both underscore and lower camelcase conventions.. I'll change to _getExecutableName.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But isn't that what the $ prefix is supposed to be for? I thought the $ was the way we prevent symbols from being exposed to the native code?

For example if you are building with MAIN_MODULE=1 or some other flag this is was to exclude the JS-only internal symbols.

Or am I missunderstanding?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding from the past years has been that $ prefix is to avoid C name mangling taking place for the symbol. Functions that have a $ prefix have the prefix so that they are more convenient to call from JS code (to make them look nicer to call without the underscore, that makes them look 'more public').

If there is a feature/semantic that symbols that start with $ should not be imported to Wasm/asm module, that is something I haven't been aware of before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It certainly looks from the codebase like $camelCase should be used for any symbols you don't expect native code to call (i.e. any symbols that is called by JS only such as a utility function).

If you take $readAsmConstArgs for exaple, if I remove the $ I can write a C program that references that symbols.

With the $ prefix in there I get error: undefined symbol: readAsmConstArgs. I'm not sure if this was deliberate but its sure nice to limit the symbols exposed to the native code.

I think this is how @kripken described it to me.. but maybe the actual original indent was different?

Copy link
Member

@kripken kripken Feb 7, 2020

Choose a reason for hiding this comment

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

We don't have clear rules here. My feeling is that if something could be a plain JS function,

function someThing() {
}

then if it's in the JS library - as an implementation detail, instead of putting it in the JS directly - then it should be $someThing.

However, for some "low-level" things that feel more like C implementations, things that happen to be in JS but might have been in C, perhaps __emscripten_some_thing makes more sense.

(The history of the $ prefix is just as 99% of JS library stuff used to be C impls, and to save writing the prefix we add, we wrote the C source name without the prefix. To indicate no prefix should be emitted and something is really a JS thing, we added the $ at some point.)

@juj juj force-pushed the more_minimal_runtime_code_building branch from d372644 to ce20cb4 Compare February 6, 2020 16:25
@juj juj merged commit 00eda2b into emscripten-core:master Feb 6, 2020
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