Can someone explain hard ilogical bug on C use of #define UMAX (a, b) ((a) > (b) ? (a) : (b)) directive, that returns lower value, in 2 compilers

Question

I'm trying to identify the reason of a bug that happens on a MUD c code base. Where the use of the #define UMAX(a, b) ((a) > (b) ? (a) : (b)) , is used to return the max of two values. It sometimes returns the lower value, and even debuging I cannot discover the reason.

I isolated the relevant code for replication and ran it in two compilers, the online C compiler at https://www.tutorialspoint.com/compile_c_online.php (GCC) and using Visual studio 2019. The both have the following code spit negative values:

dur = UMAX(0, 4 - number_fuzzy(level / 10));
printf("Dur = %i\n", dur);

Basically, UMAX is being used to guard against non-negative values in a calculation. We are dividing level by 10, and then using a fuzzy/random function to randomly change it by -1/+0/+1. The we subtract this value from 4, and if the value is bellow zero, we return 0 using UMAX. But for level 50 to 59 this sometimes spits negative values, as seen bellow. But I never seen it do negative for 60-69. I understand that the fuzzy random is changing the value randomly, but the UMAX guard should always work, not just for some levels.

The full code is as follow:

//==================================================================
//= SLAP ME ON https://www.tutorialspoint.com/compile_c_online.php =
//==================================================================

#include <stdio.h>
#include<time.h> // pd 5

#define UMAX(a, b)      ((a) > (b) ? (a) : (b))
#define OLD_RAND true
static  int     rgiState[2+55];

// Replacement
//https://p2p.wrox.com/c-programming/37911-my-coin-flip-game.html
void init_mm()
{
    int *piState;
    int iState;
    piState = &rgiState[2];
    piState[-2] = 55 - 55;
    piState[-1] = 55 - 24;
    piState[0] = ((int) time(NULL))&((1 << 30)- 1);
    piState[1] = 1;
    for(iState = 2; iState < 55; iState++)
    {
        piState[iState] = (piState[iState - 1] + piState[iState - 2])
        & ((1 << 30)- 1);
    }
    return;
}




long number_mm( void )
{
#if defined (OLD_RAND)
    int *piState;
    int iState1, iState2, iRand;
    piState             = &rgiState[2];
    iState1             = piState[-2];
    iState2             = piState[-1];
    iRand               = (piState[iState1] + piState[iState2]) & ((1 << 30) - 1);
    piState[iState1]    = iRand;
    if ( ++iState1 == 55 )
        iState1 = 0;
    if ( ++iState2 == 55 )
        iState2 = 0;
    piState[-2]         = iState1;
    piState[-1]         = iState2;
    return iRand >> 6;
#else
    //return random() >> 6;
#endif
}

// db.c line 6267
int number_bits( int width )
{
    return number_mm( ) & ( ( 1 << width ) - 1 );
}

// db.c line 6195
/* Stick a little fuzz on a number. */
int number_fuzzy( int number )
{
    switch ( number_bits( 2 ) )
    {
    case 0:  number -= 1; break;
    case 3:  number += 1; break;
    }
    return UMAX( 1, number );
}

int lixo(int a){
    return UMAX( 1, a );
    
}

int main()
{
    init_mm(); // Needed cause I guess this is for seeding for random number generation


int level = 50;
int dur = 0;
dur = UMAX(0, 4 - number_fuzzy(level / 10));
printf("Dur = %i\n", dur);
dur = UMAX(0, 4 - number_fuzzy(level / 10));
printf("Dur = %i\n", dur);
dur = UMAX(0, 4 - number_fuzzy(level / 10));
printf("Dur = %i\n", dur);
dur = UMAX(0, 4 - number_fuzzy(level / 10));
printf("Dur = %i\n", dur);
dur = UMAX(0, 4 - number_fuzzy(level / 10));
printf("Dur = %i\n", dur);
dur = UMAX(0, 4 - number_fuzzy(level / 10));
printf("Dur = %i\n", dur);
dur = UMAX(0, 4 - number_fuzzy(level / 10));
printf("Dur = %i\n", dur);

int test = 0;
test = UMAX(0, 4 - lixo(level / 10));
printf("Test = %i\n", test);
}

Which can return/prints the following:

Dur = 0
Dur = 0
Dur = 0
Dur = -1
Dur = 0
Dur = -1
Dur = 0
Test = 0

Using visual studio 2019 I went down on to assembly as seen bellow:

    dur = UMAX(0, 4 - number_fuzzy(level / 10));
00971A89  mov         eax,dword ptr [level]  
00971A8C  cdq  
00971A8D  mov         ecx,0Ah  
00971A92  idiv        eax,ecx  
00971A94  push        eax  
00971A95  call        _number_fuzzy (0971181h)  
00971A9A  add         esp,4  
00971A9D  mov         edx,4  
00971AA2  sub         edx,eax  
00971AA4  jns         main+72h (0971AB2h)  
00971AA6  mov         dword ptr [ebp-100h],0  
00971AB0  jmp         main+93h (0971AD3h)  
00971AB2  mov         eax,dword ptr [level]  
00971AB5  cdq  
00971AB6  mov         ecx,0Ah  
00971ABB  idiv        eax,ecx  
00971ABD  push        eax  
00971ABE  call        _number_fuzzy (0971181h)  
00971AC3  add         esp,4  
00971AC6  mov         edx,4  
00971ACB  sub         edx,eax  
00971ACD  mov         dword ptr [ebp-100h],edx  
00971AD3  mov         eax,dword ptr [ebp-100h]  
00971AD9  mov         dword ptr [dur],eax  
    printf("Dur = %i\n", dur);
00971ADC  mov         eax,dword ptr [dur]  
00971ADF  push        eax  
00971AE0  push        offset string "Dur = %i\n" (0977B30h)  
00971AE5  call        _printf (09710D2h)  
00971AEA  add         esp,8  
    dur = UMAX(0, 4 - number_fuzzy(level / 10));

But couldn't find the reason. This is mostly likely due to my low skill in assembly.

A friend suggested that it might be something to do with unsigned integers. But I coded the Lixo function and it doesn't spit negatives. I understand that this bug is easily corrected by replacing UMAX with an IF, but I'm super interested in this bug due to a technical curiosity. Also this ancient MUD code is full of UMAX calls, so understanding what why it's happening is somewhat important.

So can someone who is more skilled in C than I explain why this behavior happens?

Typical example of what *not* to do with a macro. Thanks. – Déjà vu Feb 14 '21 at 00:36 — Déjà vu, Feb 14 '21 at 00:36

paxdiablo · Answer 1 · 2021-02-14T10:13:32.373

You have discovered one of the numerous problems with function-like macros. With their simple text substitution abilities, the code:

#define UMAX(a, b) ((a) > (b) ? (a) : (b))
dur = UMAX(0, 4 - number_fuzzy(level / 10));

will end up as:

dur = ((0) > (4 - number_fuzzy(level / 10))) ? (0) : (4 - number_fuzzy(level / 10)));

That is not going to work as you expect if number_fuzzy() can return a different value when called with the same arguments.

If you must do it with a macro^(a), you can possibly use something like the following, which only evaluates each argument once:

#define UMAX_ASSIGN(var, val1, val2) { \
    int var1 = val1; \
    int var2 = val2; \
    var = var1 > var2 ? var1 : var2; \
}

UMAX_ASSIGN(dur, 0, 4 - number_fuzzy(level / 10));

^(a) A dubious proposition. You should probably just make it a function, leaving macros to be used only for non-function-like things.

Function-like macros were very handy in the early days of C, when compilers were relatively dumb, and computers relatively slow. Their advantages are far less nowadays.

Something like this should be a good start:

int UMax(int a, int b) { // use inline suggestion if desired.
    if (a > b) return a;
    return b;
}

score 2 · Answer 2 · answered Feb 14 '21 at 00:32

2

You have this code:

#define UMAX(a, b) ((a) > (b) ? (a) : (b))
dur = UMAX(0, 4 - number_fuzzy(level / 10));

Which is equivalent to:

fuzz1 = 4 - number_fuzzy(level / 10);
fuzz2 = 4 - number_fuzzy(level / 10);
dur = 0 > fuzz1 ? 0 : fuzz2;

The problem is that fuzz1 and fuzz2 are not the same, because they are generated using randomness.

A simple fix:

fuzz1 = 4 - number_fuzzy(level / 10);
dur = UMAX(0, fuzz1);

Which of course is far more efficient, as well as correct.

answered Feb 14 '21 at 00:32

John Zwinck

239,568
38
324
436

2

In the original, the code corresponding to `fuzz2` is not always evaluated, I think. – Jonathan Leffler Feb 14 '21 at 00:54
1

@JonathanLeffler: That's true, though it only blunts the inefficiency, doesn't affect the (in)correctness. – John Zwinck Feb 14 '21 at 02:34

Can someone explain hard ilogical bug on C use of #define UMAX (a, b) ((a) > (b) ? (a) : (b)) directive, that returns lower value, in 2 compilers

2 Answers2