bugAVR C Runtime Library - Bugs: bug #48765, math.h: isfinite might generate...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #48765: math.h: isfinite might generate wrong code due to early-clobber

Submitter:  Georg-Johann Lay <gjlayde>
Submitted:  Fri 12 Aug 2016 04:59:39 PM UTC
   
 
Category:  None Severity:  4 - Important
Priority:  5 - Normal Item Group:  Header files
Status:  Confirmed Assigned to:  None
Percent Complete:  0% Open/Closed:  Open
Release:  2.0.0 Fixed Release:  None

Fri 12 Aug 2016 08:41:02 PM UTC, comment #3: 

Okay, your example proves me wrong. Yes, this is indeed a potential bug.

The proposed replacement looks good to me.

Thank you for your report.

Chris Pavlina <c4757p>
Group Member
Fri 12 Aug 2016 08:34:58 PM UTC, comment #2: 

No, that's not how GCC inline assembler works.  The only constraints on the registers are:

o input: must be in a GRP register and starts with an even register number (as it's wider than 1 byte, i.e. %C1 is even, %D1 is odd).

o output: must be in a GRP (and it's completely fine if it overlaps the input).

Suppose you want to swap bytes of a 2-byte value as follows:


int swap (int x)
{
    int y;
    __asm ("mov %A0, %B1"  "\n\t"
           "mov %B0, %A1"
           : "=r" (y) : "r" (x));
    return y;
}


$ avr-gcc -S -Os file.c

will get you:


swap:
/* #APP */
        mov r24, r25
        mov r25, r24
/* #NOAPP */
        ret


Bummer! (and GCC is right)

This is because the input dies in the asm and is no more needed, and GCC takes the asm as being one chunk of code executed at the SAME time, not as a sequence if instructions.

If there is such an early-clobber situation, i.e. bits of the input are needed after the asm started writing the outputs, you'll need special care like early-clobber '&':


int swap (int x)
{
    int y;
    __asm ("mov %A0, %B1"  "\n\t"
           "mov %B0, %A1"
           : "=&r" (y) : "r" (x));
    return y;
}


This yields correct

-verbatim-
swap:
mov r18,r24
mov r19,r25
/* #APP */
mov r24, r19
mov r25, r18
/* #NOAPP */
ret
-verbatim-

Notice that if you swapped nibbles of a 1-byte value by a single SWAP instruction, it's completely fine if input and output live in the same register, no early-clobber is needed.

GCC history tells us (and also Mr. Murphy) that if there is the potential for a bug, the compiler will take that route, be it sooner or later...

Georg-Johann Lay <gjlayde>
Fri 12 Aug 2016 07:51:57 PM UTC, comment #1: 

I'm not so sure. %C1 and %D1 are defined as bytes of __x, and %0 is __exp - surely the compiler should keep those separate as they are separate variables existing in the same scope?

I'm going to mark this Need Info unless someone else can correct me on this. I can't see the potential issue here.

The only way I was able to force a collision was to rewrite my own version of isfinite() with 'register unsigned char __exp asm("r27")' to force it to use a colliding register. Otherwise, surely it should be required to keep them separate while they are still valid in the scope?

Chris Pavlina <c4757p>
Group Member
Fri 12 Aug 2016 04:59:39 PM UTC, original submission:  

Skimming math.h I came across the following implementation of isfinie:


__ATTR_CONST__ static inline int isfinite (double __x)
{
    unsigned char __exp;
    __asm__ (
        "mov        %0, %C1                \n\t"
        "lsl        %0                \n\t"
        "mov        %0, %D1                \n\t"
        "rol        %0                "
        : "=r" (__exp)
        : "r" (__x)        );
    return __exp != 0xff;
}


The potential problem is that nothing keeps the compiler from assigning the registers in such a way that %0 = %D1, i.e. there might be an earaly-clobber situation where the first MOV instruction overwrites %D1 with %C1.

Tried around but I could not trick avr-gcc into generating wrong code, but the problem is obviously at hand.

A possible fix is to use tmp_reg in the first two instructions:


__ATTR_CONST__ static inline int isfinite (double __x)
{
    unsigned char __exp;
    __asm__ (
        "mov __tmp_reg__, %C1" "\n\t"
        "lsl __tmp_reg__"      "\n\t"
        "mov %0, %D1"          "\n\t"
        "rol %0"
        : "=r" (__exp)
        : "r" (__x)        );
    return __exp != 0xff;
}


This avoids a "=&r" early-clobber of the output operand.  It works because the outcome of the first 2 instructions is only to set carry.

Georg-Johann Lay <gjlayde>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by c4757p (Posted a comment)
  • -email is unavailable- added by gjlayde (Submitted the item)
  •  

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-12 c4757p Severity3 - Normal 4 - Important
        StatusNeed Info Confirmed
    2016-08-12 c4757p StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code