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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
