-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the ability to opt-out of ASan container annotations on a per-allocator basis #5241
base: main
Are you sure you want to change the base?
Changes from 8 commits
3428840
790ac7e
846061c
71c7716
0cfbb2f
a040f8c
7bec17a
76e0723
516d2c0
6b04254
a5de291
4cc951b
3f2111d
9823cb4
6188041
57e7aff
6c6545c
6d0b7b8
cbb714d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -274,6 +274,93 @@ struct implicit_allocator : custom_test_allocator<T, Pocma, Stateless> { | |
STATIC_ASSERT(_Container_allocation_minimum_asan_alignment<vector<char, implicit_allocator<char>>> == 1); | ||
STATIC_ASSERT(_Container_allocation_minimum_asan_alignment<vector<wchar_t, implicit_allocator<wchar_t>>> == 2); | ||
|
||
// Helper class for `ArenaAllocator`, where data is linearly allocated in a finite buffer. | ||
class Arena { | ||
public: | ||
Arena(size_t allocation_size, size_t capacity): | ||
data(new char[capacity* allocation_size]), | ||
allocation_size(allocation_size), | ||
capacity(capacity), | ||
offset(0) {} | ||
|
||
Arena(const Arena& other): | ||
data(other.data), | ||
allocation_size(other.allocation_size), | ||
capacity(other.capacity), | ||
offset(other.offset) {} | ||
|
||
void* allocate(size_t n) { | ||
assert(offset + n <= capacity && "Allocation failed: the arena is out of memory. You may call `reset` to free up space."); | ||
void* result = data + (offset * allocation_size); | ||
offset += n; | ||
return result; | ||
} | ||
|
||
// Deallocates memory in the arena, and `memset`s it all to zero | ||
void reset() noexcept { | ||
offset = 0; | ||
memset(data, 0, capacity*allocation_size); | ||
} | ||
|
||
char* const data; | ||
const size_t allocation_size; | ||
const size_t capacity; | ||
size_t offset; | ||
}; | ||
|
||
// An allocator that allocates memory in a pre-allocated buffer (the arena). | ||
template <typename T> | ||
class ArenaAllocator { | ||
public: | ||
|
||
using value_type = T; | ||
Arena* arena; | ||
|
||
ArenaAllocator(size_t alloc_size, size_t capacity) { | ||
assert(sizeof(T) <= alloc_size && "Constructor failed: allocation size is too small for target type."); | ||
arena = new Arena(alloc_size, capacity); | ||
} | ||
|
||
template <class U> | ||
ArenaAllocator(const ArenaAllocator<U>& other) { | ||
assert(sizeof(U) <= arena->allocation_size && "Constructor failed: allocation size is too small for target type."); | ||
davidmrdavid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
arena = other.arena; | ||
} | ||
|
||
T* allocate(size_t n) { | ||
void* ptr = arena->allocate(n); | ||
return reinterpret_cast<T*>(ptr); | ||
} | ||
|
||
// no-op. Memory is deallocated in the `reset` method | ||
void deallocate(value_type*, size_t) noexcept {} | ||
|
||
// Deallocates memory in the arena, and `memset`s it all to zero. | ||
// the `memset` would normally trigger an ASan AV, | ||
// but we'll have the ArenaAllocator opt out of ASan analysis | ||
void reset() noexcept { | ||
arena->reset(); | ||
} | ||
|
||
template <typename U> | ||
bool operator==(ArenaAllocator<U> const& other) | ||
{ | ||
return this->arena == other.arena; | ||
} | ||
|
||
template<typename U> | ||
bool operator!=(ArenaAllocator<U> const other) | ||
{ | ||
return !(this == other); | ||
} | ||
}; | ||
|
||
// Opt out of ASan analysis for the ArenaAllocator | ||
// FIXME: IntelliSense claims '_Is_ASan_enabled_for_allocator is not a templateC/C++(864)', | ||
// but it works somehow. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably due to IntelliSense picking up the installed STL headers instead of the headers in your STL repo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, that makes sense. I'll remove this comment in my next commit then. Just curious - do we have a good way of teling intellisense about this, or do we just 'ignore' the false alarm? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We test IntelliSense via the As for actual IntelliSense in the IDE, no idea. I would expect that if you've built something with the freshly built STL picked up (remember There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll do some digging and find out, would be good to document :-) . Thanks! |
||
template <typename T> | ||
constexpr bool _Is_ASan_enabled_for_allocator<ArenaAllocator<T>> = false; | ||
|
||
template <class Alloc> | ||
void test_push_pop() { | ||
using T = typename Alloc::value_type; | ||
|
@@ -1002,12 +1089,40 @@ void run_custom_allocator_matrix() { | |
run_tests<AllocT<T, false_type, false_type>>(); | ||
} | ||
|
||
// Tests that ASan analysis can be disabled for a vector with an arena allocator. | ||
void run_asan_disablement_test() { | ||
|
||
// The arena allocator stores integers in 32-bit alignment. | ||
// It can hold up to 100 such allocations. | ||
// The 32-bit alignment is a bit excessive for `int`s, but it ensures the allocator | ||
// can be rebound to allocate larger types as well (up to 32-bit types). | ||
const int size = 100; | ||
const int alloc_size = 32; | ||
ArenaAllocator<int> allocator(alloc_size, size); | ||
|
||
// We'll give the vector capacity 1, and allocate a single integer (99). | ||
std::vector<int, ArenaAllocator<int>> vec(allocator); | ||
vec.reserve(1); | ||
vec.push_back(99); | ||
|
||
// When calling reset, the arena would memset all 100 entries of it's buffer to zero. | ||
// If the allocator was naively annotated by ASan, this would trigger an AV, because | ||
// the arena is accessing memory not tracked by the vector's ASan annotations. | ||
|
||
// However, the allocator is annotated to opt out of ASan analysis through, | ||
// `_Is_ASan_enabled_for_allocator`, so this should not trigger an AV. | ||
allocator.reset(); | ||
|
||
// TODO: is it possible to add a 'negative' test case here? One where ASan expectedly fails? | ||
} | ||
|
||
template <class T> | ||
void run_allocator_matrix() { | ||
run_tests<allocator<T>>(); | ||
run_custom_allocator_matrix<T, aligned_allocator>(); | ||
run_custom_allocator_matrix<T, explicit_allocator>(); | ||
run_custom_allocator_matrix<T, implicit_allocator>(); | ||
run_asan_disablement_test(); | ||
davidmrdavid marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
int main() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I may have missed some conversation about it, but I'm wondering if the name should be changed. The feature is disabling annotations based on the container's underlying allocator.
Since the options for compiler are
_DISABLE_X_ANNOTATION
should we try to follow suite withDisable
in the name?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't realize the compiler options / flags followed that naming convention? Do you have a link or example where I can see this?
But yeah, don't feel strongly about naming. If it matches the flags, I'm happy to have
Disabled
overEnabled
in the name. I'll tackle that on my next push.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm basically commenting on staying in line with these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorporated: 57e7aff