[PATCH] D58379: [compiler-rt] Intercept the bcmp() function.

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: lib/asan/tests/asan_mem_test.cc:212
 
-
-TEST(AddressSanitizer, MemCmpOOBTest) {
+template <int(*cmpfn)(const void*, const void*, size_t)>
+void CmpOOBTestCommon() {
----------------
Maybe https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#how-to-write-value-parameterized-tests instead of function?


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:874
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, bcmp, a1, a2, size);
+  if (common_flags()->intercept_memcmp) {
----------------
courbet wrote:
> krytarowski wrote:
> > Can we reuse code with memcmp(3)? bcmp(3) is a subset of memcmp(3).
> Looking at `COMMON_INTERCEPTOR_ENTER` implementation for, e.g.  tsan:
> 
> ```
> 
> #define SCOPED_TSAN_INTERCEPTOR(func, ...) \
>     SCOPED_INTERCEPTOR_RAW(func, __VA_ARGS__); \
>     if (REAL(func) == 0) { \
>       Printf("FATAL: ThreadSanitizer: failed to intercept %s\n", #func); \
>       Die(); \
>     } \
>     if (thr->in_rtl > 1) \
>       return REAL(func)(__VA_ARGS__); 
> 
> 
> #define COMMON_INTERCEPTOR_ENTER(ctx, func, ...)      \
>   SCOPED_TSAN_INTERCEPTOR(func, __VA_ARGS__);         \
>   TsanInterceptorContext _ctx = {thr, caller_pc, pc}; \
>   ctx = (void *)&_ctx;                                \
>   (void) ctx;
> 
> ```
> 
> Given that the implementation calls `REAL(func)`, it feels preferable to call the real `bcmp` instead of real `memcmp`, even though calling memcmp does work. But again, I'm not familiar with how you guys typically approach this, so you tell me :)
> 
Sometimes we do macro
But I'd prefer inline function, e.g.

int memcpy_interceptor_impl(ctx, a1, a2, size, real_fn_ptr) {
....
}


================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:869
+#if SANITIZER_INTERCEPT_BCMP
+
+INTERCEPTOR(int, bcmp, const void *a1, const void *a2, uptr size) {
----------------
usually we don't have empty line here


================
Comment at: lib/sanitizer_common/sanitizer_platform_interceptors.h:145
 #define SANITIZER_INTERCEPT_MEMCMP SI_NOT_FUCHSIA
+#define SANITIZER_INTERCEPT_BCMP SANITIZER_INTERCEPT_MEMCMP && ( \
+    (SI_POSIX && _GNU_SOURCE) || \
----------------
```
#define SANITIZER_INTERCEPT_BCMP (SANITIZER_INTERCEPT_MEMCMP && ( \
    (SI_POSIX && _GNU_SOURCE) || \
    SI_NETBSD || SI_OPENBSD || SI_FREEBSD))
```

also please reformat it with "git clang-format -f --style=file --extensions=inc,cc,h,c,cpp HEAD^"


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58379/new/

https://reviews.llvm.org/D58379