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

 
 

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

Submitted by:  Georg-Johann Lay <gjlayde>
Submitted on:  Fri 12 Aug 2016 04:59:39 PM UTC  
 
Category: NoneSeverity: 4 - Important
Priority: 5 - NormalItem Group: Header files
Status: ConfirmedPercent Complete: 0%
Assigned to: NoneOpen/Closed: Open
Release: 2.0.0Fixed Release: None

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

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>
Project 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:

$ avr-gcc -S -Os file.c

will get you:

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 '&':

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>
Project Member
Fri 12 Aug 2016 04:59:39 PM UTC, original submission:

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

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:

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.)

Attach File(s):
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 12 Aug 2016 08:41:02 PM UTCc4757pSeverity3 - Normal=>4 - Important
      StatusNeed Info=>Confirmed
    Fri 12 Aug 2016 07:51:57 PM UTCc4757pStatusNone=>Need Info

    Back to the top


    Powered by Savane 3.1-cleanup1