Angband.oook.cz
Angband.oook.cz
AboutVariantsLadderForumCompetitionComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old January 28, 2016, 13:27   #11
takkaria
Veteran
 
takkaria's Avatar
 
Join Date: Apr 2007
Posts: 1,947
Donated: $40
takkaria is on a distinguished road
Quote:
Originally Posted by Pete Mack View Post
Question: What does making angband ANSI compliant mean? I have done some simple cleanup. I'd like to get this over with, and submit it back to github if at all possible.

In a fresh angband master clone I have:
* got rid of my_str* functions and replaced with the standard where possible, or renamed them otherwise.
* Checked that the return codes from the "real" functions are treated properly. (One of the my_str* functions returned int where it should really return character pointer.)
* Removed all the #if standard C code from headers.
* Removed special declarations of MAX_SHORT, MAX_UCHAR.
* Changed BOOL, TRUE, FALSE to bool, true, false
* added <stdbool.h> and <stdint.h> unconditionally.

Other possible steps:
* Use "official" c types, and replace u32b with uint32_t, etc.
* Use htonl <arpa/inet.h> or <ws32.h> type stuff in the save file instead of home grown functions in savefile.c
Some thoughts on this:

It's a mistake to replace my_strcat and my_strcpy with strncat and strncpy, since they do different things. The Angband functions are versions of the BSD strlcat and strlcpy, and just replacing them will introduce buffer overruns.

'str' is also a reserved prefix in implementations so prefixing with my_ makes sense IMO, as well as making it clearer to casual hackers what functions are defined just in Angband and what are external.

But if you want to go that route, strcasecmp is the more widely available version of my_stricmp, so it's probably worth using that and then keeping an ifdefed version for when it doesn't exist - if there is a reliable ifdef. If not, the status quo is less complex.

I think the main thing test that is needed is to make sure that MSVC++2015 can still compile the code after you make your changes. That is the compiler that has been holding us back for years. I think now it should have standard bool and int types, but there's not that much information on the web about it and I don't have a Windows set-up.

When replacing MAX_UCHAR, too, some parts of the code should probably be using UCHAR_MAX and other parts should be using UINT8_MAX, depending on whether they're using 'char' or 'byte'. MAX_SHORT should similarly be UINT16_MAX.

Also, I think the savefile code can safely keep its own byte-flipping code. It's less complex than adding more code conditional on what platform we're compiling on.
__________________
takkaria whispers something about options. -more-
takkaria is offline   Reply With Quote
Old January 28, 2016, 23:04   #12
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 6,589
Donated: $40
Pete Mack is on a distinguished road
Tak--
strncat and strncpy are ANSI C99 string functions, and they do exactly the right thing. (The only difference is strncat returns a pointer to the end of the string, not the string length.)

stricmp, however, is an ANSI extension, and it is not reliably available. The better option is to use the my_str* functions only where there's no ansi version available, but use ansi-style names and #define them to the my_str* implementation. If you look at existing code, you will see there are already uses of the ansi functions mixed in with my_str*!

Since VS 2012, basic ANSI C99 support is available for the MS compiler, so all that cruft can go away. (It's still missing some features, but it does have <stdint.h>, <stdstring.h> and <stdbool.h>)

Finally, I discovered that the infinite loop bug only exists in the optimized build OR it is intermittent. I'm not sure yet. :/
Pete Mack is offline   Reply With Quote
Old January 29, 2016, 03:07   #13
AnonymousHero
Veteran
 
AnonymousHero's Avatar
 
Join Date: Jun 2007
Posts: 1,383
AnonymousHero is on a distinguished road
Quote:
Originally Posted by Pete Mack View Post
Finally, I discovered that the infinite loop bug only exists in the optimized build OR it is intermittent. I'm not sure yet. :/
You want to try running with clang/gcc's -fsanitize=undefined [1] to see if you're hitting undefined behavior.

[1] Or, indeed, one of the other modes to see if it's memory corruption, or...
AnonymousHero is offline   Reply With Quote
Old January 29, 2016, 04:08   #14
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 6,589
Donated: $40
Pete Mack is on a distinguished road
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.
Pete Mack is offline   Reply With Quote
Old January 29, 2016, 07:16   #15
hjklyubn
Rookie
 
Join Date: Jan 2014
Posts: 19
hjklyubn is on a distinguished road
Quote:
Originally Posted by Pete Mack View Post
Tak--
strncat and strncpy are ANSI C99 string functions, and they do exactly the right thing. (The only difference is strncat returns a pointer to the end of the string, not the string length.)
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...
hjklyubn is offline   Reply With Quote
Old January 29, 2016, 08:38   #16
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 6,589
Donated: $40
Pete Mack is on a distinguished road
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.


Quote:
Originally Posted by hjklyubn View Post
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...
Pete Mack is offline   Reply With Quote
Old January 29, 2016, 11:35   #17
Nick
Vanilla maintainer
 
Nick's Avatar
 
Join Date: Apr 2007
Location: Canberra, Australia
Age: 56
Posts: 9,167
Donated: $60
Nick will become famous soon enoughNick will become famous soon enough
Quote:
Originally Posted by Pete Mack View Post
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.
__________________
One for the Dark Lord on his dark throne
In the Land of Mordor where the Shadows lie.
Nick is offline   Reply With Quote
Old January 29, 2016, 17:52   #18
takkaria
Veteran
 
takkaria's Avatar
 
Join Date: Apr 2007
Posts: 1,947
Donated: $40
takkaria is on a distinguished road
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...
__________________
takkaria whispers something about options. -more-

Last edited by takkaria; January 29, 2016 at 18:00.
takkaria is offline   Reply With Quote
Old January 29, 2016, 18:07   #19
Pete Mack
Prophet
 
Join Date: Apr 2007
Location: Seattle, WA
Posts: 6,589
Donated: $40
Pete Mack is on a distinguished road
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.

Quote:
Originally Posted by Nick View Post
I'm pretty sure I've had difficulties with optimisation, but can't recall where. Certainly we have -O0 set for the linux makefile.
Pete Mack is offline   Reply With Quote
Old January 29, 2016, 18:08   #20
AnonymousHero
Veteran
 
AnonymousHero's Avatar
 
Join Date: Jun 2007
Posts: 1,383
AnonymousHero is on a distinguished road
Quote:
Originally Posted by Pete Mack View Post
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.)
AnonymousHero 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
[3.5 dev] Tiny bug in dev 581 (permanent scrolls!) Ingwe Ingweron Vanilla 8 October 15, 2013 06:21
3.5-dev bug eMeM Vanilla 3 May 14, 2013 09:07
First 3.4 dev version Magnate Development 33 August 29, 2011 15:30
Tracking Angband using Mercurial (DVCS) d_m Vanilla 5 June 18, 2009 23:45
[D] Bug tracking help request. APWhite Variants 1 July 24, 2007 11:35


All times are GMT +1. The time now is 10:58.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2021, vBulletin Solutions Inc.