Stream: git-wasmtime

Topic: wasmtime / PR #1570 Fix long-range (non-colocated) aarch6...

Wasmtime GitHub notifications bot (Apr 21 2020 at 19:32):

cfallin requested bnjbvr and julian-seward1 for a review on PR #1570.

Wasmtime GitHub notifications bot (Apr 21 2020 at 19:32):

cfallin opened PR #1570 from fix-long-range-aarch64-call to master:

Previously, every call was lowered on AArch64 to a call instruction, which
takes a signed 26-bit PC-relative offset. Including the 2-bit left shift, this
gives a range of +/- 128 MB. Longer-distance offsets would cause an impossible
relocation record to be emitted (or rather, a record that a more sophisticated
linker would fix up by inserting a shim/veneer).

This commit adds a notion of "relocation distance" in the MachInst backends,
and provides this information for every call target and symbol reference. The
intent is that backends on architectures like AArch64, where there are different
offset sizes / addressing strategies to choose from, can either emit a regular
call or a load-64-bit-constant / call-indirect sequence, as necessary. This
avoids the need to implement complex linking behavior.

The MachInst driver code provides this information based on the "colocated" bit
in the CLIF symbol references, which appears to have been designed for this
purpose, or at least a similar one. Combined with the use_colocated_libcalls
setting, this allows client code to ensure that library calls can link to
library code at any location in the address space.

Separately, the simplejit example did not handle Arm64Call; rather than doing
so, it appears all that is necessary to get its tests to pass is to set the
use_colocated_libcalls flag to false, to make use of the above change. This
fixes the libcall_function unit-test in this crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 21 2020 at 19:32):

cfallin requested bnjbvr and julian-seward1 for a review on PR #1570.

Wasmtime GitHub notifications bot (Apr 21 2020 at 19:34):

cfallin updated PR #1570 from fix-long-range-aarch64-call to master:

Previously, every call was lowered on AArch64 to a call instruction, which
takes a signed 26-bit PC-relative offset. Including the 2-bit left shift, this
gives a range of +/- 128 MB. Longer-distance offsets would cause an impossible
relocation record to be emitted (or rather, a record that a more sophisticated
linker would fix up by inserting a shim/veneer).

This commit adds a notion of "relocation distance" in the MachInst backends,
and provides this information for every call target and symbol reference. The
intent is that backends on architectures like AArch64, where there are different
offset sizes / addressing strategies to choose from, can either emit a regular
call or a load-64-bit-constant / call-indirect sequence, as necessary. This
avoids the need to implement complex linking behavior.

The MachInst driver code provides this information based on the "colocated" bit
in the CLIF symbol references, which appears to have been designed for this
purpose, or at least a similar one. Combined with the use_colocated_libcalls
setting, this allows client code to ensure that library calls can link to
library code at any location in the address space.

Separately, the simplejit example did not handle Arm64Call; rather than doing
so, it appears all that is necessary to get its tests to pass is to set the
use_colocated_libcalls flag to false, to make use of the above change. This
fixes the libcall_function unit-test in this crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 21 2020 at 19:40):

cfallin updated PR #1570 from fix-long-range-aarch64-call to master:

Previously, every call was lowered on AArch64 to a call instruction, which
takes a signed 26-bit PC-relative offset. Including the 2-bit left shift, this
gives a range of +/- 128 MB. Longer-distance offsets would cause an impossible
relocation record to be emitted (or rather, a record that a more sophisticated
linker would fix up by inserting a shim/veneer).

This commit adds a notion of "relocation distance" in the MachInst backends,
and provides this information for every call target and symbol reference. The
intent is that backends on architectures like AArch64, where there are different
offset sizes / addressing strategies to choose from, can either emit a regular
call or a load-64-bit-constant / call-indirect sequence, as necessary. This
avoids the need to implement complex linking behavior.

The MachInst driver code provides this information based on the "colocated" bit
in the CLIF symbol references, which appears to have been designed for this
purpose, or at least a similar one. Combined with the use_colocated_libcalls
setting, this allows client code to ensure that library calls can link to
library code at any location in the address space.

Separately, the simplejit example did not handle Arm64Call; rather than doing
so, it appears all that is necessary to get its tests to pass is to set the
use_colocated_libcalls flag to false, to make use of the above change. This
fixes the libcall_function unit-test in this crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 22 2020 at 14:09):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 14:09):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 14:09):

bnjbvr created PR Review Comment:

Is the clone necessary, since it happens above already?

Wasmtime GitHub notifications bot (Apr 22 2020 at 14:09):

bnjbvr created PR Review Comment:

I think this comment ought to complete the documentation of colocated in the ExtFuncData data structure, because it's more likely CL users will look at ExtFuncData rather than RelocDistance.

Wasmtime GitHub notifications bot (Apr 22 2020 at 14:09):

bnjbvr created PR Review Comment:

Could this be a getter on ExtFuncData and on GlobalValueData instead?

Wasmtime GitHub notifications bot (Apr 22 2020 at 14:10):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 14:10):

bnjbvr created PR Review Comment:

Actually not sure how I feel about putting it on GlobalValueData, since it should only be for GlobalVataData::Symbol, really...

Wasmtime GitHub notifications bot (Apr 22 2020 at 21:47):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 21:47):

cfallin created PR Review Comment:

Ah -- the earlier clone was just cloning a tuple with a borrow (call_target() returns a borrow of the ExternalName), which is a no-op as it's already Copy; the new clone was to get around a conflicting borrow issue with ctx.emit taking a mut self. Removed first clone. The joys of proper Rust usage :-)

Wasmtime GitHub notifications bot (Apr 22 2020 at 21:57):

cfallin updated PR #1570 from fix-long-range-aarch64-call to master:

Previously, every call was lowered on AArch64 to a call instruction, which
takes a signed 26-bit PC-relative offset. Including the 2-bit left shift, this
gives a range of +/- 128 MB. Longer-distance offsets would cause an impossible
relocation record to be emitted (or rather, a record that a more sophisticated
linker would fix up by inserting a shim/veneer).

This commit adds a notion of "relocation distance" in the MachInst backends,
and provides this information for every call target and symbol reference. The
intent is that backends on architectures like AArch64, where there are different
offset sizes / addressing strategies to choose from, can either emit a regular
call or a load-64-bit-constant / call-indirect sequence, as necessary. This
avoids the need to implement complex linking behavior.

The MachInst driver code provides this information based on the "colocated" bit
in the CLIF symbol references, which appears to have been designed for this
purpose, or at least a similar one. Combined with the use_colocated_libcalls
setting, this allows client code to ensure that library calls can link to
library code at any location in the address space.

Separately, the simplejit example did not handle Arm64Call; rather than doing
so, it appears all that is necessary to get its tests to pass is to set the
use_colocated_libcalls flag to false, to make use of the above change. This
fixes the libcall_function unit-test in this crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 22 2020 at 21:57):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Apr 22 2020 at 21:57):

cfallin created PR Review Comment:

Done -- added an additional note to both colocated flags, in both ExtFuncData and GlobalValueData::Symbol.

Wasmtime GitHub notifications bot (Apr 22 2020 at 21:58):

cfallin created PR Review Comment:

Good idea -- done (made the GlobalValueData getter return an Option, only Some for the Symbol case).

Wasmtime GitHub notifications bot (Apr 22 2020 at 21:58):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Apr 23 2020 at 20:23):

cfallin updated PR #1570 from fix-long-range-aarch64-call to master:

Previously, every call was lowered on AArch64 to a call instruction, which
takes a signed 26-bit PC-relative offset. Including the 2-bit left shift, this
gives a range of +/- 128 MB. Longer-distance offsets would cause an impossible
relocation record to be emitted (or rather, a record that a more sophisticated
linker would fix up by inserting a shim/veneer).

This commit adds a notion of "relocation distance" in the MachInst backends,
and provides this information for every call target and symbol reference. The
intent is that backends on architectures like AArch64, where there are different
offset sizes / addressing strategies to choose from, can either emit a regular
call or a load-64-bit-constant / call-indirect sequence, as necessary. This
avoids the need to implement complex linking behavior.

The MachInst driver code provides this information based on the "colocated" bit
in the CLIF symbol references, which appears to have been designed for this
purpose, or at least a similar one. Combined with the use_colocated_libcalls
setting, this allows client code to ensure that library calls can link to
library code at any location in the address space.

Separately, the simplejit example did not handle Arm64Call; rather than doing
so, it appears all that is necessary to get its tests to pass is to set the
use_colocated_libcalls flag to false, to make use of the above change. This
fixes the libcall_function unit-test in this crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (May 05 2020 at 00:29):

cfallin updated PR #1570 from fix-long-range-aarch64-call to master:

Previously, every call was lowered on AArch64 to a call instruction, which
takes a signed 26-bit PC-relative offset. Including the 2-bit left shift, this
gives a range of +/- 128 MB. Longer-distance offsets would cause an impossible
relocation record to be emitted (or rather, a record that a more sophisticated
linker would fix up by inserting a shim/veneer).

This commit adds a notion of "relocation distance" in the MachInst backends,
and provides this information for every call target and symbol reference. The
intent is that backends on architectures like AArch64, where there are different
offset sizes / addressing strategies to choose from, can either emit a regular
call or a load-64-bit-constant / call-indirect sequence, as necessary. This
avoids the need to implement complex linking behavior.

The MachInst driver code provides this information based on the "colocated" bit
in the CLIF symbol references, which appears to have been designed for this
purpose, or at least a similar one. Combined with the use_colocated_libcalls
setting, this allows client code to ensure that library calls can link to
library code at any location in the address space.

Separately, the simplejit example did not handle Arm64Call; rather than doing
so, it appears all that is necessary to get its tests to pass is to set the
use_colocated_libcalls flag to false, to make use of the above change. This
fixes the libcall_function unit-test in this crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (May 05 2020 at 00:47):

cfallin updated PR #1570 from fix-long-range-aarch64-call to master:

Previously, every call was lowered on AArch64 to a call instruction, which
takes a signed 26-bit PC-relative offset. Including the 2-bit left shift, this
gives a range of +/- 128 MB. Longer-distance offsets would cause an impossible
relocation record to be emitted (or rather, a record that a more sophisticated
linker would fix up by inserting a shim/veneer).

This commit adds a notion of "relocation distance" in the MachInst backends,
and provides this information for every call target and symbol reference. The
intent is that backends on architectures like AArch64, where there are different
offset sizes / addressing strategies to choose from, can either emit a regular
call or a load-64-bit-constant / call-indirect sequence, as necessary. This
avoids the need to implement complex linking behavior.

The MachInst driver code provides this information based on the "colocated" bit
in the CLIF symbol references, which appears to have been designed for this
purpose, or at least a similar one. Combined with the use_colocated_libcalls
setting, this allows client code to ensure that library calls can link to
library code at any location in the address space.

Separately, the simplejit example did not handle Arm64Call; rather than doing
so, it appears all that is necessary to get its tests to pass is to set the
use_colocated_libcalls flag to false, to make use of the above change. This
fixes the libcall_function unit-test in this crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (May 05 2020 at 08:42):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (May 05 2020 at 08:42):

bnjbvr created PR Review Comment:

nit: If

Wasmtime GitHub notifications bot (May 05 2020 at 08:42):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (May 05 2020 at 14:23):

cfallin updated PR #1570 from fix-long-range-aarch64-call to master:

Previously, every call was lowered on AArch64 to a call instruction, which
takes a signed 26-bit PC-relative offset. Including the 2-bit left shift, this
gives a range of +/- 128 MB. Longer-distance offsets would cause an impossible
relocation record to be emitted (or rather, a record that a more sophisticated
linker would fix up by inserting a shim/veneer).

This commit adds a notion of "relocation distance" in the MachInst backends,
and provides this information for every call target and symbol reference. The
intent is that backends on architectures like AArch64, where there are different
offset sizes / addressing strategies to choose from, can either emit a regular
call or a load-64-bit-constant / call-indirect sequence, as necessary. This
avoids the need to implement complex linking behavior.

The MachInst driver code provides this information based on the "colocated" bit
in the CLIF symbol references, which appears to have been designed for this
purpose, or at least a similar one. Combined with the use_colocated_libcalls
setting, this allows client code to ensure that library calls can link to
library code at any location in the address space.

Separately, the simplejit example did not handle Arm64Call; rather than doing
so, it appears all that is necessary to get its tests to pass is to set the
use_colocated_libcalls flag to false, to make use of the above change. This
fixes the libcall_function unit-test in this crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (May 05 2020 at 14:24):

cfallin created PR Review Comment:

Fixed, thanks!

Wasmtime GitHub notifications bot (May 05 2020 at 14:24):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (May 05 2020 at 16:55):

cfallin updated PR #1570 from fix-long-range-aarch64-call to master:

Previously, every call was lowered on AArch64 to a call instruction, which
takes a signed 26-bit PC-relative offset. Including the 2-bit left shift, this
gives a range of +/- 128 MB. Longer-distance offsets would cause an impossible
relocation record to be emitted (or rather, a record that a more sophisticated
linker would fix up by inserting a shim/veneer).

This commit adds a notion of "relocation distance" in the MachInst backends,
and provides this information for every call target and symbol reference. The
intent is that backends on architectures like AArch64, where there are different
offset sizes / addressing strategies to choose from, can either emit a regular
call or a load-64-bit-constant / call-indirect sequence, as necessary. This
avoids the need to implement complex linking behavior.

The MachInst driver code provides this information based on the "colocated" bit
in the CLIF symbol references, which appears to have been designed for this
purpose, or at least a similar one. Combined with the use_colocated_libcalls
setting, this allows client code to ensure that library calls can link to
library code at any location in the address space.

Separately, the simplejit example did not handle Arm64Call; rather than doing
so, it appears all that is necessary to get its tests to pass is to set the
use_colocated_libcalls flag to false, to make use of the above change. This
fixes the libcall_function unit-test in this crate.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (May 05 2020 at 17:45):

cfallin merged PR #1570.

Last updated: Nov 22 2024 at 17:03 UTC