This is a part of https://reviews.llvm.org/D101204
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
compiler-rt/lib/dfsan/dfsan.cpp | ||
---|---|---|
24 | We still need dfsan_flags.h. | |
543 | The start + size implementation seems cleaner to me. Can we use that for WriteShadowWithSize and make WriteShadowInRange call that instead? | |
626 | This might be redundant since on Linux releasing the memory zeroes it out on the next load. | |
637 | Should we just call ReleaseOrigins? | |
670–671 | What's the reason to change this from ReleaseMemoryPagesToOS? | |
709 | Can we simplify by moving all this logic into WriteShadowWithSize? It's confusing to have multiple layers of functions to set the shadow, with some repeated checks in each layer. | |
711–712 | Does this do anything since we already called MmapFixedSuperNoReserve? | |
965 | What's going on here? intercept_tld_get_addr appears unused, so why do we need to override it? | |
1080 | dfsan_init is now unused. Why do we need this? | |
compiler-rt/lib/dfsan/dfsan.h | ||
24 | This is unused. | |
70 | Why int instead of bool? | |
71 | Since we currently only call dfsan_init from preinit_array, which happens once in a single thread, do we actually need to guard against multiple initialization? | |
122 | This looks unused. | |
compiler-rt/lib/dfsan/dfsan_allocator.cpp | ||
32 | Maybe we don't need this. We release (zero) shadow on unmap, so it should still be zero if we mmap it again. | |
43 |
|
compiler-rt/lib/dfsan/dfsan.cpp | ||
---|---|---|
24 | dfsan_flags.h is included in dfsan.h | |
543 | If we use "WriteShadowWithSize(dfsan_label label, uptr beg_shadow_addr, uptr size)" with the base function, But if size is # fo labels, we would have WriteShadowInRange(dfsan_label label, uptr beg_shadow_addr, uptr end_shadow_addr) { assert(beg_shadow_addr <= end_shadow_addr); assert((end_shadow_addr - beg_shadow_addr) % 2 == 0); WriteShadowWithSize(label, (end_shadow_addr - beg_shadow_addr) / 2); } I feel after the fast16label -> fast8label change, it is possible to do this because then #bytes == #labels. | |
626 | This mainly follows how MSan does when allocator does unmmap: https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/msan/msan_allocator.cpp#L33 dfsan_set_label only does mmap, at unmmap event, we call ReleaseMemoryPagesToOS explicitly. | |
637 | ReleaseOrigins does not call ReleaseMemoryPagesToOS. | |
670–671 | This mainly follows MSan's approach. Although MSan does not have a similar function to release origin, it has SetShadow https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/msan/msan_poisoning.cpp#L216 to zero out Shadow, it calls MmapFixedSuperNoReserve w/o ReleaseMemoryPagesToOS, To be consistent with this, ReleaseOrigins replaces ReleaseMemoryPagesToOS by MmapFixedSuperNoReserve. | |
709 | WriteShadowInRange works for both 0 and non-zero labels. | |
711–712 | This should have been removed to be consistent with | |
965 | This will be used by D101204 when interceptors are defined. | |
1080 | This will be used by D101204. | |
compiler-rt/lib/dfsan/dfsan.h | ||
24 | This would be used by those new/delete injections dfsan_new_delete.cpp in D101204. | |
71 | I found dfsan_init_is_running is mainly to ensure interceptors use real calls w/o wrappers before dfsan_init is done. | |
122 | This will be called by dfsan_interceptors.cpp in D101204. I also feel like dfsan_init/dfsan_is_running/dfsan_inited are not straightforward. But from MSan's code blame, they seem useful to some corner cases. | |
compiler-rt/lib/dfsan/dfsan_allocator.cpp | ||
32 | Thank you. Removed. | |
43 | inlined the constant. |
compiler-rt/lib/dfsan/dfsan.cpp | ||
---|---|---|
24 | Yes. It's just a nit, but I usually try to explicitly include the things used in each file, so that we can change header files without touching all the places where they're included. | |
546–553 | ||
637 | Shouldn't mmap(MAP_NORESERVE) have a similar effect on resident memory to madvise(MADV_DONTNEED)? Besides, we already have ReleaseOrClearShadow and ReleaseOrigins. Why do we need to reimplement that functionality in this function? | |
670–671 | Don't they have a similar effect? It's confusing to use two different ways of releasing memory without at least a comment explaining why. | |
compiler-rt/lib/dfsan/dfsan.h | ||
24 | In that case, maybe we should define it in the file it will be used in, instead of here. |
compiler-rt/lib/dfsan/dfsan.cpp | ||
---|---|---|
24 | moved #include "dfsan/dfsan_flags.h" to each cc file. | |
546–553 | Thank you. | |
637 | removed madvise(MADV_DONTNEED), and replaced dfsan_release_meta_memory by dfsan_set_label(0). | |
670–671 | removed those madvise calls. | |
compiler-rt/lib/dfsan/dfsan.h | ||
24 | moved to dfsan_new_delete.cpp |
compiler-rt/lib/dfsan/dfsan_allocator.cpp | ||
---|---|---|
32 | Looks like you added it back. I think we only need to set 0 label on unmap. |
compiler-rt/lib/dfsan/dfsan_allocator.cpp | ||
---|---|---|
32 | I found those mmap interceptors in dfsan_intercerptors.cp also do dfsan_set_label(0). https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/dfsan/dfsan_interceptors.cpp#L41 For the same reason they are not needed either. |
compiler-rt/lib/dfsan/dfsan_allocator.cpp | ||
---|---|---|
32 | Thought about the problem a bit more. I think our discussion is based on an assumption that all unmmap will be precisely intercepted. But for this OnMap, it should be safe because its corresponding OnUmmap is injected by our code. So in the following change, I will only remove dfsan_set_label(0) for OnMap. |
compiler-rt/lib/dfsan/dfsan_allocator.cpp | ||
---|---|---|
32 |
Ah yes, good point.
Well, I'm not 100% sure it's safe. Something else could do syscall(SYS_mmap), then taint the memory, then syscall_(SYS_munmap). Then we could end up mapping that same region for our heap and calling this OnMap callback.
|
This is unused.