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

export aliasee if not export all and if alias is exported #8295

Merged
merged 9 commits into from
Apr 8, 2019

Conversation

waterlike86
Copy link
Contributor

we found that when using an exports list (i.e. not export_all), the side module imports the alias.
but just exporting the alias is not enough, we need to export the actual implementation as well (i.e. the aliasee, aliased)

@waterlike86
Copy link
Contributor Author

waterlike86 commented Mar 14, 2019

@kripken do you think this makes sense? we were trying to obtain the list of aliases by scanning the imports and using llvm to scan the BC files, but there was always some which cannot be obtained and the BC level.
for example
in stdlib, __ZNSt12length_errorD1Ev maps to __ZNSt11logic_errorD2Ev and this information is not available at the BC level, but only available after JSBackend.
would you have any idea how we could get this from outside of the tool chain so we can avoid this modification?

@waterlike86
Copy link
Contributor Author

cc @awtcode

@kripken
Copy link
Member

kripken commented Mar 18, 2019

I think this is right, yes. I thought we already handled it, though, but I'm not sure where.

Please add a testcase - we need one anyhow, and it will also help understand things.

@waterlike86
Copy link
Contributor Author

@kripken added a test case

@waterlike86
Copy link
Contributor Author

waterlike86 commented Mar 21, 2019

I think this is right, yes. I thought we already handled it, though, but I'm not sure where.

@kripken actually i see in here it might have been handled for side_modules?

for k, v in metadata['aliases'].items():

@waterlike86
Copy link
Contributor Author

waterlike86 commented Mar 21, 2019

I think this is right, yes. I thought we already handled it, though, but I'm not sure where.

@kripken actually i see in here it might have been handled for side_modules?
emscripten/emscripten.py

Line 1609 in 6aea2c2

for k, v in metadata['aliases'].items():

tried to edit this such that it works from main_modules as well but it doesnt seem to work

@waterlike86 waterlike86 reopened this Mar 21, 2019
emscripten.py Outdated
@@ -874,6 +874,17 @@ def get_exported_implemented_functions(all_exported_functions, all_implemented,
if key in all_exported_functions or export_all or (export_bindings and key.startswith('_emscripten_bind')):
funcs.add(key)

if not export_all:
aliases = metadata['aliases'].keys()
for key in all_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.

Is this outer loop needed? (all_exported_functions may be very large, which worries me)

Instead, how about

for name, alias in metadata['aliases']:
  if name in all_implemented:
    funcs.add(alias)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats is actually to reduce the number of exports. we could just export all the alias like what you did, i think there arent many.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats also why its only run if not export_all, so there shouldnt be that many exports i hope.

text = re.sub(r'\$var\$*.', '', text)
text = re.sub(r'param \$\d+', 'param ', text)
text = re.sub(r' +', ' ', text)
# print("text: %s" % text)
Copy link
Member

Choose a reason for hiding this comment

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

can we remove all the text parsing here? if it is enough to just test for success in running the program, that is simpler and better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure let me do that.

@kripken
Copy link
Member

kripken commented Mar 25, 2019

I think that existing code (line 1609 you found) handles exporting from the asm.js module, but your new code here is still needed to have the functions in the final list of exports overall. To confirm, I assume the test fails without your code change?

@waterlike86
Copy link
Contributor Author

I think that existing code (line 1609 you found) handles exporting from the asm.js module, but your new code here is still needed to have the functions in the final list of exports overall. To confirm, I assume the test fails without your code change?

yes you are right. the test still fails without my code change.

@waterlike86
Copy link
Contributor Author

strangely i see test which seem to fail which are unrelated to my change

@kripken
Copy link
Member

kripken commented Apr 3, 2019

I think those errors can be ignored (should vanish with merging in latest incoming to here), except for other.test_only_force_stdlibs which looks like we need to figure it out here.

@waterlike86
Copy link
Contributor Author

@kripken seems like all tests passed after i did a merge from incoming

@kripken
Copy link
Member

kripken commented Apr 8, 2019

Thanks @waterlike86, looks good!

@kripken kripken merged commit 0d77e16 into emscripten-core:incoming Apr 8, 2019
@waterlike86 waterlike86 deleted the export-aliases branch April 9, 2019 02:32
sbc100 added a commit that referenced this pull request May 2, 2019
This test was testing a lot more than it needed to.  This simple
reproducer also exposes the same issue.  Verified by temporarily
reverting #8295.
sbc100 added a commit that referenced this pull request May 2, 2019
This test was testing a lot more than it needed to.  This simple
reproducer also exposes the same issue.  Verified by temporarily
reverting #8295.
sbc100 added a commit that referenced this pull request May 2, 2019
This test was testing a lot more than it needed to.  This simple
reproducer also exposes the same issue.  Verified by temporarily
reverting #8295.
VirtualTim pushed a commit to VirtualTim/emscripten that referenced this pull request May 21, 2019
VirtualTim pushed a commit to VirtualTim/emscripten that referenced this pull request May 21, 2019
This test was testing a lot more than it needed to.  This simple
reproducer also exposes the same issue.  Verified by temporarily
reverting emscripten-core#8295.
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
VirtualTim added a commit to VirtualTim/emscripten that referenced this pull request May 23, 2019
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
This test was testing a lot more than it needed to.  This simple
reproducer also exposes the same issue.  Verified by temporarily
reverting emscripten-core#8295.
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