[NFC][RISCV][builtins] remove some hard-coded values from i-cache clear routine

[EccoTheDolphin]

Summary

remove some hard-coded values from i-cache clear routine

[luismarques]

You'll notice that there is already an implementation of __clear_cache for RISC-V in the file you are contributing to, in line 160. That existing implementation directly performs a syscall. Your new implementation calls a (RISC-V-specific) GNU libc function __riscv_flush_icache, which internally either uses a vDSO (if available) or performs the syscall.

We don't need to have the two implementations, and in fact with this patch you'll probably get the cache cleared twice. Still, there might be some opportunities for improving the existing code. Some more specific comments:

(1) Do we want to depend on the glibc / versions of libc that implement __riscv_flush_icache? Some older versions of glibc didn't implement that, I think. The main advantage is that we would get the vDSO support, although we could also implement that directly, without depending on glibc.
(2) The existing code doesn't check for RV64. I wrote that code, but I don't remember if that was intentional (i.e. I had confirmed that the syscall should also work for RV32 Linux) or an oversight. So the check for __riscv_xlen == 64 might be a good idea.
(3) For the direct syscall I had avoided #including the headers by directly defining a few syscall constants, but we can revisit that decision if it's too hardcoded. Still, I think you might be #including some headers you don't actually need for the function call you are using.

compiler-rt/lib/builtins/clear_cache.c

49–53

Do we actually need all of these headers? Isn't sys/cachectl.h sufficient?

93–95

I think the option that we really want to document is the 0 (clear for all threads), not the one we don't use (SYS_RISCV_FLUSH_ICACHE_LOCAL).

[EccoTheDolphin]

In D87578#2270786, @luismarques wrote:

You'll notice that there is already an implementation of __clear_cache for RISC-V in the file you are contributing to, in line 160. That existing implementation directly performs a syscall. Your new implementation calls a (RISC-V-specific) GNU libc function __riscv_flush_icache, which internally either uses a vDSO (if available) or performs the syscall.

We don't need to have the two implementations, and in fact with this patch you'll probably get the cache cleared twice. Still, there might be some opportunities for improving the existing code. Some more specific comments:

(1) Do we want to depend on the glibc / versions of libc that implement __riscv_flush_icache? Some older versions of glibc didn't implement that, I think. The main advantage is that we would get the vDSO support, although we could also implement that directly, without depending on glibc.
(2) The existing code doesn't check for RV64. I wrote that code, but I don't remember if that was intentional (i.e. I had confirmed that the syscall should also work for RV32 Linux) or an oversight. So the check for __riscv_xlen == 64 might be a good idea.
(3) For the direct syscall I had avoided #including the headers by directly defining a few syscall constants, but we can revisit that decision if it's too hardcoded. Still, I think you might be #including some headers you don't actually need for the function call you are using.

Thank you very much for your review. As for the code duplication - sorry once again, at the time this was written we had no such code in the mainline (we used llvm 10 as the initial codebase). We'll look into the suggested adjustments.

[EccoTheDolphin]

We don't need to have the two implementations, and in fact with this patch you'll probably get the cache cleared twice. Still, there might be some opportunities for improving the existing code. Some more specific comments:

I agree. Once again, thank you for pointing that out. Also it should be noted that we don't really need icache_clear implementation for ASAN to function. We stumbled upon an unimplemented syscall when we were debugging our port of HWASAN (which we have plans to publish, but it requires an additional ISA extention). As such we decided to isolate this patch (that is the modified version with the suggested improvements to the existing codebase) from the main patch bundle and will re-upload it as a standalone patch.

(1) Do we want to depend on the glibc / versions of libc that implement __riscv_flush_icache? Some older versions of glibc didn't implement that, I think. The main advantage is that we would get the vDSO support, although we could also implement that directly, without depending on glibc.

I don't have enough knowledge about the consequences of such decision. As such I would prefer to not avoid introducing an additional dependency for now (till we get a better understanding of that).

(2) The existing code doesn't check for RV64. I wrote that code, but I don't remember if that was intentional (i.e. I had confirmed that the syscall should also work for RV32 Linux) or an oversight. So the check for __riscv_xlen == 64 might be a good idea.

I've scanned through the source code of Linux kernel - and I believe that we don't need to check for encoding. Looks like the respected syscall is available on both 32-bit and 64 bit versions.

(3) For the direct syscall I had avoided #including the headers by directly defining a few syscall constants, but we can revisit that decision if it's too hardcoded. Still, I think you might be #including some headers you don't actually need for the function call you are using.

I believe that the hard-coding of "__NR_riscv_flush_icache" constant is excessive, since it should be available if we include <sys/syscall.h> header (this define is provided by the Linux kernel). Other header files are not needed.

[EccoTheDolphin]

addressing improvements suggested by Luís Marques

[luismarques]

LGTM. Thanks for the patch.

[luismarques]

I can commit this for you. Is assume this the correct author? Alexey Baturo <[e-mail]>

IMO, I think you could request commit access. You have contributed a whole patch series and have been thoughfully participating in its review. Since most commits should go through pre-review anyway, the bar to gain commit access probably shouldn't be too high. IMO it should just be just high enough that random people aren't always screwing everything and disrupting other's work. If you do gain commit access, just keep an eye on the buildbots when you commit a patch, and revert the patch if builds start to fail and you can't immediately fix the issue.

[EccoTheDolphin]

In D87578#2292023, @luismarques wrote:

I can commit this for you. Is assume this the correct author? Alexey Baturo <[e-mail]>

IMO, I think you could request commit access. You have contributed a whole patch series and have been thoughfully participating in its review. Since most commits should go through pre-review anyway, the bar to gain commit access probably shouldn't be too high. IMO it should just be just high enough that random people aren't always screwing everything and disrupting other's work. If you do gain commit access, just keep an eye on the buildbots when you commit a patch, and revert the patch if builds start to fail and you can't immediately fix the issue.

Please, do. Yes, please, change the author is Alexey Baturo - he is the one who debugged this and provided the original patch.

IMO, I think you could request commit access. You have contributed a whole patch series and have been thoughtfully participating in its review. Since most commits should go through pre-review anyway, the bar to gain commit access probably shouldn't be too high.

Thanks. I'll try to request one in the meantime.

compiler-rt/lib/builtins/clear_cache.c

 #endif
 
 #if defined(__linux__) && defined(__mips__)
 #include <sys/cachectl.h>
 #include <sys/syscall.h>
 #include <unistd.h>
 #endif
 
+#if defined(__linux__) && defined(__riscv)
+// to get platform-specific syscall definitions
+#include <linux/unistd.h>
+#endif
+
 // The compiler generates calls to __clear_cache() when creating
 // trampoline functions on the stack for use with nested functions.
 // It is expected to invalidate the instruction cache for the
 // specified range.
 
 void __clear_cache(void *start, void *end) {
 #if __i386__ || __x86_64__ || defined(_M_IX86) || defined(_M_X64)
 // Intel processors have a unified instruction and data cache
#define __ARM_NR_cacheflush 0x0f0002
   __asm __volatile("svc 0x0"
                    : "=r"(start_reg)
                    : "r"(syscall_nr), "r"(start_reg), "r"(end_reg), "r"(flags));
   assert(start_reg == 0 && "Cache flush syscall failed.");
 #else
   compilerrt_abort();
 #endif
 #elif defined(__linux__) && defined(__mips__)
   const uintptr_t start_int = (uintptr_t)start;
   const uintptr_t end_int = (uintptr_t)end;
   syscall(__NR_cacheflush, start, (end_int - start_int), BCACHE);
 #elif defined(__mips__) && defined(__OpenBSD__)
   cacheflush(start, (uintptr_t)end - (uintptr_t)start, BCACHE);
 #elif defined(__aarch64__) && !defined(__APPLE__)
   uint64_t xstart = (uint64_t)(uintptr_t)start;
   uint64_t xend = (uint64_t)(uintptr_t)end;
 
   // Get Cache Type Info.
   static uint64_t ctr_el0 = 0;
#elif defined(__sparc__)
 
   const uintptr_t mask = ~(dword_size - 1);
   const uintptr_t start_dword = ((uintptr_t)start) & mask;
   const uintptr_t end_dword = ((uintptr_t)start + len + dword_size - 1) & mask;
 
   for (uintptr_t dword = start_dword; dword < end_dword; dword += dword_size)
     __asm__ volatile("flush %0" : : "r"(dword));
 #elif defined(__riscv) && defined(__linux__)
-#define __NR_riscv_flush_icache (244 + 15)
+  // See: arch/riscv/include/asm/cacheflush.h, arch/riscv/kernel/sys_riscv.c
   register void *start_reg __asm("a0") = start;
   const register void *end_reg __asm("a1") = end;
+  // "0" means that we clear cache for all threads (SYS_RISCV_FLUSH_ICACHE_ALL)
   const register long flags __asm("a2") = 0;
   const register long syscall_nr __asm("a7") = __NR_riscv_flush_icache;
   __asm __volatile("ecall"
                    : "=r"(start_reg)
                    : "r"(start_reg), "r"(end_reg), "r"(flags), "r"(syscall_nr));
   assert(start_reg == 0 && "Cache flush syscall failed.");
 #else
 #if __APPLE__
   // On Darwin, sys_icache_invalidate() provides this functionality
   sys_icache_invalidate(start, end - start);
 #else
   compilerrt_abort();
 #endif
 #endif
 }