This is an archive of the discontinued LLVM Phabricator instance.

Use Intel CPU flags to determine target supported features.
ClosedPublic

Authored by valentinagiusti on Sep 14 2016, 6:27 AM.

Details

Summary

This patch uses the instruction CPUID to verify that FXSAVE, XSAVE, AVX
and MPX are supported by the target hardware. In case the HW supports XSAVE,
and at least one of the extended register sets, it further checks if the
target software has the kernel support for such features, by verifying that
their XSAVE part is correctly managed.

Diff Detail

Event Timeline

valentinagiusti retitled this revision from to Use Intel CPU flags to determine target supported features..
valentinagiusti updated this object.
labath edited edge metadata.Sep 14 2016, 8:19 AM

I have to admit I have very little knowledge of this part of the code. Could you provide a bit of a high-level overview of this change? Does this fix an existing problem ? (If so, should it have a test?) Or is it just a refactor?

source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
12

Please include the system header last.

187

I like the short names, but let's make this an enum class, so the names don't leak into the global namespace.

valentinagiusti marked 2 inline comments as done.Sep 14 2016, 8:28 AM

This fixes the fact that there is no proper check that the kernel or the hardware are actually supporting either AVX or MPX. Before this patch, the code only relied on a "hack" that checks if it's possible to do a ptrace to retrieve the XSAVE or FXSAVE areas: the assumption was that if XSAVE is there, then there must be also AVX and MPX, which obviously is not the correct thing to do.
The 'cpuid' calls (wrappers for the CPUID instruction) get the info directly from the hardware, and then the ptrace call is made to actually get either FXSAVE or XSAVE. If XSAVE is there, then 'cpuid' is used again to check the hardware for AVX and MPX, and then if this step is also successful, the XSAVE memory region is further checked to verify that the kernel is properly handling these features.
Basically it's both a refactoring and a fix, and it doesn't require a dedicated test: the fact that the current register tests succeed is proof enough.

valentinagiusti edited edge metadata.

moved header to the bottom and moved enum into header file

clayborg accepted this revision.Sep 14 2016, 9:17 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Sep 14 2016, 9:17 AM
This revision was automatically updated to reflect the committed changes.