[Lldb-commits] [PATCH] D130796: [LLDB][NativePDB] Switch to use DWARFLocationList.

[Zequan Wu via Phabricator via lldb-commits]

zequanwu added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp:843-846
+    std::map<uint64_t, size_t> offset_to_size;
+    // Get the size of each fields if it's udt.
+    if (!FindMembersSize::GetMemberSizesForUdt(result.type, index.tpi(), 0,
+                                               offset_to_size))
----------------
rnk wrote:
> I'm a bit worried about performance. This code runs for every `S_LOCAL` record. So, every time we encounter a `std::string` local variable, we walk over the entire string class hierarchy field list to compute this map, which we may or may not need later.
> 
> This code is pretty thorough, but can we reduce the scope of this patch by ignoring subfield records stored in memory, since they lack size information? I think that would make it easier to review and test. Just focus on variables in registers, and subfields in registers, since those are easiest to test and understand.
`std::__1::basic_string<char,std::__1::char_traits<char>,std::__1::allocator<char> >` (aka `std::string`) has a very shallow class hierarchy, only 1 parent ,`std::__1::__basic_string_common<1>`, which is the top in class hierarchy.
I agree that it's doing duplicate work by revisiting the parent class multiple times, but keeping the extra info could consume more memory, doesn't seems worth it. I think it shouldn't be much a performance problem most time.

> but can we reduce the scope of this patch by ignoring subfield records stored in memory, since they lack size information?
Yes, we could, but it's more tedious. `llvm/include/llvm/DebugInfo/CodeView/CodeViewRegisters.def` has all the register names, then we need to map each register name to its the corresponding byte size.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130796/new/

https://reviews.llvm.org/D130796