[PATCH] D35690: [Sanitizers] TSan allocator set errno on failure.

Dmitry Vyukov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 20 22:34:15 PDT 2017


dvyukov added inline comments.


================
Comment at: lib/tsan/rtl/tsan_mman.cc:174
 
+void *tsan_malloc(ThreadState *thr, uptr pc, uptr sz) {
+  return SetErrnoOnNull(user_alloc(thr, pc, sz, kDefaultAlignment));
----------------
alekseyshl wrote:
> dvyukov wrote:
> > Please don't prefix functions, variables and types in tsan with tsan. It's all tsan already. It's just plain pointless to prefix everything in tsan with tsan, everything in asan with asan, etc, and does not add any information for reader about purpose of a function or a type.
> > 
> > Also, diff will be much smaller if you name this function user_alloc and rename the old one to, say, user_alloc_noerrno. It will be just few lines instead of churning the whole codebase.
> Well, not sure about a few lines, but will do. It just seem weird to mention errno in the code which does not care about errno at all. Maybe user_alloc_internal?
Much better now. Thanks.
user_alloc_internal is fine.


================
Comment at: lib/tsan/tests/unit/tsan_mman_test.cc:72
     void *p2 = user_realloc(thr, pc, p, 0);
-    EXPECT_NE(p2, (void*)0);
+    EXPECT_EQ(p2, (void*)0);
   }
----------------
Is there a particular reason for this change? If not, then I would prefer to leave the current behavior. It worked for 5 years. We don't know what will happen with this one.


================
Comment at: test/tsan/allocator_returns_null.cc:40
+// Until we figure out why errno is not properly set on windows
+// UNSUPPORTED: win32
+
----------------
Are C++ tsan tests run on windows? That's surprising to me.


https://reviews.llvm.org/D35690





More information about the llvm-commits mailing list