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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
