Page MenuHomePhabricator

[lldb] [Process/NetBSD] Fix crash on unsupported i386 regs
ClosedPublic

Authored by mgorny on Thu, Oct 1, 10:43 AM.

Details

Summary

Multiple fixes related to bugs discovered while debugging a crash
when reading all registers on i386.

The underlying problem was that GetSetForNativeRegNum() did not account
for MPX registers on i386, and since it only compared against upper
bounds of each known register set, the MPX registers were classified
into the wrong set and therefore considered supported. However, they
were not expected in RegNumX86ToX86_64() and caused the assertion
to fail.

This includes:

  • adding (unused) i386 → x86_64 translations for MPX registers
  • fixing GetSetForNativeRegNum() to check both lower and upper bound for register sets, to avoid wrongly classifying unhandled register sets
  • adding missing range check for MPX registers on i386
  • renaming k_last_mpxr to k_last_mpxr_i386 for consistency
  • replacing return-assertions with llvm_unreachable() and adding more checks for unexpected parameters

Diff Detail

Event Timeline

mgorny requested review of this revision.Thu, Oct 1, 10:43 AM
mgorny created this revision.

It sounds like the assertion could be useful (to detect the missing translations). Under what circumstances can an unknown register make its way to this function? How did you run into this code in the first place?

It sounds like the assertion could be useful (to detect the missing translations). Under what circumstances can an unknown register make its way to this function? How did you run into this code in the first place?

Very good question. I've ran into it because it crashed under me. But now I see that x86 variant of NativeRegisterContextNetBSD_x86_64::GetSetForNativeRegNum() doesn't handle mpx*, so they fall into wrong regsets. I suppose I should change the conditions to check both lower and upper bound of each group. Thanks!

mgorny updated this revision to Diff 295753.Fri, Oct 2, 1:25 AM
mgorny edited the summary of this revision. (Show Details)

Ok, replaced with a few more assorted fixes ;-).

labath accepted this revision.Fri, Oct 2, 1:34 AM

Yes, looks good. It might not be unreasonable to add asserts to these functions too, as I don't think we should ever have a register that doesn't belong to any register set.

lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
404–426

Could you also remove all of these else's while you're in there, as per http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return.

This revision is now accepted and ready to land.Fri, Oct 2, 1:34 AM
mgorny updated this revision to Diff 295758.Fri, Oct 2, 1:53 AM
mgorny marked an inline comment as done.
mgorny edited the summary of this revision. (Show Details)

Fixed coding style, improved description, and added more assertions for unexpected registers.

krytarowski added inline comments.Fri, Oct 2, 10:30 AM
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
394

Use llvm_unreachable ? Same in other places where we add similar assert().

410

k_first_mpxr_i386 -> k_first_mpxc_i386 ?

424

k_first_mpxr_x86_64 -> k_first_mpxc_x86_64?

mgorny marked 2 inline comments as done.Fri, Oct 2, 10:42 AM
mgorny added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
394

@labath, what is the recommended practice for LLDB? Should I just replace these asserts with llvm_unreachable(message)?

mgorny updated this revision to Diff 295867.Fri, Oct 2, 10:43 AM

Fixed wrong constant as pointed out by Kamil.

labath accepted this revision.Fri, Oct 2, 10:51 AM
labath added inline comments.
lldb/source/Plugins/Process/NetBSD/NativeRegisterContextNetBSD_x86_64.cpp
394

Yes, llvm_unreachable is better.

mgorny updated this revision to Diff 295875.Fri, Oct 2, 11:06 AM
mgorny marked 2 inline comments as done.
mgorny edited the summary of this revision. (Show Details)

Use llvm_unreachable().

krytarowski accepted this revision.Fri, Oct 2, 11:15 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSat, Oct 3, 10:55 AM