Re: [DynInst_API:] [PATCH RFC] Issue a diagnostic error if x86_64 lacks LAHF/SAHF

[Bill Williams <bill@xxxxxxxxxxx>]

On 09/18/2014 04:03 PM, Josh Stone wrote:
> It was noted in Dyninst 7.0 known bugs that x86_64 CPUs which lacked
> LAHF/SAHF would not be able to run instrumentation.  This limitation
> still holds in 8.x, even though it's no longer mentioned.
>
> There was already one cpuid check for xmm capability.  It's also
> possible to check lahf_lm in cpuid 80000001H ECX.  This commit moves to
> using more generic cpuid support from compilers, and then issues a
> diagnostic error message if lahf_lm is missing.
> ---
>
> RFC: I used the __cpuid() compiler intrinsic for Windows, and cpuid.h
> __get_cpuid() elsewhere.  I know Linux GCC provides that header, but I'm
> not sure about other platforms/compilers that dyninst cares about.
>
> Opinions are also welcome on the whole concept of issuing this message.
> It doesn't actually stop anything from proceeding, but at least should
> give a clue if things blow up as expected.
Questions:

1) Is there a specific motivating system for this, or just general code 
cleanup? My understanding of the evolution of the instruction set says 
that LAHF should be missing from a vanishingly rare set of processors.
2) Despite what I thought I remembered, pushf/popf are still a 64-bit 
thing, they're just pretty pessimal instructions to use. Is there a 
reason not to fall back on generating them if we're already doing a 
CPUID check?
3) As with most CPUID based stuff, we do want to be somewhat careful 
about the heterogeneous systems case with binary rewriting. Any checks 
that we can delay until load time are going to give more accurate/useful 
results than checks at instrumentation time. This suggests to me that we 
should be looking at moving CPUID-based functionality into the RTlib; 
thoughts?
--bw

> ---
>   dyninstAPI/CMakeLists.txt  |  4 ---
>   dyninstAPI/src/cpuid-x86.S | 18 -------------
>   dyninstAPI/src/emit-x86.C  | 11 ++++++++
>   dyninstAPI/src/inst-x86.C  | 64 +++++++++++++++++++++++-----------------------
>   dyninstAPI/src/inst-x86.h  |  6 +++++
>   5 files changed, 49 insertions(+), 54 deletions(-)
>   delete mode 100644 dyninstAPI/src/cpuid-x86.S
>
> diff --git a/dyninstAPI/CMakeLists.txt b/dyninstAPI/CMakeLists.txt
> index e5e4e10ff875..6b56c9d192b4 100644
> --- a/dyninstAPI/CMakeLists.txt
> +++ b/dyninstAPI/CMakeLists.txt
> @@ -161,10 +161,6 @@ endif()
>
>   set_source_files_properties(${SRC_LIST} PROPERTIES LANGUAGE CXX)
>   add_definitions(-DBPATCH_DLL_BUILD)
> -if (PLATFORM MATCHES i386 AND UNIX)
> -set (SRC_LIST ${SRC_LIST} src/cpuid-x86.S)
> -set_source_files_properties(src/cpuid-x86.S PROPERTIES LANGUAGE C)
> -endif ()
>
>   add_library (dyninstAPI ${SRC_LIST})
>   add_library (dyninstAPI_static STATIC ${SRC_LIST})
> diff --git a/dyninstAPI/src/cpuid-x86.S b/dyninstAPI/src/cpuid-x86.S
> deleted file mode 100644
> index 912b69036193..000000000000
> --- a/dyninstAPI/src/cpuid-x86.S
> +++ /dev/null
> @@ -1,18 +0,0 @@
> -	.text
> -.globl cpuidCall
> -	.type	cpuidCall, @function
> -cpuidCall:
> -	pushl   %ebp
> -	movl    %esp, %ebp
> -	subl    $8, %esp
> -	movl    %ebx, 4(%esp)
> -	movl    $1, %eax
> -	cpuid
> -	movl	%edx, %eax
> -	movl    4(%esp), %ebx
> -	movl    %ebp, %esp
> -	popl    %ebp	
> -	ret
> -	.size	cpuidCall, .-cpuidCall
> -	.section	.note.GNU-stack,"",@progbits
> -	.ident	"GCC: (GNU) 3.3.3"
> diff --git a/dyninstAPI/src/emit-x86.C b/dyninstAPI/src/emit-x86.C
> index 00e1a3f27156..ca7fc7f19c83 100644
> --- a/dyninstAPI/src/emit-x86.C
> +++ b/dyninstAPI/src/emit-x86.C
> @@ -2328,6 +2328,17 @@ bool shouldSaveReg(registerSlot *reg, baseTramp *inst, bool saveFlags)
>
>   void EmitterAMD64::emitStackAlign(int offset, codeGen &gen)
>   {
> +   static bool checked_lahf = false;
> +   if (!checked_lahf) {
> +      /* Bit 0 of ECX from CPUID 80000001H tells whether LAHF/SAHF are
> +       * available in 64-bit mode.  (Early steppings lacked this.)  */
> +      unsigned int eax, ebx, ecx, edx;
> +      get_cpuid(0x80000001, eax, ebx, ecx, edx);
> +      if (!(ecx & 1U))
> +         bperr("This processor doesn't support LAHF/SAHF in 64-bit mode.");
> +      checked_lahf = true;
> +   }
> +
>      int off = offset + 8 + AMD64_STACK_ALIGNMENT;
>      int saveSlot1 =    0 + AMD64_STACK_ALIGNMENT;
>      int saveSlot2 =    8 + AMD64_STACK_ALIGNMENT;
> diff --git a/dyninstAPI/src/inst-x86.C b/dyninstAPI/src/inst-x86.C
> index fa25e528ce71..db84c54fb544 100644
> --- a/dyninstAPI/src/inst-x86.C
> +++ b/dyninstAPI/src/inst-x86.C
> @@ -67,11 +67,33 @@
>   #include <sstream>
>   #include <assert.h>
>
> +
> +#if !defined(os_windows)
> +#include <cpuid.h>
> +#endif
> +
> +void get_cpuid(unsigned int level,
> +               unsigned int& eax,
> +               unsigned int& ebx,
> +               unsigned int& ecx,
> +               unsigned int& edx)
> +{
> +#if defined(os_windows)
> +  int cpuInfo[4];
> +  __cpuid(cpuInfo, level);
> +  eax = cpuInfo[0];
> +  ebx = cpuInfo[1];
> +  ecx = cpuInfo[2];
> +  edx = cpuInfo[3];
> +#else
> +  __get_cpuid(level, &eax, &ebx, &ecx, &edx);
> +#endif
> +}
> +
> +
>   class ExpandInstruction;
>   class InsertNops;
>
> -extern "C" int cpuidCall();
> -
>   /****************************************************************************/
>   /****************************************************************************/
>   /****************************************************************************/
> @@ -530,43 +552,21 @@ void registerSpace::initialize()
>   #endif
>   }
>
> -/* This makes a call to the cpuid instruction, which returns an int where each bit is
> -   a feature.  Bit 24 contains whether fxsave is possible, meaning that xmm registers
> -   are saved. */
> -#if defined(os_windows)
> -int cpuidCall() {
> -    DWORD result = 0;
> -// Note: mov <target> <source>, so backwards from what gnu uses
> -	_asm {
> -		push ebx
> -		mov eax, 1
> -		cpuid
> -		pop ebx
> -		mov result, edx
> -    }
> -    return result;
> -}
> -#endif
>
> +bool xmmCapable()
> +{
>   #if !defined(x86_64_unknown_linux2_4)              \
>    && !(defined(os_freebsd) && defined(arch_x86_64)) \
>    && !defined(os_vxworks)
> -bool xmmCapable()
> -{
> -  int features = cpuidCall();
> -  char * ptr = (char *)&features;
> -  ptr += 3;
> -  if (0x1 & (*ptr))
> -    return true;
> -  else
> -    return false;
> -}
> +   /* Bit 24 of EDX from CPUID 1 tells whether fxsave is possible,
> +    * meaning that xmm registers should be saved. */
> +  unsigned int eax, ebx, ecx, edx;
> +  get_cpuid(1, eax, ebx, ecx, edx);
> +  return edx & (1U << 24);
>   #else
> -bool xmmCapable()
> -{
>     return true;
> -}
>   #endif
> +}
>
>   bool baseTramp::generateSaves(codeGen& gen, registerSpace*) {
>      return gen.codeEmitter()->emitBTSaves(this, gen);
> diff --git a/dyninstAPI/src/inst-x86.h b/dyninstAPI/src/inst-x86.h
> index 9c0ec27c46dd..ad4363d3b880 100644
> --- a/dyninstAPI/src/inst-x86.h
> +++ b/dyninstAPI/src/inst-x86.h
> @@ -148,6 +148,12 @@
>
>   class codeGen;
>
> +void get_cpuid(unsigned int level,
> +               unsigned int& eax,
> +               unsigned int& ebx,
> +               unsigned int& ecx,
> +               unsigned int& edx);
> +
>   // Define access method for saved register (GPR)
>   #define GET_GPR(x, insn) emitMovRMToReg(REGNUM_EAX, REGNUM_EBP, SAVED_EAX_OFFSET-(x*4), insn)

--
--bw

Bill Williams
Paradyn Project
bill@xxxxxxxxxxx