Re: Jenkins build is still unstable: FreeBSD_HEAD-tests2 #867

On Sun, Mar 22, 2015 at 3:01 PM, Craig Rodrigues <rodrigc_at_freebsd.org>
wrote:

>
>
> On Sun, Mar 22, 2015 at 2:36 PM, Dimitry Andric <dim_at_freebsd.org> wrote:
>
>> On 22 Mar 2015, at 22:32, Craig Rodrigues <rodrigc_at_FreeBSD.org> wrote:
>> >
>> > On Sun, Mar 22, 2015 at 2:29 PM, Dimitry Andric <dim_at_freebsd.org>
>> wrote:
>> >
>> > Ah right, that was on i386, on amd64 it does result in -2^63.  It is
>> indeed caused by reliance on signed integer wrapping.
>> >
>> > This diff should fix it, without rewriting the utility:
>> >
>> > Index: bin/expr/Makefile
>> > ===================================================================
>> > --- bin/expr/Makefile   (revision 280156)
>> > +++ bin/expr/Makefile   (working copy)
>> > _at__at_ -6,6 +6,9 _at__at_ PROG=   expr
>> >  SRCS=  expr.y
>> >  YFLAGS=
>> >
>> > +# expr relies on signed integer wrapping
>> > +CFLAGS+= -fwrapv
>> > +
>> >  NO_WMISSING_VARIABLE_DECLARATIONS=
>> >
>> >  .if ${MK_TESTS} != "no"
>> >
>> >
>> > Well, another alternative is to patch expr.y:
>> >
>> > Index: expr.y
>> > ===================================================================
>> > --- expr.y      (revision 280353)
>> > +++ expr.y      (working copy)
>> > _at__at_ -393,7 +393,7 _at__at_
>> >  }
>> >
>> >  void
>> > -assert_plus(intmax_t a, intmax_t b, intmax_t r)
>> > +assert_plus(intmax_t a, intmax_t b, volatile intmax_t r)
>> >  {
>> >         /*
>> >          * sum of two positive numbers must be positive,
>> > _at__at_ -420,7 +420,7 _at__at_
>> >  }
>> >
>> >  void
>> > -assert_minus(intmax_t a, intmax_t b, intmax_t r)
>> > +assert_minus(intmax_t a, intmax_t b, volatile intmax_t r)
>> >  {
>> >         /* special case subtraction of INTMAX_MIN */
>> >         if (b == INTMAX_MIN && a < 0)
>> >
>> >
>> > There were already some patches previously done to this
>> > file to add "volatile", so maybe this would be OK to do.
>> >
>> > What do you think?
>>
>> Volatile is not the solution, it is completely orthogonal.  The correct
>> way would be to use unsigned integers, for which wrapping is defined,
>> then convert those back and forth when presenting the results to the
>> user.
>>
>
> OK, converting expr.y to use unsigned integers would require a bit of work.
>
> Can you commit your patch to the Makefile?  It fixes the problem for now.
>
>
Thanks for committing the fix.  I wasn't aware of this topic, but it is
explained
quite nicely in this LLVM blog post:

http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html#signed_overflow

Do you think we should further change expr.y with something like this:

Index: expr.y
===================================================================
--- expr.y      (revision 280357)
+++ expr.y      (working copy)
_at__at_ -445,12 +445,13 _at__at_
 }

 /*
- * We depend on undefined behaviour giving a result (in r).
- * To test this result, pass it as volatile.  This prevents
- * optimizing away of the test based on the undefined behaviour.
+ * We depend on undefined signed integer overflow behaviour
+ * giving a result (in r).
+ * This file must be compiled with the "-fwrapv" compiler
+ * flag which forces defined behavior for signed integer overflow.
  */
 void
-assert_times(intmax_t a, intmax_t b, volatile intmax_t r)
+assert_times(intmax_t a, intmax_t b, intmax_t r)
 {
        /*
         * If the first operand is 0, no overflow is possible,


--
Craig