[LLDB][NativePDB] Fix the case when S_DEFRANGE_SUBFIELD_REGISTERs are out of order.

[zequanwu]

Summary

Previously, I was assuming that S_DEFRANGE_SUBFIELD_REGISTERs are always in the
increasing order of offset_in_parent until I saw a counter example.

Using std::map so that they are sorted by offset_in_parent.

[labath]

seems reasonable

lldb/test/Shell/SymbolFile/NativePDB/local-variables-registers.s

247–250

Maybe add a comment about the order being deliberatele inverted.

lldb/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.h

 //===-- DWARFLocationExpression.h -------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_SOURCE_PLUGINS_SYMBOLFILE_NATIVEPDB_DWARFLOCATIONEXPRESSION_H
 #define LLDB_SOURCE_PLUGINS_SYMBOLFILE_NATIVEPDB_DWARFLOCATIONEXPRESSION_H
 
 #include "lldb/lldb-forward.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/DebugInfo/CodeView/CodeView.h"
+#include <map>
 
 namespace llvm {
 class APSInt;
 class StringRef;
 namespace codeview {
 class TypeIndex;
 }
 namespace pdb {
DWARFExpression MakeVFrameRelLocationExpression(llvm::StringRef fpo_program,
                                                 int32_t offset,
                                                 lldb::ModuleSP module);
 DWARFExpression MakeGlobalLocationExpression(uint16_t section, uint32_t offset,
                                              lldb::ModuleSP module);
 DWARFExpression MakeConstantLocationExpression(
     llvm::codeview::TypeIndex underlying_ti, llvm::pdb::TpiStream &tpi,
     const llvm::APSInt &constant, lldb::ModuleSP module);
 DWARFExpression MakeEnregisteredLocationExpressionForClass(
-    llvm::ArrayRef<std::pair<llvm::codeview::RegisterId, uint32_t>>
+    std::map<uint64_t, std::pair<llvm::codeview::RegisterId, uint32_t>>
         &members_info,
     lldb::ModuleSP module);
 } // namespace npdb
 } // namespace lldb_private
 
 #endif

lldb/source/Plugins/SymbolFile/NativePDB/DWARFLocationExpression.cpp

bytes = llvm::makeArrayRef(reinterpret_cast<const uint8_t *>(&Value), 8)
               .take_front(size);
   buffer->CopyData(bytes.data(), size);
   DataExtractor extractor(buffer, lldb::eByteOrderLittle, address_size);
   DWARFExpression result(nullptr, extractor, nullptr);
   return result;
 }
 
 DWARFExpression lldb_private::npdb::MakeEnregisteredLocationExpressionForClass(
-    llvm::ArrayRef<std::pair<RegisterId, uint32_t>> &members_info,
+    std::map<uint64_t, std::pair<RegisterId, uint32_t>> &members_info,
     lldb::ModuleSP module) {
   return MakeLocationExpressionInternal(
       module, [&](Stream &stream, RegisterKind &register_kind) -> bool {
-        for (auto member_info : members_info) {
+        for (auto pair : members_info) {
+          std::pair<RegisterId, uint32_t> member_info = pair.second;
           if (member_info.first != llvm::codeview::RegisterId::NONE) {
             uint32_t reg_num =
                 GetRegisterNumber(module->GetArchitecture().GetMachine(),
                                   member_info.first, register_kind);
             if (reg_num == LLDB_INVALID_REGNUM)
               return false;
             if (reg_num > 31) {
               stream.PutHex8(llvm::dwarf::DW_OP_regx);

lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp

MakeRangeList(const PdbIndex &index, const LocalVariableAddrRange &range,
 
   result.Append(start, end - start);
   return result;
 }
 
 namespace {
 struct FindMembersSize : public TypeVisitorCallbacks {
   FindMembersSize(
-      llvm::SmallVectorImpl<std::pair<RegisterId, uint32_t>> &members_info,
+      std::map<uint64_t, std::pair<RegisterId, uint32_t>> &members_info,
       TpiStream &tpi)
       : members_info(members_info), tpi(tpi) {}
-  llvm::SmallVectorImpl<std::pair<RegisterId, uint32_t>> &members_info;
+  std::map<uint64_t, std::pair<RegisterId, uint32_t>> &members_info;
   TpiStream &tpi;
   llvm::Error visitKnownMember(CVMemberRecord &cvr,
                                DataMemberRecord &member) override {
-    members_info.emplace_back(llvm::codeview::RegisterId::NONE,
-                              GetSizeOfType(member.Type, tpi));
+    auto it = members_info.insert(
+        {member.getFieldOffset(),
+         {llvm::codeview::RegisterId::NONE, GetSizeOfType(member.Type, tpi)}});
+    if (!it.second)
+      return llvm::make_error<CodeViewError>(cv_error_code::corrupt_record);
     return llvm::Error::success();
   }
 };
 } // namespace
 
 CVTagRecord CVTagRecord::create(CVType type) {
   assert(IsTagRecord(type) && "type is not a tag record!");
   switch (type.kind()) {
case S_DEFRANGE_REGISTER: {
           loc_specifier_cvs, loc));
 
       RegisterId base_reg = (RegisterId)(uint16_t)loc.Hdr.Register;
       result.ranges = MakeRangeList(index, loc.Range, loc.Gaps);
       result.location = MakeEnregisteredLocationExpression(base_reg, module);
       break;
     }
     case S_DEFRANGE_SUBFIELD_REGISTER: {
-      // A vector of register id and member size pairs. If the variable is a
-      // simple type, then we don't know the number of subfields. Otherwise, the
-      // vector size will be the number of subfields in the udt type.
-      llvm::SmallVector<std::pair<RegisterId, uint32_t>> members_info;
+      // A map from offset in parent to pair of register id and size. If the
+      // variable is a simple type, then we don't know the number of subfields.
+      // Otherwise, the size of the map should be greater than or equal to the
+      // number of sub field record.
+      std::map<uint64_t, std::pair<RegisterId, uint32_t>> members_info;
       bool is_simple_type = result.type.isSimple();
       if (!is_simple_type) {
         CVType class_cvt = index.tpi().getType(result.type);
         ClassRecord class_record = CVTagRecord::create(class_cvt).asClass();
         CVType field_list = index.tpi().getType(class_record.FieldList);
         FindMembersSize find_members_size(members_info, index.tpi());
         if (llvm::Error err =
                 visitMemberRecordStream(field_list.data(), find_members_size)) {
           llvm::consumeError(std::move(err));
           break;
         }
       }
 
-      uint32_t prev_offset = 0;
-      uint32_t cur_offset = 0;
       size_t member_idx = 0;
       // Assuming S_DEFRANGE_SUBFIELD_REGISTER is followed only by
       // S_DEFRANGE_SUBFIELD_REGISTER, need to verify.
       while (loc_specifier_cvs.kind() == S_DEFRANGE_SUBFIELD_REGISTER) {
         if (!is_simple_type && member_idx >= members_info.size())
           break;
 
         DefRangeSubfieldRegisterSym loc(
             SymbolRecordKind::DefRangeSubfieldRegisterSym);
         cantFail(SymbolDeserializer::deserializeAs<DefRangeSubfieldRegisterSym>(
             loc_specifier_cvs, loc));
 
         if (result.ranges) {
           result.ranges = Variable::RangeList::GetOverlaps(
               *result.ranges, MakeRangeList(index, loc.Range, loc.Gaps));
         } else {
           result.ranges = MakeRangeList(index, loc.Range, loc.Gaps);
           result.ranges->Sort();
         }
 
         if (is_simple_type) {
-          // Fix last member's size.
-          if (!members_info.empty())
-            members_info.back().second =
-                loc.Hdr.OffsetInParent - prev_offset;
-          members_info.emplace_back((RegisterId)(uint16_t)loc.Hdr.Register, 0);
-          prev_offset = loc.Hdr.OffsetInParent;
+          if (members_info.count(loc.Hdr.OffsetInParent)) {
+            // Malformed record.
+            result.ranges->Clear();
+            return result;
+          }
+          members_info[loc.Hdr.OffsetInParent] = {
+              (RegisterId)(uint16_t)loc.Hdr.Register, 0};
         } else {
-          // Some fields maybe optimized away and have no
-          // S_DEFRANGE_SUBFIELD_REGISTER to describe them. Skip them.
-          while (loc.Hdr.OffsetInParent != cur_offset &&
-                 member_idx < members_info.size()) {
-            cur_offset += members_info[member_idx].second;
-            ++member_idx;
+          if (!members_info.count(loc.Hdr.OffsetInParent)) {
+            // Malformed record.
+            result.ranges->Clear();
+            return result;
           }
-          if (member_idx < members_info.size()) {
-            members_info[member_idx].first =
-                (RegisterId)(uint16_t)loc.Hdr.Register;
-            cur_offset += members_info[member_idx].second;
-            ++member_idx;
-          }
+          members_info[loc.Hdr.OffsetInParent].first =
         }
         // Go to next S_DEFRANGE_SUBFIELD_REGISTER.
         loc_specifier_id = PdbCompilandSymId(
             loc_specifier_id.modi,
             loc_specifier_id.offset + loc_specifier_cvs.RecordData.size());
         loc_specifier_cvs = index.ReadSymbolRecord(loc_specifier_id);
       }
-      if (is_simple_type && !members_info.empty())
-        members_info.back().second =
-            GetTypeSizeForSimpleKind(result.type.getSimpleKind()) - prev_offset;
-      auto member_info_ref = llvm::makeArrayRef(members_info);
+      // Fix size for simple type.
+      if (is_simple_type) {
+        auto cur = members_info.begin();
+        auto end = members_info.end();
+        auto next = cur;
+        ++next;
+        uint32_t size = 0;
+        while (next != end) {
+          cur->second.second = next->first - cur->first;
+          size += cur->second.second;
+          cur = next++;
+        }
+        cur->second.second =
+            GetTypeSizeForSimpleKind(result.type.getSimpleKind()) - size;
+      }
       result.location =
-          MakeEnregisteredLocationExpressionForClass(member_info_ref, module);
+          MakeEnregisteredLocationExpressionForClass(members_info, module);
       break;
     }
     default:
       // FIXME: Handle other kinds. LLVM only generates the 4 types of records
       // above.
       break;
     }
     return result;

lldb/test/Shell/SymbolFile/NativePDB/local-variables-registers.s

 # clang-format off
 # REQUIRES: lld, x86
 
 # RUN: llvm-mc -triple=x86_64-windows-msvc --filetype=obj %s > %t.obj
 # RUN: lld-link /debug:full /nodefaultlib /entry:main %t.obj /out:%t.exe /base:0x140000000
 # RUN: env LLDB_USE_NATIVE_PDB_READER=1 %lldb -f %t.exe -s \
 # RUN:     %p/Inputs/local-variables-registers.lldbinit 2>&1 | FileCheck %s
 
 # This file is compiled from following source file:
 # clang-cl /Z7 /O1 /Falocal-variables-registers.s a.cpp
 # struct S {
 #   int x;
 #   char y;
 # };
-# 
+#
 # __attribute__((noinline)) S CreateS(int p1, char p2) {
 #   S s;
 #   s.x = p1 + 1;
 #   s.y = p2 + 2;
 #   ++s.x;
 #   ++s.y;
 #   return s;
 # }
-# 
+#
 # int main(int argc, char** argv) {
 #   int local = argc * 2;
 #   S s = CreateS(local, 'a');
 #   return s.x + s.y;
 # }
 
-# FIXME: The following variable location have wrong register numbers due to 
-# https://github.com/llvm/llvm-project/issues/53575. Fix them after resolving 
+# FIXME: The following variable location have wrong register numbers due to
+# https://github.com/llvm/llvm-project/issues/53575. Fix them after resolving
 # the issue.
 
 # CHECK:      (lldb) image lookup -a 0x140001000 -v
 # CHECK:          LineEntry: [0x0000000140001000-0x0000000140001003): C:\src\test\a.cpp:10
 # CHECK-NEXT:      Variable: id = {{.*}}, name = "p1", type = "int", valid ranges = [0x0000000140001000-0x0000000140001003), location = DW_OP_reg26 XMM9
 # CHECK-NEXT:      Variable: id = {{.*}}, name = "p2", type = "char", valid ranges = [0x0000000140001000-0x0000000140001006), location = DW_OP_regx 0x3f
 # CHECK-EMPTY:
 # CHECK:      (lldb) image lookup -a 0x140001003 -v
.Ltmp22:
   .short  .Ltmp24-.Ltmp23                 # Record length
 .Ltmp23:
   .short  4414                            # Record kind: S_LOCAL
   .long 4100                            # TypeIndex
   .short  0                               # Flags
   .asciz  "s"
   .p2align  2
 .Ltmp24:
-  .cv_def_range   .Ltmp0 .Lfunc_end0, subfield_reg, 18, 0
-  .cv_def_range   .Ltmp1 .Lfunc_end0, subfield_reg, 3, 4
+  # The following .cv_def_range order is inverted on purpose for testing.
+  .cv_def_range   .Ltmp0 .Lfunc_end0, subfield_reg, 3, 4
+  .cv_def_range   .Ltmp1 .Lfunc_end0, subfield_reg,18, 0
   .short  2                               # Record length
   .short  4431                            # Record kind: S_PROC_ID_END
 .Ltmp14:
   .p2align  2
   .cv_linetable 0, "?CreateS@@YA?AUS@@HD@Z", .Lfunc_end0
   .section  .debug$S,"dr",associative,main
   .p2align  2
   .long 4                               # Debug section magic
   .long 241                             # Symbol subsection for main