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

compiler alias analysis: Correctly handle repeated tuple extractions #9024

Open
wants to merge 1 commit into
base: maint-27
Choose a base branch
from

Conversation

frej
Copy link
Contributor

@frej frej commented Nov 6, 2024

Correct an omission in the alias analysis pass which could make it fail to detect aliasing of a tuple element if the same element was extracted more than once in a function.

If we had code looking like this:

function bad:foo(_0) {
0:
_1 = get_tuple_element _0, 1
_2 = call (bar/1), _1
...
1:
_3 = get_tuple_element _0, 1

The alias analysis would decide that _1 died with the call to bar/1 and thus prune _1 from the sharing state database when leaving block 0 and thus fail to detect the aliasing of element 1 in _0. This in turn could allow bar/1 to destructively update elements of its argument, which is not safe.

This omission is corrected by detecting when the same element is extracted from a tuple multiple times in a function. Normally the CSE pass ensures that this is only done once, but sometimes it decides that it is more efficient to keep the tuple around and extract the element again. This interacts badly with the alias analysis which takes care to minimize the database it keeps about aliasing status to variables that are live, and can therefore in rare cases fail to detect aliasing.

Instead of complicating and slowing down the main alias analysis, we do a once over on all functions and detect when the same field is extracted twice and store the afflicted variables in a set. During the main alias analysis pass we consult the set and forcibly alias the variable when it is defined.

Thanks to @intarga for finding this bug.

Closes #9014

Copy link
Contributor

github-actions bot commented Nov 6, 2024

CT Test Results

    2 files    324 suites   10m 18s ⏱️
  817 tests   815 ✅ 2 💤 0 ❌
5 420 runs  5 418 ✅ 2 💤 0 ❌

Results for commit cd87fed.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@frej
Copy link
Contributor Author

frej commented Nov 6, 2024

This patch is based on OTP-27.1.2, but does not cleanly merge to
master as the alias pass has changed significantly since
OTP-27. Luckily the conflicts are only in function signatures. The
branch gh9014 contains
a clean patch against master to guide the person doing the forward
fake-merging into master.

Of note is that this fix does not change any of the output for the modules compiled by the diffable script for 27. For 28 the much more aggressive alias analysis, there is a single miscompilation in xmerl_xpath.erl.

@bjorng bjorng self-assigned this Nov 6, 2024
@bjorng bjorng added the team:VM Assigned to OTP team VM label Nov 6, 2024
Correct an omission in the alias analysis pass which could make it
fail to detect aliasing of a tuple element if the same element was
extracted more than once in a function.

If we had code looking like this:

function `bad`:`foo`(_0) {
0:
_1 = get_tuple_element _0, `1`
_2 = call (`bar`/1), _1
...
1:
_3 = get_tuple_element _0, `1`

The alias analysis would decide that _1 died with the call to bar/1
and thus prune _1 from the sharing state database when leaving block 0
and thus fail to detect the aliasing of element 1 in _0. This in turn
could allow bar/1 to destructively update elements of its argument,
which is not safe.

This omission is corrected by detecting when the same element is
extracted from a tuple multiple times in a function. Normally the CSE
pass ensures that this is only done once, but sometimes it decides
that it is more efficient to keep the tuple around and extract the
element again. This interacts badly with the alias analysis which
takes care to minimize the database it keeps about aliasing status to
variables that are live, and can therefore in rare cases fail to
detect aliasing.

Instead of complicating and slowing down the main alias analysis,
we do a once over on all functions and detect when the same field
is extracted twice and store the afflicted variables in a
set. During the main alias analysis pass we consult the set and
forcibly alias the variable when it is defined.

Thanks to @intarga for finding this bug.

Closes erlang#9014
@bjorng
Copy link
Contributor

bjorng commented Nov 7, 2024

Thanks! I've added both branches to our daily builds.

@bjorng
Copy link
Contributor

bjorng commented Nov 11, 2024

I've merged this pull request to maint and master. It will be merged to maint-27 when the next OTP-27 patch will be released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants