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.