Page MenuHomePhabricator

[ARC] Basic support in gdb-remote process plugin
ClosedPublic

Authored by tatyana-krasnukha on Dec 14 2018, 1:56 PM.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Dec 17 2018, 9:45 AM
tatyana-krasnukha marked 2 inline comments as done.Dec 18 2018, 4:47 AM

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

Addressed review comments

See inline comments and let me know what you think.

include/lldb/Utility/ArchSpec.h
88–89

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?

tatyana-krasnukha marked an inline comment as not done.Dec 19 2018, 2:44 AM

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...

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.

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.

Clear m_register_info if it is incorrect.

Friendly ping. I would like to have this in the nearest release.

clayborg added inline comments.Jan 14 2019, 10:34 AM
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?

tatyana-krasnukha marked an inline comment as done.Jan 14 2019, 12:09 PM
tatyana-krasnukha added inline comments.
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.

labath added a subscriber: labath.Jan 17 2019, 8:07 AM
labath added inline comments.
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:

  • you cannot test even the most basic unwind functionality (like parsing eh_frame) without having a live process (or a faithful mock) somewhere
  • perhaps more importantly: the unwind info that does get parsed is cached in the object file, which can be shared between multiple targets. So if another target tries to use that info, and this target's stub sends us different register numbers than the previous one, then it's unwinding will be all wrong.

Anyway, my point here is that I would be supportive of transitioning to a different source of dwarf register numbers than the remote stub.

tatyana-krasnukha marked an inline comment as not done.Jan 17 2019, 8:24 AM
tatyana-krasnukha added inline comments.
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.

Removed registers adjustment, dwarf numbers are corrected by AugmentRegisterInfoViaABI now.

clayborg added inline comments.Jan 17 2019, 10:42 AM
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.

After all, I moved ARC configuring routines to the ArchitechtureArc plug-in.

clayborg requested changes to this revision.Jan 18 2019, 7:16 AM
clayborg added inline comments.
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);

This revision now requires changes to proceed.Jan 18 2019, 7:16 AM
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.

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).

Keep trying to hide the processor's specifics from the ProcessGDBRemote

clayborg requested changes to this revision.Jan 22 2019, 7:18 AM
clayborg added inline comments.
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; }
};
This revision now requires changes to proceed.Jan 22 2019, 7:18 AM

Addressed comments

clayborg added inline comments.Jan 23 2019, 3:37 PM
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).

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.

Updated new file headers to reflect the new license

tatyana-krasnukha removed a subscriber: llvm-commits.

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.

Kind reminder. I believe all discussions have been resolved.

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.

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.

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.

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.

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.

+1000

This sounds really great to me.

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.

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.

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.

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.

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.

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.

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.

TBE, the best way would be no not even hardcode that info but ask llvm about these things.

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.

Hey Tatyana, what's the plan with regards to testing this?

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.

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.

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.

Removed any ARC-specific logic from the ProcessGDBRemote.cpp.

It seems, there is nothing to test now;)

clayborg accepted this revision.Oct 17 2019, 7:59 AM
This revision is now accepted and ready to land.Oct 17 2019, 7:59 AM
This revision was automatically updated to reflect the committed changes.