[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The main reason I don't want the Architecture class in ArchSpec is to keep the dependency graph of lldb-server clean (it's not very clean to begin with, but at least I want to avoid making it worse). lldb-server should not know anything about the Target class (that's a liblldb concept), but it has plenty of reasons to play around with ArchSpecs. If ArchSpec contained an Architecture instance, then lldb-server would end up transitively (Architecture::GetBreakableLoadAddress and friends) depending on Target. That's why I think we need two repositories for architecture-specific code. ArchSpec could be the home for the shared liblldb+lldb-server stuff (which will end up being low level code only, because lldb-server is low-level). The home for the second stuff might as well be the Architecture class. The &~1 logic seems sufficiently low-level to fall into the first category -- even though lldb-server does not seem to need it right now, it's not hard to imagine it needing that in the future.

A secondary issue is the inheritance. ArchSpec is currently a simple, (mostly) trivially copyable class. Introducing inheritance would complicate that. It's true that virtual dispatch can be cleaner than a switch, but I think this only applies if the logic inside the switch cases is complicated. And even then the code can be made relatively clean with a bit of care (and llvm is does not seem to be afraid of switches in general).  In this particular case, I think that the logic is actually so simple that the virtual dispatch would only obscure what is going on.

If, in the future, we end up needing to put some more complicated code here, we can revisit this decision (in that case, I'd probably argue for creating some sort of a different entity to hold this functionality), but for now, I don't see a reason to complicate the ArchSpec design on account of this.

I can see how, from the perspective of someone maintaining a downstream target, having everything architecture-specific in a single place would be appealing, but I wouldn't want to put this objective too high on the priority list. Otherwise, I fear that this entity (whatever it would end up being called) will end up being a central nexus for everything in lldb. For example there's a lot of arm-specific code in ObjectFileELF. Some of that could be restructured to be independent of elf, but doing that for everything would be tricky, and so we may end up with everything depending on ELF.


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

https://reviews.llvm.org/D70840