This is an archive of the discontinued LLVM Phabricator instance.

Refactor NativeRegisterContextLinux_x86_64 code.
ClosedPublic

Authored by valentinagiusti on Sep 20 2016, 7:37 AM.

Details

Summary

This patch refactors the way the XState type is checked and, in order to
simplify the code, it removes the usage of the 'cpuid' instruction: just checking
if the ptrace calls done throuhg ReadFPR is enough to verify both if there is
HW support and if there is kernel support. Also the XCR0 bits are enough to check if
there is both HW and kernel support for AVX and MPX.

Diff Detail

Repository
rL LLVM

Event Timeline

valentinagiusti retitled this revision from to Refactor NativeRegisterContextLinux_x86_64 code..
valentinagiusti updated this object.
valentinagiusti added a subscriber: lldb-commits.
zturner added inline comments.Sep 20 2016, 9:00 AM
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
818 ↗(On Diff #71930)

Is the AVX case supposed to fall-through to the mpx case?

labath requested changes to this revision.Sep 20 2016, 9:21 AM
labath edited edge metadata.

I am going to assume you know more about the exact details of the intel cpu's than me, so I am not going comment on the technical details. The code seems cleaner, so this looks like a good thing to do. Just please try to avoid using the const_cast if it's no absolutely necessary (and it almost never is).

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
807 ↗(On Diff #71930)

I don't think the const_cast here is a good idea. How about we just make this functions non-const ?

808 ↗(On Diff #71930)

I remember you #including some header to get the __get_cpuid function. Can that be removed now?

This revision now requires changes to proceed.Sep 20 2016, 9:21 AM
valentinagiusti marked an inline comment as done.Sep 20 2016, 9:40 AM

Thanks for your review! Please find my answers inline.

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
807 ↗(On Diff #71930)

IsCPUFeatureAvailable must also be called by IsRegisterSetAvailable, which is also const...

818 ↗(On Diff #71930)

good catch, it's not supposed to fall-through

valentinagiusti edited edge metadata.

Removed unnecessary header, corrected switch-case.

clayborg accepted this revision.Sep 20 2016, 10:23 AM
clayborg edited edge metadata.
labath added inline comments.Sep 20 2016, 1:26 PM
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
805 ↗(On Diff #71949)

Then I think we should make that non-const as well. I mean, what's the point of making it const if it does actually modify the state of the object. Either that, or implement it in a way that does not modify the state.

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
805 ↗(On Diff #71949)

This involves changing the interface of NativeRegisterContextLinux just because NativeRegisterContextLinux_x86_64 has some specific needs that make GetRegisterCount use a non-const function. Also, I don't have all the hardware to test such a cross-platform change.

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
805 ↗(On Diff #71949)

Anyway I guess I can submit a follow-up patch with this, I will do it today ;)

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
805 ↗(On Diff #71949)

So I have looked into this and actually in "source/Host/common/NativeRegisterContextRegisterInfo.cpp" the method GetRegisterCount() is implemented as const (and the pure virtual method is defined in "include/lldb/Host/common/NativeRegisterContext.h"). Unfortunately I don't see an easy way 1. to make the method non-const or 2. to avoid that GetRegisterSetCount() in NativeRegisterContextLinux_x86_64 needs to call a non const function: in order to get the register set count, it must know if the XState type is XSAVE, and in order to do that it must call ReadFPR(), or directly PtraceWrapper(), which are both non-const.
Could you please tell me what you think is best?
Imho, it would be better to first merge this patch and then clean up the NativeRegisterContext code, but I am open to suggestions.

So, the thing is that you already are changing the interface. The difference is that you are using the const cast to hide that fact, which is why I dont approve of it.

Also, since this is not an existing problem but rather something you are introducing in this change, I dont thinl it's appropriate to do it as a follow-up, but rather as a prep work for this change.

One possibility I would consider is to do any work you need to do before this function even executes (e.g. in the constructor - the result of IsCpuFeatureAvailable cannot change during the lifetime of the process anyway, right?). I am OOO today so I cannot really tell how feasible that ideaIis. I can look at It tomorrow.

Thechnically it's not correct that I am introducing this issue, because the old code already used a cast. It was done in the old and now not existing method "GetFPRType()", long before I introduced the MPX changes, and then I later moved it into HasXSave()/HasXSave() and now with this current refactoring patch I am moving it into IsCPUFeatureAvailable().

labath accepted this revision.Sep 21 2016, 6:37 AM
labath edited edge metadata.

I'm sorry, I did not notice that. Go ahead with this patch in that case. It looks great apart from this eexisting problem. If you're going to do further cleanups here,, I would recommend looking at the cast problem as well.

This revision is now accepted and ready to land.Sep 21 2016, 6:37 AM
This revision was automatically updated to reflect the committed changes.

ok, I will keep it in mind for some further refactoring, thanks!