Add ARC architecture (bare-metal) that can be debugged through an RSP-server.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
It would be nice to see context when you submit diffs. with SVN:
svn diff -x -U9999999 ...
Or git:
git diff -U9999999
| include/lldb/Core/Architecture.h | ||
|---|---|---|
| 15 ↗ | (On Diff #178276) | Is DynamicRegisterInfo really in the top level namespace? |
| 18 ↗ | (On Diff #178276) | include the lldb-forward.h and remove this |
| include/lldb/Utility/ArchSpec.h | ||
| 91–92 ↗ | (On Diff #178276) | It would be great to find a way that ArchSpec.h doesn't get polluted with everyone's flags by putting the flag definitions in the Architecture.h file. Not mandatory for this patch, but something to think about. |
| source/Plugins/Architecture/Arc/ArchitectureArc.cpp | ||
| 76–93 ↗ | (On Diff #178276) | What is this code doing? Please add comments. Also, you have a ConfigureArchitecture in ProcessGDBRemote.cpp, can't you just do this work in that function and avoid the need to add Architecture::AdjustRegisterInfo() in the first place? And if so, is the architecture plug-in even needed? |
| source/Plugins/Process/Utility/DynamicRegisterInfo.cpp | ||
| 625 ↗ | (On Diff #178276) | Context would really help here to see the surrounding code. |
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
| 4530–4541 ↗ | (On Diff #178276) | Make: const lldb_private::RegisterInfo *DynamicRegisterInfo::GetRegisterInfo(const lldb_private::ConstString ®_name) const; public and call it. No need to duplicate the name search here.. Code will be: const RegisterInfo *rf_build_info = dyn_reg_info.GetRegisterInfo(reg_name); |
| 4683–4691 ↗ | (On Diff #178276) | This code seems like it could be cleaned up.
|
| source/Target/Platform.cpp | ||
| 1872–1876 ↗ | (On Diff #178276) | The software breakpoint opcode would be a great candidate to move to the Architecture plug-ins. Not needed for this patch since this is how is has been done for all architectures up till now, but just noting this here. |
| source/Target/Target.cpp | ||
| 70 ↗ | (On Diff #178276) | is this change needed? |
| 74 ↗ | (On Diff #178276) | is this change needed? |
Comment Actions
My intent was to move ARC-specific code to the architecture plugin as much as possible, but it requires to add undesired dependencies to Architecture interface. So, you are right, it seems to be better to keep these functions in ProcessGDBRemote.cpp and remove ArcArchitecture plugin at all.
| include/lldb/Core/Architecture.h | ||
|---|---|---|
| 15 ↗ | (On Diff #178276) | Yes, it is, just double-checked |
| source/Target/Target.cpp | ||
| 74 ↗ | (On Diff #178276) | These 2 changes were needed because ArcArchitecture held a reference to m_spec |
Comment Actions
See inline comments and let me know what you think.
| include/lldb/Utility/ArchSpec.h | ||
|---|---|---|
| 91–92 ↗ | (On Diff #178643) | Since no other place needs these flags, it would be nice to define them in ProcessGDBRemote.cpp and remove them from here? Eventually we should get rid of all flags in here and move then to the Architecture.h subclass headers. |
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
| 4549 ↗ | (On Diff #178643) | Do we need to set this flags in the arch_to_use still? The eARC_rf16 only seems to be used in this file. If you plan on using eARC_rf16 in other parts of the code, then we should define it in ArchSpec as you have it, otherwise we should remove it and avoid polluting that header if possible |
| 4564 ↗ | (On Diff #178643) | the rf16 could be passed into this function as an argument and then the enum can be removed from ArchSpec.h? |
| 4704–4707 ↗ | (On Diff #178643) | This logic still seems possibly incorrect as we will skip the finalize call below? Shouldn't this just be: if (arc::ConfigureArchitecture(*this, m_register_info, arch_to_use)) arc::AdjustRegisterInfo(m_register_info, arch_to_use); And fall through to below? |
Comment Actions
ARCflags are used by ABISysV_arc (related patch D55724). I would be glad to move it to architecture plugin, but I ought to add SetFlags/GetFlags to Architecture interface in this case. Then we'll have the same members in ArchSpec and in Architecture, that may look confusing.
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
|---|---|---|
| 4704–4707 ↗ | (On Diff #178643) | If we cannot configure the architecture it is a failure and the register info is incorrect, so just return false. Maybe should clear m_register_info in this case... |
Comment Actions
If the flags are used in other code, then leave them where they are. We can clean this up when a patch moves all flags out into Architecture headers eventually.
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
|---|---|---|
| 4707 ↗ | (On Diff #178643) | ok, makes sense. Clearing the register info would be a good idea. One extra question: why do we need to fix up this information? Does it come over incorrectly and in the XML data and then need to be fixed up? I would prefer to fix the data if possible so this doesn't have to be done. Another way to make things work when registers are incorrect is to specify a target definition file for GDB remote with: (lldb) settings set plugin.process.gdb-remote.target-definition-file /path/to/regs.py |
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
|---|---|---|
| 4707 ↗ | (On Diff #178643) | "dwarf" field doesn't exist in standard target register description given by a gdb-server (it looks like an LLDB's extension), that's why the adjustment is required. Since the ARC is a configurable architecture, we have to create a huge count of regs.py files. This is not only ARC_rf16/ARC_rf32, some modules like floating point unit may be disabled in current configuration and related registers are absent. As an alternative way we could have a super-target-definition-file with full set of possible registers and remove absent registers during this step. But I don't think it is much better than current adjustment. |
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
|---|---|---|
| 4707 ↗ | (On Diff #178933) | Is this a GDB server that you can modify? Or is the code fixed? Seems weird to ask for registers, and get something back that doesn't work, and then have to fix it up. Why even tell us the registers then? |
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
|---|---|---|
| 4707 ↗ | (On Diff #178933) | None of 3 gdb-servers we use provides us with "dwarf" field (including Ashling gdb-server and OpenOCD). It's not their business to be aware of DWARF. I'm going to hardcode necessary registers in SysVABI_arc, then AugmentRegisterInfoViaABI will solve the problem. |
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
|---|---|---|
| 4707 ↗ | (On Diff #178933) | BTW, I was looking at how we generate unwind plans last week (I need that for breakpad symbols), and was surprised to see that we need a running target to generate any kind of an unwind plan. After some examination, it turned out that all we use from that target is the list of registers. This is suboptimal for two reasons:
Anyway, my point here is that I would be supportive of transitioning to a different source of dwarf register numbers than the remote stub. |
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
|---|---|---|
| 4707 ↗ | (On Diff #178933) | I would move these lists of registers from an ABI to according Architecture plugin, as well as trap opcodes from the Platform (as Greg has mentioned above). But for now, I'm trying to introduce ARC target with the minimally invasive way for common code. |
Comment Actions
Removed registers adjustment, dwarf numbers are corrected by AugmentRegisterInfoViaABI now.
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
|---|---|---|
| 4707 ↗ | (On Diff #182325) | My two cents: I fully support a stub being able to provide the register info for DWARF, but it doesn't need to if there are well defined ABI plug-ins or architecture plug-ins that can assist. Why? It allows people to use a different lldb-server and test out new compiler changes without having to modify LLDB itself. There are kernel debugging tools that provide GDB server connections, and it is great to be able to let them iterate on their compilers and debugger connections without having to change LLDB. So yes, I am all for not requiring a stub to provide them as long as we can put the functionality in a good place. |
| include/lldb/Core/Architecture.h | ||
|---|---|---|
| 118–127 ↗ | (On Diff #182499) | We currently have been using the lldb_private::Flags class for this kind of stuff. Best to use that here. |
| source/Plugins/Architecture/Arc/ArchitectureArc.cpp | ||
| 29 ↗ | (On Diff #182499) | Not a great human readable architecture name here. All other plug-ins use the short architecture name ("arm", "mipc", "ppc64"). Best to just use "arc"? |
| 46–78 ↗ | (On Diff #182499) | All this code belongs elsewhere. SetFeatures probably needs to be changed to be: Architecture::GetFlags().XXX() where XXX is a method on the lldb_private::Flags class, so this code should go where the code that was calling it was. Kind of weird to have register reading function passed in. |
| source/Plugins/Architecture/Arc/ArchitectureArc.h | ||
| 32–40 ↗ | (On Diff #182499) | Use lldb_private::Flags |
| source/Plugins/Process/Utility/DynamicRegisterInfo.cpp | ||
| 642–669 ↗ | (On Diff #182499) | Magic numbers? Can we use enums? |
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
| 1222–1223 ↗ | (On Diff #182499) | All this work should be done in this function then use arch_plug->GetFlags().Set(mask); |
| source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp | ||
|---|---|---|
| 1222–1223 ↗ | (On Diff #182499) | Then I'll return to polluting this code with ARC-specific flags and registers. I would avoid passing this monstrous reg_reader to the Architecture if I could pass RegisterContext there. However, RegisterContext doesn't exist yet at this step. And there is no entity that can read registers without a context. |
Comment Actions
Why lldb_private::Flags is required? std::bitset provides the same functionality and even more.
| source/Plugins/Architecture/Arc/ArchitectureArc.cpp | ||
|---|---|---|
| 29 ↗ | (On Diff #182499) | You are writing about GetPluginNameStatic. The description string is same as for other plug-ins (just copy-pasted). |
| include/lldb/Core/Architecture.h | ||
|---|---|---|
| 119–123 ↗ | (On Diff #182848) | Any reason these definitions need to be in this class? Seems like something the plug-ins that access these should do, and this code shouldn't be in this file. |
| 125 ↗ | (On Diff #182848) | We should add a "Flags m_flags" as an instance variable and just provide accessors: class Architecture {
Flags GetFlags() { return m_flags; }
const Flags GetFlags() const { return m_flags; }
}; |
| 127 ↗ | (On Diff #182848) | It is not clear what this function does and it doesn't seem like a function that belongs on this class. |
| source/Plugins/Architecture/Arc/ArchitectureArc.cpp | ||
| 44–71 ↗ | (On Diff #182848) | This entire function's contents should be placed where it is called and not in the Architecture class. |
| 73–75 ↗ | (On Diff #182848) | Remove and use accessor to m_flags. |
| 77–79 ↗ | (On Diff #182848) | Remove from this class and inline at call site. |
| 81–86 ↗ | (On Diff #182848) | Remove from this class and inline at call site. |
| source/Plugins/Architecture/Arc/ArchitectureArc.h | ||
| 36–42 ↗ | (On Diff #182848) | These four functions don't seem like they belong on the Architecture class. It would be best to just have the code that calls these methods inline the code where needed. |
| 48 ↗ | (On Diff #182848) | I would put this in the base class and then add just an accessor: class Architecture {
Flags GetFlags() { return m_flags; }
const Flags GetFlags() const { return m_flags; }
}; |
| source/Plugins/Architecture/Arc/ArchitectureArc.cpp | ||
|---|---|---|
| 1–8 ↗ | (On Diff #183110) | Does this file need to exist anymore? If not remove it. If so, fill in at least one virtual function that is needed. |
| source/Plugins/Architecture/Arc/ArchitectureArc.h | ||
| 18–22 ↗ | (On Diff #183110) | Would this be better as a public enum inside of the ArchitectureArc class named Flags? class ArchitectureArc: public Architecture {
enum Flags {
reduced_register_file = 1u << 0,
big_endian = 1u << 1,
address_size_32 = 1u << 2
};
};There is also a definition that marks a enum as a bitfield. |
| 34 ↗ | (On Diff #183110) | Are we missing this? Any reason for this to exist? If this function need to doesn't exist then this ArchitectureArc class doesn't need to exist and can be removed. |
| source/Plugins/Architecture/Arc/ArchitectureArc.cpp | ||
|---|---|---|
| 1–8 ↗ | (On Diff #183110) | It's needed to not keep static functions implementation in the header file. And it will definitely be required when I'll implement GetBreakableLoadAddress to handle delay slots, but this will not be in this revision. |
| source/Plugins/Architecture/Arc/ArchitectureArc.h | ||
| 34 ↗ | (On Diff #183110) | It's because OverrideStopInfo is a pure virtual function. No one but ArchitectureArm really needs to override this. I'll be glad to replace = 0 in the base class with the empty body (by a separate commit). |
Comment Actions
Moved ARC flags enum inside the ArchitectureArc class;
Made OverrideStopInfo with empty body instead of being pure virtual. If this change is Ok I'll commit it before ARC support.
Comment Actions
Changed configuration register handling logic: if an RSP-server doesn't provide a register, don't count this as a failure - just use default values for flags.
Comment Actions
It would be really nice to get the GDB remote server for ARC so that it vends the correct regs to begin with, but if that isn't possible I guess we need to do what we are doing here. I would really like to not see more architectures have to add code to ProcessGDBRemote.cpp at all. If that isn't possible this solution will need to be used.
Comment Actions
Unrelated to this patchset, but just last week I was working on something and thinking about how our system of "the RSP stub is the canonical source of register knowledge" is probably not the best architecture. IMO the remote stub should teach us (1) the name of each register, (2) the numbers it will use for each register, (3) the size in bits of each register, and (4) the offset into the g/G packet of that register.
Everything else about registers should come from lldb in either architecture-specific definitions (rax has 64 bits, and it has a slice register eax of 32 bits, an ax of 16 bits etc; it is printed as Uint64/32/16), or ABI definitions (rdi is arg1 in the posix x86_64 ABI; rsp is eh_frame regnum 7, dwarf regnum 7). Coordination between the RSP stub and lldb should be done by register name, confirmed by register size (the remote stub says it has a 64-bit r0 and I only know about a 32-bit r0, we have a problem).
Back in 2015 I added some code in r247121 that would fill in eh_frame and dwarf register numbers from the ABI plugin if the remote stub didn't provide them in ProcessGDBRemote's AugmentRegisterInfoViaABI. It might be better for ARC to hardcode these generic register additions some place like that. The change in 247121 was a step in the direction of having lldb be the canonical source of information about registers, but only doing the easy first bit.
Comment Actions
As long as the numbers _can_ still come from the GDB server, I am all for that. If they are not supplied, then we use arch defaults. That allows new system/arch bringups where the info might be changing often, and also allows the info to be locked down. The architecture plug-ins are a great place for this. We should also move the breakpoint opcode logic into the architecture plug-ins.
Comment Actions
I have no opinion about the RSP stub overriding lldb's definitions - fine by me. I'm mostly interested in making lldb work better with rando RSP implementations in the wild which often give us very little information about the registers.
Moving the breakpoint opcode into lldb is interesting, would you want to move away from the Z0/z0 packets for setting/removing breakpoints then? The set-breakpoint packet takes an address and iirc an instruction size. So on armv7 processors, we know whether we're replacing a 2-byte or 4-byte instruction. I don't think we want to move the logic of breakpoint setting & clearing into lldb and use generic read/write memory - the RSP stub may know how to clear instruction caches when we're modifying instruction data, for instance. So we'd make a variation of the z0 packet which takes an address and a byte sequence to put at that address? tbh it seems like the RSP stub is the correct place for this knowledge, even though we have to provide the hack of "use the 2-byte breakpoint instruction" / "use the 4-byte breakpoint instruction" on targets like armv7 because lldb has the knowledge of the underlying instructions.
Comment Actions
Which part of the information that Jason mentioned do you expect to change during bringup?
- eh/debug_frame numbers aren't something that the stub can unilaterally change, as it needs to match what the compiler thinks. Since we're not going to get the compiler to ask our stub about the eh_frame numbers, I don't see the problem in hardcoding that info. TBE, the best way would be no not even hardcode that info but ask llvm about these things. That way if somebody develops the whole platform (compiler, abi, and everything), and uses llvm technology everywhere, he would just need to change the numbers once and recompile -- lldb, lldb-server and clang would automatically pick that up
- the ABI is also something that needs to match with the compiler, and is something that is largely irrelevant for the operation of the rsp stub
- how to print a register is also completely irrelevant for the stub, and (IMO) also largely unimportant during early bringup stages
The things I can see changing frequently while developing a stub for a new platform are the register numbers and g packet offsets, but that's already something that Jason included in the list of things that the stub should provide.
Comment Actions
Latest versions of this patch use AugmentRegisterInfoViaABI to adjust eh_frame/dwarf numbers. But I still need to examine configuration registers to determine current configuration of the ARC processor. The Architecture plug-in would be a better place to do this, however it need to obtain register values when RegisterContext doesn't exist. It is not possible for now. As I see it, we should rather make Process able to read "fixed" (context-independent) registers even when there is no threads yet. Also the RegisterContext might avoid updating those registers on every stop. Does someone think this would be a good idea? Anyway, it ought to be a separate differential revision, I think.
Comment Actions
llvm::MCRegisterInfo doesn't provide us with all necessary fields to create a full RegisterInfo array for an ABI. But we can use its llvm/eh_frame/dwarf register numbers mapping at least.
Comment Actions
Hi, currently we have a private build server that executes the test-suite on ARC. There are failures for now, mostly due to unimplemented features for ARC like expressions support.
I'll take care of a public build-bot, if it is required.
Comment Actions
I was more thinking of unit/regression testing, though I don't know how good our coverage is today for things like architectures. That being said, a public build bot would be a great addition and might pay off in the long run.
Comment Actions
Removed any ARC-specific logic from the ProcessGDBRemote.cpp.
It seems, there is nothing to test now;)