zequanwu marked an inline comment as done. 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)) ---------------- zequanwu wrote: > 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. I updated to only accept subfield locations that are on register. I didn't realize it's that complicated until I tried to add a test case for it. 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
