Dev tracking for pete

[Pete Mack]

Yep, I misread Tak's post. Had to revert that part. my_strncpy is a bad name for it. Why not just strlcpy and strcasecmp, with conditional definitions?

In any case, Nick had an item to "make it ansi C99". What does that mean, then, if it's not just getting rid of BOOL, TRUE, FALSE?
Also, should get rid of that silly warning about only using declarations at the beginning of code blocks. That is sooo old fashioned.

strncat does something sensible and strlcat does something sensible but those sensible things are, as takkaria said, not the same thing, even aside from the return value. The meaning of the size_t argument is quite different between the two functions. Maybe this is the cause of your infinite loop...

[Nick]

Finally, I discovered that the infinite loop bug only exists in the optimized build OR it is intermittent. I'm not sure yet. :/

I'm pretty sure I've had difficulties with optimisation, but can't recall where. Certainly we have -O0 set for the linux makefile.

[takkaria]

This post was originally made before I read further down the thread. If you're interested more in the rationale behind strlcpy/strlcat, here's the original paper. I guess it would make sense to just use strlcpy and strcat as the names.

And thank god MSVC finally supports mixed declarations and code. Shame it took them 14 years...

[Pete Mack]

Yuck. Better solution is to actually find the bugs. gcc supports -g -O2, so it is possible to debug optimized code pretty easily, even if line numbers get broken a little bit. I will see what I can do now that I have figured out how to interrupt running programs in gdb.

As a side note, can anyone recommend a decent gdb GUI for windows? I remember trying one in the distant past, but it didn't work as well at all.

I'm pretty sure I've had difficulties with optimisation, but can't recall where. Certainly we have -O0 set for the linux makefile.

[AnonymousHero]

Finding memory corruption without purify is just massively annoying. That is just an awesome product. If anyone has a site license at work, it'd be a huge public service to do a quick run. We don't have it anymore because all our code is managed.

Have you tried -fsanitize?

EDIT: I think you'll be surprised at how effective it is at instantly finding loads of bugs. Granted you need to actually hit the case in question... but this means that you're also properly motivated to fix the bug since you have proof positive that it can actually happen. (As opposed to fixing bugs pointed out by linters and the like -- you may think that the bug is entirely theoretical and/or exceedingly unlikely and thus not be motivated to fix it... and just disable the warning.)

[Pete Mack]

Thanks for the pointer AnonymousHero. I forgot all about that, since (a) I haven't tried C compiling in forever and (b) it didn't used to work well on windows. I was thinking you were talking about the really horrible memory error detection I tried in the past. It just detected out of bounds errors by making lots of free space and filling it in with 0x0BADBEEF or whatever, then detecting where it was changed at the end of execution. It was really awful. You are right clang looks perfect. No wonder IBM sold off Rational.

EDIT: none of them work on windows, drats.
I would have to make a Linux 64-bit partition.

[AnonymousHero]

EDIT: none of them work on windows, drats.
I would have to make a Linux 64-bit partition.

Aww, damn. Wasn't aware of that. Sorry about that

They really are incredibly effective yet people don't tend to believe you when you tell them... (You're not the first to be very skeptical, believe me )

[Pete Mack]

AH: I wasn't skeptical, I was stupid! I didn't investigate to realize this was different from the old days. Purify was the first good instrumentation package, but that doesn't mean it was only. With a good free competitor, it's no wonder that IBM sold off Rational products.
To others: memory instrumentation is the ONLY way to verify your software really works. And it is easy! Building -O0 is really not the right option.

Further examination shows that yes, it does work on windows, and it integrates with MSVS, but to use it with MinGW/cygwin, you need do your own build *and add your local mingw lib/include paths to a source file.* This is beyond my current ambitions.

[Pete Mack]

OK, I'm looking for the infinite loop in the optimized build (-g -O2 flags.) All the values look good-- there's a good map laid out, f_info values are OK, and ddy_ddd and ddx_ddd are initialized correctly. But in the loop checking neighbors, (xx, yy) == (x, y) for all values of d. (lines 1490-1495). It looks like a compiler bug.

I forced the loop to run properly by changing the loop condition to (ddy_ddd[d] != 0 || ddx_ddd[d] != 0). The lava code ran properly, but the LOS code fails hopelessly when the screen is displayed: the floors and shop doors show up, but the walls and lava are all dark.

Yikes. I will update the windows makefile to reflect this.

Further.... There is an easier way to do this. First make a cave of solid granite and make lava streamers; THEN make a starburst room and change the granite to permarock.
There is already code to make streamers in ordinary rock.

Also, I will install an updated MinGW. Mine is a couple years old. Maybe it was a bad version.

[AnonymousHero]

It looks like a compiler bug.

I'm not trying to be an ass, but... no. Whenever you think "oh, a compiler bug!" you're probably barking up the wrong tree unless you have really solid evidence. Don't get me wrong, compiler people do make mistakes, but not nearly frequently enough for "oh, a compiler bug!" to be a reasonable response to any strange phenomenon you're seeing . Usually it's just plain ol' UB or IDB rearing its head.

[Pete Mack]

I get that compiler bugs are exceeding rare. I've only found one ever before in gcc, and it was in -O1, which no one ever uses. It is likely that my installation is damaged.
I spent 3 hours verifying that the execution was not doing what the code said, and that the various necessary values were not corrupt.
In this case, the code was expected to step through the ddx_ddd and ddy_ddd tables, and get xx and yy values in cardinal directions from x and y. The values were instead equal to x and y.
The version of MinGW I had is sufficiently non-functional that the MinGW installer crashes when attempting to upgrade the gcc compiler on Windows 10. I am not going to worry about the cause any further.

[Pete Mack]

64-bit windows build, pure cygwin, graphics work!

Here are my work notes.

BUILD NOTES: CYGWIN 64-bit BUILD GCC 8.9.1
Windows woes. I am unable to install mingw on windows 8.1. (MinGW crashes immediately when executing update. 
Has not been updated in 3 years. No idea if this is a problem with my computer or MinGW. Don't care.) 
Install the latest version of Cygwin64. (Should have done Cygwin32. Lets see what happens...)

Obvious changes:
-mno-cygwin is no longer a valid flag. Compile fails.
Remove flag.
Now I get
* SILLY WARNING:
gen-room.c:2088:4: warning: array subscript has type ‘char’ [-Wchar-subscripts]
    if (isalpha(*t) && (*t != 'x') && (*t != 'X')) {
    ^
It even warns on isupper macro/inline. (Duh, of course it's a character.)
z-dice.c:112:2: warning: array subscript has type ‘char’ [-Wchar-subscripts]
  if (isupper(c))

* FILE direct.h does not list. 
z-file.c:30:21: fatal error: direct.h: No such file or directory
 # include <direct.h>
                     ^
Fix: use dirent.h, get rid of my_mkdir #define.

* libpng.lib and libz.lib no longer link:
/usr/lib/gcc/x86_64-pc-cygwin/4.9.3/../../../../x86_64-pc-cygwin/bin/ld: skipping incompatible win/lib/zlib.lib when searching for -lzlib

It's a 64 bit build. No kidding.  Try -m32. No 32 bit libraries installed by default. OK....

Look into PNG native support for windows. Requires MSVCRT C++ mangled names. Too much work right now.

Download and build latest libz libng versions.

Temporarily fix Makefile.win to compile against .a libraries, since I can't figure out how to make them work as dlls.
Remove reference to mainCRTStartup from LDFLAGS, since it's not present in cygwin. MainWin is standard entry point. Success!

* Build and execute.
Fonts totally wrecked. Changing fonts crashes. Able to select character from saves directory; 
instead create new character, though can't read any text. Crash after character creation on divide-by-zero 
(tile size set to zero see below gdb.)

Open in gdb.
In WinMain, program is unable to load fonts despite correct path. Eventually tile width and height become zero. 
Try generating c preprocessor file for win-main to see what PASCAL FAR gets turned into, to verify that 
Windows dllexport funcs are executing correctly.

Preprocessor output looks good.

* D'oh! gcc is complaining about argument sizes (pointer cast to long). Add some #ifdef _WIN64 stuff to the main-win.c
Works fine!

Rerun ANSI-fy shell script to be able to use size_t; remove #ifdef.

Woah! Floor tiles are rad! Revert to '.' for less distracting LOS color change.

Verify working GFX.

[Pete Mack]

One last check:
Try running from Windows, rather than Cygwin environment. FAIL. Linked against dynamic Cygwin dlls.

[Pete Mack]

Few more observations

1. Plasma hounds colors all have the same cycle, rather than being out of sync.
2. P&H should have a copy of the ordinary Healing spell for Paladins.
3. Town gen code is easier with LAVA streamers, and doesn't look significantly worse. Update code as follows after creating starburst room:

[COLOR="palegreen"]	/* Add some lava streamers */
	for (n = 0; n < 3 + num_lava; n++)
		build_streamer(c, FEAT_LAVA, 0);	[/COLOR]					
	/* Make everything else permanent wall, and none of it a room */
	for (y = 0; y < c->height; y++)
		for (x = 0; x < c->width; x++) {
			if (square_isfloor(c, y, x))
				sqinfo_off(c->squares[y][x].info, SQUARE_ROOM);
			else [COLOR="PaleGreen"]if (!square_isrock(c, x, y))[/COLOR]
				square_set_feat(c, y, x, FEAT_PERM);
		}

This gets rid of around 50 lines of code. Result looks...kind of cool, with lava streams disappearing around corners.

[Nick]

3. Town gen code is easier with LAVA streamers, and doesn't look significantly worse. Update code as follows after creating starburst room:

[COLOR="palegreen"]	/* Add some lava streamers */
	for (n = 0; n < 3 + num_lava; n++)
		build_streamer(c, FEAT_LAVA, 0);	[/COLOR]					
	/* Make everything else permanent wall, and none of it a room */
	for (y = 0; y < c->height; y++)
		for (x = 0; x < c->width; x++) {
			if (square_isfloor(c, y, x))
				sqinfo_off(c->squares[y][x].info, SQUARE_ROOM);
			else [COLOR="PaleGreen"]if (!square_isrock(c, x, y))[/COLOR]
				square_set_feat(c, y, x, FEAT_PERM);
		}

This gets rid of around 50 lines of code. Result looks...kind of cool, with lava streams disappearing around corners.

This is a definite win. I'll just make the change at some stage, or if you'd prefer you can make a pull request on github, and I can pull it in so you get recorded as the author.

While on pull requests, feel free to do one for the ANSI compliance stuff once you and takkaria have sorted out the whys and wherefores of it.