[Lldb-commits] [PATCH] D67965: Have ABI plugins vend llvm MCRegisterInfo data

[Pavel Labath via Phabricator via lldb-commits]

labath marked an inline comment as done.
labath added inline comments.


================
Comment at: source/Target/ABI.cpp:216
+
+std::unique_ptr<llvm::MCRegisterInfo> ABI::MakeMCRegisterInfo(const ArchSpec &arch) {
+  std::string triple = arch.GetTriple().getTriple();
----------------
JDevlieghere wrote:
> 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?
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.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67965/new/

https://reviews.llvm.org/D67965