qiyao wrote: > The only high level piece of feedback I have left is that I, personally, > would not have tried to handle the cases where address + length overflow, as > it makes some parts of the code harder to reason about. This is for two > reasons: > > 1. if this overflows in the LLDB code, it would likely also overflow in the > process itself, so realistically this code should never trigger. > 2. To provide some safety, I would simply have had an early return at the > entry point of the caches: if a addr+length cache request overflows, just > don't cache it.
That is a fair point on the insert path, and I will have a followup PR for it. On the flush path I would rather keep the clamp. The reason is below, first of all, the overflow handling is not new in this PR. `MemoryCache::Flush` has carried an explicit guard since 2012, de0e9d04ad17 and `MemoryCache::Read` has avoided overflow in cee6c47a62c4 (2019) too. `main` handles overflow in the L2 walk and gets it wrong in the L1 walk, that is the FIXME on `TestFlushAtTheTopOfTheAddressSpace`, and this PR fixes/removes it. The early return works for the insert path, but not for the flush path. `Flush` is invalidation, and its only caller is `Process::WriteMemory`, which flushes before it writes. Not flushing an overflowing range leaves the pre-write bytes cached and the next read hands them back as valid, and it may cause a wrong read value. https://github.com/llvm/llvm-project/pull/222688 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
