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

Fix #5288 #5299

Merged
merged 1 commit into from
Jun 17, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/jsifier.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,8 @@ function JSify(data, functionsOnly) {
if (mainPass) {
// Add additional necessary items for the main pass. We can now do this since types are parsed (types can be used through
// generateStructInfo in library.js)
//B.start('jsifier-libload');

LibraryManager.load();
//B.stop('jsifier-libload');

var libFuncsToInclude;
if (INCLUDE_FULL_LIBRARY) {
Expand All @@ -68,10 +67,6 @@ function JSify(data, functionsOnly) {
libFuncsToInclude.push(key);
}
}
// mark implemented functions as already added (so if memcpy is in the forced full JS library, but also done in C, we just need the C)
for (var added in IMPLEMENTED_FUNCTIONS) {
addedLibraryItems[added.substr(1)] = true;
}
} else {
libFuncsToInclude = DEFAULT_LIBRARY_FUNCS_TO_INCLUDE;
}
Expand Down Expand Up @@ -134,6 +129,13 @@ function JSify(data, functionsOnly) {
var finalName = '_' + ident;
}

// if the function was implemented in compiled code, we just need to export it so we can reach it from JS
if (finalName in IMPLEMENTED_FUNCTIONS) {
EXPORTED_FUNCTIONS[finalName] = 1;
// stop here: we don't need to add anything from our js libraries, not even deps, compiled code is on it
return '';
}

// Don't replace implemented functions with library ones (which can happen when we add dependencies).
// Note: We don't return the dependencies here. Be careful not to end up where this matters
if (finalName in Functions.implementedFunctions) return '';
Expand Down
32 changes: 32 additions & 0 deletions tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -4741,6 +4741,38 @@ def test_unicode_js_library(self):
self.emcc_args += ['--js-library', path_from_root('tests', 'unicode_library.js')]
self.do_run(open(os.path.join(self.get_dir(), 'main.cpp'), 'r').read(), u'Unicode snowman \u2603 says hello!')

def test_js_lib_dep_memset(self):
open('lib.js', 'w').write(r'''
mergeInto(LibraryManager.library, {
depper__deps: ['memset'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Btw, this interacts with what was observed in #2908. Isn't it still the case that JS code cannot add dependencies to C code unless these dependencies are specified in src/deps_info.json? (or was that fixed at some point?)

If that's correct, then, say for clarity, if we replace memset by any other function that user has implemented in C side myFunctionOnCSide, then it's incorrect to specify depper__deps: ['myFunctionOnCSide'] since that won't currently do what user intended due to #2908, but user would have to specify the dependency depper-> myFunctionOnCSide in the system file src/deps_info.json for it to work?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's still the case that deps_info.json is the only way for a JS library to depend on something in a C system library.

About the second paragraph, if the user defined something on the C side, it wouldn't be needed as part of __deps anyhow - the deps are for system library code, it determines which system libs we link in. If myFunctionOnCSide is in one of the source or bitcode files the users gives as input to emcc, they will be linked in anyhow.

(Although the user may need to add KEEPALIVE if there is no other use of the function - in theory it might be nice if the deps handled that somehow, but it might also be more confusing, I'm not sure.)

depper: function(ptr) {
_memset(ptr, 'd'.charCodeAt(0), 10);
},
});
''')
src = r'''
#include <string.h>
#include <stdio.h>

extern "C" {
extern void depper(char*);
}

int main(int argc, char** argv) {
char buffer[11];
buffer[10] = '\0';
// call by a pointer, to force linking of memset, no llvm intrinsic here
volatile auto ptr = memset;
(*ptr)(buffer, 'a', 10);
depper(buffer);
puts(buffer);
}
'''
self.emcc_args += ['--js-library', 'lib.js']
self.do_run(src, 'dddddddddd')
Settings.INCLUDE_FULL_LIBRARY = 1
self.do_run(src, 'dddddddddd')

def test_funcptr_import_type(self):
self.emcc_args += ['--js-library', path_from_root('tests', 'core', 'test_funcptr_import_type.js'), '-std=c++11']
self.do_run_in_out_file_test('tests', 'core', 'test_funcptr_import_type')
Expand Down