Wasmtime GitHub notifications bot (Nov 28 2022 at 17:35): Wasmtime GitHub notifications bot (Nov 28 2022 at 17:35):bnjbvr edited PR #5331 from flush-icache-on-android-aarch64 to main:
This updates the icache flushing code so it does the flushing on Android as well. From fuzzy memories working on this in the past and quick inspection of what Firefox does these days, this is required for both Android and linux, and Android is guaranteed to have access to the
membarrierfunction.I've also slightly refactored the code to use an internal module named
details, I find it slightly cleaner as it avoids all the dead code in other combinations of targets/archs, but it's mostly esthetic, so I could revert that part.In addition to correctness, this also fixes the build of wasmtime 3.0.0 on the android architecture, which we rely on. If that fix is deemed acceptable, it would be extra nice to get a dot release of wasmtime with that build fix too!
cc @afonso360 @akirilov-arm @alexcrichton @cfallin
Wasmtime GitHub notifications bot (Nov 28 2022 at 17:51):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Nov 28 2022 at 18:02):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Nov 28 2022 at 18:02):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Nov 28 2022 at 18:02):cfallin created PR review comment:
It looks like
membarrieris now defined only on AArch64, whereas previously it was defined on all architectures (and IIRC it's used on RISC-V, or at least will eventually need to be, as well) -- could you revert this bit of code-motion?
Wasmtime GitHub notifications bot (Nov 28 2022 at 18:53):afonso360 created PR review comment:
RISC-V is going to need a custom syscall for this operation. IIRC the kernel membarrier interface is not a good fit for the RISC-V model.
See #5033 and #5078 for more details.
Moving this out may still be useful for other arch's though.
Wasmtime GitHub notifications bot (Nov 28 2022 at 18:53):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Nov 29 2022 at 10:10):bnjbvr created PR review comment:
Fine, I can revert this change. I try to favor the current state of things over future possible uses, but I don't have a strong opinion here.
Wasmtime GitHub notifications bot (Nov 29 2022 at 10:10):bnjbvr submitted PR review.
Wasmtime GitHub notifications bot (Nov 29 2022 at 10:12):bnjbvr submitted PR review.
Wasmtime GitHub notifications bot (Nov 29 2022 at 10:12):bnjbvr created PR review comment:
Actually, let me have a semi-strong opinion here: if I put the function back where it was, then it needs to be either annotated with the exact same
cfg, or with an#[allow(unused)]directive so the compiler doesn't complain about it being unused... So overall it seems better to keep it here, to reflect the current state?
Wasmtime GitHub notifications bot (Nov 30 2022 at 11:15):bnjbvr submitted PR review.
Wasmtime GitHub notifications bot (Nov 30 2022 at 11:15):bnjbvr created PR review comment:
@cfallin what are your thoughts regarding ^?
Wasmtime GitHub notifications bot (Nov 30 2022 at 15:14):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Nov 30 2022 at 15:14):cfallin created PR review comment:
Ah, yeah, given the above it seems it's actually only needed on aarch64 right now, so current state is fine. Thanks :-)
Wasmtime GitHub notifications bot (Nov 30 2022 at 15:15):bnjbvr requested cfallin for a review on PR #5331.
Wasmtime GitHub notifications bot (Nov 30 2022 at 15:15):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Nov 30 2022 at 15:15):cfallin merged PR #5331.
Last updated: Nov 22 2024 at 17:03 UTC