Please note that that this code is not used. It will be connected at some point when the rest of the code is ready.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
compiler-rt/lib/hwasan/hwasan_allocator.h | ||
---|---|---|
34–36 | we should pack this into 16 bytes: private: atomic_uint64_t alloc_context_id; u32 requested_size_low; u16 requested_size_high; public: u8 right_aligned; u8 lsan_tag; | |
37–38 | please make alloc_context_id private so only it can be accessed with getters? | |
38–50 | Naming consistency would be nice, but it's better to rename get_requested_size and set_requested_size in followup patch | |
58 | please add static_assert(sizeof(Metadata) == 16) |
compiler-rt/lib/hwasan/hwasan_allocator.h | ||
---|---|---|
38–50 | ACK. |
Removed ForEachChunk.
compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
---|---|---|
487 | I am basing this code on the ASAN implentation and there it stores the asan::AsanChunk * in the metadata_ variable. I could modify this code to actually store hwasan::Metadata. What do you think? |
compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
---|---|---|
96 | CheckInvalidFree tries to match tag, but for LSAN we need this info even without user pointer, so we need something else alloc_context_id is optional info, we cannot use it for the check note, right_aligned is always false, so unused. I recommend to remove it in a separate patch and use this byte for is_allocated | |
487 |
Please clarify, I don't understand the question | |
496 | We need something like atomic_uint8_t chunk_state of asan | |
compiler-rt/lib/hwasan/hwasan_allocator.h | ||
54 | Maybe move after the class? |
compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
---|---|---|
487 | What I meant is to have this code like so: LsanMetadata::LsanMetadata(uptr chunk) { auto *c = chunk ? reinterpret_cast<__hwasan::HwasanChunkView *>( chunk - __hwasan::kChunkHeaderSize) : nullptr; metadata_ = c->metadata_; } |
compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
---|---|---|
496 | There is no HwasanChunkView in the memory, |
compiler-rt/lib/hwasan/hwasan_allocator.h | ||
---|---|---|
40 | right_aligned? something wrong with rebase |
compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
---|---|---|
84–85 | this must be be the last store if you list combine them I would call it Still make sure atomic_store(&chunk_state is the last action on allocation HwasanAllocate | |
227 | I would expect here meta->SetState | |
500 | and below |
compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
---|---|---|
80–81 | Question: do we even need thread id? Where is is used? |
compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
---|---|---|
80–81 | asan uses thread id you you want to drop it, maybe just make "uptr requested_size" | |
91–94 | Here we do an oppisit acquire stuff, e.g. like in AtomicallySetQuarantineFlagIfAllocated called from asan Deallocate | |
276–278 | alloc_context_id is better name here | |
298 | // TODO: consider meta->SetUnallocated(free_context_id); |
Addressed more comments.
compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
---|---|---|
80–81 | I will keep it as is. | |
276–278 | Should we use the whole thing then? I mean stack + tid instead of just stack? | |
298 | Just to confirm this would check if free_context_id is the same in SetUnallocated to make sure that we are deallocating the right chunk? |
compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
---|---|---|
98–99 | ||
276–278 | Sure, you can name it alloc_stack_id, but please rename in a separate patch together with free_context_id | |
298 | No, we may use free_context_id instead of alloc_context_id. Now, for some reasons, t->heap_allocations() is used to keep free_context_id |
we should pack this into 16 bytes: