[Lldb-commits] [PATCH] D66744: NativeProcessLinux: Remove some register context boilerplate

Tue Aug 27 02:31:23 PDT 2019

labath marked an inline comment as done.
labath added a comment.

In D66744#1646514 <https://reviews.llvm.org/D66744#1646514>, @jankratochvil wrote:

> > {Read,Write}[GF]PR now no longer check whether the supplied buffers are null, because they never are
>
> But then there is `NativeRegisterContextLinux_x86_64::GetFPRBuffer()`. Which is never used so maybe together with `NativeRegisterContextLinux_x86_64::GetFPRSize()` they could be just `assert(0);`. It can be also considered as a different cleanup patch.

Hm.. I didn't notice that. I'll put that in separately. Ideally, I'd say these functions should be used, but that may require more cleanups in the x86 register context, such as detecting the register type earlier on instead of just when the operations fail. Overall, there's a lot of room for improvement in this area...

> I did not test these patches but at least I am finally setting up now a (silent) buildbot for `s390x`+`ppc64le` (I haven't found a suitable ARM offer yet).

That would be awesome. Btw, we already have some arm (32&64) lldb bots set up by linaro, though they don't seem to be in a particularly good shape right now...

================
Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_arm.cpp:918

-  return ReadRegisterSet(&ioVec, buf_size, NT_PRSTATUS);
+  return ReadRegisterSet(&ioVec, GetGPRSize(), NT_PRSTATUS);
 #endif // __arm__
----------------
jankratochvil wrote:
> This keeps the existing bug in place as the size should be rather `sizeof(ioVec)`. But then `sizeof(ioVec)` is also not much useful as it is used only for `PtraceDisplayBytes`. It is probably considered as a different cleanup patch so this patch does not touch it.
> 
Yeah, I noticed that. I am keeping that for some other patch. Ideally, I'd like to refactor this even more so that one does not have to construct the iovec manually, and fix PtraceDisplayBytes to show the contents of the iovec, but I don't have a clear idea on the best way to do that.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66744/new/

https://reviews.llvm.org/D66744