[lldb] Add RegisterFlags class

[DavidSpickett]

Summary

This models the "flags" node from GDB's target XML:
https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html

This node is used to describe the fields of registers like cpsr on AArch64.

RegisterFlags is a class that contains a list of register fields.
These fields will be extracted from the XML sent by the remote.

We assume that there is at least one field, that the fields are
sorted in descending order and do not overlap. That will be
enforced by the XML processor (the GDB client code in our case).

The fields may not cover the whole register. To account for this
RegisterFields will add anonymous padding fields so that
sizeof(all fields) == sizeof(register). This will save a lot
of hasssle later.

[tschuett]

lldb/include/lldb/Target/RegisterFlags.h

21

There are a lot of const std::string& . Are you allowed to use llvm::StringRef or even std::string_view in LLDB?

lldb/unittests/Target/RegisterFlagsTest.cpp

124

Missing line.

[DavidSpickett]

lldb/include/lldb/Target/RegisterFlags.h

21

We can use llvm::StringRef, but in this case this will be passed a std::string so I just defaulted to the most conservative version (this happens in a later patch).

I suppose you could move here, or pass by copy and assume the compiler will figure it out.

[DavidSpickett]

lldb/include/lldb/Target/RegisterFlags.h

21

Actually, it doesn't have to be a std::string the XML parser does give you a StringRef.

I will update this and the subsequent patches.

[tschuett]

My only comment is: views look more modern than const-ref. The efficiency is not affected at all.

[DavidSpickett]

Use StringRef for parameters. This is what the XML parser gives you
by default in any case.

Internally we want to keep a copy of the string because the XML document
will go away after parsing.

[DavidSpickett]

Add newline at end of file.

[jasonmolenda]

LGTM. I'm not really advocating to changing m_type to be an enum, but curious how that might fit into the total context of this feature, and if it makes any sense.

lldb/include/lldb/Target/RegisterFlags.h

71

Should we store the type as an eFormat enum instead of a string? Maybe it's just informational for logging/humans and not too important. ProcessGDBRemote::BuildDynamicRegisterInfo translates our own register type names into these values (more often seen in the old lldb-created qRegisterInfos).

lldb/source/Target/RegisterFlags.cpp

50

describing

[DavidSpickett]

Fix typo, remove m_type which is not used by the following patches.

We could use it down the road, but we can also just guess that single
bit fields are bools and anything else is a number. So we don't lose much.

Everything I've seen out of gdbserver only sends a type for the other
information types, struct and unions.

[JDevlieghere]

LGTM modulo comments

lldb/include/lldb/Target/RegisterFlags.h

22–23

If you're going to store the string anyway, you might as well take it by value and move it into the member. That'll save you a copy in case someone calls the ctor with an rvalue reference.

27

This applies to the rest of this file as well.

73

Same here

82–84

Looks like these can be const?

[DavidSpickett]

Address review comments.

[DavidSpickett]

Use LOG instead of LOGF.

[JDevlieghere]

LGTM

[DavidSpickett]

Rebase

lldb/include/lldb/Target/RegisterFlags.h

+//===-- RegisterFlags.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_TARGET_REGISTERFLAGS_H
+#define LLDB_TARGET_REGISTERFLAGS_H
+
+#include "lldb/Utility/Log.h"
+
+namespace lldb_private {
+
+class RegisterFlags {
+public:
+  class Field {
+  public:
+    Field(std::string name, unsigned start, unsigned end)
+        : m_name(std::move(name)), m_start(start), m_end(end) {
+      assert(m_start <= m_end && "Start bit must be <= end bit.");
+    }
+
+    /// Get size of the field in bits. Will always be at least 1.
+    unsigned GetSizeInBits() const { return m_end - m_start + 1; }
+
+    /// A mask that covers all bits of the field.
+    uint64_t GetMask() const {
+      return (((uint64_t)1 << (GetSizeInBits())) - 1) << m_start;
+    }
+
+    /// Extract value of the field from a whole register value.
+    uint64_t GetValue(uint64_t register_value) const {
+      return (register_value & GetMask()) >> m_start;
+    }
+
+    const std::string &GetName() const { return m_name; }
+    unsigned GetStart() const { return m_start; }
+    unsigned GetEnd() const { return m_end; }
+    bool Overlaps(const Field &other) const;
+    void log(Log *log) const;
+
+    /// Return the number of bits between this field and the other, that are not
+    /// covered by either field.
+    unsigned PaddingDistance(const Field &other) const;
+
+    bool operator<(const Field &rhs) const {
+      return GetStart() < rhs.GetStart();
+    }
+
+    bool operator==(const Field &rhs) const {
+      return (m_name == rhs.m_name) && (m_start == rhs.m_start) &&
+             (m_end == rhs.m_end);
+    }
+
+  private:
+    std::string m_name;
+    /// Start/end bit positions. Where start N, end N means a single bit
+    /// field at position N. We expect that start <= end. Bit positions begin
+    /// at 0.
+    /// Start is the LSB, end is the MSB.
+    unsigned m_start;
+    unsigned m_end;
+  };
+
+  /// This assumes that:
+  /// * There is at least one field.
+  /// * The fields are sorted in descending order.
+  /// Gaps are allowed, they will be filled with anonymous padding fields.
+  RegisterFlags(std::string id, unsigned size,
+                const std::vector<Field> &fields);
+
+  const std::vector<Field> &GetFields() const { return m_fields; }
+  const std::string &GetID() const { return m_id; }
+  unsigned GetSize() const { return m_size; }
+  void log(Log *log) const;
+
+private:
+  const std::string m_id;
+  /// Size in bytes
+  const unsigned m_size;
+  std::vector<Field> m_fields;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_TARGET_REGISTERFLAGS_H

lldb/source/Target/CMakeLists.txt

add_lldb_library(lldbTarget
   Platform.cpp
   Process.cpp
   ProcessTrace.cpp
   Queue.cpp
   QueueItem.cpp
   QueueList.cpp
   RegisterContext.cpp
   RegisterContextUnwind.cpp
+  RegisterFlags.cpp
   RegisterNumber.cpp
   RemoteAwarePlatform.cpp
   SectionLoadHistory.cpp
   SectionLoadList.cpp
   StackFrame.cpp
   StackFrameList.cpp
   StackFrameRecognizer.cpp
   StackID.cpp

lldb/source/Target/RegisterFlags.cpp

+//===-- RegisterFlags.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 "lldb/Target/RegisterFlags.h"
+
+#include <optional>
+
+using namespace lldb_private;
+
+void RegisterFlags::Field::log(Log *log) const {
+  LLDB_LOG(log, "  Name: \"{0}\" Start: {1} End: {2}", m_name.c_str(), m_start,
+           m_end);
+}
+
+bool RegisterFlags::Field::Overlaps(const Field &other) const {
+  unsigned overlap_start = std::max(GetStart(), other.GetStart());
+  unsigned overlap_end = std::min(GetEnd(), other.GetEnd());
+  return overlap_start <= overlap_end;
+}
+
+unsigned RegisterFlags::Field::PaddingDistance(const Field &other) const {
+  assert(!Overlaps(other) &&
+         "Cannot get padding distance for overlapping fields.");
+  assert((other < (*this)) && "Expected fields in MSB to LSB order.");
+
+  // If they don't overlap they are either next to each other or separated
+  // by some number of bits.
+
+  // Where left will be the MSB and right will be the LSB.
+  unsigned lhs_start = GetStart();
+  unsigned rhs_end = other.GetStart() + other.GetSizeInBits() - 1;
+
+  if (*this < other) {
+    lhs_start = other.GetStart();
+    rhs_end = GetStart() + GetSizeInBits() - 1;
+  }
+
+  return lhs_start - rhs_end - 1;
+}
+
+RegisterFlags::RegisterFlags(std::string id, unsigned size,
+                             const std::vector<Field> &fields)
+    : m_id(std::move(id)), m_size(size) {
+  // We expect that the XML processor will discard anything describing flags but
+  // with no fields.
+  assert(fields.size() && "Some fields must be provided.");
+
+  // We expect that these are unsorted but do not overlap.
+  // They could fill the register but may have gaps.
+  std::vector<Field> provided_fields = fields;
+  m_fields.reserve(provided_fields.size());
+
+  // ProcessGDBRemote should have sorted these in descending order already.
+  assert(std::is_sorted(provided_fields.rbegin(), provided_fields.rend()));
+
+  // Build a new list of fields that includes anonymous (empty name) fields
+  // wherever there is a gap. This will simplify processing later.
+  std::optional<Field> previous_field;
+  unsigned register_msb = (size * 8) - 1;
+  for (auto field : provided_fields) {
+    if (previous_field) {
+      unsigned padding = previous_field->PaddingDistance(field);
+      if (padding) {
+        // -1 to end just before the previous field.
+        unsigned end = previous_field->GetStart() - 1;
+        // +1 because if you want to pad 1 bit you want to start and end
+        // on the same bit.
+        m_fields.push_back(Field("", field.GetEnd() + 1, end));
+      }
+    } else {
+      // This is the first field. Check that it starts at the register's MSB.
+      if (field.GetEnd() != register_msb)
+        m_fields.push_back(Field("", field.GetEnd() + 1, register_msb));
+    }
+    m_fields.push_back(field);
+    previous_field = field;
+  }
+
+  // The last field may not extend all the way to bit 0.
+  if (previous_field && previous_field->GetStart() != 0)
+    m_fields.push_back(Field("", 0, previous_field->GetStart() - 1));
+}
+
+void RegisterFlags::log(Log *log) const {
+  LLDB_LOG(log, "ID: \"{0}\" Size: {1}", m_id.c_str(), m_size);
+  for (const Field &field : m_fields)
+    field.log(log);
+}

lldb/unittests/Target/CMakeLists.txt

 add_lldb_unittest(TargetTests
   ABITest.cpp
   DynamicRegisterInfoTest.cpp
   ExecutionContextTest.cpp
   MemoryRegionInfoTest.cpp
   MemoryTest.cpp
   MemoryTagMapTest.cpp
   ModuleCacheTest.cpp
   PathMappingListTest.cpp
+  RegisterFlagsTest.cpp
   RemoteAwarePlatformTest.cpp
   StackFrameRecognizerTest.cpp
   FindFileTest.cpp
 
   LINK_LIBS
       lldbCore
       lldbHost
       lldbPluginObjectFileELF

lldb/unittests/Target/RegisterFlagsTest.cpp

+//===-- RegisterFlagsTest.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 "lldb/Target/RegisterFlags.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+TEST(RegisterFlagsTest, Field) {
+  // We assume that start <= end is always true, so that is not tested here.
+
+  RegisterFlags::Field f1("abc", 0, 0);
+  ASSERT_EQ(f1.GetName(), "abc");
+  // start == end means a 1 bit field.
+  ASSERT_EQ(f1.GetSizeInBits(), (unsigned)1);
+  ASSERT_EQ(f1.GetMask(), (uint64_t)1);
+  ASSERT_EQ(f1.GetValue(0), (uint64_t)0);
+  ASSERT_EQ(f1.GetValue(3), (uint64_t)1);
+
+  // End is inclusive meaning that start 0 to end 1 includes bit 1
+  // to make a 2 bit field.
+  RegisterFlags::Field f2("", 0, 1);
+  ASSERT_EQ(f2.GetSizeInBits(), (unsigned)2);
+  ASSERT_EQ(f2.GetMask(), (uint64_t)3);
+  ASSERT_EQ(f2.GetValue(UINT64_MAX), (uint64_t)3);
+  ASSERT_EQ(f2.GetValue(UINT64_MAX & ~(uint64_t)3), (uint64_t)0);
+
+  // If the field doesn't start at 0 we need to shift up/down
+  // to account for it.
+  RegisterFlags::Field f3("", 2, 5);
+  ASSERT_EQ(f3.GetSizeInBits(), (unsigned)4);
+  ASSERT_EQ(f3.GetMask(), (uint64_t)0x3c);
+  ASSERT_EQ(f3.GetValue(UINT64_MAX), (uint64_t)0xf);
+  ASSERT_EQ(f3.GetValue(UINT64_MAX & ~(uint64_t)0x3c), (uint64_t)0);
+
+  // Fields are sorted lowest starting bit first.
+  ASSERT_TRUE(f2 < f3);
+  ASSERT_FALSE(f3 < f1);
+  ASSERT_FALSE(f1 < f2);
+  ASSERT_FALSE(f1 < f1);
+}
+
+static RegisterFlags::Field make_field(unsigned start, unsigned end) {
+  return RegisterFlags::Field("", start, end);
+}
+
+TEST(RegisterFlagsTest, FieldOverlaps) {
+  // Single bit fields
+  ASSERT_FALSE(make_field(0, 0).Overlaps(make_field(1, 1)));
+  ASSERT_TRUE(make_field(1, 1).Overlaps(make_field(1, 1)));
+  ASSERT_FALSE(make_field(1, 1).Overlaps(make_field(3, 3)));
+
+  ASSERT_TRUE(make_field(0, 1).Overlaps(make_field(1, 2)));
+  ASSERT_TRUE(make_field(1, 2).Overlaps(make_field(0, 1)));
+  ASSERT_FALSE(make_field(0, 1).Overlaps(make_field(2, 3)));
+  ASSERT_FALSE(make_field(2, 3).Overlaps(make_field(0, 1)));
+
+  ASSERT_FALSE(make_field(1, 5).Overlaps(make_field(10, 20)));
+  ASSERT_FALSE(make_field(15, 30).Overlaps(make_field(7, 12)));
+}
+
+TEST(RegisterFlagsTest, PaddingDistance) {
+  // We assume that this method is always called with a more significant
+  // (start bit is higher) field first and that they do not overlap.
+
+  // [field 1][field 2]
+  ASSERT_EQ(make_field(1, 1).PaddingDistance(make_field(0, 0)), 0ULL);
+  // [field 1][..][field 2]
+  ASSERT_EQ(make_field(2, 2).PaddingDistance(make_field(0, 0)), 1ULL);
+  // [field 1][field 1][field 2]
+  ASSERT_EQ(make_field(1, 2).PaddingDistance(make_field(0, 0)), 0ULL);
+  // [field 1][30 bits free][field 2]
+  ASSERT_EQ(make_field(31, 31).PaddingDistance(make_field(0, 0)), 30ULL);
+}
+
+static void test_padding(const std::vector<RegisterFlags::Field> &fields,
+                         const std::vector<RegisterFlags::Field> &expected) {
+  RegisterFlags rf("", 4, fields);
+  EXPECT_THAT(expected, ::testing::ContainerEq(rf.GetFields()));
+}
+
+TEST(RegisterFlagsTest, RegisterFlagsPadding) {
+  // When creating a set of flags we assume that:
+  // * There are >= 1 fields.
+  // * They are sorted in descending order.
+  // * There may be gaps between each field.
+
+  // Needs no padding
+  auto fields =
+      std::vector<RegisterFlags::Field>{make_field(16, 31), make_field(0, 15)};
+  test_padding(fields, fields);
+
+  // Needs padding in between the fields, single bit.
+  test_padding({make_field(17, 31), make_field(0, 15)},
+               {make_field(17, 31), make_field(16, 16), make_field(0, 15)});
+  // Multiple bits of padding.
+  test_padding({make_field(17, 31), make_field(0, 14)},
+               {make_field(17, 31), make_field(15, 16), make_field(0, 14)});
+
+  // Padding before first field, single bit.
+  test_padding({make_field(0, 30)}, {make_field(31, 31), make_field(0, 30)});
+  // Multiple bits.
+  test_padding({make_field(0, 15)}, {make_field(16, 31), make_field(0, 15)});
+
+  // Padding after last field, single bit.
+  test_padding({make_field(1, 31)}, {make_field(1, 31), make_field(0, 0)});
+  // Multiple bits.
+  test_padding({make_field(2, 31)}, {make_field(2, 31), make_field(0, 1)});
+
+  // Fields need padding before, in between and after.
+  // [31-28][field 27-24][23-22][field 21-20][19-12][field 11-8][7-0]
+  test_padding({make_field(24, 27), make_field(20, 21), make_field(8, 11)},
+               {make_field(28, 31), make_field(24, 27), make_field(22, 23),
+                make_field(20, 21), make_field(12, 19), make_field(8, 11),
+                make_field(0, 7)});
+}