Re: 4.4.0-rc2 tagged

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/01/14 10:12, Jan Beulich wrote:
>>>> On 15.01.14 at 10:44, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>> On 15.01.14 at 01:09, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> On 14/01/2014 22:49, Don Slutz wrote:
>>>> On 01/14/14 11:29, Ian Campbell wrote:
>>>>> We've just tagged 4.4.0-rc2, please test and report bugs.
>>>>>
>>>>> The tarball can be downloaded here:
>>>>>
>>>>> http://bits.xensource.com/oss-xen/release/4.4.0-rc2/xen-4.4.0-rc2.tar.gz 
>>>>>
>>>>> Ian.
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> Xen-devel mailing list
>>>>> Xen-devel@lists.xen.org 
>>>>> http://lists.xen.org/xen-devel 
>>>> This tarball does not build on CentOS 5.10:
>>>>
>>>>
>>>> gcc -O1 -fno-omit-frame-pointer -m64 -g -fno-strict-aliasing
>>>> -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement
>>>> -I/home/don/xen-4.4.0-rc2/xen/include
>>>> -I/home/don/xen-4.4.0-rc2/xen/include/asm-x86/mach-generic
>>>> -I/home/don/xen-4.4.0-rc2/xen/include/asm-x86/mach-default
>>>> -msoft-float -fno-stack-protector -fno-exceptions -Wnested-externs
>>>> -DHAVE_GAS_VMX -mno-red-zone -mno-sse -fpic
>>>> -fno-asynchronous-unwind-tables -DGCC_HAS_VISIBILITY_ATTRIBUTE
>>>> -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith
>>>> -pipe -g -D__XEN__ -include
>>>> /home/don/xen-4.4.0-rc2/xen/include/xen/config.h -nostdinc
>>>> -iwithprefix include -fno-optimize-sibling-calls -DVERBOSE -DHAS_ACPI
>>>> -DHAS_GDBSX -DHAS_PASSTHROUGH -DHAS_PCI -DHAS_IOPORTS
>>>> -fno-omit-frame-pointer -DCONFIG_FRAME_POINTER -MMD -MF .memory.o.d -c
>>>> memory.c -o memory.o
>>>> cc1: warnings being treated as errors
>>>> memory.c: In function 'compat_memory_op':
>>>> memory.c:213: warning: comparison is always true due to limited range
>>>> of data type
>>>> memory.c:214: warning: comparison is always true due to limited range
>>>> of data type
>>>> memory.c:215: warning: comparison is always true due to limited range
>>>> of data type
>>>> make[5]: *** [memory.o] Error 1
>>>> make[5]: Leaving directory `/home/don/xen-4.4.0-rc2/xen/common/compat'
>>>> make[4]: *** [compat/built_in.o] Error 2
>>>> make[4]: Leaving directory `/home/don/xen-4.4.0-rc2/xen/common'
>>>> make[3]: *** [/home/don/xen-4.4.0-rc2/xen/common/built_in.o] Error 2
>>>> make[3]: Leaving directory `/home/don/xen-4.4.0-rc2/xen/arch/x86'
>>>> make[2]: *** [/home/don/xen-4.4.0-rc2/xen/xen] Error 2
>>>> make[2]: Leaving directory `/home/don/xen-4.4.0-rc2/xen'
>>>> make[1]: *** [install] Error 2
>>>> make[1]: Leaving directory `/home/don/xen-4.4.0-rc2/xen'
>>>> make: *** [install-xen] Error 2
>>>> dcs-xen-53:~/xen-4.4.0-rc2>uname -a
>>>> Linux dcs-xen-53 2.6.18-371.el5xen #1 SMP Tue Oct 1 09:15:30 EDT 2013
>>>> x86_64 x86_64 x86_64 GNU/Linux
>>>> dcs-xen-53:~/xen-4.4.0-rc2>cat /etc/redhat-release
>>>> CentOS release 5.10 (Final)
>>>> dcs-xen-53:~/xen-4.4.0-rc2>gcc -v
>>>> Using built-in specs.
>>>> Target: x86_64-redhat-linux
>>>> Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
>>>> --infodir=/usr/share/info --enable-shared --enable-threads=posix
>>>> --enable-checking=release --with-system-zlib --enable-__cxa_atexit
>>>> --disable-libunwind-exceptions --enable-libgcj-multifile
>>>> --enable-languages=c,c++,objc,obj-c++,java,fortran,ada
>>>> --enable-java-awt=gtk --disable-dssi --disable-plugin
>>>> --with-java-home=/usr/lib/jvm/java-1.4.2-gcj-1.4.2.0/jre
>>>> --with-cpu=generic --host=x86_64-redhat-linux
>>>> Thread model: posix
>>>> gcc version 4.1.2 20080704 (Red Hat 4.1.2-54)
>>> I have also just encountered this build error, but am currently on the
>>> fence as to whether it is a compiler bug in 4.1.2 or bad code.  Using
>>> newer compilers causes the complaint to go away.  I would certainly like
>>> to hope that compat_handle_ok() is correct, and does appear to be
>>> correct from code inspection.
>>>
>>> The if statement becomes gigantic after preprocessing, and I ran out of
>>> effort today to sanitise the preprocessed output and check it for
>>> correctness.  (At the very least, it would be kind to the compiler to
>>> factor out the paging_mode_external(current->domain) check and degrade
>>> the compat_handle_okay()s to compat_array_access_ok())
>> It's not that bad; breaking the if() up a little got me to quickly
>> see that this is due to struct xen_add_to_physmap_batch's
>> size field being uint16_t. Using a separate local variable to
>> latch the structure value makes the problem go away. The
>> warning isn't really a compiler bug, but also not very useful.
>>
>> Question now is: Should we replace the checks with
>> BUILD_BUG_ON()s (I wouldn't want to drop them altogether)
>> or suppress the warning via intermediate variable?
> And looking at the implications I'm much in favor of using
> an intermediate variable (see below).
>
> Jan
>
> compat/memory: fix build with old gcc
>
> struct xen_add_to_physmap_batch's size field being uint16_t causes old
> compiler versions to warn about the pointless range check done inside
> compat_handle_okay().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -206,18 +206,20 @@ int compat_memory_op(unsigned int cmd, X
>          {
>              unsigned int limit = (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.atpb))
>                                   / (sizeof(nat.atpb->idxs.p) + sizeof(nat.atpb->gpfns.p));
> +            /* Use an intermediate variable to suppress warnings on old gcc: */
> +            unsigned int size = cmp.atpb.size;
>              xen_ulong_t *idxs = (void *)(nat.atpb + 1);
>              xen_pfn_t *gpfns = (void *)(idxs + limit);
>  
>              if ( copy_from_guest(&cmp.atpb, compat, 1) ||
> -                 !compat_handle_okay(cmp.atpb.idxs, cmp.atpb.size) ||
> -                 !compat_handle_okay(cmp.atpb.gpfns, cmp.atpb.size) ||
> -                 !compat_handle_okay(cmp.atpb.errs, cmp.atpb.size) )
> +                 !compat_handle_okay(cmp.atpb.idxs, size) ||
> +                 !compat_handle_okay(cmp.atpb.gpfns, size) ||
> +                 !compat_handle_okay(cmp.atpb.errs, size) )
>                  return -EFAULT;
>  
>              end_extent = start_extent + limit;
> -            if ( end_extent > cmp.atpb.size )
> -                end_extent = cmp.atpb.size;
> +            if ( end_extent > size )
> +                end_extent = size;
>  
>              idxs -= start_extent;
>              gpfns -= start_extent;
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel