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–52 | Naming consistency would be nice, but it's better to rename get_requested_size and set_requested_size in followup patch | |
| 59–60 | please add static_assert(sizeof(Metadata) == 16) | |
| compiler-rt/lib/hwasan/hwasan_allocator.h | ||
|---|---|---|
| 38–52 | ACK. | |
Removed ForEachChunk.
| compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
|---|---|---|
| 485 | 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 | ||
|---|---|---|
| 91 | 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 | |
| 485 |
Please clarify, I don't understand the question | |
| 494 | We need something like atomic_uint8_t chunk_state of asan | |
| compiler-rt/lib/hwasan/hwasan_allocator.h | ||
| 56 | Maybe move after the class? | |
| compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
|---|---|---|
| 485 | 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 | ||
|---|---|---|
| 494 | 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 | ||
|---|---|---|
| 79–80 | 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 | |
| 231 | I would expect here meta->SetState | |
| 498 | and below | |
| compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
|---|---|---|
| 75–76 | Question: do we even need thread id? Where is is used? | |
| compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
|---|---|---|
| 75–76 | asan uses thread id you you want to drop it, maybe just make "uptr requested_size" | |
| 86–89 | Here we do an oppisit acquire stuff, e.g. like in AtomicallySetQuarantineFlagIfAllocated called from asan Deallocate | |
| 280–282 | alloc_context_id is better name here | |
| 302 | // TODO: consider meta->SetUnallocated(free_context_id); | |
Addressed more comments.
| compiler-rt/lib/hwasan/hwasan_allocator.cpp | ||
|---|---|---|
| 75–76 | I will keep it as is. | |
| 280–282 | Should we use the whole thing then? I mean stack + tid instead of just stack? | |
| 302 | 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 | ||
|---|---|---|
| 93–94 | ||
| 280–282 | Sure, you can name it alloc_stack_id, but please rename in a separate patch together with free_context_id | |
| 302 | 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: