clayborg added a comment. There was a fix that was never submitted for Google Stadia for the memory cache here:
https://github.com/googlestadia/vsi-lldb/tree/master/patches/llvm-project Might be worth checking what they did to ensure we have all of the same abilities. ================ Comment at: lldb/source/Target/Memory.cpp:181-183 + // FIXME: We should be able to wrap this into the check near the beginning + // of this function but we don't check for overlapping ranges so we will + // additionally check each cache line we read. ---------------- Not on the FIXME: We can't really check this near the beginning, because this happens for each cache line we as we advance the "curr_cache_line_base_addr" right? One thing to note about this code is that we might need to read at most 2 cache lines for any requests that make it to this code since we check above for "if (dst_len > m_L2_cache_line_byte_size)..." and use the L1 cache if that is true. So we know that we will read at most 2 cache lines depending on the offset. Might be nice to read the 2 cache lines in one memory access below if possible, and then make two cache entries with the result, but it will be either one cache line read, or two ================ Comment at: lldb/source/Target/Memory.cpp:196-198 + // If we didn't fill out the cache line completely and the offset is + // bigger than what we have available, we can't do anything further + // here. ---------------- JDevlieghere wrote: > Why can't we read from the process? Same question below. Because if we did a memory request before from a valid "curr_cache_line_base_addr", and we got back fewer bytes that requested, then the bytes won't be available later right? ================ Comment at: lldb/source/Target/Memory.cpp:245 + assert((curr_cache_line_base_addr % cache_line_byte_size) == 0); + std::unique_ptr<DataBufferHeap> data_buffer_heap_up( + new DataBufferHeap(cache_line_byte_size, 0)); ---------------- Should we just create a DataBufferSP right away here instead of creating a unique pointer and releasing it later? ================ Comment at: lldb/source/Target/Memory.cpp:255-256 - // We need to read from the process + if (process_bytes_read < cache_line_byte_size) + data_buffer_heap_up->SetByteSize(process_bytes_read); ---------------- If we don't read an entire cache line, should we populate this into the L1 cache instead? It might make the logic for accessing data in the L2 cache a bit simpler? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145624/new/ https://reviews.llvm.org/D145624 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
