[Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
This revision was automatically updated to reflect the committed changes. Closed by commit rL284003: [LLDB][MIPS] fix Floating point register read/write for big endian (authored by nitesh.jain). Changed prior to commit: https://reviews.llvm.org/D24603?vs=73278&id=74357#toc Repository: rL LLVM https://reviews.llvm.org/D24603 Files: lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h lldb/trunk/source/Plugins/Process/Utility/RegisterInfos_mips.h lldb/trunk/source/Plugins/Process/Utility/RegisterInfos_mips64.h lldb/trunk/source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h Index: lldb/trunk/source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h === --- lldb/trunk/source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h +++ lldb/trunk/source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h @@ -282,6 +282,84 @@ k_num_fpr_registers_mips64 + k_num_msa_registers_mips64 }; + +// Register no. for RegisterKind = eRegisterKindProcessPlugin +// The ptrace request PTRACE_PEEKUSER/PTRACE_POKEUSER used this number +enum { + ptrace_zero_mips, + ptrace_r1_mips, + ptrace_r2_mips, + ptrace_r3_mips, + ptrace_r4_mips, + ptrace_r5_mips, + ptrace_r6_mips, + ptrace_r7_mips, + ptrace_r8_mips, + ptrace_r9_mips, + ptrace_r10_mips, + ptrace_r11_mips, + ptrace_r12_mips, + ptrace_r13_mips, + ptrace_r14_mips, + ptrace_r15_mips, + ptrace_r16_mips, + ptrace_r17_mips, + ptrace_r18_mips, + ptrace_r19_mips, + ptrace_r20_mips, + ptrace_r21_mips, + ptrace_r22_mips, + ptrace_r23_mips, + ptrace_r24_mips, + ptrace_r25_mips, + ptrace_r26_mips, + ptrace_r27_mips, + ptrace_gp_mips, + ptrace_sp_mips, + ptrace_r30_mips, + ptrace_ra_mips, + ptrace_f0_mips, + ptrace_f1_mips, + ptrace_f2_mips, + ptrace_f3_mips, + ptrace_f4_mips, + ptrace_f5_mips, + ptrace_f6_mips, + ptrace_f7_mips, + ptrace_f8_mips, + ptrace_f9_mips, + ptrace_f10_mips, + ptrace_f11_mips, + ptrace_f12_mips, + ptrace_f13_mips, + ptrace_f14_mips, + ptrace_f15_mips, + ptrace_f16_mips, + ptrace_f17_mips, + ptrace_f18_mips, + ptrace_f19_mips, + ptrace_f20_mips, + ptrace_f21_mips, + ptrace_f22_mips, + ptrace_f23_mips, + ptrace_f24_mips, + ptrace_f25_mips, + ptrace_f26_mips, + ptrace_f27_mips, + ptrace_f28_mips, + ptrace_f29_mips, + ptrace_f30_mips, + ptrace_f31_mips, + ptrace_pc_mips, + ptrace_cause_mips, + ptrace_badvaddr_mips, + ptrace_mulhi_mips, + ptrace_mullo_mips, + ptrace_fcsr_mips, + ptrace_fir_mips, + ptrace_sr_mips, + ptrace_config5_mips +}; } #endif // #ifndef lldb_mips_linux_register_enums_h Index: lldb/trunk/source/Plugins/Process/Utility/RegisterInfos_mips.h === --- lldb/trunk/source/Plugins/Process/Utility/RegisterInfos_mips.h +++ lldb/trunk/source/Plugins/Process/Utility/RegisterInfos_mips.h @@ -35,11 +35,11 @@ LLVM_EXTENSION offsetof(MSA_linux_mips, regname)) // Note that the size and offset will be updated by platform-specific classes. -#define DEFINE_GPR(reg, alt, kind1, kind2, kind3, kind4) \ +#define DEFINE_GPR(reg, alt, kind1, kind2, kind3) \ {\ #reg, alt, sizeof(((GPR_linux_mips *) NULL)->reg) / 2, \ GPR_OFFSET(reg), eEncodingUint, eFormatHex, \ - {kind1, kind2, kind3, kind4, \ + {kind1, kind2, kind3, ptrace_##reg##_mips,\ gpr_##reg##_mips }, \ NULL, NULL, NULL, 0 \ } @@ -49,21 +49,21 @@ llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_and, llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shr}; -#define DEFINE_FPR(reg, alt, kind1, kind2, kind3, kind4) \ +#define DEFINE_FPR(reg, alt, kind1, kind2, kind3) \ {\ #reg, alt, sizeof(((FPR_linux_mips *) NULL)->reg), \ FPR_OFFSET(reg), eEncodingIEEE754, eFormatFloat, \ - {kind1, kind2, kind3, kind4, \ + {kind1, kind2, kind3, ptrace_##reg##_mips,\ fpr_##reg##_mips }, \ NULL, NULL, dwarf_opcode_mips, \ sizeof(dwarf_opcode_mips)\ } -#define DEFINE_FPR_I
[Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain added a comment. In https://reviews.llvm.org/D24603#566575, @clayborg wrote: > Commit anytime. Thanks https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
clayborg added a comment. Commit anytime. https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain added a comment. Hi Greg, The diff is update as per suggestion so should I commit this ? Thanks https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain updated this revision to Diff 73278. nitesh.jain added a comment. These diff remove manually bit twiddling due to the size of the floating point register that can change. We use register offset to get floating point register data based on endianess and it's size. We have not remove bit twiddling for MSA register since it's need to be tested( require support in ptrace). Thanks https://reviews.llvm.org/D24603 Files: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h source/Plugins/Process/Utility/RegisterInfos_mips.h source/Plugins/Process/Utility/RegisterInfos_mips64.h source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h Index: source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h === --- source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h +++ source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h @@ -282,6 +282,84 @@ k_num_fpr_registers_mips64 + k_num_msa_registers_mips64 }; + +// Register no. for RegisterKind = eRegisterKindProcessPlugin +// The ptrace request PTRACE_PEEKUSER/PTRACE_POKEUSER used this number +enum { + ptrace_zero_mips, + ptrace_r1_mips, + ptrace_r2_mips, + ptrace_r3_mips, + ptrace_r4_mips, + ptrace_r5_mips, + ptrace_r6_mips, + ptrace_r7_mips, + ptrace_r8_mips, + ptrace_r9_mips, + ptrace_r10_mips, + ptrace_r11_mips, + ptrace_r12_mips, + ptrace_r13_mips, + ptrace_r14_mips, + ptrace_r15_mips, + ptrace_r16_mips, + ptrace_r17_mips, + ptrace_r18_mips, + ptrace_r19_mips, + ptrace_r20_mips, + ptrace_r21_mips, + ptrace_r22_mips, + ptrace_r23_mips, + ptrace_r24_mips, + ptrace_r25_mips, + ptrace_r26_mips, + ptrace_r27_mips, + ptrace_gp_mips, + ptrace_sp_mips, + ptrace_r30_mips, + ptrace_ra_mips, + ptrace_f0_mips, + ptrace_f1_mips, + ptrace_f2_mips, + ptrace_f3_mips, + ptrace_f4_mips, + ptrace_f5_mips, + ptrace_f6_mips, + ptrace_f7_mips, + ptrace_f8_mips, + ptrace_f9_mips, + ptrace_f10_mips, + ptrace_f11_mips, + ptrace_f12_mips, + ptrace_f13_mips, + ptrace_f14_mips, + ptrace_f15_mips, + ptrace_f16_mips, + ptrace_f17_mips, + ptrace_f18_mips, + ptrace_f19_mips, + ptrace_f20_mips, + ptrace_f21_mips, + ptrace_f22_mips, + ptrace_f23_mips, + ptrace_f24_mips, + ptrace_f25_mips, + ptrace_f26_mips, + ptrace_f27_mips, + ptrace_f28_mips, + ptrace_f29_mips, + ptrace_f30_mips, + ptrace_f31_mips, + ptrace_pc_mips, + ptrace_cause_mips, + ptrace_badvaddr_mips, + ptrace_mulhi_mips, + ptrace_mullo_mips, + ptrace_fcsr_mips, + ptrace_fir_mips, + ptrace_sr_mips, + ptrace_config5_mips +}; } #endif // #ifndef lldb_mips_linux_register_enums_h Index: source/Plugins/Process/Utility/RegisterInfos_mips64.h === --- source/Plugins/Process/Utility/RegisterInfos_mips64.h +++ source/Plugins/Process/Utility/RegisterInfos_mips64.h @@ -42,11 +42,11 @@ // Note that the size and offset will be updated by platform-specific classes. #ifdef LINUX_MIPS64 -#define DEFINE_GPR(reg, alt, kind1, kind2, kind3, kind4) \ +#define DEFINE_GPR(reg, alt, kind1, kind2, kind3) \ {\ #reg, alt, sizeof(((GPR_linux_mips *) 0)->reg),\ GPR_OFFSET(reg), eEncodingUint, eFormatHex, \ - {kind1, kind2, kind3, kind4, \ + {kind1, kind2, kind3, ptrace_##reg##_mips,\ gpr_##reg##_mips64 },\ NULL, NULL, NULL, 0 \ } @@ -61,11 +61,11 @@ } #endif -#define DEFINE_GPR_INFO(reg, alt, kind1, kind2, kind3, kind4) \ +#define DEFINE_GPR_INFO(reg, alt, kind1, kind2, kind3) \ {\ #reg, alt, sizeof(((GPR_linux_mips *) 0)->reg) / 2,\ GPR_OFFSET(reg), eEncodingUint, eFormatHex, \ - {kind1, kind2, kind3, kind4, \ + {kind1, kind2, kind3, ptrace_##reg##_mips,\ gpr_##reg##_mips64 },\ NULL, NULL, NULL, 0 \ } @@ -75,21 +75,21 @@ llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_and, llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shr}; -#define DEFINE_FPR(reg, alt, kind1, kind2, kind3, kind4) \ +#define DEFINE_FPR(reg, alt, kind1, ki
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
clayborg added a comment. Please comment why you are manually bit twiddling due to the size of the register that can change. We don't want anyone else copying this kind of code. Another solution would be to update the offset of the register when you change the byte size so none of this would need to be done. We are already updating the byte size of the register in the RegisterInfo, so why not do the same for the offset. You could store the original offset in the RegisterInfo.kinds[eRegisterKindProcessPlugin] as this field is only for the process plug-in's use. https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain added a comment. In https://reviews.llvm.org/D24603#548902, @clayborg wrote: > So it seems like this can be fixed by doing a few things: > 1 - just set the RegisterInfo.offset to the actual offset in the buffer and > don't use the offset as the ptrace key used to grab the register > 2 - modify the RegisterInfo.kinds[eRegisterKindProcessPlugin] to be the > offset to you to use for ptrace > > There should be no need what so ever to do manual bit twiddling with src and > dst. The floating point register size can be dynamically changes . Hence following cases are possible when we read registers from ptrace in FPR_linux_mips buffer (In case of MIPS, ptrace always return value in 64 bit container irrespective of Arch). 1. Floating point register size is 32 (SR.FR = 0). a) In case of big endian system, the FPR_linux_mips.f0 will contains two floating point registers . The $f0 register will be store in upper 32 bit and $f1 register in lower 32 bit of FPR_linux_mips.f0. The FPR_linux_mips.f1 will contain don't care value and similarly for all other registers. Thus each even buffer will contain valid value and represent two registers. b) In case of liitle endian , the $f0 will be store in lower 32 bit and $f1 in upper 32bit of FPR_linux_mips.f0. The FPR_linux_mips.f1 will contain don't care value and similarly for all other registers. 2. Floating point register size is 64 (SR.FR = 1). In these case all buffer will have valid value. Hence we need bit twiddling with src and dst (in case of SR.FR = 0) so that each buffer will represent value for single register. https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain updated this revision to Diff 72618. nitesh.jain added a comment. Herald added a subscriber: ki.stfu. Updated patch as per suggestion. https://reviews.llvm.org/D24603 Files: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h source/Plugins/Process/Utility/RegisterInfos_mips.h source/Plugins/Process/Utility/RegisterInfos_mips64.h source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h Index: source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h === --- source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h +++ source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h @@ -282,6 +282,84 @@ k_num_fpr_registers_mips64 + k_num_msa_registers_mips64 }; + +// Register no. for RegisterKind = eRegisterKindProcessPlugin +// The ptrace request PTRACE_PEEKUSER/PTRACE_POKEUSER used this number +enum { + ptrace_zero_mips, + ptrace_r1_mips, + ptrace_r2_mips, + ptrace_r3_mips, + ptrace_r4_mips, + ptrace_r5_mips, + ptrace_r6_mips, + ptrace_r7_mips, + ptrace_r8_mips, + ptrace_r9_mips, + ptrace_r10_mips, + ptrace_r11_mips, + ptrace_r12_mips, + ptrace_r13_mips, + ptrace_r14_mips, + ptrace_r15_mips, + ptrace_r16_mips, + ptrace_r17_mips, + ptrace_r18_mips, + ptrace_r19_mips, + ptrace_r20_mips, + ptrace_r21_mips, + ptrace_r22_mips, + ptrace_r23_mips, + ptrace_r24_mips, + ptrace_r25_mips, + ptrace_r26_mips, + ptrace_r27_mips, + ptrace_gp_mips, + ptrace_sp_mips, + ptrace_r30_mips, + ptrace_ra_mips, + ptrace_f0_mips, + ptrace_f1_mips, + ptrace_f2_mips, + ptrace_f3_mips, + ptrace_f4_mips, + ptrace_f5_mips, + ptrace_f6_mips, + ptrace_f7_mips, + ptrace_f8_mips, + ptrace_f9_mips, + ptrace_f10_mips, + ptrace_f11_mips, + ptrace_f12_mips, + ptrace_f13_mips, + ptrace_f14_mips, + ptrace_f15_mips, + ptrace_f16_mips, + ptrace_f17_mips, + ptrace_f18_mips, + ptrace_f19_mips, + ptrace_f20_mips, + ptrace_f21_mips, + ptrace_f22_mips, + ptrace_f23_mips, + ptrace_f24_mips, + ptrace_f25_mips, + ptrace_f26_mips, + ptrace_f27_mips, + ptrace_f28_mips, + ptrace_f29_mips, + ptrace_f30_mips, + ptrace_f31_mips, + ptrace_pc_mips, + ptrace_cause_mips, + ptrace_badvaddr_mips, + ptrace_mulhi_mips, + ptrace_mullo_mips, + ptrace_fcsr_mips, + ptrace_fir_mips, + ptrace_sr_mips, + ptrace_config5_mips +}; } #endif // #ifndef lldb_mips_linux_register_enums_h Index: source/Plugins/Process/Utility/RegisterInfos_mips64.h === --- source/Plugins/Process/Utility/RegisterInfos_mips64.h +++ source/Plugins/Process/Utility/RegisterInfos_mips64.h @@ -42,11 +42,11 @@ // Note that the size and offset will be updated by platform-specific classes. #ifdef LINUX_MIPS64 -#define DEFINE_GPR(reg, alt, kind1, kind2, kind3, kind4) \ +#define DEFINE_GPR(reg, alt, kind1, kind2, kind3) \ {\ #reg, alt, sizeof(((GPR_linux_mips *) 0)->reg),\ GPR_OFFSET(reg), eEncodingUint, eFormatHex, \ - {kind1, kind2, kind3, kind4, \ + {kind1, kind2, kind3, ptrace_##reg##_mips,\ gpr_##reg##_mips64 },\ NULL, NULL, NULL, 0 \ } @@ -61,11 +61,11 @@ } #endif -#define DEFINE_GPR_INFO(reg, alt, kind1, kind2, kind3, kind4) \ +#define DEFINE_GPR_INFO(reg, alt, kind1, kind2, kind3) \ {\ #reg, alt, sizeof(((GPR_linux_mips *) 0)->reg) / 2,\ GPR_OFFSET(reg), eEncodingUint, eFormatHex, \ - {kind1, kind2, kind3, kind4, \ + {kind1, kind2, kind3, ptrace_##reg##_mips,\ gpr_##reg##_mips64 },\ NULL, NULL, NULL, 0 \ } @@ -75,21 +75,21 @@ llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shl, llvm::dwarf::DW_OP_and, llvm::dwarf::DW_OP_lit26, llvm::dwarf::DW_OP_shr}; -#define DEFINE_FPR(reg, alt, kind1, kind2, kind3, kind4) \ +#define DEFINE_FPR(reg, alt, kind1, kind2, kind3) \ {\ #reg, alt, sizeof(((FPR_linux_mips *) 0)->reg),\ FPR_OFFSET(reg), e
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. So it seems like this can be fixed by doing a few things: 1 - just set the RegisterInfo.offset to the actual offset in the buffer and don't use the offset as the ptrace key used to grab the register 2 - modify the RegisterInfo.kinds[eRegisterKindProcessPlugin] to be the offset to you to use for ptrace There should be no need what so ever to do manual bit twiddling with src and dst. https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Ok, lets leave that as-is then.. the issue seem s pretty contained for now. https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain added inline comments. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1183 @@ +1182,3 @@ + case dwarf_config5_mips64: +return reg_info->byte_offset; + case dwarf_cause_mips: Changing LLDB numbering scheme is not feasible since there are lots of changes both at host and target site. Should I add new numbering scheme ? https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
labath added inline comments. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1161 @@ +1160,3 @@ + // In case of MIPS, the PTRACE_PEEKUSER/PTRACE_POKEUSER + // take register number has an offset + // Apart from GPR registers , the offset for other registers are Ok I think I understand this now. `s/has/as/` Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1183 @@ +1182,3 @@ + case dwarf_config5_mips64: +return reg_info->byte_offset; + case dwarf_cause_mips: nitesh.jain wrote: > labath wrote: > > Why do we need to do this remapping? Couldn't we structure the register > > infos in a way that reg_info->byte_offset is exactly the offset that ptrace > > expects? > > > > Or are you saying that ptrace does not accept register offsets, but some > > random register numbers instead? (I cannot tell, as the comment above is > > confusing.) > In case of MIPS, ptrace request PTRACE_PEEKUSER/PTRACE_POKEUSER accept > register number as an offset. We used reg_info->byte_offset to find register > value in the struct GPR_linux_mips. The struct GPR_linux_mips is same for 32 > and 64 bit since ptrace always return 64 bit value irrespective of Arch (32 > and 64) . Hence we can't modify reg_info->byte_offset to match exactly the > offset that ptrace expects. Ok, I see what you mean. byte_offset is out in that case. However, I am wondering if we couldn't tweak one of the existing numbering schemes to fit that one. For example, the LLDB numbering scheme is completely arbitrary, and under our control, so what if we just reordered that a bit to match what ptrace expects? I feel that we have enough numbering schemes already, so it should be possible to find one that works without introducing a new one. This is not that bad, as the "scheme" is confined to the single file, but still you have to admit that this function looks very odd: sometimes you return a register index, sometimes a register offset and sometimes a completely made up number... Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1207 @@ +1206,3 @@ + + if ((reg_index == dwarf_sr_mips) || (strcmp(reg_info->name, "config5") == 0)) +return Read_SR_Config(offset, reg_info->name, reg_info->byte_size, value); Isn't it possible to match the "config5" register by one of the numbering schemes, instead of by name? https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain marked an inline comment as done. nitesh.jain added a comment. https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain updated this revision to Diff 71505. nitesh.jain added a comment. Updated diff as per suggestion. https://reviews.llvm.org/D24603 Files: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h === --- source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h +++ source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h @@ -76,11 +76,16 @@ static bool IsMSAAvailable(); protected: - Error DoReadRegisterValue(uint32_t offset, const char *reg_name, -uint32_t size, RegisterValue &value) override; + Error Read_SR_Config(uint32_t offset, const char *reg_name, uint32_t size, + RegisterValue &value); - Error DoWriteRegisterValue(uint32_t offset, const char *reg_name, - const RegisterValue &value) override; + uint32_t GetPtraceOffset(uint32_t reg_index, + const RegisterInfo *const reg_info); + + Error ReadRegisterRaw(uint32_t reg_index, RegisterValue &value) override; + + Error WriteRegisterRaw(uint32_t reg_index, + const RegisterValue &value) override; Error DoReadWatchPointRegisterValue(lldb::tid_t tid, void *watch_readback); Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp === --- source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp +++ source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp @@ -28,9 +28,16 @@ #include "lldb/Host/HostInfo.h" #include "lldb/lldb-enumerations.h" #include "lldb/lldb-private-enumerations.h" + #define NT_MIPS_MSA 0x600 #define CONFIG5_FRE (1 << 8) #define SR_FR (1 << 26) +#define FPR_BASE 32 +#define PC 64 +#define CAUSE 65 +#define BADVADDR 66 +#define MMHI 67 +#define MMLO 68 #define NUM_REGISTERS 32 #include @@ -466,21 +473,23 @@ } if (IsFPR(reg_index) || IsMSA(reg_index)) { -uint8_t *dst; -uint64_t *src; +uint8_t *dst = nullptr; +uint64_t *src = nullptr; +uint8_t byte_size = reg_info->byte_size; // Initialise the FP and MSA buffers by reading all co-processor 1 registers ReadCP1(); if (IsFPR(reg_index)) { assert(reg_info->byte_offset < sizeof(UserArea)); dst = (uint8_t *)&m_fpr + reg_info->byte_offset - (sizeof(m_gpr)); + byte_size = IsFR0() ? 4 : 8; } else { assert(reg_info->byte_offset < sizeof(UserArea)); dst = (uint8_t *)&m_msa + reg_info->byte_offset - (sizeof(m_gpr) + sizeof(m_fpr)); } -switch (reg_info->byte_size) { +switch (byte_size) { case 4: *(uint32_t *)dst = reg_value.GetAsUInt32(); break; @@ -611,11 +620,12 @@ Error NativeRegisterContextLinux_mips64::ReadCP1() { Error error; - uint8_t *src, *dst; + uint8_t *src = nullptr; + uint8_t *dst = nullptr; lldb::ByteOrder byte_order = GetByteOrder(); - uint32_t IsBigEndian = (byte_order == lldb::eByteOrderBig); + bool IsBigEndian = (byte_order == lldb::eByteOrderBig); if (IsMSAAvailable()) { error = NativeRegisterContextLinux::ReadRegisterSet( @@ -637,11 +647,18 @@ // TODO: Add support for FRE if (IsFR0()) { -src = (uint8_t *)&m_fpr + 4 + (IsBigEndian * 4); -dst = (uint8_t *)&m_fpr + 8 + (IsBigEndian * 4); +src = (uint8_t *)&m_fpr + (!IsBigEndian) * 4; +dst = (uint8_t *)&m_fpr + 8; for (int i = 0; i < (NUM_REGISTERS / 2); i++) { // copy odd single from top of neighbouring even double + // In case of little endian, 32 bit LSB store even FP register + // and 32 bit MSB store odd FP register + // vice-versa for big-endian *(uint32_t *)dst = *(uint32_t *)src; + + if (IsBigEndian) +// Copy 32 bit MSB to 32 bit LSB +*(uint32_t *)src = *(uint32_t *)(src + 4); src = src + 16; dst = dst + 16; } @@ -653,18 +670,23 @@ Error NativeRegisterContextLinux_mips64::WriteCP1() { Error error; - uint8_t *src, *dst; + uint8_t *src = nullptr; + uint8_t *dst = nullptr; lldb::ByteOrder byte_order = GetByteOrder(); - uint32_t IsBigEndian = (byte_order == lldb::eByteOrderBig); + bool IsBigEndian = (byte_order == lldb::eByteOrderBig); // TODO: Add support for FRE if (IsFR0()) { -src = (uint8_t *)&m_fpr + 8 + (IsBigEndian * 4); -dst = (uint8_t *)&m_fpr + 4 + (IsBigEndian * 4); +dst = (uint8_t *)&m_fpr + (!IsBigEndian) * 4; +src = dst + 8 - (!IsBigEndian) * 4; for (int i = 0; i < (NUM_REGISTERS / 2); i++) { // copy odd single to top of neighbouring even double + if (IsBigEndian) +// Copy 32 bit LSB to 32 bit MSB +*(uint32_t *)(dst + 4) = *(uint32_t *)dst; + *(uint32_t *)dst = *(uint32_t *)src; src = src +
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain added inline comments. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1183 @@ +1182,3 @@ + case dwarf_config5_mips64: +return reg_info->byte_offset; + case dwarf_cause_mips: labath wrote: > Why do we need to do this remapping? Couldn't we structure the register infos > in a way that reg_info->byte_offset is exactly the offset that ptrace expects? > > Or are you saying that ptrace does not accept register offsets, but some > random register numbers instead? (I cannot tell, as the comment above is > confusing.) In case of MIPS, ptrace request PTRACE_PEEKUSER/PTRACE_POKEUSER accept register number as an offset. We used reg_info->byte_offset to find register value in the struct GPR_linux_mips. The struct GPR_linux_mips is same for 32 and 64 bit since ptrace always return 64 bit value irrespective of Arch (32 and 64) . Hence we can't modify reg_info->byte_offset to match exactly the offset that ptrace expects. https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
labath added inline comments. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1183 @@ +1182,3 @@ + case dwarf_config5_mips64: +return reg_info->byte_offset; + case dwarf_cause_mips: Why do we need to do this remapping? Couldn't we structure the register infos in a way that reg_info->byte_offset is exactly the offset that ptrace expects? Or are you saying that ptrace does not accept register offsets, but some random register numbers instead? (I cannot tell, as the comment above is confusing.) Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp:1222 @@ +1221,3 @@ + if (reg_info->invalidate_regs) +assert(false && "In MIPS, reg_info->invalidate_regs is unhandled"); + This seems like a pretty complicated way to write `assert(!reg_info->invalidate_regs);` https://reviews.llvm.org/D24603 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D24603: [LLDB][MIPS] fix Floating point register read/write for big endian
nitesh.jain created this revision. nitesh.jain added reviewers: clayborg, labath, jaydeep. nitesh.jain added subscribers: bhushan, slthakur, lldb-commits. Herald added a subscriber: sdardis. This patch add fix for reading and writing floating point register based on SR.FR bit. https://reviews.llvm.org/D24603 Files: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h === --- source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h +++ source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.h @@ -76,11 +76,16 @@ static bool IsMSAAvailable(); protected: - Error DoReadRegisterValue(uint32_t offset, const char *reg_name, -uint32_t size, RegisterValue &value) override; + Error Read_SR_Config(uint32_t offset, const char *reg_name, uint32_t size, + RegisterValue &value); - Error DoWriteRegisterValue(uint32_t offset, const char *reg_name, - const RegisterValue &value) override; + uint32_t GetPtraceOffset(uint32_t reg_index, + const RegisterInfo *const reg_info); + + Error ReadRegisterRaw(uint32_t reg_index, RegisterValue &value) override; + + Error WriteRegisterRaw(uint32_t reg_index, + const RegisterValue &value) override; Error DoReadWatchPointRegisterValue(lldb::tid_t tid, void *watch_readback); Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp === --- source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp +++ source/Plugins/Process/Linux/NativeRegisterContextLinux_mips64.cpp @@ -28,9 +28,16 @@ #include "lldb/Host/HostInfo.h" #include "lldb/lldb-enumerations.h" #include "lldb/lldb-private-enumerations.h" + #define NT_MIPS_MSA 0x600 #define CONFIG5_FRE (1 << 8) #define SR_FR (1 << 26) +#define FPR_BASE 32 +#define PC 64 +#define CAUSE 65 +#define BADVADDR 66 +#define MMHI 67 +#define MMLO 68 #define NUM_REGISTERS 32 #include @@ -466,21 +473,23 @@ } if (IsFPR(reg_index) || IsMSA(reg_index)) { -uint8_t *dst; -uint64_t *src; +uint8_t *dst = nullptr; +uint64_t *src = nullptr; +uint8_t byte_size = reg_info->byte_size; // Initialise the FP and MSA buffers by reading all co-processor 1 registers ReadCP1(); if (IsFPR(reg_index)) { assert(reg_info->byte_offset < sizeof(UserArea)); dst = (uint8_t *)&m_fpr + reg_info->byte_offset - (sizeof(m_gpr)); + byte_size = IsFR0() ? 4 : 8; } else { assert(reg_info->byte_offset < sizeof(UserArea)); dst = (uint8_t *)&m_msa + reg_info->byte_offset - (sizeof(m_gpr) + sizeof(m_fpr)); } -switch (reg_info->byte_size) { +switch (byte_size) { case 4: *(uint32_t *)dst = reg_value.GetAsUInt32(); break; @@ -611,11 +620,12 @@ Error NativeRegisterContextLinux_mips64::ReadCP1() { Error error; - uint8_t *src, *dst; + uint8_t *src = nullptr; + uint8_t *dst = nullptr; lldb::ByteOrder byte_order = GetByteOrder(); - uint32_t IsBigEndian = (byte_order == lldb::eByteOrderBig); + bool IsBigEndian = (byte_order == lldb::eByteOrderBig); if (IsMSAAvailable()) { error = NativeRegisterContextLinux::ReadRegisterSet( @@ -637,11 +647,18 @@ // TODO: Add support for FRE if (IsFR0()) { -src = (uint8_t *)&m_fpr + 4 + (IsBigEndian * 4); -dst = (uint8_t *)&m_fpr + 8 + (IsBigEndian * 4); +src = (uint8_t *)&m_fpr + (!IsBigEndian) * 4; +dst = (uint8_t *)&m_fpr + 8; for (int i = 0; i < (NUM_REGISTERS / 2); i++) { // copy odd single from top of neighbouring even double + // In case of little endian, 32 bit LSB store even FP register + // and 32 bit MSB store odd FP register + // vice-versa for big-endian *(uint32_t *)dst = *(uint32_t *)src; + + if (IsBigEndian) +// Copy 32 bit MSB to 32 bit LSB +*(uint32_t *)src = *(uint32_t *)(src + 4); src = src + 16; dst = dst + 16; } @@ -653,18 +670,23 @@ Error NativeRegisterContextLinux_mips64::WriteCP1() { Error error; - uint8_t *src, *dst; + uint8_t *src = nullptr; + uint8_t *dst = nullptr; lldb::ByteOrder byte_order = GetByteOrder(); - uint32_t IsBigEndian = (byte_order == lldb::eByteOrderBig); + bool IsBigEndian = (byte_order == lldb::eByteOrderBig); // TODO: Add support for FRE if (IsFR0()) { -src = (uint8_t *)&m_fpr + 8 + (IsBigEndian * 4); -dst = (uint8_t *)&m_fpr + 4 + (IsBigEndian * 4); +dst = (uint8_t *)&m_fpr + (!IsBigEndian) * 4; +src = dst + 8 - (!IsBigEndian) * 4; for (int i = 0; i < (NUM_REGISTERS / 2); i++) { // copy odd single to top of neighbouring even double + if (IsB