Fix UB in SFTMap implementations

[playXE]

1. Replaces casts of references to mutable references with UnsafeCell<T>.
2. Spaces are stored inside each of implementation as pointers in order to make Rust compiler happy because of lifetimes.

[qinsoon]

1. Replaces casts of references to mutable references with UnsafeCell<T>.
2. Spaces are stored inside each of implementation as pointers in order to make Rust compiler happy because of lifetimes.

Thanks for the PR. The use of UnsafeCell looks fine as a workaround for the UB (eventually we would like to have a safe implementation). Anyway, it looks good. But I don't understand the second point -- is the new version of Rust compiler complaining about the lifetime? The current SFTMap implementation does not assume the lifetime of &SFT, and the creation of SFTMap states &SFT is 'static. This allows us to easily change the scope of SFTMap in the future (e.g. we can confine the lifetime of SFTMap to the same lifetime as MMTK).

[playXE]

1. Replaces casts of references to mutable references with UnsafeCell<T>.
2. Spaces are stored inside each of implementation as pointers in order to make Rust compiler happy because of lifetimes.

Thanks for the PR. The use of UnsafeCell looks fine as a workaround for the UB (eventually we would like to have a safe implementation). Anyway, it looks good. But I don't understand the second point -- is the new version of Rust compiler complaining about the lifetime? The current SFTMap implementation does not assume the lifetime of &SFT, and the creation of SFTMap states &SFT is 'static. This allows us to easily change the scope of SFTMap in the future (e.g. we can confine the lifetime of SFTMap to the same lifetime as MMTK).

The second point arises from the fact that mut_self is removed. Before it would create a reference to SFTMap implementation that had different lifetime and space that is passed to ugprade could get assigned to it, now when replaced with UnsafeCell lifetimes do not match

[playXE]

Hmm I would not merge this PR for now, I'm going to make storage of pointers in SFT vectors Atomic as well, using Atomic from atomic

[playXE]

Hmm I would not merge this PR for now, I'm going to make storage of pointers in SFT vectors Atomic as well, using Atomic from atomic

So far @qinsoon told me that update method is invoked behind the lock so probably push() and update of spaces inside it are fine without atomic pointers. In that case merging this PR should be fine.

[qinsoon]

Hmm I would not merge this PR for now, I'm going to make storage of pointers in SFT vectors Atomic as well, using Atomic from atomic

So far @qinsoon told me that update method is invoked behind the lock so probably push() and update of spaces inside it are fine without atomic pointers. In that case merging this PR should be fine.

Can you fix the style check? Then I will start a test run with our bindings to test the PR, then we can merge it.

[playXE]

Hmm I would not merge this PR for now, I'm going to make storage of pointers in SFT vectors Atomic as well, using Atomic from atomic

So far @qinsoon told me that update method is invoked behind the lock so probably push() and update of spaces inside it are fine without atomic pointers. In that case merging this PR should be fine.

Can you fix the style check? Then I will start a test run with our bindings to test the PR, then we can merge it.

Hmm, weirdly enough I don't get the same error when I git clone it and run cargo fmt check and clippy, could you maybe restart pre code review check? I ran cargo fmt on sft_map.rs and it fixed whitespace

[qinsoon]

LGTM