[PATCH] D37788: [ARM] builtins: Do not abort in clear_cache.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

> So, I'm in two minds about this. While I understand the frustration of virtualisation users, I find it hard to concede without harder evidence (which so far has been anecdotal).

I've not been able to come to a firm conclusion on this either, I think that there is a good chance that there is call to the builtin that doesn't specify the address range with sufficient precision. However on a pragmatic basis I'm tending towards removing/altering the check for the following reasons:

- The clear_cache() implementations on all the other architectures and platforms do not call compilerrt_abort(), for example the return value of sys_icache_invalidate() on Apple is not checked.
- We are adding a stronger pre-condition [start, end) virtual addresses must be currently mapped, to libgcc's implementation. In return we can offer a stronger post-condition. True we don't need to slavishly follow libgcc, but we don't have any documentation stating this anywhere and I think that most people porting applications to compiler-rt will assume that the clear-cache function has the same interface.
- Unlike the other uses of compilerrt_abort which usually represent unimplemented cases or precondition failures, I think that only the application can truly judge whether the program should be aborted.

I don't think the above argument is strong enough to go against the consensus opinion though.

> Being pragmatic, I would be ok with a change that had a non-default pre-processor guard (something like UNSAFE_CLEAR_CACHE), which platforms are free to set in their build systems, avoiding the abort.
> 
> In that case, I'd like people that are building compiler-rt on multiple platforms (like Peter, Saleem, Joerg) to make sure the solution is good for them.

I think that would be difficult to do without making it ARM_UNSAFE_CLEAR_CACHE, for example on AArch64 the cache invalidation instructions are in user-space so clear-cache uses them directly, there is a bit in the SCTLR for EL1 that controls whether these instructions trap to EL1 or are ignored. An alternative might be to call another helper function, something like clear_cache_error() which would have a default weak implementation that calls compilerrt_abort().


https://reviews.llvm.org/D37788