Shop OBEX P1 Docs P2 Docs Learn Events
Prop GCC -fwrapv implementation and rollover handling — Parallax Forums

Prop GCC -fwrapv implementation and rollover handling

pmrobertpmrobert Posts: 673
edited 2017-04-26 15:03 in Propeller 1
I have an application where I am comparing CNT to a previously calculated value. I'm handling wraparound by casting them to a signed int as shown here.
#ifdef NEWCODE
if( ((int)(trl_charge2 - CNT) < 0) && (p[1] == 0))
#else
if((CNT > trl_charge2) && (p[1] == 0))
#endif
The NEWCODE def'd line works properly, the rollover symptoms displayed by the #ELSE line screws up the pulse every 43 seconds @ 100mHz as expected. I think that overflow handling is undefined in C and GCC in particular so have an odd feeling about using an undefined "feature" such as it appears to be. However, while perusing the extensive list of GCC options I noted the existence of -fwrapv and -ftrapv. I guess I'm looking for a comment or opinion from the compiler wizards as to what would be the best option for reliable code? -fwrapv or not? As an aside, -ftrapv doesn't appear to do anything other than increase the code size considerably while -fwrapv generates slightly smaller code than no option at all. All 3 versions appear to work well with accurate, reliable pulse generation.

-Mike R...
Edit: Corrected spelling error in title.

Comments

  • ElectrodudeElectrodude Posts: 1,657
    edited 2017-04-26 16:14
    Unsigned overflow is defined in C (but signed overflow isn't), and it's equivalent to signed addition in 2's complement. If you just cast everything to an unsigned int and then do your arithmetic, I think you should be OK without using -fwrapv

    In general, don't use compiler flags like -fwrapv if you can avoid it. It just hurts optimization.
  • Heater.Heater. Posts: 21,230
    That's right. Overflow of signed integers is undefined behavior in C. It left to the platform/compiler implementers to define.

    It just so happens that all modern processors use 2's complement arithmetic so the overflow behaves as you would expect. It could go wrong when compiled for some weird machine that does not use 2's complement. But no one worries about that and I have never seen -fwrap used.


  • Heater. wrote: »
    That's right. Overflow of signed integers is undefined behavior in C. It left to the platform/compiler implementers to define.

    It just so happens that all modern processors use 2's complement arithmetic so the overflow behaves as you would expect. It could go wrong when compiled for some weird machine that does not use 2's complement. But no one worries about that and I have never seen -fwrap used.

    Actually no, overflow is not implementation defined, it's really undefined. That means the compiler/platform is free to do anything it wants in the presence of overflow. This can bite you in nasty unexpected ways. For example, consider the code:
    void foo(int a, int b) {
      if (a <= 0 || b <= 0) return; // only deal with positive numbers
      if (a+b >= 0) {
        printf("positive sum\n");
      } else {
        printf("overflow\n");
      }
    }
    
    The compiler is free to optimize the final if statement way and always print "positive sum". It already knows that a and b are positive, and so (absent overflow) a+b is positive too. Overflow behavior is undefined, so the compiler doesn't have to worry about it!

    This may seem perverse, and it is, but it's the reason that -fwrapv was introduced -- it changes undefined behavior (anything can happen on overflow) into implementation defined behavior (the overflow wraps 2's complement). I think this was done at the behest of Linus Torvalds, who was *not happy* when a GCC optimization produced exactly the kind of unexpected result we saw above in the Linux kernel.

    Bottom line: yes, -fwrapv is probably a good option to give if you think you might be encountering signed integer overflows.

    Eric
  • Excellent - thanks for the clarifications. I obviously have the wrong approach when using unsigned longs; that was what I was doing (the #else comparison listed above) and was unable to prevent the rollover hiccup. Attached is the entire code for the cog. If NEWCODE is defined, there is no rollover issue, otherwise it exists. What am I doing wrong with using just unsigned longs? Thanks to both of you, it is truly appreciated.
    void ign_sched(void *par)
    {
        uint32_t btdc_cnts;
        uint32_t next_tdc;
        uint32_t lead_ign1, lead_ign2; // lead_ign1 is for rotor 1, 2 likewise for r 2
        uint32_t trl_ign1, trl_ign2; // lead_ign1 is for rotor 1, 2 likewise for r 2
        uint32_t dwell = 2450;
        uint32_t lead_charge1, lead_charge2;
        uint32_t trl_charge1, trl_charge2;
        uint32_t pending;
        uint32_t p[8];
        uint32_t t,q;
    
    
        DIRA |= (1 << lead_ign1_p);
        DIRA |= (1 << lead_ign2_p);
        DIRA |= (1 << trl_ign1_p);
        DIRA |= (1 << trl_ign2_p);
        while(1) {
            if (newigncycle == 1) {
                newigncycle = 0;
                if(cntsperdeg > 100000) {
                    cntsperdeg = 2000;
                }            
                // ******************************
                //btdc_cnts = (cntsperdeg * 3000) / 100; // this is debugging substitution here - 30 BTDC
                btdc_cnts = (cntsperdeg * base_ign) / 100; // This is production code
                // ******************************            
                next_tdc = tdc + (cntsperdeg*360);
                lead_ign1 = (next_tdc - btdc_cnts);
                trl_ign1 = lead_ign1 + (cntsperdeg*trl_split);
                lead_ign2 = (lead_ign1 - (cntsperdeg*180));
                trl_ign2 = lead_ign2 + (cntsperdeg*trl_split);
                lead_charge1 = lead_ign1 - (dwell * 100);
                trl_charge1 = lead_charge1;
                lead_charge2 = lead_ign2 - (dwell * 100);
                trl_charge2 = lead_charge2;
                for (t = 0; t < 8; t++)
                {
                    p[t] = 0;
                }
                pending = 1;
                while(pending == 1)
                {
                    //if((CNT > lead_charge2) && (p[0] == 0))
                    #ifdef NEWCODE
                    if( ((int)(lead_charge2 - CNT) < 0) && (p[0] == 0))
                    #else
                    if((CNT > lead_charge2) && (p[0] == 0))
                    #endif
                    {
                        pinhigh(lead_ign2_p);
                        p[0] = 1;
                    }
                    //if((CNT > trl_charge2) && (p[1] == 0))
                    #ifdef NEWCODE
                    if( ((int)(trl_charge2 - CNT) < 0) && (p[1] == 0))
                    #else
                    if((CNT > trl_charge2) && (p[1] == 0))
                    #endif
                    {
                        pinhigh(trl_ign2_p);
                        p[1] = 1;
                    }
                    //if ((CNT > lead_ign2) && (p[2] == 0))
                    #ifdef NEWCODE
                    if( ((int)(lead_ign2 - CNT) < 0) && (p[2] == 0))
                    #else
                    if ((CNT > lead_ign2) && (p[2] == 0))
                    #endif
                    {
                        pinlow(lead_ign2_p);
                        p[2] = 1;
                    }
                    //if((CNT > trl_ign2) && (p[3] == 0))
                    #ifdef NEWCODE
                    if( ((int)(trl_ign2 - CNT) < 0) && (p[3] == 0))
                    #else
                    if((CNT > trl_ign2) && (p[3] == 0))
                    #endif
                    {
                        pinlow(trl_ign2_p);
                        p[3] = 1;
                    }
                    //if(( CNT > lead_charge1) && (p[4] == 0))
                    #ifdef NEWCODE
                    if( ((int)(lead_charge1 - CNT) < 0) && (p[4] == 0))
                    #else
                    if(( CNT > lead_charge1) && (p[4] == 0))
                    #endif
                    {
                        pinhigh(lead_ign1_p);
                        p[4] = 1;
                    }
                    //if ((CNT > trl_charge1) && (p[5] == 0))
                    #ifdef NEWCODE
                    if( ((int)(trl_charge1 - CNT) < 0) && (p[5] == 0))
                    #else
                    if ((CNT > trl_charge1) && (p[5] == 0))
                    #endif
                    {
                        pinhigh(trl_ign1_p);
                        p[5] = 1;
                    }
    
                    //if ((CNT > lead_ign1) && (p[6] == 0))
                    #ifdef NEWCODE
                    if( ((int)(lead_ign1 - CNT) < 0) && (p[6] == 0))
                    #else
                    if ((CNT > lead_ign1) && (p[6] == 0))
                    #endif
                    {
                        pinlow(lead_ign1_p);
                        p[6] = 1;
                    }
                    //if ((CNT > trl_ign1) && (p[7] == 0))
                    #ifdef NEWCODE
                    if( ((int)(trl_ign1 - CNT) < 0) && (p[7] == 0))
                    #else
                    if ((CNT > trl_ign1) && (p[7] == 0))
                    #endif
                    {
                        pinlow(trl_ign1_p);
                        p[7] = 1;
                    }
    
                    q = 0;
                    for (t = 0; t < 8; t++)
                    {
                        q = q + p[t];
                    }               
    
                    if (q == 8)
                    {
                        pending = 0;
                    }            
                } 
            }
        }
    }
    

    pinlow and pinhigh are defined as
    #define pinhigh(x) OUTA |= 1 << x;
    #define pinlow(x) OUTA &= ~1 << x;
    
    elsewhere. Variables not defined in this code are volatile globals which are calculated in other cogs.

    -Mike R...
  • As Electrodude said, I think your NEWCODE version is fine, it's doing the math as unsigned integers (so overflow is defined) and then casting to int to check the sign. It's a common idiom for wrapping unsigned numbers. The undefined overflow issue will only come up if the original subtraction is done on two signed ints, and even then you can use -fwrapv to avoid it.

    Eric
  • pmrobertpmrobert Posts: 673
    edited 2017-04-26 22:22
    Eric, thank you - your answer, in conjunction with the prior messages has cleared things up for me. Now I understand the hows and whys. I'm going to stick with NEWCODE version and avoid fooling around with compiler options.

    -Mike R.
  • TorTor Posts: 2,010
    ersmith wrote: »
    For example, consider the code:
    void foo(int a, int b) {
      if (a <= 0 || b <= 0) return; // only deal with positive numbers
      if (a+b >= 0) {
        printf("positive sum\n");
      } else {
        printf("overflow\n");
      }
    }
    
    The compiler is free to optimize the final if statement way and always print "positive sum". It already knows that a and b are positive, and so (absent overflow) a+b is positive too. Overflow behavior is undefined, so the compiler doesn't have to worry about it!

    And I tested it. With gcc version 6.3 and -O2 that is exactly what happens: 'positive sum' always. With -O0 it'll print the 'overflow' string if you add e.g. 2147483647 and 1 (but 'positive sum' if -O2).
    With Intel's compiler (an old one, version 11.1) it behaves differently with optimization - it will detect the overflow (Intel's compiler is generally more aggresive than GCC when optimizing, so that's actually a surprise). IBM's xlc compiler version 13.1 will also still print 'overflow' with -O2, so that one also behaves differently from gcc. (It could also be argued, based on this, that 'gcc' is the only compiler I've tested that actually needs that -fwrapv flag.. and yes, it works, I tested that as well).

Sign In or Register to comment.