Page MenuHomePhabricator

[Linux/x86] Fix writing of non-gpr registers on newer processors
ClosedPublic

Authored by labath on Mar 29 2019, 6:49 AM.

Details

Summary

We're using ptrace(PTRACE_SETREGSET, NT_X86_XSTATE) to write all non-gpt
registers on x86 linux. Unfortunately, this method has a quirk, where
the kernel rejects all attempts to write to this area if one supplies a
buffer which is smaller than the area size (even though the kernel will
happily accept partial reads from it).

This means that if the CPU supports some new registers/extensions that
we don't know about (in my case it was the PKRU extension), we will fail
to write *any* non-gpr registers, even those that we know about.

Since this is a situation that's likely to appear again and again, I add
code to NativeRegisterContextLinux_x86_64 to detect the runtime size of
the area, and allocate an appropriate buffer. This does not mean that we
will start automatically supporting all new extensions, but it does mean
that the new extensions will not prevent the old ones from working.

This fixes tests attempting to write to non-gpr registers on new intel
processors (cca Kaby Lake Refresh).

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Mar 29 2019, 6:49 AM
jankratochvil accepted this revision.Mar 29 2019, 8:43 AM

LGTM and it has fixed for me on Kaby Lake Refresh (i7-8650U):

lldb-Suite :: functionalities/register/register_command/TestRegisters.py
lldb-Suite :: tools/lldb-server/TestGdbRemoteRegisterState.py
This revision is now accepted and ready to land.Mar 29 2019, 8:43 AM
davezarzycki accepted this revision.Mar 29 2019, 9:17 AM

I've verified that this fixes my Skylake-SP (Xeon 8168) workstation. Thanks!

Can we cherry-pick this into a release branch? Skylake (and newer) CPUs are far from rare these days, especially on cloud hosting providers.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2019, 1:11 AM
labath added a comment.Apr 1 2019, 1:27 AM

Thank you for the quick review.

Can we cherry-pick this into a release branch? Skylake (and newer) CPUs are far from rare these days, especially on cloud hosting providers.

I think cherry-picking this would be a good idea (though I'd let it sit on the master branch for a bit of time first). However, I think the only release we can cherry-pick this to is 8.0.1, and that one is still a couple of months away (we've literally just wrapped 8.0). Nonetheless, I've filed a bug to do that https://bugs.llvm.org/show_bug.cgi?id=41330. If you're interested, you could also try to talk to the distro of your choice to see if they can roll out a quicker fix.

JosephTremoulet added inline comments.
lldb/trunk/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
282

This doesn't compile for me (on stock Ubuntu 16.04, so using gcc/libstdc++ 5.4.0), since my cpuid.h doesn't have __get_cpuid_count. Do I just need to update/fix my toolset, or is that a supported one that we should change this code to accommodate?

gcc-5.4 technically supported, so we can try to make things work for you. I'd be best if you can create a patch that will make things work for you (since it's kinda hard for me to test that). Or at least, can you paste the contents of your cpuid.h somewhere?

gcc-5.4 technically supported, so we can try to make things work for you. I'd be best if you can create a patch that will make things work for you (since it's kinda hard for me to test that). Or at least, can you paste the contents of your cpuid.h somewhere?

Ok. I'd be happy to put a patch together... is there an existing pattern for this sort of thing that I should follow? E.g. define a helper llvm::get_cpuid_count function somewhere, similar to STLExtras? Or just define it in this file? Looking at the implementation of __get_cpuid_count in libc++, it's pretty short, my instinct would be to just inline that logic into the callsite here... LMK if there's a preferred approach.

gcc-5.4 technically supported, so we can try to make things work for you. I'd be best if you can create a patch that will make things work for you (since it's kinda hard for me to test that). Or at least, can you paste the contents of your cpuid.h somewhere?

Ok. I'd be happy to put a patch together... is there an existing pattern for this sort of thing that I should follow? E.g. define a helper llvm::get_cpuid_count function somewhere, similar to STLExtras? Or just define it in this file? Looking at the implementation of __get_cpuid_count in libc++, it's pretty short, my instinct would be to just inline that logic into the callsite here... LMK if there's a preferred approach.

You can just put create a helper function at the top of this file. (This is assuming that we actually need to define a helper function, and that it's not possible to tweak this code slightly so that it works on all cpuid.h versions -- hard to say without looking at what your cpuid.h looks like.)