Page MenuHomePhabricator

[WebAssembly] Print error message for llvm.clear_cache intrinsic
ClosedPublic

Authored by aheejin on Jul 8 2019, 4:00 AM.

Details

Summary

Wasm does not currently support llvm.clear_cache intrinsic, and this
prints a proper error message instead of segfault.

Diff Detail

Repository
rL LLVM

Event Timeline

aheejin created this revision.Jul 8 2019, 4:00 AM
aheejin edited the summary of this revision. (Show Details)Jul 8 2019, 4:01 AM
aheejin retitled this revision from Add support for llvm.clear_cache intrinsic to [WebAssembly] Add support for llvm.clear_cache intrinsic.Jul 8 2019, 4:04 AM

Any code that thinks it needs to "clear the cache" is very likely doing something which won't actually work on wasm. Would it be reasonable to issue a report_fatal_error here?

sbc100 added a comment.Jul 8 2019, 7:23 AM

This looks like we we doing the exact same thing that x86 is. So it would make sense to me that whatever we end up doing we should continue to match x86. Perhaps landing this as-is and then opening a bug to turn this into an unreachable (for both x86 and wasm) is one option?

aheejin added a comment.EditedJul 8 2019, 6:08 PM

Yes it is what x86 does too. And it looks like they do it on purpose (= it's not a bug). From the llvm.clear_cache intrinsic semantics description:
"On platforms with coherent instruction and data caches (e.g. x86), this intrinsic is a nop. On platforms with non-coherent instruction and data cache (e.g. ARM, MIPS), the intrinsic is lowered either to appropriate instructions or a system call, if cache flushing requires special privileges. The default behavior is to emit a call to __clear_cache from the run time library."

So it sounds like only platforms with non-coherent instruction and data caches should do something. Wasm does not have concepts of instruction or data cache, but because it is designed to be more of a virtual machine that does not expose incoherences of hardwares, I thought it might make sense to do nothing like x86 as if we have coherent caches. But I don't have strong opinions on either side.

Any code that thinks it needs to "clear the cache" is very likely doing something which won't actually work on wasm. Would it be reasonable to issue a report_fatal_error here?

I'm not sure it would be easy, because the only hook we have for each target is that getClearCacheBuiltinName function.

The actual compiler crash here came from the fact that we don't have an entry for llvm.clear_cache in the builtin function signature table. Suppose we added a signature for it. Then, we'd get LLVM's and compiler-rt's default behavior, which would be to call __clear_cache, and ultimately just abort at runtime.

So, what if we do report_fatal_error("llvm.clear_cache is not supported on wasm"); from within getClearCacheBuiltinName? That's friendlier than aborting at runtime at least.

I think of wasm as being at the opposite end of the spectrum from x86 in this area. When you write to memory:

  • x86: you can execute it immediately, given the right permissions
  • arm: you can't execute it until you flush the cache
  • wasm: you can't execute it ever

Another view would be that we accept that this probably doesn't get used in code that would be useful in wasm, but maybe it's in a file that has other useful code, so this would basically be a convenience feature that allows users to have fewer ifdefs in their files (even with the assumption that they aren't going to be calling this code). Someone did, after all, actually run into this problem. Speaking of, maybe we should ask them what their use case is, why they even want to compile this code?

Another view would be that we accept that this probably doesn't get used in code that would be useful in wasm, but maybe it's in a file that has other useful code, so this would basically be a convenience feature that allows users to have fewer ifdefs in their files (even with the assumption that they aren't going to be calling this code). Someone did, after all, actually run into this problem. Speaking of, maybe we should ask them what their use case is, why they even want to compile this code?

Don't know, but I guess most likely he was trying to port some code that runs on other platforms...?

I thought about it, and maybe it would be safer to be explicit that we don't support clearing caches after all. I don't have strong opinions though.

aheejin updated this revision to Diff 208788.Jul 9 2019, 12:57 PM
  • Print an error message instead
dschuff accepted this revision.EditedJul 9 2019, 1:04 PM

This is fine (assuming getClearCacheBuiltinName is only called when clear_cache is itself called in the source), and an improvement over just asserting. I'm curious to hear what the OP says about their use case, we can always discuss changing this in the future.

This revision is now accepted and ready to land.Jul 9 2019, 1:04 PM
aheejin retitled this revision from [WebAssembly] Add support for llvm.clear_cache intrinsic to [WebAssembly] Print error message for llvm.clear_cache intrinsic.Jul 10 2019, 10:50 PM
aheejin edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.