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
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
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
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
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,
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 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 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
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
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
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 ==
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
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
13 matches
Mail list logo
