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

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

[Cédric Le Goater]

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 <clg@kaod.org>
---

 Compile tested with --cross-prefix=x86_64-w64-mingw32-. When I have
 some more time, I might try runtime on windows also.

 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))
 #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)
 #else
 # error Unknown sizeof long
 #endif
-- 
2.17.2

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2755 bytes --]

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 <clg@kaod.org>
> ---
> 
>  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.

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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 <clg@kaod.org>
>> ---
>>
>>  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.

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

[Peter Maydell]

On Mon, 17 Dec 2018 at 05:01, David Gibson <david@gibson.dropbear.id.au> wrote:
>
> On Sat, Dec 15, 2018 at 07:42:22PM +0100, Cédric Le Goater wrote:

> > -#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.

This is true, but I think "UL" is worth changing, as it's generally
not what you want, being a type that might be 32 or 64 bit.
Sensible suffix choices are "U", "ULL", or no suffix IMHO.

thanks
-- PMM