Skip to content

Fix module export symbols should not be marked as having non-local references #1384

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

Merged

Conversation

boingoing
Copy link
Contributor

When we do BindPidRefsInScope, we walk the PidRef stack and check to see
if any ref is non-local. Global and module export symbols should never be
marked as non-local.

Problem is that not all refs in the PidRef stack are necessarily marked to
indicate that the symbol is a module export. If a symbol which is an
export has a non-local ref which does not indicate it is a module export,
we will erroneously set the hasNonLocalReference flag on that symbol.

Here's an example of how this can happen:
function foo() { };
export { foo };

In this case, the top ref to symbol foo does not say that foo is a module
export so we might set the hasNonLocalReference flag on symbol foo when we
look at the top ref. The next ref in the PidRef stack would indicate that
foo is a symbol but by then we've already marked it.

A fix is to move the module export flag out of PidRef and into the Ident
itself. Then we can check to see if the pid is a module export before we
walk the PidRef stack in BindPidRefsInScope.

@boingoing
Copy link
Contributor Author

@pleath this is the restructuring of the PidRef module export flag we talked about. Could you give it a glance?

@pleath
Copy link
Contributor

pleath commented Aug 5, 2016

Cool. Nice that this could be resolved with a simple change.

@pleath
Copy link
Contributor

pleath commented Aug 5, 2016

👍

@boingoing boingoing force-pushed the moduleexporthasnonlocalreference branch 2 times, most recently from 75e62ad to 2f7daf7 Compare August 9, 2016 19:44
@boingoing
Copy link
Contributor Author

@dotnet-bot please test windows x64_release

@boingoing
Copy link
Contributor Author

@dotnet-bot please test OSX osx_osx_debug_static

…ferences

When we do BindPidRefsInScope, we walk the PidRef stack and check to see
if any ref is non-local. Global and module export symbols should never be
marked as non-local.

Problem is that not all refs in the PidRef stack are necessarily marked to
indicate that the symbol is a module export. If a symbol which is an
export has a non-local ref which does not indicate it is a module export,
we will erroneously set the hasNonLocalReference flag on that symbol.

Here's an example of how this can happen:
function foo() { };
export { foo };

In this case, the top ref to symbol foo does not say that foo is a module
export so we might set the hasNonLocalReference flag on symbol foo when we
look at the top ref. The next ref in the PidRef stack would indicate that
foo is a symbol but by then we've already marked it.

A fix is to move the module export flag out of PidRef and into the Ident
itself. Then we can check to see if the pid is a module export before we
walk the PidRef stack in BindPidRefsInScope.
@boingoing boingoing force-pushed the moduleexporthasnonlocalreference branch from 2f7daf7 to 5d9613d Compare August 9, 2016 21:27
@chakrabot chakrabot merged commit 5d9613d into chakra-core:master Aug 9, 2016
chakrabot pushed a commit that referenced this pull request Aug 9, 2016
…ed as having non-local references

Merge pull request #1384 from boingoing:moduleexporthasnonlocalreference

When we do BindPidRefsInScope, we walk the PidRef stack and check to see
if any ref is non-local. Global and module export symbols should never be
marked as non-local.

Problem is that not all refs in the PidRef stack are necessarily marked to
indicate that the symbol is a module export. If a symbol which is an
export has a non-local ref which does not indicate it is a module export,
we will erroneously set the hasNonLocalReference flag on that symbol.

Here's an example of how this can happen:
function foo() { };
export { foo };

In this case, the top ref to symbol foo does not say that foo is a module
export so we might set the hasNonLocalReference flag on symbol foo when we
look at the top ref. The next ref in the PidRef stack would indicate that
foo is a symbol but by then we've already marked it.

A fix is to move the module export flag out of PidRef and into the Ident
itself. Then we can check to see if the pid is a module export before we
walk the PidRef stack in BindPidRefsInScope.
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.

4 participants