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

Thu Dec 19 14:30:53 PST 2019

clayborg added a comment.

In D70840#1783351 <https://reviews.llvm.org/D70840#1783351>, @labath wrote:

> I am strongly opposed to ArchSpec owing an Architecture object. The latter is far more complicated -- it reads bytes from target memory and disassembles them -- whereas ArchSpec just returns a bunch of constants. If anything, it should be the other way around. That way the code which just fiddles with triples does not need to depend on the whole world.

I know part of this re-org was to keep the ArchSpec class from accessing any Target stuff to keep each directory with source code from accessing stuff in another tree. So from that perspective I can see your objection. I don't think it adds anything at all to the code by splitting this up other than achieving this separation though. It makes sense to ask an architecture to do architecture specific things. No sure why they need to be split IMHO.

> Having an Architecture instance in the Module object (to ensure it's independent of the target) seems /ok/, though I am not really convinced that it is needed, as the &~1 logic seems like a good candidate for ArchSpec (which already knows a lot about arm vs. thumb etc.

I would rather keep all architecture specific code in the Architecture plug-ins somehow. It keeps code modifications clearer as only on file changes that is architecture specific. If we inline stuff into ArchSpec we end up with a big switch statement for the core and every adds their code to the same function.

So not sure what the right solution is here. I still like folding the Architecture into ArchSpec myself, but I am open to any and all opinions.

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

https://reviews.llvm.org/D70840