[lldb] [DynamicRegisterInfo] Unset value_regs/invalidate_regs before Finalize()

[mgorny]

Summary

Set value_regs and invalidate_regs in RegisterInfo pushed onto m_regs
to nullptr, to ensure that the temporaries passed there are not
accidentally used.

[mgorny]

Now, a fun fact: ProcessGDBRemote has already been passing temporaries. We probably didn't notice it's broken simply because lldb-server didn't send value_regs.

[labath]

So, does this fix like an existing test or something? Maybe you could write a unit test for it?

[mgorny]

Now with a trivial unit test.

[mgorny]

Ok, I see now that I was wrong. DynamicRegisterInfo::Finalize() already does that. I've finally managed to figure out what this scary code does ;-).

[labath]

FWIW, I think that doing this stuff immediately is a actually good idea. It avoids having a time window where info->value_regs is a dangling pointer, and it doesn't save us any work, since this is something we have to do anyway...

[labath]

(Though, if we're going to be modifying/expanding these lists in subsequent AddRegister calls, then maybe it does not make that much sense...)

[mgorny]

Revive. Move sorting & making unique into AddRegister(). Add a unittest.

[mgorny]

clang-format

[labath]

As I alluded to in the second comment, I'm not sure this is really that much helpful, since it's also nice to have all invalidate_regs handling happen in a single function. Maybe AddRegister should just set this field to null (to give a more predictable behavior (crash) if anyone accesses it), and have a comment saying that it will be filled in in Finalize ?

[mgorny]

Sure, I suppose either way works for me.

[mgorny]

Unset the pointers instead.

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.h

 //===-- DynamicRegisterInfo.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_PROCESS_UTILITY_DYNAMICREGISTERINFO_H
 #define LLDB_SOURCE_PLUGINS_PROCESS_UTILITY_DYNAMICREGISTERINFO_H
 
 #include <map>
 #include <vector>
 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-private.h"
 
 class DynamicRegisterInfo {
 protected:
   DynamicRegisterInfo(DynamicRegisterInfo &) = default;
   DynamicRegisterInfo &operator=(DynamicRegisterInfo &) = default;
 
 public:
   DynamicRegisterInfo() = default;
 
   DynamicRegisterInfo(const lldb_private::StructuredData::Dictionary &dict,
                       const lldb_private::ArchSpec &arch);
 
   virtual ~DynamicRegisterInfo() = default;
 
   DynamicRegisterInfo(DynamicRegisterInfo &&info);
   DynamicRegisterInfo &operator=(DynamicRegisterInfo &&info);
 
   size_t SetRegisterInfo(const lldb_private::StructuredData::Dictionary &dict,
                          const lldb_private::ArchSpec &arch);
 
-  void AddRegister(lldb_private::RegisterInfo &reg_info,
+  void AddRegister(lldb_private::RegisterInfo reg_info,
                    lldb_private::ConstString &set_name);
 
   void Finalize(const lldb_private::ArchSpec &arch);
 
   size_t GetNumRegisters() const;
 
   size_t GetNumRegisterSets() const;
 

lldb/source/Plugins/Process/Utility/DynamicRegisterInfo.cpp

for (uint32_t i = 0; i < num_regs; ++i) {
 
     m_regs.push_back(reg_info);
     m_set_reg_nums[set].push_back(i);
   }
   Finalize(arch);
   return m_regs.size();
 }
 
-void DynamicRegisterInfo::AddRegister(RegisterInfo &reg_info,
+void DynamicRegisterInfo::AddRegister(RegisterInfo reg_info,
                                       ConstString &set_name) {
   assert(!m_finalized);
   const uint32_t reg_num = m_regs.size();
   assert(reg_info.name);
   uint32_t i;
   if (reg_info.value_regs) {
     for (i = 0; reg_info.value_regs[i] != LLDB_INVALID_REGNUM; ++i)
       m_value_regs_map[reg_num].push_back(reg_info.value_regs[i]);
+
+    // invalidate until Finalize() is called
+    reg_info.value_regs = nullptr;
   }
   if (reg_info.invalidate_regs) {
     for (i = 0; reg_info.invalidate_regs[i] != LLDB_INVALID_REGNUM; ++i)
       m_invalidate_regs_map[reg_num].push_back(reg_info.invalidate_regs[i]);
+
+    // invalidate until Finalize() is called
+    reg_info.invalidate_regs = nullptr;
   }
   if (reg_info.dynamic_size_dwarf_expr_bytes) {
     for (i = 0; i < reg_info.dynamic_size_dwarf_len; ++i)
       m_dynamic_reg_size_map[reg_num].push_back(
           reg_info.dynamic_size_dwarf_expr_bytes[i]);
 
     reg_info.dynamic_size_dwarf_expr_bytes =
         m_dynamic_reg_size_map[reg_num].data();

lldb/unittests/Process/Utility/DynamicRegisterInfoTest.cpp

 //===-- DynamicRegisterInfoTest.cpp ---------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 #include "Plugins/Process/Utility/DynamicRegisterInfo.h"
 
 #include "lldb/Utility/ArchSpec.h"
 
 using namespace lldb_private;
 
TEST(DynamicRegisterInfoTest, finalize_regs) {
 
   ASSERT_REG(b1, 0);
   ASSERT_REG(b2, 8);
   ASSERT_REG(s1, 0, {b1});
   ASSERT_REG(s2, 8, {b2});
   ASSERT_REG(i1, 16, {}, {b1});
   ASSERT_REG(i2, 8, {b2}, {b1, i1});
 }
+
+TEST(DynamicRegisterInfoTest, no_finalize_regs) {
+  TestDynamicRegisterInfo info;
+
+  // Add regular registers
+  uint32_t b1 = info.AddTestRegister("b1", 8);
+  uint32_t b2 = info.AddTestRegister("b2", 8);
+
+  // Add a few sub-registers
+  uint32_t s1 = info.AddTestRegister("s1", 4, {b1});
+  uint32_t s2 = info.AddTestRegister("s2", 4, {b2});
+
+  // Add a register with invalidate_regs
+  uint32_t i1 = info.AddTestRegister("i1", 8, {}, {b1});
+
+  // Add a register with indirect invalidate regs to be expanded
+  // TODO: why is it done conditionally to value_regs?
+  uint32_t i2 = info.AddTestRegister("i2", 4, {b2}, {i1});
+
+  ASSERT_REG(b1, LLDB_INVALID_INDEX32);
+  ASSERT_REG(b2, LLDB_INVALID_INDEX32);
+  ASSERT_REG(s1, LLDB_INVALID_INDEX32);
+  ASSERT_REG(s2, LLDB_INVALID_INDEX32);
+  ASSERT_REG(i1, LLDB_INVALID_INDEX32);
+  ASSERT_REG(i2, LLDB_INVALID_INDEX32);
+}