Help: Bad Code Generation?

[chris]

Hi,

I'm wondering if anyone can help me with what looks to be a compiler bug. The symptom is that

lookup_kind(TV_FLASK, SV_ANY);

is returning 0 when there is a perfectly good object available (namely, flasks of oil). I'm trying to track the problem down and it only manifests in release build, and even then, intermittently. I'm using MSVC, but let's leave the Microsoft bashing aside for now.

Here is the code:

s16b lookup_kind(int tval, int sval)
{
	int k;
	int num = 0;
	int bk = 0;

	/* Look for it */
	for (k = 1; k < max_k_idx; k++)
	{
		object_kind *k_ptr = &k_info[k];

		/* Require correct tval */
		if (k_ptr->tval != tval) continue;

		msg_format("Found tval = %d, sval = %d, num = %d", k_ptr->tval, k_ptr->sval, num);

		/* Found a match */
		if (k_ptr->sval == sval) return (k);

		/* Ignore illegal items */
		if (sval != SV_ANY) continue;

		/* Apply the randomizer */
		if (!one_in_(++num)) continue;

		/* Use this value */
		msg_format("Set bk = %d", k);
		bk = k;
	}

	/* Return this choice */
	if (sval == SV_ANY)
	{
		return bk;
	}

#if 0
	/* Oops */
#ifdef JP
	msg_format("ŽŤŽ¤Ž&#198;Ž&#224;¤&#172;¤&#202;¤¤ (%d,%d)", tval, sval);
#else
	msg_format("No object (%d,%d)", tval, sval);
#endif
#endif


	/* Oops */
	return (0);
}

It looks like someone has tried to debug this very problem before given the #if 0 block.

Here is disassembly:

		/* Apply the randomizer */
		if (!one_in_(++num)) continue;
01252948  inc         ebx  
01252949  test        ebx,ebx  
0125294B  jle         lookup_kind+7Bh (125295Bh)  
0125294D  inc         ebx  
0125294E  push        ebx  
0125294F  call        Rand_div (12D8690h)

I'm frankly baffled by this. EBX contains the num variable, initialized to 0. But we increment *twice* and end up calling Rand_div(2) rather than Rand_div(1). So that one_in_(1) ends up executing as one_in_(2) which, well, sometimes is false.

Any thoughts on this one? Would you like to see more disassembly? Frankly, I suck at assembly and can't make heads or tails of the machine code generated for this routine, but it is clearly incorrect.

Thanks in advance,
chris

[chris]

Never mind:

#define one_in_(X) \
	(X <= 0 || randint0(X) == 0)

expands to

(++num <= 0 || randint0(++num) == 0)

I hate macros ...

chris

[Derakon]

You have a macro that replaces "X" with "++num"? Yikes.

[AnonymousHero]

You have a macro that replaces "X" with "++num"?

Huh? I think you may need to re-read the definition of the macro

EDIT: Or maybe I do?

[Derakon]

Ah, quite right. "++num" was the argument to the macro, so it's evaluated twice since it occurs twice in the macro. My mistake.

Macros aren't a terrible idea in principle, but they really need to have a different calling convention than normal functions, because their behavior is totally different.

[chris]

You have a macro that replaces "X" with "++num"? Yikes.

Sorry, I meant that

if (!one_in_(++num)) continue

expands to

if (!((++num) <= 0 || randint0((++num)) == 0)) continue;

based on the macro definition

#define one_in_(X) ((X) <= 0 || randint0((X)) == 0)

But if one_in_ were a function, its argument would only be evaluated once. Thus, the evil-ness of using macros ... They look too much like functions. That's why most coders code macros in all caps (e.g. ONE_IN_(++num) and then you know something is wrong).