Page MenuHomePhabricator

[LLDB] Add LoongArch register definitions and operations
ClosedPublic

Authored by seehearfeel on Mon, Nov 21, 1:33 AM.

Details

Summary

Use the same register layout as Linux kernel, implement the
related read and write operations.

Diff Detail

Event Timeline

seehearfeel created this revision.Mon, Nov 21, 1:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 21, 1:33 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
seehearfeel requested review of this revision.Mon, Nov 21, 1:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 21, 1:33 AM
SixWeining added inline comments.Tue, Nov 22, 4:47 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_loongarch64.cpp
27
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?

DavidSpickett added inline comments.Tue, Nov 22, 8:20 AM
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.

seehearfeel added inline comments.Wed, Nov 23, 1:57 AM
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,
they will be override in the following file which is not implemented now:
lldb/source/Plugins/Process/elf-core/RegisterContextPOSIXCore_loongarch64.h/cpp,
otherwise build failed:

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.

DavidSpickett added inline comments.Wed, Nov 23, 3:05 AM
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.

seehearfeel added inline comments.Wed, Nov 23, 4:19 AM
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.

DavidSpickett added inline comments.Wed, Nov 23, 4:23 AM
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

SixWeining added inline comments.Wed, Nov 23, 8:41 PM
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?

xen0n added inline comments.Wed, Nov 23, 10:38 PM
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...)

seehearfeel added inline comments.Wed, Nov 23, 11:42 PM
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
https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html#_register_convention

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

DavidSpickett accepted this revision.Thu, Nov 24, 5:06 AM

Looks good from the LLDB side.

This revision is now accepted and ready to land.Thu, Nov 24, 5:06 AM
SixWeining accepted this revision.Thu, Nov 24, 6:34 AM

My comments are all addressed. LGTM from the LoongArch side.

xen0n accepted this revision.Thu, Nov 24, 10:33 PM

Also LGTM for the LoongArch bits. Thanks!

This revision was automatically updated to reflect the committed changes.