[Lldb-commits] [PATCH] D25864: Fix arm64 floating point register spill recording in UnwindPlan analysis

[Tamas Berghammer via lldb-commits]

tberghammer accepted this revision.
tberghammer added a comment.
This revision is now accepted and ready to land.

The patch generally looks good to me. I added a few high level thought about register context but they are clearly out of scope for this change. Also next time please upload your patch with full context as it is much easier to review that way.

In https://reviews.llvm.org/D25864#578173, @jasonmolenda wrote:

> Hi Tamas, sorry for not replying earlier, something urgent came up that I needed to look at.
>
> Thanks for the review.  I agree with using your existing arm64 register enums in lldb-arm64-register-enums.h.  I'd like to remove the enums in RegisterInfos_arm64.h instead of having two copies that could diverge.
>
> You asked about having EmulateInstructionARM64 handle both eRegisterKindDWARF and eRegisterKindLLDB.  I'm not sure how that would work - each UnwindPlan must can use only one register numbering scheme.  We could use eRegisterKindDWARF if there were no floating point register save/restores - but that seems a bit complicated; we'd conditionally use eRegisterKindLLDB sometimes, eRegisterKindDWARF most of the time.


My idea is based on the fact that every register access goes through the GetRegisterInfo function what gets a "register kind" and a "register number" so if we can teach that function to be able to fetch the correct register info both for eRegisterKindLLDB and for eRegisterKindDWARF then the actual emulate functions can use both. It would mean something like the following, but I don't know what should we put in place of the "...":

  if (reg_kind == eRegisterKindLLDB)
    return LLDBTableGetRegisterInfo(reg_num, reg_info);
  if (reg_kind == eRegisterKindDwarf)
    return ...



> As for the GetRegisterName() function in ARM64_LLDB_Registers.cpp, we could have EmulateInstructionARM64 with the register file definition (we need to define seven preprocessor symbols before we can include RegisterInfos_arm64.h and then we'll need to write a GetRegisterName() method in EmulateInstructionARM64.cpp.  Wouldn't it be clearer to have a lldb-arm64-register-enums.cpp with this method?

RegisterInfos_arm64.h already contains a register name and an alternative name so I would prefer to use them as they are already specified but I have no problem with having a GetRegisterName() function if we can have a single copy of it. My main problem with your previous approach was that you are adding a second copy of it (current one is in ARM64_DWARF_Registers.cpp)

Regarding including RegisterInfos_arm64.h I am not too keen on the approach of having a large table and defining a set of macros as it is very verbose and limits the flexibility (e.g. big/little endian issue we found recently) but I haven't come up with a better idea yet.

> I was talking about these register numbering problems with Greg Clayton and he half-jokingly said we should switch from register numbers to using ConstString register names in the UnwindPlans.  It's not a bad idea!  As long as everyone can agree on the register names for an architecture, anyway.

Personally I really don't like ConstString because of the global pool behind it (kind of leaks memory) so I think having some sort of register number is better. I think a betetr approach would be to agree that inside LLDB everything should use eRegisterKindLLDB and we do the register number translation at the communication boundaries (gdb-remote communication, eh_frame parsing, dwarf reading)


Repository:
  rL LLVM

https://reviews.llvm.org/D25864