Re: [Qemu-devel] [PATCH] target/ppc: fix compilation breakage on windows

[Cédric Le Goater]

On 12/17/18 5:50 AM, David Gibson wrote:
> On Sat, Dec 15, 2018 at 07:42:22PM +0100, Cédric Le Goater wrote:
>> Fix the PPC_BIT definitions to use ULL instead in UL and replace
>> __builtin_ffssl() by the equivalent ctz routines.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>
>>  Compile tested with --cross-prefix=x86_64-w64-mingw32-. When I have
>>  some more time, I might try runtime on windows also.
> 
> Which version of the mingw compiler do you have?  As I've noted
> elsewhere, I haven't been able to reproduce the problem with the mingw
> in Fedora 29.

I have :

        mingw64-gcc-7.3.0-1.fc28.x86_64

>>  The PPC compile failures have been there for a while (pre 2.12) and
>>  the MASK_TO_LSH macro was not used until the XIVE definitions were
>>  introduced.
>>
>>  I let you guys decide on how you want to proceed, but I think it is
>>  safe to merge this patch as a prereq of the pull request.
>>
>>  Thanks,
>>
>>  C.
>>
>>  target/ppc/cpu.h | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index ab68abe8a23c..a3d3e91eb4ce 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -70,18 +70,18 @@
>>  #define PPC_ELF_MACHINE     EM_PPC
>>  #endif
>>  
>> -#define PPC_BIT(bit)            (0x8000000000000000UL >> (bit))
>> -#define PPC_BIT32(bit)          (0x80000000UL >> (bit))
>> -#define PPC_BIT8(bit)           (0x80UL >> (bit))
>> +#define PPC_BIT(bit)            (0x8000000000000000ULL >> (bit))
>> +#define PPC_BIT32(bit)          (0x80000000ULL >> (bit))
>> +#define PPC_BIT8(bit)           (0x80ULL >> (bit))
> 
> We shouldn't need the ULL for the 32-bit and (especially) 8-bit
> versions.
> 
>>  #define PPC_BITMASK(bs, be)     ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs))
>>  #define PPC_BITMASK32(bs, be)   ((PPC_BIT32(bs) - PPC_BIT32(be)) | \
>>                                   PPC_BIT32(bs))
>>  #define PPC_BITMASK8(bs, be)    ((PPC_BIT8(bs) - PPC_BIT8(be)) | 
>> PPC_BIT8(bs))
>>  
>>  #if HOST_LONG_BITS == 32
>> -# define MASK_TO_LSH(m)          (__builtin_ffsll(m) - 1)
>> +# define MASK_TO_LSH(m)          ctz32(m)
>>  #elif HOST_LONG_BITS == 64
>> -# define MASK_TO_LSH(m)          (__builtin_ffsl(m) - 1)
>> +# define MASK_TO_LSH(m)          ctz64(m)
> 
> Having another look at this, this seems bogus: we're defining an
> interface that's only correct on a *host long*.  But when we use it
> we're almost certain to be working on guest values with a fixed size,
> so using this wrapper just adds confusion.
> 
> I'm not really seeing any reason to use MASK_TO_LSH() rather than
> using ctz32() and ctz64() explicitly.

Does that mean you think we should define GETFIELD32/64 and 
SETFIELD32/64 macros ? 

or should we use the extract32/64 and deposit32/64 helpers instead ?  
That's a bigger change but may be a better one.  

Also, in the definitions above, only PPC_BIT() needs to use a ULL.


C.