Use the same register layout as Linux kernel, implement the
related read and write operations.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp | ||
---|---|---|
27 | Should be sorted lexicographically by the full path. So put it before sys/uio.h. | |
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.cpp | ||
140 | Why - 1? | |
lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h | ||
36 | unnecessary indent? | |
57 | Not allow accessing FPR registers through ABI names? |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp | ||
---|---|---|
194 | I'm not sure const cast is needed. | |
221 | Possibly not needed const cast. | |
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h | ||
33 | Some of these can be override I'm pretty sure. If you don't want to check manually I think there is some "possible override" warning in gcc at least. (and they don't need to be virtual, unless there's some derived class of this that I've missed) | |
61 | This doesn't look right, I'd expect bool ... () override; here. | |
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.cpp | ||
140 | I had the same thought. From the few others I looked at, it seems that it's count not the last index. So if you've got N sets it should return N. |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp | ||
---|---|---|
27 | OK, will modify it, thank you. | |
221 | If no const cast, build failed: /home/loongson/llvm.git/lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp:221:12: error: cannot initialize a variable of type 'uint8_t *' (aka 'unsigned char *') with an rvalue of type 'const uint8_t *' (aka 'const unsigned char *') uint8_t *src = data_sp->GetBytes(); ^ ~~~~~~~~~~~~~~~~~~~ 1 error generated. | |
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h | ||
61 | Maybe we should leave it as is, the other archs do the same thing, In file included from /home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.cpp:19: /home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:57:18: error: only virtual member functions can be marked 'override' bool ReadGPR() override; ^~~~~~~~ /home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:58:18: error: only virtual member functions can be marked 'override' bool ReadFPR() override; ^~~~~~~~ /home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:59:19: error: only virtual member functions can be marked 'override' bool WriteGPR() override; ^~~~~~~~ /home/loongson/llvm.git/lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h:60:19: error: only virtual member functions can be marked 'override' bool WriteFPR() override; ^~~~~~~~ 4 errors generated. | |
lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_loongarch64.cpp | ||
140 | You are right, will modify it, thank you. | |
lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h | ||
36 | Yes, will modify it, thank you. | |
57 | Will modify it, thank you. |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp | ||
---|---|---|
221 | Yes, for the way you've written it that makes sense. But I realise I was looking at the wrong thing here. It's fine that the GetBytes() is const because this is a WriteRegister call, that's expected. You should be fine using const uint8_t* here, because you can still increment that pointer and memcpy from it. Since the pointer itself can change but the data it points to is const. https://godbolt.org/z/9M9oqr575 might explain it better. | |
lldb/source/Plugins/Process/Utility/RegisterContextPOSIX_loongarch64.h | ||
61 | Yes I think I was looking at one of the core file versions and didn't realise. |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp | ||
---|---|---|
221 | The correct code are: In ReadAllRegisterValues() uint8_t *dst = data_sp->GetBytes(); In WriteAllRegisterValues() const uint8_t *src = data_sp->GetBytes(); Thank you. |
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp | ||
---|---|---|
221 | Yes, exactly! |
(1) Put elf.h before sys/uio.h
(2) Remove unnecessary indent
(3) Remove const cast of data_sp->GetBytes()
(4) Return k_num_register_sets in GetRegisterSetCount()
(5) Add register name alias
lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h | ||
---|---|---|
22 | I'm not sure whether you could use RegisterInfoPOSIX_loongarch64 in this file directly because I think this file is a common file. What do you think? @DavidSpickett | |
99 | u0 is a unknown alias. Could we just use DEFINE_GPR64? |
lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h | ||
---|---|---|
99 | FYI the u0 name is a non-standard alias only seen in the Linux kernel. It should be harmless to just support r21 but not u0, much like how we don't support v0/v1 any more. While at it, s9 in addition to fp may be supported too. (Arguably s9 is a better description of r22 than fp, because FP usage can be disabled while generating code, in which case it's just another ordinary callee-saved register. But it seems some people believe so deeply that this usage is acceptable that the name persisted into the final ABI document...) |
lldb/source/Plugins/Process/Utility/RegisterInfos_loongarch64.h | ||
---|---|---|
22 | Maybe it is better to define them in RegisterInfoPOSIX_loongarch64.cpp, let me modify it, thank you. | |
99 | OK, according to Register Convention in LoongArch ELF ABI specification let me remove the alias u0 for r21, use fp and s9 for r22, thank you. |
(1) define *_OFFSET in RegisterInfoPOSIX_loongarch64.cpp
(2) remove the alias u0 for r21, use fp and s9 for r22
Should be sorted lexicographically by the full path. So put it before sys/uio.h.