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

[Josh Stone <jistone@xxxxxxxxxx>]

On 09/19/2014 09:58 AM, Bill Williams wrote:
> 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.

We have a large variety of machines in our test labs, including a few
Xeons that are old enough steppings to lack LAHF/SAHF.  Our QA noticed
dyninst crashes if a test run happens to be scheduled on one of these.
The only sign of failure is death by SIGILL, so I thought a diagnostic
message would be more useful.

> 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?

Yes, pushf/popf are always available.  Since this is generated in
EmitterAMD64::emitStackAlign, you'd have to be careful about setting up
that adjusted popf, but I think it's possible.

Plus there's the heterogeneous point you make next...

> 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?

It's a valid point, but I don't know a solution.  In this particular
case, the LAHF/SAHF is put at the start of __libc_start_main, and I
can't tell from the core file if the RTlib has even been loaded yet.

Given that these checks alter dyninst's codegen, how would you delay
that until runtime?  I think you'd either have to branch on some
RTlib-provided flag, or push that functionality into an RTlib function,
which could be ifunc on Linux.  (doing stack adjustment or
fxsave/restore from a function sounds painful...)

But both checks, xmm and lahf, are rarely going to be missing in
practice anymore, so I'm not sure it's worth any performance penalty on
instrumentation.