[Lldb-commits] [PATCH] D113098: [lldb] (Partially) enable formatting of utf strings before the program is started

[Pavel Labath via Phabricator via lldb-commits]

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