[lldb] Support register vector and union types (draft, no not commit!) - #196032
Draft
DavidSpickett wants to merge 16 commits into
Draft
[lldb] Support register vector and union types (draft, no not commit!)#196032DavidSpickett wants to merge 16 commits into
DavidSpickett wants to merge 16 commits into
Conversation
When printing register "flags" types (basically C bitfield structs), we have 2 goals: 1. Extract the values (the fields) correctly. 2. Display the fields in most significant to least significant order (to match architecture manuals). Currently LLDB achieves these by: * Putting the most significant field as the first member of the struct type. Though we know it is the MSB, it will in fact be put at bit 0 by Clang. * To compensate for that, we reverse the order of the fields within the register value. If the original was [a][b][c], we change that to [c][b][a]. This works when the only type we have is "flags" aka a bitfield struct. I have been trying to implement "union" types (which act like C unions), and found that this method is not compatible with "union". Consider a union of two sets of flags: ``` some_union | -> big_little: wwww_wwww_wwww_wwww_wwww_xxxx_xxxx_xxxx -> little_big: yyyy_yyyy_yyyy_zzzz_zzzz_zzzz_zzzz_zzzz ``` w is bigger than x, and y is smaller than z. Therefore these two bitfield structs have different layouts. We cannot modify the value using both field layouts, we must pick one. Whichever one we pick results in us displaying the other one incorrectly because it has a different layouts. In other words: the current method only works when there is 1, and only one, field layout. For unions, this is not true. We need to achieve goals 1 and 2 without relying on details of the register's type. The first method I prototyped was to build the struct types in reverse (so field values are correct), then print them in reverse with a Synthetic Child Provider (so the display order is correct). This works but it has the small downside that the underlying type would be backwards if a user were to inspect it. This is not possible today but eventually I want to allow register types in expressions, and with that you could make use of the underlying type. Users are very unlikely to do this, but I wondered if it was a sign that I was pursuing a half baked solution. This PR implements an alternative. The types are still built with the most significant field first, so they are visually correct even if the raw type is printed. Then an ExternalASTSource is used to tell Clang to lay out the struct in an MSB to LSB order. This means the fields will have correct values. This method will work for unions because the only change we have to make to the register value is an endian swap in some cases. This endian swap does not rely on any type information, all it needs is the size of the register. This method will not result in any user visible changes at this time. What it does is fix a fundemental issue blocking the implementation of "union", and later "vector" (see llvm#87471). I also think that this method is much cleaner and easier to explain than my previous attempt. So it is worth switching to it regardless of future plans.
We used to rely on Clang to decide the layout of the struct types that we built using RegisterFlags. Now we supply an external layout. This means we do not need the anonyous padding fields. We still need to know about these gaps when printing "register info" tables, so I have basically moved the padding logic into there.
This is refactoring to prepare for llvm#87471. Where I will be adding support for describing registers as unions. See: https://sourceware.org/gdb/current/onlinedocs/gdb.html/Target-Description-Format.html A union is like a C union and references other types defined in the XML. Just like a set of register flags might reference an enum for one of those flags. By introducing this base class I'm making the treatment of all these different types generic. So that when encoding them as XML we can emit the type's dependencies recursively, and then emit the type itself. This strategy will also be used later in RegisterTypeBuilderClang to generate AST to represent these types (this is the decode step of the XML). As GDB decided to include size in enums, whenever we emit something it will get a "user" pointer. This allows an enum type to read the size of the register it's being attached to. No other type class requires this. I would call this "parent" but it is not usually the parent. The heirarchy is: * A RegisterFlags type contains many flags. * One of those flags has an enum as its type. * That enum needs to query two levels up to get the RegisterFlag's size. LLDB does not care about this enum size attribute, but GDB does so we emit it for compatibility. I don't expect anything other than a RegisterFlags to reference an enum at this time. In theory, a vector's element could be an enum but I do not know of anything available today that does this. I'd like to support arbitrary nesting of these types, but only later once known use cases work well.
So that when more types are added, the hierarchy is clear. RegisterType -> RegisterTypeEnum -> RegisterTypeFlags (in future also...) -> RegisterTypeUnion -> RegisterTypeVector Renamed the test file as it will cover all the classes derived from RegisterType.
So we are using the generic interface that will work with all future RegisterType derived classes. Right now we'll only be asked to print RegisterTypeFlags, so there's a few dyn_cast to that. Later we will switch on the kind, and support rendering more types.
We are assuming that their ID's are unique, so there's no need to keep separate maps. We can do basic type checking by checking the kind of the type pointed to. A few more methods were added to the base RegisterType. GetSize() returns 0 for enums because enums don't have a size until they are used by a register. This is not ideal but it works for now.
In future it may be generating things other than flags. Functionality is the same, but the interface changes to use RegisterType.
https://sourceware.org/gdb/current/onlinedocs/gdb.html/Target-Description-Format.html ``` <union id=\"id\"> <field name=\"name\" type=\"type\"> <...> </union> ``` This allows you to describe a register with multiple views on the data. Primarily this is used for vector registers where you use a union of the <vector> type (which I have yet to implement) to show the register with different element sizes. This work relates to llvm#87471 which covers that in more detail. This first commit introduces the type class and XML emitter, but does not yet parse it from XML or produce C types from it. It's unlikely we will be using <union> in lldb-server, but I figured it was best to implement ToElementXML for it anyway to be consistent with the rest of the classes. It's possible that "type" may not be the ID of another element but instead some generic name like "uint32". I don't know of any debug server that sends that, so for now I'm assuming that "type" always refers to some other type element.
This prepares it for emitting union types. Major changes: * Entry function is now a dispatcher to builder functions for each type. * Name mangling is standardised. * The register name parameter is no longer needed and so was removed.
TODO: tests????
TODO: add some XML tests to prove this works TODO: register info support
TODO: backport the size limit removal to the flags only patches
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp,h -- lldb/include/lldb/Target/RegisterType.h lldb/include/lldb/Target/RegisterTypeUnion.h lldb/include/lldb/Target/RegisterTypeVector.h lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.cpp lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.h lldb/source/Target/RegisterType.cpp lldb/source/Target/RegisterTypeUnion.cpp lldb/source/Target/RegisterTypeVector.cpp lldb/unittests/Target/RegisterTypeTest.cpp lldb/include/lldb/Core/DumpRegisterInfo.h lldb/include/lldb/Core/FormatEntity.h lldb/include/lldb/DataFormatters/DumpValueObjectOptions.h lldb/include/lldb/Target/DynamicRegisterInfo.h lldb/include/lldb/Target/RegisterTypeBuilder.h lldb/include/lldb/Target/Target.h lldb/include/lldb/lldb-private-types.h lldb/source/Core/DumpRegisterInfo.cpp lldb/source/Core/DumpRegisterValue.cpp lldb/source/Core/FormatEntity.cpp lldb/source/DataFormatters/DumpValueObjectOptions.cpp lldb/source/DataFormatters/ValueObjectPrinter.cpp lldb/source/Plugins/Process/FreeBSD/NativeRegisterContextFreeBSD_arm64.cpp lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_arm64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.cpp lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_arm64.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h lldb/source/Target/DynamicRegisterInfo.cpp lldb/source/Target/Target.cpp lldb/unittests/Core/DumpRegisterInfoTest.cpp lldb/include/lldb/Target/RegisterTypeFlags.h lldb/source/Target/RegisterTypeFlags.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.cpp b/lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.cpp
index b99493570..605807f03 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.cpp
@@ -34,26 +34,26 @@
using namespace lldb_private;
-const RegisterType *Arm64RegisterTypeDetector::DetectPOREL0Type(uint64_t hwcap,
- uint64_t hwcap2,
- uint64_t hwcap3) {
+const RegisterType *
+Arm64RegisterTypeDetector::DetectPOREL0Type(uint64_t hwcap, uint64_t hwcap2,
+ uint64_t hwcap3) {
(void)hwcap;
(void)hwcap3;
if (!(hwcap2 & HWCAP2_POE))
return {};
- static const RegisterTypeEnum por_el0_perm_enum("por_el0_perm_enum",
- {
- {0b0000, "No Access"},
- {0b0001, "Read"},
- {0b0010, "Execute"},
- {0b0011, "Read, Execute"},
- {0b0100, "Write"},
- {0b0101, "Write, Read"},
- {0b0110, "Write, Execute"},
- {0b0111, "Read, Write, Execute"},
- });
+ static const RegisterTypeEnum por_el0_perm_enum(
+ "por_el0_perm_enum", {
+ {0b0000, "No Access"},
+ {0b0001, "Read"},
+ {0b0010, "Execute"},
+ {0b0011, "Read, Execute"},
+ {0b0100, "Write"},
+ {0b0101, "Write, Read"},
+ {0b0110, "Write, Execute"},
+ {0b0111, "Read, Write, Execute"},
+ });
static const RegisterTypeFlags por_el0_flags(
"por_el0_flags", 8,
@@ -267,29 +267,30 @@ const RegisterType *Arm64RegisterTypeDetector::DetectV0Type(uint64_t hwcap,
}
const RegisterType *Arm64RegisterTypeDetector::DetectX0Type(uint64_t hwcap,
- uint64_t hwcap2,
- uint64_t hwcap3) {
+ uint64_t hwcap2,
+ uint64_t hwcap3) {
(void)hwcap;
(void)hwcap2;
(void)hwcap3;
- static RegisterTypeFlags x0_flags_big_little("x0_flags_big_little", 8, {
- {"w", 16, 63}, {"x", 0, 15}});
- static RegisterTypeFlags x0_flags_little_big("x0_flags_little_big", 8, {
- {"y", 48, 63}, {"z", 0, 47}});
+ static RegisterTypeFlags x0_flags_big_little("x0_flags_big_little", 8,
+ {{"w", 16, 63}, {"x", 0, 15}});
+ static RegisterTypeFlags x0_flags_little_big("x0_flags_little_big", 8,
+ {{"y", 48, 63}, {"z", 0, 47}});
- static RegisterTypeVector x0_vec8( "x0_vec8", "uint8", 8);
+ static RegisterTypeVector x0_vec8("x0_vec8", "uint8", 8);
static RegisterTypeVector x0_vec16("x0_vec16", "uint16", 4);
static RegisterTypeVector x0_vec32("x0_vec32", "uint32", 2);
static RegisterTypeVector x0_vec64("x0_vec64", "uint64", 1);
- static RegisterTypeUnion x0_vec_union(
- "x0_vec_union",
- {{"8", &x0_vec8}, {"16", &x0_vec16}, {"32", &x0_vec32}, {"64", &x0_vec64}});
-
- static RegisterTypeUnion x0_union(
- "x0_union",
- {{"big_little", &x0_flags_big_little}, {"little_big", &x0_flags_little_big},
- {"vector", &x0_vec_union}});
+ static RegisterTypeUnion x0_vec_union("x0_vec_union", {{"8", &x0_vec8},
+ {"16", &x0_vec16},
+ {"32", &x0_vec32},
+ {"64", &x0_vec64}});
+
+ static RegisterTypeUnion x0_union("x0_union",
+ {{"big_little", &x0_flags_big_little},
+ {"little_big", &x0_flags_little_big},
+ {"vector", &x0_vec_union}});
return &x0_union;
}
diff --git a/lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.h b/lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.h
index e1f70c8cc..28b9c83e7 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterTypeDetector_arm64.h
@@ -68,13 +68,13 @@ private:
static const RegisterType *DetectFPMRType(uint64_t hwcap, uint64_t hwcap2,
uint64_t hwcap3);
static const RegisterType *DetectX0Type(uint64_t hwcap, uint64_t hwcap2,
- uint64_t hwcap3);
+ uint64_t hwcap3);
static const RegisterType *DetectV0Type(uint64_t hwcap, uint64_t hwcap2,
uint64_t hwcap3);
static const RegisterType *
DetectGCSFeaturesType(uint64_t hwcap, uint64_t hwcap2, uint64_t hwcap3);
static const RegisterType *DetectPOREL0Type(uint64_t hwcap, uint64_t hwcap2,
- uint64_t hwcap3);
+ uint64_t hwcap3);
struct RegisterEntry {
RegisterEntry(llvm::StringRef name, unsigned size, DetectorFn detector)
diff --git a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp
index 01b3e1b75..16e073657 100644
--- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp
+++ b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.cpp
@@ -59,10 +59,9 @@ static std::string MakeTypeName(const RegisterType &type_info,
return type_name + type_info.GetID();
}
-CompilerType
-RegisterTypeBuilderClang::BuildVectorType(const lldb_private::RegisterTypeVector &vector_info,
- uint32_t register_byte_size,
- lldb::TypeSystemClangSP type_system) {
+CompilerType RegisterTypeBuilderClang::BuildVectorType(
+ const lldb_private::RegisterTypeVector &vector_info,
+ uint32_t register_byte_size, lldb::TypeSystemClangSP type_system) {
// Don't need to check for existing types because all array types are
// pre-existing. This also means they do not have unique names.
auto element_info = vector_info.GetElementTypeInfo();
@@ -74,9 +73,10 @@ RegisterTypeBuilderClang::BuildVectorType(const lldb_private::RegisterTypeVector
/*is_vector=*/true);
}
-CompilerType RegisterTypeBuilderClang::BuildEnumType(const RegisterTypeEnum &enum_type_info,
- uint32_t register_byte_size,
- lldb::TypeSystemClangSP type_system) {
+CompilerType
+RegisterTypeBuilderClang::BuildEnumType(const RegisterTypeEnum &enum_type_info,
+ uint32_t register_byte_size,
+ lldb::TypeSystemClangSP type_system) {
std::string enum_type_name = MakeTypeName(enum_type_info, register_byte_size);
// Reuse existing type if we can.
@@ -160,10 +160,9 @@ CompilerType RegisterTypeBuilderClang::BuildFlagsType(
return flags_type;
}
-CompilerType
-RegisterTypeBuilderClang::BuildUnionType(const lldb_private::RegisterTypeUnion &union_info,
- uint32_t register_byte_size,
- lldb::TypeSystemClangSP type_system) {
+CompilerType RegisterTypeBuilderClang::BuildUnionType(
+ const lldb_private::RegisterTypeUnion &union_info,
+ uint32_t register_byte_size, lldb::TypeSystemClangSP type_system) {
std::string union_type_name = MakeTypeName(union_info, register_byte_size);
// Reuse existing type if we can.
diff --git a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h
index 498e4b43f..7c355e492 100644
--- a/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h
+++ b/lldb/source/Plugins/RegisterTypeBuilder/RegisterTypeBuilderClang.h
@@ -14,8 +14,8 @@
#include "Plugins/TypeSystem/Clang/TypeSystemClang.h"
#include "lldb/Target/RegisterTypeBuilder.h"
#include "lldb/Target/RegisterTypeFlags.h"
-#include "lldb/Target/RegisterTypeVector.h"
#include "lldb/Target/RegisterTypeUnion.h"
+#include "lldb/Target/RegisterTypeVector.h"
#include "lldb/Target/Target.h"
namespace lldb_private {
@@ -89,14 +89,15 @@ private:
CompilerType BuildFlagsType(const RegisterTypeFlags &flags_info,
uint32_t register_byte_size,
lldb::TypeSystemClangSP type_system);
-
- CompilerType BuildVectorType(const lldb_private::RegisterTypeVector &vector_info,
- uint32_t register_byte_size,
- lldb::TypeSystemClangSP type_system);
+
+ CompilerType
+ BuildVectorType(const lldb_private::RegisterTypeVector &vector_info,
+ uint32_t register_byte_size,
+ lldb::TypeSystemClangSP type_system);
CompilerType BuildUnionType(const lldb_private::RegisterTypeUnion &union_info,
- uint32_t register_byte_size,
- lldb::TypeSystemClangSP type_system);
+ uint32_t register_byte_size,
+ lldb::TypeSystemClangSP type_system);
// This is created the first time a register type is requested, then handed
// to the type system. We keep a reference to it so we can add more layouts
|
🐧 Linux x64 Test Results
Failed Tests(click on a test name to see its output) lldb-shelllldb-shell.Register/Core/aarch64-freebsd-register-fields.testIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
Contributor
Author
Contributor
Author
|
The refactoring parts of this have been merged into main now. The problem of union of flags remains unsolved and the vector and union work will hopefully be continued by others. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of work for #87471.
This is a WIP branch so we can compare and contrast with #195887.
The main changes are, in this order:
flags(should eventually be done by [lldb] Override default struct layout when building register types #189590).uniontypes from XML.vectortypes from XML.My original intent was to solve the first bit and then upstream the rest but with interest in this feature from others, I think we can probably do some of this out of order and use the code from the other author's PR for parts of it.