Re: rewrote DECIMAL_DIGIT_ACCUMULATE to no longer need typeof

[Paul Eggert]

Jim Meyering <address@hidden> writes:

> this new version of the macro always fails for signed
> accumulators.  Why?

I'll be a little long-winded here, not for your benefit (you know this
stuff!) but for others who are reading along.

The problem is signed integer overflow.  The comparison "Accum * 10 +
Digit_val < Accum" has undefined behavior if overflow occurs and Accum
is signed.  This is true even if we cast each subexpression of the
left-hand side to Accum's type.  I've run into compilers that silently
return the "wrong" result for that comparison.  (Even GCC has done
that to me.)  Also, when compiling with gcc -ftrapv (or similar
options on othe compilers), the program could abort.

This can be worked around by complicating the
DECIMAL_DIGIT_ACCUMULUATE macro, but I thought it simpler merely to
check that the accumulator is unsigned, since that's all coreutils
needs.

Admittedly we aren't 100% clean on this issue elsewhere in coreutils,
but one bug at a time....


> Admittedly, that'd work for me, since I always run `make distcheck'
> which does compile with gcc -Werror, but I'd prefer a test that
> evokes a failure even without -Werror, even if it means relying on
> __typeof__.

Is this because you want a test that always provokes a failure when
the installer attempts to build the program?  I.e., you're worried
about the case where the program builds fine in your environment, but
fails in someone else's?

But in that case there's no solution that works in general.  The C
standard doesn't require that a C compiler must reject a nonconforming
program; all it requires is that a diagnostic must occur.  Even our
"verify" macro doesn't guarantee a compilation failure, since a
conforming C compiler is allowed to process a program containing "char
c[-1];" so long as it prints a diagnostic.  So the old solution won't
work in general either.

That being said, we can address the case where the installer is using
GCC, on a platform that differs sufficiently from yours that an error
would be appropriate there.  I installed this patch to catch that:

2005-07-05  Paul Eggert  <address@hidden>

        * src/system.h (DECIMAL_DIGIT_ACCUMULATE): Generate a hard error
        (not just a warning) if GCC is used and the types don't match.

--- system.h.~1.130.~   2005-07-04 23:14:06.000000000 -0700
+++ system.h    2005-07-05 15:12:42.000000000 -0700
@@ -811,13 +811,18 @@ ptr_align (void const *ptr, size_t align
    then don't update Accum and return false to indicate it would
    overflow.  Otherwise, set Accum to that new value and return true.
    Verify at compile-time that Type is Accum's type, and that Type is
-   unsigned.  Accum must be an object, so that we can take its address.
-   Accum and Digit_val may be evaluated multiple times.  */
+   unsigned.  Accum must be an object, so that we can take its
+   address.  Accum and Digit_val may =be evaluated multiple times.
+
+   The "Added check" below is not strictly required, but it causes GCC
+   to return a nonzero exit status instead of merely a warning
+   diagnostic, and that is more useful.  */
 
 #define DECIMAL_DIGIT_ACCUMULATE(Accum, Digit_val, Type)               \
   (                                                                    \
    (void) (&(Accum) == (Type *) NULL),  /* The type matches.  */       \
    verify_expr (! TYPE_SIGNED (Type)),  /* The type is unsigned.  */   \
+   verify_expr (sizeof (Accum) == sizeof (Type)),  /* Added check.  */ \
    (((Type) -1 / 10 < (Accum)                                          \
      || (Type) ((Accum) * 10 + (Digit_val)) < (Accum))                 \
     ? false : (((Accum) = (Accum) * 10 + (Digit_val)), true))          \