Angband.oook.cz
Angband.oook.cz
AboutDownloadVariantsLadderForumCompetitionSpoilersComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old July 18, 2012, 16:56   #1
chris
Knight
 
Join Date: Jan 2008
Posts: 561
chris is on a distinguished road
Help: Bad Code Generation?

Hi,

I'm wondering if anyone can help me with what looks to be a compiler bug. The symptom is that
Code:
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:
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("Æà¬Ê (%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:
Code:
		/* 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 is offline   Reply With Quote
Old July 18, 2012, 16:58   #2
chris
Knight
 
Join Date: Jan 2008
Posts: 561
chris is on a distinguished road
Never mind:

Code:
#define one_in_(X) \
	(X <= 0 || randint0(X) == 0)
expands to
Code:
(++num <= 0 || randint0(++num) == 0)
I hate macros ...

chris
chris is offline   Reply With Quote
Old July 18, 2012, 18:02   #3
Derakon
Prophet
 
Derakon's Avatar
 
Join Date: Dec 2009
Posts: 5,974
Derakon is on a distinguished road
You have a macro that replaces "X" with "++num"? Yikes.
Derakon is offline   Reply With Quote
Old July 18, 2012, 18:10   #4
AnonymousHero
Knight
 
AnonymousHero's Avatar
 
Join Date: Jun 2007
Posts: 589
AnonymousHero is on a distinguished road
Quote:
Originally Posted by Derakon View Post
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?
AnonymousHero is offline   Reply With Quote
Old July 18, 2012, 18:30   #5
Derakon
Prophet
 
Derakon's Avatar
 
Join Date: Dec 2009
Posts: 5,974
Derakon is on a distinguished road
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.
Derakon is offline   Reply With Quote
Old July 18, 2012, 18:31   #6
chris
Knight
 
Join Date: Jan 2008
Posts: 561
chris is on a distinguished road
Quote:
Originally Posted by Derakon View Post
You have a macro that replaces "X" with "++num"? Yikes.
Sorry, I meant that
Code:
if (!one_in_(++num)) continue
expands to
Code:
if (!((++num) <= 0 || randint0((++num)) == 0)) continue;
based on the macro definition
Code:
#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).
chris is offline   Reply With Quote
Reply


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
Is this just really bad luck? Therem Harth Vanilla 17 December 11, 2011 18:52
Using bad items offensively? Starhawk Vanilla 27 January 31, 2011 01:48
Bad pointer in vcmd_insert_repeated Sirridan Development 1 August 24, 2010 19:11
Bad memory read jbu Development 1 August 14, 2010 14:57
Selling bad weapons z118 Vanilla 10 February 14, 2010 12:02


All times are GMT +1. The time now is 07:16.


Powered by vBulletin® Version 3.8.7
Copyright ©2000 - 2014, vBulletin Solutions, Inc.