Page MenuHomePhabricator

Have ABI plugins vend llvm MCRegisterInfo data
ClosedPublic

Authored by labath on Sep 24 2019, 7:12 AM.

Details

Summary

I was recently surprised to learn that there is a total of 2 (two) users
of the register info definitions contained in the ABI plugins. Yet, the
defitions themselves span nearly 10kLOC.
The two users are:

  • dwarf expression pretty printer
  • the mechanism for augmenting the register info definitions obtained over gdb-remote protocol (AugmentRegisterInfoViaABI)

Both of these uses need the DWARF an EH register numbers, which is
information that is already available in LLVM. This patch makes it
possible to do so.

It adds a GetMCRegisterInfo method to the ABI class, which every class
is expected to implement. Normally, it should be sufficient to obtain
the definitions from the appropriate llvm::Target object (for which I
provide a utility function), but the subclasses are free to construct it
in any way they deem fit.

We should be able to always get the MCRegisterInfo object from llvm,
with one important exception: if the relevant llvm target was disabled
at compile time. To handle this, I add a mechanism to disable the
compilation of ABI plugins based on the value of LLVM_TARGETS_TO_BUILD
cmake setting. This ensures all our existing are able to create their
MCRegisterInfo objects.

The new MCRegisterInfo api is not used yet, but the intention is to make
use of it in follow-up patches.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Sep 24 2019, 7:12 AM
JDevlieghere added inline comments.Sep 24 2019, 9:11 AM
source/Target/ABI.cpp
216 ↗(On Diff #221540)

Should this return an llvm::Expected instead? I understand it will cause a lot of churn around the call sites, but now if this fails it will trigger an assert in the ABI constructor?

include/lldb/Target/ABI.h
141 ↗(On Diff #221540)

Since this and derived class's constructors are protected/private it can make sense to make their parameters r-value and not rely on copy elision.

labath marked an inline comment as done.Sep 24 2019, 10:26 AM
labath added inline comments.
source/Target/ABI.cpp
216 ↗(On Diff #221540)

Yes, it will, but the idea is that one should call this function only if one is certain that it will succeed (or if he's uncertain, he should at least check the result before passing it to the ABI constructor). As far as I am aware of, there are only two ways that this can fail:

  1. we pass it some completely bogus triple
  2. llvm was compiled without support for the given triple (this is kind of already covered by the first item, but anyway...)

All ABI plugins check that the archspec is "reasonable" (they at least check the architecture, some also check other fields), so the first case should not occur. And I am excluding the second case here, by making sure the ABI plugin is initialized only if the relevant llvm target is built.

I actually first coded an implementation which did something like if (is_supported(arch_spec)) { if (auto info = MakeMCRegisterInfo(arch_spec)) return ABI(process, info), but then I ditched it in favor of this. I can go back to that if you want (because the current version is not completely ideal either), but I kind of like this one more, because if anything goes wrong, it will be easier to track down the failure. (and it's pretty unlikely that this will only fail in some corner cases, as the only variable here is the ArchSpec, so even the most superficial testing of a given target should uncover any problems here.)

aprantl added inline comments.Sep 24 2019, 12:08 PM
source/Plugins/ABI/CMakeLists.txt
1 ↗(On Diff #221540)

This looks like a good idea regardless.

This revision is now accepted and ready to land.Sep 24 2019, 1:59 PM
JDevlieghere accepted this revision.Sep 24 2019, 6:47 PM

Thanks for the explanation. LGTM

labath marked 3 inline comments as done.Sep 25 2019, 5:50 AM
labath added inline comments.
include/lldb/Target/ABI.h
141 ↗(On Diff #221540)

Technically, this is not copy elision, since std::move supresses that. :)
But anyway, I generally try to avoid overusing rvalue references as they tend to make one wonder whether there is something funny going on (and in particular with std::unique_ptr, a rvalue reference can have subtly different behavior than a plain object). I also believe the advice given (by some experts at least) since c++11 is to prefer plain objects where that makes sense.

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2019, 6:02 AM

@labath
I am building with AVR as experimental target and this change probably broke the build.

In file included from /home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/tools/lldb/source/API/SystemInitializerFull.cpp:208:
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/build-llvm/tools/clang/stage2-bins/include/llvm/Config/Targets.def:42:1: error: reference to non-static member function must be called; did you mean to call it with no arguments?
LLVM_TARGET(AVR)
^~~~~~~~~~~~~~~~
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/tools/lldb/source/API/SystemInitializerFull.cpp:207:43: note: expanded from macro 'LLVM_TARGET'
#define LLVM_TARGET(t) LLDB_PROCESS_ ## t(Initialize)
                                          ^~~~~~~~~~
In file included from /home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/tools/lldb/source/API/SystemInitializerFull.cpp:208:
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/build-llvm/tools/clang/stage2-bins/include/llvm/Config/Targets.def:42:1: error: use of undeclared identifier 'LLDB_PROCESS_AVR'
/home/sylvestre/dev/debian/pkg-llvm/llvm-toolchain/branches/llvm-toolchain-snapshot_10~svn372978/tools/lldb/source/API/SystemInitializerFull.cpp:207:24: note: expanded from macro 'LLVM_TARGET'
#define LLVM_TARGET(t) LLDB_PROCESS_ ## t(Initialize)
                       ^

Could you please have a look?
thanks

@labath
I am building with AVR as experimental target and this change probably broke the build.

Could you please have a look?
thanks

Thanks for the heads up. r372998 ought to fix that.