labath marked 7 inline comments as done. labath added a comment. Thanks for the review.
================ Comment at: lldb/include/lldb/Target/Target.h:1028 + /// + /// This function will read a cache page at a time until a NULL string + /// terminator is found. It will stop reading if an aligned sequence of NULL ---------------- jingham wrote: > ReadCStringFromMemory (both flavors) set "force_live_memory" to be true. > Note that doesn't mean "don't read from the target" it means "always read > from the process first." It does fall back to the Target if there's no > process, AND if your Address is section relative. > > OTOH, you make it a default argument and then set it to "false" - the > opposite behavior. Is there a reason for that? That seems confusing. > > force_live_memory is really an efficiency thing. It will only make a > difference if the data to be read is in a read-only section of a binary, in > which case it will read from there. lldb always used to prefer reading > locally if it can, but if someone has been able to flip permissions and > overwrite read-only DATA through some kind of attack, you wouldn't be able to > see that if this is the default. So in actual fact, we pass > force_live_memory -> true most of the places we use these Target Memory > Reading API's. > > So it seems better to follow ReadCStringFromMemory and just force this to > true. I changed the value to true. However, I've have copied this pattern from `ReadMemory`, `ReadScalarIntegerFromMemory`, etc. Should we be setting the value to true for those functions as well? ================ Comment at: lldb/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp:58 StringPrinter::ReadStringAndDumpToStreamOptions options(valobj); options.SetLocation(valobj_addr); + options.SetTargetSP(valobj.GetTargetSP()); ---------------- jingham wrote: > This should take some form of lldb_private::Address not an lldb::addr_t or it > won't work pre-run. See the overall comment for details. I believe I've found the right incantation to do that. Please have a look at the new version. ================ Comment at: lldb/source/Target/Target.cpp:1922 + + addr_t curr_addr = addr.GetLoadAddress(this); + Address address = addr; ---------------- jingham wrote: > This is wrong. If the incoming address was section relative, you've thrown > that information away. Just pass Target::ReadMemory the full Address (addr), > it will know what to do with it. And you can use: > > addr.Slide(bytes_read); > > instead of lines 1951-1952 to update the address. I've removed it. BTW, this was cargo-culted from `Target::ReadCStringFromMemory`, so it has the same bug. I'll create a patch to just delete that whole function ================ Comment at: lldb/source/Target/Target.cpp:1924 + Address address = addr; + const size_t cache_line_size = 512; + char *curr_dst = dst; ---------------- jingham wrote: > This is a seemingly arbitrary choice. > > From what I can tell you are copying this from GetCStringFromMemory, so the > decision to do this isn't yours... > > IIUC the whole point of this paging so we don't read a big chunk of memory > and then find the terminator a couple of characters in. The optimal size for > that doesn't have to line up with the process cache size, but I don't see why > you wouldn't use that if it was handy. > > Anyway, having this here with no comment is confusing. Either repeat the > original justification or get the process cache line size if there's a > process. I've created a helper function which returns a (properly alinged/rounded) size if we have a process, and a fixed value if we don't. Let me know what you think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113098/new/ https://reviews.llvm.org/D113098 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
