Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
valentinagiusti added a comment. ok, I will keep it in mind for some further refactoring, thanks! Repository: rL LLVM https://reviews.llvm.org/D24764 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL282072: Refactor NativeRegisterContextLinux_x86_64 code.
(authored by valentinagiusti).
Changed prior to commit:
https://reviews.llvm.org/D24764?vs=71949=72036#toc
Repository:
rL LLVM
https://reviews.llvm.org/D24764
Files:
lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
Index: lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
===
--- lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
+++ lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
@@ -117,18 +117,12 @@
uint32_t m_fctrl_offset_in_userarea;
// Private member methods.
- bool HasFXSAVE() const;
-
- bool HasXSAVE() const;
-
bool IsCPUFeatureAvailable(RegSet feature_code) const;
bool IsRegisterSetAvailable(uint32_t set_index) const;
bool IsGPR(uint32_t reg_index) const;
- XStateType GetXStateType() const;
-
bool IsFPR(uint32_t reg_index) const;
bool CopyXSTATEtoYMM(uint32_t reg_index, lldb::ByteOrder byte_order);
Index: lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
===
--- lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
+++ lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
@@ -20,8 +20,6 @@
#include "Plugins/Process/Utility/RegisterContextLinux_i386.h"
#include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
-#include
-
using namespace lldb_private;
using namespace lldb_private::process_linux;
@@ -664,9 +662,9 @@
::memcpy(dst, _gpr_x86_64, GetRegisterInfoInterface().GetGPRSize());
dst += GetRegisterInfoInterface().GetGPRSize();
- if (GetXStateType() == XStateType::FXSAVE)
+ if (m_xstate_type == XStateType::FXSAVE)
::memcpy(dst, _fpr.xstate.fxsave, sizeof(m_fpr.xstate.fxsave));
- else if (GetXStateType() == XStateType::XSAVE) {
+ else if (m_xstate_type == XStateType::XSAVE) {
lldb::ByteOrder byte_order = GetByteOrder();
if (IsCPUFeatureAvailable(RegSet::avx)) {
@@ -756,16 +754,16 @@
return error;
src += GetRegisterInfoInterface().GetGPRSize();
- if (GetXStateType() == XStateType::FXSAVE)
+ if (m_xstate_type == XStateType::FXSAVE)
::memcpy(_fpr.xstate.fxsave, src, sizeof(m_fpr.xstate.fxsave));
- else if (GetXStateType() == XStateType::XSAVE)
+ else if (m_xstate_type == XStateType::XSAVE)
::memcpy(_fpr.xstate.xsave, src, sizeof(m_fpr.xstate.xsave));
error = WriteFPR();
if (error.Fail())
return error;
- if (GetXStateType() == XStateType::XSAVE) {
+ if (m_xstate_type == XStateType::XSAVE) {
lldb::ByteOrder byte_order = GetByteOrder();
if (IsCPUFeatureAvailable(RegSet::avx)) {
@@ -801,58 +799,28 @@
return error;
}
-bool NativeRegisterContextLinux_x86_64::HasFXSAVE() const {
- unsigned int rax, rbx, rcx, rdx;
-
- // Check if FXSAVE is enabled.
- if (!__get_cpuid(1, , , , ))
-return false;
- if ((rdx & bit_FXSAVE) == bit_FXSAVE) {
-m_xstate_type = XStateType::FXSAVE;
-if (const_cast(this)->ReadFPR().Fail())
- return false;
-return true;
- }
- return false;
-}
-
-bool NativeRegisterContextLinux_x86_64::HasXSAVE() const {
- unsigned int rax, rbx, rcx, rdx;
-
- // Check if XSAVE is enabled.
- if (!__get_cpuid(1, , , , ))
-return false;
- if ((rcx & bit_OSXSAVE) == bit_OSXSAVE) {
-m_xstate_type = XStateType::XSAVE;
+bool NativeRegisterContextLinux_x86_64::IsCPUFeatureAvailable(
+RegSet feature_code) const {
+ if (m_xstate_type == XStateType::Invalid) {
if (const_cast(this)->ReadFPR().Fail())
return false;
-return true;
}
- return false;
-}
-
-bool NativeRegisterContextLinux_x86_64::IsCPUFeatureAvailable(
-RegSet feature_code) const {
- unsigned int rax, rbx, rcx, rdx;
-
- // Check if XSAVE is enabled.
- if (!HasXSAVE())
-return false;
-
- __get_cpuid(1, , , , );
switch (feature_code) {
- case RegSet::avx: // Check if CPU has AVX and if there is kernel support, by reading in the XCR0 area of XSAVE.
-if (((rcx & bit_AVX) != 0) && ((m_fpr.xstate.xsave.i387.xcr0 & mask_XSTATE_AVX) == mask_XSTATE_AVX))
+ case RegSet::gpr:
+ case RegSet::fpu:
+return true;
+ case RegSet::avx: // Check if CPU has AVX and if there is kernel support, by
+// reading in the XCR0 area of XSAVE.
+if ((m_fpr.xstate.xsave.i387.xcr0 & mask_XSTATE_AVX) == mask_XSTATE_AVX)
return true;
- case RegSet::mpx: // Check if CPU has MPX and if there is kernel support, by reading in the XCR0 area of XSAVE.
-if (__get_cpuid_max(0, NULL) > 7) {
- __cpuid_count(7, 0, rax, rbx, rcx, rdx);
- if (((rbx & bit_MPX) != 0) &&
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. 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. https://reviews.llvm.org/D24764 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
valentinagiusti added a comment. 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(). https://reviews.llvm.org/D24764 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
labath added a comment. 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. https://reviews.llvm.org/D24764 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
valentinagiusti added inline comments.
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:805
@@ -827,2 +804,3 @@
+ if (m_xstate_type == XStateType::Invalid) {
if (const_cast(this)->ReadFPR().Fail())
return false;
valentinagiusti wrote:
> valentinagiusti wrote:
> > labath wrote:
> > > 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.
> > 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.
> Anyway I guess I can submit a follow-up patch with this, I will do it today ;)
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.
https://reviews.llvm.org/D24764
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
valentinagiusti added inline comments.
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:805
@@ -827,2 +804,3 @@
+ if (m_xstate_type == XStateType::Invalid) {
if (const_cast(this)->ReadFPR().Fail())
return false;
valentinagiusti wrote:
> labath wrote:
> > 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.
> 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.
Anyway I guess I can submit a follow-up patch with this, I will do it today ;)
https://reviews.llvm.org/D24764
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
valentinagiusti added inline comments.
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:805
@@ -827,2 +804,3 @@
+ if (m_xstate_type == XStateType::Invalid) {
if (const_cast(this)->ReadFPR().Fail())
return false;
labath wrote:
> 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.
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.
https://reviews.llvm.org/D24764
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
labath added inline comments.
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:805
@@ -827,2 +804,3 @@
+ if (m_xstate_type == XStateType::Invalid) {
if (const_cast(this)->ReadFPR().Fail())
return false;
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.
https://reviews.llvm.org/D24764
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
valentinagiusti updated this revision to Diff 71949.
valentinagiusti added a comment.
Removed unnecessary header, corrected switch-case.
https://reviews.llvm.org/D24764
Files:
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
===
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.h
@@ -117,18 +117,12 @@
uint32_t m_fctrl_offset_in_userarea;
// Private member methods.
- bool HasFXSAVE() const;
-
- bool HasXSAVE() const;
-
bool IsCPUFeatureAvailable(RegSet feature_code) const;
bool IsRegisterSetAvailable(uint32_t set_index) const;
bool IsGPR(uint32_t reg_index) const;
- XStateType GetXStateType() const;
-
bool IsFPR(uint32_t reg_index) const;
bool CopyXSTATEtoYMM(uint32_t reg_index, lldb::ByteOrder byte_order);
Index: source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
===
--- source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
+++ source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
@@ -20,8 +20,6 @@
#include "Plugins/Process/Utility/RegisterContextLinux_i386.h"
#include "Plugins/Process/Utility/RegisterContextLinux_x86_64.h"
-#include
-
using namespace lldb_private;
using namespace lldb_private::process_linux;
@@ -664,9 +662,9 @@
::memcpy(dst, _gpr_x86_64, GetRegisterInfoInterface().GetGPRSize());
dst += GetRegisterInfoInterface().GetGPRSize();
- if (GetXStateType() == XStateType::FXSAVE)
+ if (m_xstate_type == XStateType::FXSAVE)
::memcpy(dst, _fpr.xstate.fxsave, sizeof(m_fpr.xstate.fxsave));
- else if (GetXStateType() == XStateType::XSAVE) {
+ else if (m_xstate_type == XStateType::XSAVE) {
lldb::ByteOrder byte_order = GetByteOrder();
if (IsCPUFeatureAvailable(RegSet::avx)) {
@@ -756,16 +754,16 @@
return error;
src += GetRegisterInfoInterface().GetGPRSize();
- if (GetXStateType() == XStateType::FXSAVE)
+ if (m_xstate_type == XStateType::FXSAVE)
::memcpy(_fpr.xstate.fxsave, src, sizeof(m_fpr.xstate.fxsave));
- else if (GetXStateType() == XStateType::XSAVE)
+ else if (m_xstate_type == XStateType::XSAVE)
::memcpy(_fpr.xstate.xsave, src, sizeof(m_fpr.xstate.xsave));
error = WriteFPR();
if (error.Fail())
return error;
- if (GetXStateType() == XStateType::XSAVE) {
+ if (m_xstate_type == XStateType::XSAVE) {
lldb::ByteOrder byte_order = GetByteOrder();
if (IsCPUFeatureAvailable(RegSet::avx)) {
@@ -801,58 +799,28 @@
return error;
}
-bool NativeRegisterContextLinux_x86_64::HasFXSAVE() const {
- unsigned int rax, rbx, rcx, rdx;
-
- // Check if FXSAVE is enabled.
- if (!__get_cpuid(1, , , , ))
-return false;
- if ((rdx & bit_FXSAVE) == bit_FXSAVE) {
-m_xstate_type = XStateType::FXSAVE;
-if (const_cast(this)->ReadFPR().Fail())
- return false;
-return true;
- }
- return false;
-}
-
-bool NativeRegisterContextLinux_x86_64::HasXSAVE() const {
- unsigned int rax, rbx, rcx, rdx;
-
- // Check if XSAVE is enabled.
- if (!__get_cpuid(1, , , , ))
-return false;
- if ((rcx & bit_OSXSAVE) == bit_OSXSAVE) {
-m_xstate_type = XStateType::XSAVE;
+bool NativeRegisterContextLinux_x86_64::IsCPUFeatureAvailable(
+RegSet feature_code) const {
+ if (m_xstate_type == XStateType::Invalid) {
if (const_cast(this)->ReadFPR().Fail())
return false;
-return true;
}
- return false;
-}
-
-bool NativeRegisterContextLinux_x86_64::IsCPUFeatureAvailable(
-RegSet feature_code) const {
- unsigned int rax, rbx, rcx, rdx;
-
- // Check if XSAVE is enabled.
- if (!HasXSAVE())
-return false;
-
- __get_cpuid(1, , , , );
switch (feature_code) {
- case RegSet::avx: // Check if CPU has AVX and if there is kernel support, by reading in the XCR0 area of XSAVE.
-if (((rcx & bit_AVX) != 0) && ((m_fpr.xstate.xsave.i387.xcr0 & mask_XSTATE_AVX) == mask_XSTATE_AVX))
+ case RegSet::gpr:
+ case RegSet::fpu:
+return true;
+ case RegSet::avx: // Check if CPU has AVX and if there is kernel support, by
+// reading in the XCR0 area of XSAVE.
+if ((m_fpr.xstate.xsave.i387.xcr0 & mask_XSTATE_AVX) == mask_XSTATE_AVX)
return true;
- case RegSet::mpx: // Check if CPU has MPX and if there is kernel support, by reading in the XCR0 area of XSAVE.
-if (__get_cpuid_max(0, NULL) > 7) {
- __cpuid_count(7, 0, rax, rbx, rcx, rdx);
- if (((rbx & bit_MPX) != 0) && ((m_fpr.xstate.xsave.i387.xcr0 & mask_XSTATE_MPX) == mask_XSTATE_MPX))
-return true;
-}
- default:
-return false;
+ break;
+ case RegSet::mpx: // Check if CPU has MPX and if there is kernel support, by
+// reading in
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
valentinagiusti marked an inline comment as done.
valentinagiusti added a comment.
Thanks for your review! Please find my answers inline.
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:807
@@ -827,2 +806,3 @@
+ if (m_xstate_type == XStateType::Invalid) {
if (const_cast(this)->ReadFPR().Fail())
return false;
labath wrote:
> I don't think the `const_cast` here is a good idea. How about we just make
> this functions non-const ?
IsCPUFeatureAvailable must also be called by IsRegisterSetAvailable, which is
also const...
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:818
@@ +817,3 @@
+ return true;
+ case RegSet::mpx: // Check if CPU has MPX and if there is kernel support, by
+// reading in the XCR0 area of XSAVE.
zturner wrote:
> Is the AVX case supposed to fall-through to the mpx case?
good catch, it's not supposed to fall-through
https://reviews.llvm.org/D24764
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
labath requested changes to this revision.
labath added a comment.
This revision now requires changes to proceed.
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).
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:807
@@ -827,2 +806,3 @@
+ if (m_xstate_type == XStateType::Invalid) {
if (const_cast(this)->ReadFPR().Fail())
return false;
I don't think the `const_cast` here is a good idea. How about we just make this
functions non-const ?
Comment at:
source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:808
@@ -807,3 @@
- // Check if FXSAVE is enabled.
- if (!__get_cpuid(1, , , , ))
-return false;
I remember you #including some header to get the __get_cpuid function. Can that
be removed now?
https://reviews.llvm.org/D24764
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D24764: Refactor NativeRegisterContextLinux_x86_64 code.
zturner added inline comments. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:818 @@ +817,3 @@ + return true; + case RegSet::mpx: // Check if CPU has MPX and if there is kernel support, by +// reading in the XCR0 area of XSAVE. Is the AVX case supposed to fall-through to the mpx case? https://reviews.llvm.org/D24764 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
