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
membarrier
function.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
afonso360 submitted PR review.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
It looks like
membarrier
is 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?
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.
afonso360 submitted PR review.
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.
bnjbvr submitted PR review.
bnjbvr submitted PR review.
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?
bnjbvr submitted PR review.
bnjbvr created PR review comment:
@cfallin what are your thoughts regarding ^?
cfallin submitted PR review.
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 :-)
bnjbvr requested cfallin for a review on PR #5331.
cfallin submitted PR review.
cfallin merged PR #5331.
Last updated: Dec 23 2024 at 12:05 UTC