This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizers] TSan allocator set errno on failure.
ClosedPublic

Authored by alekseyshl on Jul 20 2017, 10:28 AM.

Details

Summary

Set proper errno code on allocation failures and change realloc, pvalloc,
aligned_alloc, memalign and posix_memalign implementation to satisfy
their man-specified requirements.

Modify allocator API implementation to bring it closer to other
sanitizers allocators.

Diff Detail

Repository
rL LLVM

Event Timeline

alekseyshl created this revision.Jul 20 2017, 10:28 AM
dvyukov added inline comments.Jul 20 2017, 10:48 AM
lib/tsan/rtl/tsan_mman.cc
174 ↗(On Diff #107544)

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.

  • Do not use "tsan_" prefix.
alekseyshl added inline comments.Jul 20 2017, 4:21 PM
lib/tsan/rtl/tsan_mman.cc
174 ↗(On Diff #107544)

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?

dvyukov added inline comments.Jul 20 2017, 10:34 PM
lib/tsan/rtl/tsan_mman.cc
174 ↗(On Diff #107544)

Much better now. Thanks.
user_alloc_internal is fine.

lib/tsan/tests/unit/tsan_mman_test.cc
72 ↗(On Diff #107601)

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.

test/tsan/allocator_returns_null.cc
40 ↗(On Diff #107601)

Are C++ tsan tests run on windows? That's surprising to me.

dvyukov added inline comments.Jul 20 2017, 10:41 PM
lib/tsan/tests/unit/tsan_mman_test.cc
72 ↗(On Diff #107601)

Note: the current behavior is correct according to both POSIX, linux man pages and C:

If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() shall be returned.

If size was equal to 0, either NULL or a pointer suitable to be passed to free() is returned.

alekseyshl added inline comments.Jul 21 2017, 7:57 AM
lib/tsan/tests/unit/tsan_mman_test.cc
72 ↗(On Diff #107601)

Wait, that's what the man I quoted in comments: "if size is equal to zero, and ptr is not NULL, then the call is equivalent to free(ptr)", so I'm freeing the pointer and it seems only natural to return NULL in this case, cause what else would I return?

What you're saying is correct for user_realloc(thr, pc, NULL, 0) case.

  • Address comments
alekseyshl added inline comments.Jul 21 2017, 10:26 AM
lib/tsan/tests/unit/tsan_mman_test.cc
72 ↗(On Diff #107601)

What I mean to say, it seems strange to return a dangling pointer and stdlib's realloc returns NULL in this case anyway.

user_realloc(thr, pc, NULL, 0) is tested first hand in this test.

test/tsan/allocator_returns_null.cc
40 ↗(On Diff #107601)

Yep, got carried away unifying tests with other sanitizers.

dvyukov accepted this revision.Jul 23 2017, 12:58 AM
dvyukov added inline comments.
lib/tsan/tests/unit/tsan_mman_test.cc
72 ↗(On Diff #107601)

Okay, let's try this.

This revision is now accepted and ready to land.Jul 23 2017, 12:58 AM
This revision was automatically updated to reflect the committed changes.