| Message ID | 54C1ACCE.1000308@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On Jan 22, 2015, at 6:07 PM, Sandra Loosemore <sandra@codesourcery.com> wrote: > The ARM run-time ABI says that long long division by zero should return the result > This isn't a regression (AFAICT, this has never worked properly), so I don't know if it's appropriate to consider this for trunk at this time. So, the question is, do you think it should back port? If no, you can ask, I don’t think this should be back ported, ok for stage 1? If yes, then can ask, ok for all release branches? :-)
On 01/23/2015 10:06 AM, Mike Stump wrote: > On Jan 22, 2015, at 6:07 PM, Sandra Loosemore > <sandra@codesourcery.com> wrote: >> The ARM run-time ABI says that long long division by zero should >> return the result > >> This isn't a regression (AFAICT, this has never worked properly), >> so I don't know if it's appropriate to consider this for trunk at >> this time. > > So, the question is, do you think it should back port? If no, you > can ask, I don’t think this should be back ported, ok for stage 1? > If yes, then can ask, ok for all release branches? :-) Well, from my perspective, getting the patch into our local source tree and in some future upstream release is all that is required to keep the customer happy. I'll backport to older release branches too if the target maintainers think that is appropriate. -Sandra
On Fri, Jan 23, 2015 at 2:07 AM, Sandra Loosemore <sandra@codesourcery.com> wrote: > The ARM run-time ABI says that long long division by zero should return the > result of calling __aeabi_ldiv0 with an argument that is either zero if the > numerator is zero, the largest value of the type if the numerator is > positive, or the smallest value of the type if the numerator is negative. > We had an internal bug report from Mentor's Nucleus RTOS team that it is > incorrectly passing the negative magic value for positive numerators between > 0x80000000LL and 0xffffffffLL. > > There are three implementations of this code in libgcc -- ARM, Thumb-2, and > ARMv6-m -- and they are all structured similarly and all have the same bug. > The source of the trouble is that the divide-by-zero handling is first > checking to see if the high word is zero and substituting the low word > instead if so, before proceeding to use signed conditionals to handle the > positive/negative cases. So, it's doing the wrong thing if the high word is > zero and the high-order bit is set in the low word. The solution I've > implemented is to test the negative case on the high word first, and then > bypass the rest of the code, which can simply test zero/nonzero on whichever > word to distinguish the remaining two cases. > > I've regression-tested this on mainline head with arm-none-eabi, and also in > our local 4.9-based tree with a much larger set of multilibs to get test > coverage of all three implementations. > > This isn't a regression (AFAICT, this has never worked properly), so I don't > know if it's appropriate to consider this for trunk at this time. If not, > is it OK for when trunk re-opens for GCC 6? While we are at it I'd rather get this fixed in time for 5.0 and all release branches rather than wait for 6.0. Therefore ok. regards Ramana > > -Sandra > > > 2015-01-22 Sandra Loosemore <sandra@codesourcery.com> > > libgcc/ > * bpabi.S (test_div_by_zero): Make label names consistent > between thumb2 and arm mode cases. Separate the signed > comparison on the high word of the numerator from the > unsigned comparison on the low word. > * bpabi-v6m.S (test_div_by_zero): Similarly separate signed > comparison. > > gcc/testsuite/ > * gcc.target/arm/divzero.c: New test case. >
Index: libgcc/config/arm/bpabi.S =================================================================== --- libgcc/config/arm/bpabi.S (revision 219956) +++ libgcc/config/arm/bpabi.S (working copy) @@ -80,26 +80,29 @@ ARM_FUNC_START aeabi_ulcmp /* Tail-call to divide-by-zero handlers which may be overridden by the user, so unwinding works properly. */ #if defined(__thumb2__) - cbnz yyh, 1f - cbnz yyl, 1f + cbnz yyh, 2f + cbnz yyl, 2f cmp xxh, #0 + .ifc \signed, unsigned do_it eq cmpeq xxl, #0 - .ifc \signed, unsigned - beq 2f - mov xxh, #0xffffffff - mov xxl, xxh -2: + do_it ne, t + movne xxh, #0xffffffff + movne xxl, #0xffffffff .else - do_it lt, t + do_it lt, tt movlt xxl, #0 movlt xxh, #0x80000000 - do_it gt, t - movgt xxh, #0x7fffffff - movgt xxl, #0xffffffff + blt 1f + do_it eq + cmpeq xxl, #0 + do_it ne, t + movne xxh, #0x7fffffff + movne xxl, #0xffffffff .endif +1: b SYM (__aeabi_ldiv0) __PLT__ -1: +2: #else /* Note: Thumb-1 code calls via an ARM shim on processors which support ARM mode. */ @@ -107,16 +110,19 @@ ARM_FUNC_START aeabi_ulcmp cmpeq yyl, #0 bne 2f cmp xxh, #0 - cmpeq xxl, #0 .ifc \signed, unsigned + cmpeq xxl, #0 movne xxh, #0xffffffff movne xxl, #0xffffffff .else movlt xxh, #0x80000000 movlt xxl, #0 - movgt xxh, #0x7fffffff - movgt xxl, #0xffffffff + blt 1f + cmpeq xxl, #0 + movne xxh, #0x7fffffff + movne xxl, #0xffffffff .endif +1: b SYM (__aeabi_ldiv0) __PLT__ 2: #endif Index: libgcc/config/arm/bpabi-v6m.S =================================================================== --- libgcc/config/arm/bpabi-v6m.S (revision 219956) +++ libgcc/config/arm/bpabi-v6m.S (working copy) @@ -85,19 +85,21 @@ FUNC_START aeabi_ulcmp cmp yyl, #0 bne 7f cmp xxh, #0 + .ifc \signed, unsigned bne 2f cmp xxl, #0 2: - .ifc \signed, unsigned beq 3f mov xxh, #0 mvn xxh, xxh @ 0xffffffff mov xxl, xxh 3: .else - beq 5f blt 6f - mov xxl, #0 + bgt 4f + cmp xxl, #0 + beq 5f +4: mov xxl, #0 mvn xxl, xxl @ 0xffffffff lsr xxh, xxl, #1 @ 0x7fffffff b 5f Index: gcc/testsuite/gcc.target/arm/divzero.c =================================================================== --- gcc/testsuite/gcc.target/arm/divzero.c (revision 0) +++ gcc/testsuite/gcc.target/arm/divzero.c (revision 0) @@ -0,0 +1,85 @@ +/* { dg-require-effective-target arm_eabi } */ +/* { dg-options "" } */ +/* { dg-do run } */ + +/* Check that long long divmod functions pass the right argument to + __aeabi_ldiv0 on divide by zero. */ + +#ifdef DEBUGME +#include <stdio.h> +#else +extern void abort (void); +#endif + +/* Override div zero handler and simply return the provided value. */ +long long __aeabi_ldiv0 (long long r) +{ + return r; +} + +long long lldiv (long long a, long long b) +{ + return a / b; +} + +unsigned long long ulldiv (unsigned long long a, unsigned long long b) +{ + return a / b; +} + +void check (long long num, long long expected) +{ + long long res = lldiv (num, 0LL); + if (res != expected) +#ifdef DEBUGME + { + printf ("num=%08X:%08X\n", (unsigned)(num >> 32), (unsigned)num); + printf ("res=%08X:%08X\n", (unsigned)(res >> 32), (unsigned)res); + } +#else + abort (); +#endif +} + +void ucheck (unsigned long long num, unsigned long long expected) +{ + unsigned long long res = ulldiv (num, 0ULL); + if (res != expected) +#ifdef DEBUGME + { + printf ("num=%08X:%08X\n", (unsigned)(num >> 32), (unsigned)num); + printf ("res=%08X:%08X\n", (unsigned)(res >> 32), (unsigned)res); + } +#else + abort (); +#endif +} + +#define POS_BIG 0x7fffffffffffffffLL +#define NEG_BIG 0x8000000000000000LL +#define UNS_BIG 0xffffffffffffffffULL + +int main () +{ + check (0LL, 0LL); + check (1LL, POS_BIG); + check (0x000000007fffffffLL, POS_BIG); + check (0x00000000ffffffffLL, POS_BIG); + check (0x0000000100000000LL, POS_BIG); + check (POS_BIG, POS_BIG); + check (-1LL, NEG_BIG); + check (-0x000000007fffffffLL, NEG_BIG); + check (-0x00000000ffffffffLL, NEG_BIG); + check (-0x0000000100000000LL, NEG_BIG); + check (NEG_BIG, NEG_BIG); + + ucheck (0ULL, 0ULL); + ucheck (1ULL, UNS_BIG); + ucheck (0x000000007fffffffULL, UNS_BIG); + ucheck (0x00000000ffffffffULL, UNS_BIG); + ucheck (0x0000000100000000ULL, UNS_BIG); + ucheck ((unsigned long long)POS_BIG, UNS_BIG); + ucheck (UNS_BIG, UNS_BIG); + + return 0; +}