Page MenuHomePhabricator

[ARC] Basic support in gdb-remote process plugin
Needs ReviewPublic

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

Details

Summary

Add ARC architecture (bare-metal) that can be debugged through an RSP-server.

Diff Detail

Repository
rLLDB LLDB

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
clayborg requested changes to this revision.Dec 17 2018, 9:45 AM

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

Is DynamicRegisterInfo really in the top level namespace?

18

include the lldb-forward.h and remove this

include/lldb/Utility/ArchSpec.h
91–92

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
77–94

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

Context would really help here to see the surrounding code.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
4530–4541

Make:

const lldb_private::RegisterInfo *DynamicRegisterInfo::GetRegisterInfo(const lldb_private::ConstString &reg_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

This code seems like it could be cleaned up.

  • ConfigureArchitecture should be named ConfigureARCArchitecture if it truly is ARC specfic
  • why doesn't ConfigureArchitecture call GetTarget().SetArchitecture(arch_to_use) itself?
  • Do we really want to return if ConfigureArchitecture() returns false? Do we not want to adjust the register info?
  • why doesn't ConfigureArchitecture just fix up the dynamic register info and avoid the call to AdjustRegisterInfo in an architecture plug-in
source/Target/Platform.cpp
1872–1876

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?

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

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
91–92

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

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

the rf16 could be passed into this function as an argument and then the enum can be removed from ArchSpec.h?

4704–4707

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

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

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

"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

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

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

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

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

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
115–124

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
30

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"?

47–79

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
33–41

Use lldb_private::Flags

source/Plugins/Process/Utility/DynamicRegisterInfo.cpp
693–720

Magic numbers? Can we use enums?

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
1222–1223

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

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
30

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.Tue, Jan 22, 7:18 AM
clayborg added inline comments.
include/lldb/Core/Architecture.h
115–119

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.

121

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; }
};
123

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
45–72

This entire function's contents should be placed where it is called and not in the Architecture class.

74–76

Remove and use accessor to m_flags.

78–80

Remove from this class and inline at call site.

82–87

Remove from this class and inline at call site.

source/Plugins/Architecture/Arc/ArchitectureArc.h
37–43

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.

49

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.Tue, Jan 22, 7:18 AM

Addressed comments

clayborg added inline comments.Wed, Jan 23, 3:37 PM
source/Plugins/Architecture/Arc/ArchitectureArc.cpp
2–9

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
19–23

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.

35

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
2–9

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
35

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.