Re: [Valgrind-developers] Re: valgrind/coregrind

[Dirk M. <dm...@gm...>]

On Monday 26 January 2004 01:16, Jeremy Fitzhardinge wrote:

> The old code was incorrect, because it just masked out a bit without
> checking the vendor ID.  That bit is only defined for AMD CPUs.

Exactly thats why the code was correct. The application that we're emulating 
has to check before calling cpuid(0x80000001) if it is in fact a AMD CPU. If 
they don't, well, they get undefined behaviour anyway. 

Besides that, the behaviour is defined: those cpus which don't support the 
extension return 0x00 in all registers. masking out a bit there doesn't hurt 
at all. 

Looking at the kernel sources, I don't see any other CPU implementing this 
cpuid feature extension. according to sandpile.org no vendor is documented to 
support it in a different way either, and in addition, this bit has no 
overloaded meaning for other vendors besides AMD.  

I agree though that there are bits which indicate different things depending 
on the vendor, but this is not the case here. But you're right, the old cpuid 
check could have been more careful to only mask out when the cpu was in fact 
a AMD one. easy fix though, no need to change the design. 

> There are two kinds of features: those which indicate a CPU instruction
> set extension, and those which indicate some other non-ISA change.  In
> the first case, we know exactly which instruction-set extensions we
> support, so those are the only ones which we should expose to the
> client.

Correct. IMHO, we should only expose those which exist at all on the original 
host cpu. So basically we have to blacklist a few extensions which we don't 
support. 

> For the latter, they're only relevent to kernels, and not 
> user-mode code, since they don't functionally change the CPU's
> characteristics for user-mode - or if they do, then we don't emulate
> that change, and so should suppress the bit.

euhm? when does invoking the cpuid instruction change the CPU behavior? Got an 
example for that?

I think its fine for an application to be able to query CPU characteristics, 
like cache sizes etc even when running under valgrind. 

IMHO as long as the characteristics doesn't affect valgrind emulation in any 
way: don't hide the information. 

> results.  I'm very uneasy about CPUID returning partially mangled
> undefined results; we should only return stuff which we *know* to be
> true, or at worst irrelevant.

I think it should return all the information the "real" CPU would do too, 
except for those bits what we *know* to be *false* on our CPU. 

> The old code was fragile and broken, because it always assumed
> 0x80000001 was returning AMD feature flags.

Ok, on which CPU does it return something else?

I'm fine with disabling the manipulation of 0x80000001 when the host CPU is 
not an AMD one (of course). Just a matter of either another cpuid 
instruction, or querying a global variable which we fill during 
initialization. 

> emulation, but in the meantime the correct thing to do is say we don't
> support it.

Thats why we disabled the 3dnow! capabilities. We didn't disable the SSE ones 
for that reason because a lot of code uses significantly different codepaths 
nowadays when the CPU supports SSE. And if those codepaths trigger a bug in 
your application, you do want to be able to find this bug using valgrind. If 
running under valgrind makes your application avoid those code paths that 
trigger the bug, then valgrind is pointless. And we don't want that, do 
we? :-)