[Lldb-commits] [lldb] Summarize std::string's when created from data. (PR #89110)

[Greg Clayton via lldb-commits]

================
@@ -254,13 +254,17 @@ bool lldb_private::formatters::LibStdcppStringSummaryProvider(
   } else
     addr_of_string =
         valobj.GetAddressOf(scalar_is_load_addr, &addr_type);
-  if (addr_of_string != LLDB_INVALID_ADDRESS) {
+
+  // We have to check for host address here
+  // because GetAddressOf returns INVALID for all non load addresses.
+  // But we can still format strings in host memory.
+  if (addr_of_string != LLDB_INVALID_ADDRESS ||
+        addr_type == eAddressTypeHost) {
----------------
clayborg wrote:

> I think we need to be more careful here. GetAddressOf is really meant to do "can you find an address in the target for this object". We use it that way in a whole bunch of places, e.g.:
> 
> ```
>       cstr_address = GetAddressOf(true, &cstr_address_type);
>     } else {
>       // We have a pointer
>       cstr_address = GetPointerValue(&cstr_address_type);
>     }
> 
>     if (cstr_address == 0 || cstr_address == LLDB_INVALID_ADDRESS) {
>       if (cstr_address_type == eAddressTypeHost && is_array) {
>         const char *cstr = GetDataExtractor().PeekCStr(0);
> ```
> 
> So in that case we are expecting a host address type to return an invalid address from GetAddressOf.
> 
> This change worries me, I don't think it will be what other code expects? Maybe you can get the value you need like the code above does instead?

The code above works, but I think it doesn't total sense from a readability standpoint. We have a `GetAddressOf` function that return the address kind and a value, I am not sure why we wouldn't want to take advantage of that and use it? The above code is a bit interesting as it makes a bunch of assumptions like "is this a host address and if this is an array, I know my data is in the data extractor at offset zero". I mean it works, but does it make sense to duplicate these kind of assumptions?

Make sense to fix GetAddressOf to take advantage of the API it is implementing. If the address kind can be host and we can return a valid host address value, I would say we use it. We will need to look over all uses of this API internally if we do change it.


https://github.com/llvm/llvm-project/pull/89110