Inspired by self-recover, sort of. Enabled for unix by default; can
be disabled by commenting out '#define CHECK_PANIC_SAVE' in unixconf.h.
When starting the game, if there is no save file to restore and no
lock/level files to recover, check whether a panic save file exists.
If there is one, tell the player that it's there and that it might be
viable, then ask whether to start a new game.
It doesn't convert the panic save into a reconverable one (rename by
nethack, then continue trying to restore) or tell the player how to
make it viable (rename to remove ".e" by game admin), just whether it
is present. If player opts to start a new game, the panic save is
left alone and will trigger the "there's a panic save file" situation
again once the new game finishes and player starts another.
A bunch of routines return a pointer which is never Null but weren't
telling the compiler that such was the case. A couple (strsubst(),
stripchars()) were accepting Null output argument and then returning
Null, but callers had no reason to use them that way, so they've been
changed. (upstart() could have been changed similarly; I've already
forgotten why I left it as-is.)
add CRASHREPORT for Windows
add ^P info to report (via DUMPLOG)
new options: crash_email, crash_name, crash_urlmax
new game command: #bugreport
new config option: CRASHREPORT_EXEC_NOSTDERR
new command line option: --bidshow
deleted helper scripts:
NetHackCrashReport.Javascript
nhcrashreport.lua
misc:
update CRASHREPORTURL (will need to be updated before release)
update bitrot in winchain
winchain for Windows
add missing synch_wait for NetHackW --showpaths
add PANICTRACE (and CRASHREPORT) in mdlib.c:build_opts
missing:
packaging (Windows needs the pdb file)
no testing with MSVC command line build
port status:
linux: working, but glibc's backtrace doesn't show static functions
Windows VS: working. pdb file is large - looking into options
MacOS: working
msdos: not supported
VMS: not supported
MSVC: planned, but not attempted
MSYS2: working, but libbacktrace not showing symbols (yet?)
This reverts commit 378648bd9c.
The problem was triggered by marking the 'objlist' argument in
merge_choice() prototype with __attribute__((nonnull)) when it
shouldn't have been, then a followup which relied on that. The
'objlist' argument might be Null. Instead of passing its address to
force it to be non-Null, remove the attribute.
Define some macros in include/tradstdc.h, for compilers that support
__attribute__((nonnull)), to assist in identifying which parameters
on functions are not supposed to be null pointers.
Next, for the majority of functions declared in include/extern.h, this
adds the appropriate macro that matches the actual use of each function's
parameters. The additions were done after performing some analysis.
These were the rules that were followed when determining which function
parameters should be nonnul, and which are nullable:
1. If the first use of, or reference to, the pointer parameter in the
function is a dereference, then the parameter will be considered
nonnull.
2. If there is code in the function that tests for the pointer parameter
being null, and adjusts the code-path accordingly so that no segfault
will occur, then the parameter will not be considered nonnull (it can
be null).
The use of the nonnull attributes allows the compiler to detect code in
callers of the function where a null parameter could get passed to the function.
If a warning is received the developer will have to do one of the following:
- If the null being passed to the function is now appropriate,
and the function should be able to expect a null parameter, then the
NONNULLxxx macro will have to be removed from the function's prototype.
or
- If the null being passed to the function is not appropriate,
correct the caller so it is not passing null.
or
- If the warning is about comparing to null, it may indicate an
unnecessary null check in the code involved. If it is deemed to be
unnecessary, it can then be removed.
Some static analysis tools apparently can work with the attribute, as well.
Following this, it was discovered that some functions were using one of the
(now) nonnull parameters in the first argument to the 'is_art(obj, ART)'
macro, which is defined like so:
=> #define is_art(o,art) ((o) && (o)->oartifact == (art))
That macro expansion inline resulted in a diagnostic warning because of the
'(o)' portion of the expanded macro, anywhere the macro was used with one of
the nonnull parameters. A test against null for a 'nonnull parameter' causes
a diagnostic warning.
To work around that, I replaced the is_art() macro with a function in
artifact.c, that accomplishes the same thing as the macro.
=> boolean
is_art(struct obj *obj, int art)
{
if (obj && obj->oartifact == art)
return TRUE;
return FALSE;
}
Some documentation...
These are the macros that have been defined for use when specifying the nonnull
parameters in a function prototype:
----------------------------------------------------------------------------
| Macro | Purpose |
+----------------+---------------------------------------------------------+
| NONULL | The function return value is never NULL. |
+----------------+---------------------------------------------------------+
| NONNULLPTRS | Every pointer argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG1 | The 1st argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG2 | The 2nd argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG3 | The 3rd argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG4 | The 4th argument is declared nonnull (not used). |
+----------------+---------------------------------------------------------+
| NONNULLARG5 | The 5th argument is declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG7 | The 7th argument is declared nonnull (bhit). |
+----------------+---------------------------------------------------------+
| NONNULLARG12 | The 1st and 2nd arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG13 | The 1st and 3rd arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG123 | The 1st, 2nd and 3rd arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG14 | The 1st and 4th arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG134 | The 1st, 3rd and 4th arguments are declared nonnull. |
+----------------+---------------------------------------------------------+
| NONNULLARG17 | The 1st and 7th arguments are declared nonnull (this |
| | was a special-case added for askchain(), where the |
| | arguments are spread out that way. This macro |
| | could be removed if the askchain arguments in the |
| | prototype and callers were changed to make the |
| | nonnull arguments side-by-side). |
+----------------+---------------------------------------------------------+
| NONNULLARG145 | The 1st, 4th and 5th arguments are declared nonnull |
| | (this was a special-case added for find_roll_to_hit(), |
| | in uhitm.c, where the arguments are spread out that way.|
| | We can't just use NONNULLPTRS there because the 3rd |
| | argument 'weapon' can be NULL). |
+----------------+---------------------------------------------------------+
| NONNULLARG24 | The 2nd and 4th arguments are declared nonnull (this |
| | was a special-case added for query_objlist() |
| | in invent.c). |
+----------------+---------------------------------------------------------+
| NONNULLARG45 | The 4th and 5th arguments are declared nonnull (this |
| | was a special-case added for do_screen_description(), |
| | in pager.c, where the arguments are spread out that |
| | way. We can't just use NONNULLPTRS there because the |
| | 6th argument can be NULL). |
+----------------+---------------------------------------------------------+
| NO_NONNULLS | This macro expands to nothing. It is just used to |
| | mark that analysis has been done on the function, |
| | and concluded that none of the arguments could be |
| | marked nonnull.That distinguishes a function that has |
| | not been analyzed (yet), from one that has. |
+----------------+---------------------------------------------------------+
The NO_NONNULLS macro is meant to place a flag on the prototype to
make people aware that an assessed function was determined to not
be eligible for nonnull parameters. It expands to nothing.
Unfortunately, that macro was added partway through this exercise, so there
aren't many instances of it in the upper parts of include/extern.h, even though
the functions there were likely assessed and categorized as not having any
eligible nonnull parameters. It just never got any macro at all, in that case.
Following the parameter usage analysis that was done, the following was
noted:
Some NetHack functions have added a test to catch a passed null
parameter, and exit the function early as a result, or call
impossible(), and then exit. While that approach prevents segfaults
from dereferencing a null parameter, the early return is silent
(when impossible is not called anyway), and the function's true
purpose is not fulfilled. Also, the calling function may have no
awareness that the function did not complete its intended purpose,
in many instances.
Functions with such a test and early return, cannot have the parameter
declared 'nonnull', because the code to test for 'null' will cause a
diagnostic to be issued if the parameter is nonnull.
It might be good to revisit some of those functions and consider,
on a case by case basis, declaring the parameter nonnull in the
prototype, and the test/code-path commented out.
cg.zeroobj was originally added (under its previous unprefixed name)
for providing a one-line way to zero out the fields of a struct obj.
struct obj tempobj;
tempobj = cg.zeroobj;
initfn(struct obj *otmp)
{
if (otmp)
*otmp = cg.zeroobj;
}
More recently, the address of cg.zeroobj began to be used as a return
flag to indicate some things, but the 'const struct obj zeroobj' wasn't
an ideal fit for the purpose and required a number of casts, including
casting away const.
Provide a better fitting variable (gi.invalid_obj) and eliminate a
number of casts.
If there were outdated savefiles encountered during
startup, each individual one was getting a wait_synch
that required a <return> even though a message window
wasn't being used at that point.
Allow suppression of the individual per-file wait_synch()
calls on Windows, so that a single one can be done once
the selectsave processing is overwith.
This was a little messy because an indicator had to flow
down through validate(), uptodate(), etc.
There shouldn't be any change in how things behave on
any non-Windows platforms.
When calling panic() or impossible(), create the option
of opening a browser window with most of the fields
already populated. Code for MacOS and linux is included;
other ports are affected by argument change to early_init
which are done but not tested.
To enable, define CRASHREPORT in config.h and set
CRASHREPORTURL in sysconf to (for the moment at least)
http[s]://www.nethack.org/common/contactcr.html
Adds --grep-defined option to makedefs for Makefiles.
Adds "bid" (binary identifier), an MD4 of the main nethack
binary. This is ONLY for helping (in the future) contact.html
to set the "NetHack from" field automatically for our own
binaries. This can be faked, but the user can lie so nothing
lost. There's nothing magic about MD4; other ports can use
anything that prodcues a long apparently random string we can
match against.
- new option --bidshow for us to get the MD4 of a
released binary so I can add it to the website.
Only available in wizard mode and not in nethack.6.
- typo macos -> macosx in hints file
No support for packaging builds as I'm not sure what that
would look like.
Adds a javascript helper for MacOS.
Adds a lua helper for linux (and builds and installs
nhlua).
files.c: In function 'do_write_config_file':
files.c:2146:66: warning: format '%lu' expects argument of type 'long unsigned int', but argument 2 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
2146 | pline("An error occurred, wrote only partial data (%lu/%lu).",
| ~~^
| |
| long unsigned int
| %u
2147 | wrote, len);
| ~~~~~
| |
| size_t {aka unsigned int}
files.c:2146:70: warning: format '%lu' expects argument of type 'long unsigned int', but argument 3 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
2146 | pline("An error occurred, wrote only partial data (%lu/%lu).",
| ~~^
| |
| long unsigned int
| %u
2147 | wrote, len);
| ~~~
| |
| size_t {aka unsigned int}
The code to lookup a value in DEBUGFILES usually operates on a file
name, but there are few non-file uses. The latter wouldn't work on
VMS because of the way it was manipulating the name: first stripping
away path, suffix, and version, then adding hardcoded ".c" suffix on.
I thought we already had a routine to get the base part of a name
from a full path, but if so, I haven't been able to find it. This
adds new nh_basename() to do that, with the option of either keeping
or discarding the suffix or type portion.
The VMS usage that prompted this hasn't actually been tested.
This should have no difference in behavior, except on Amiga;
as it is no longer supported, I removed all the amiga-specific
config-line handling. If anyone steps forward to support
that platform in the future, it shouldn't be too hard to
add those back.
At some point this may need to be commented out, but for the time
being report failure by wait() during the fork()+exec()+wait()
sequence when compressing or uncompressing a save file.
For the benefit of 'git log': uncompressing an old save file or
compressing a new one fails when running nethack under control of
gdb (GNU Project's debugger) on OSX. Noticed and reproducible on
OSX 10.11.6; no idea about more recent versions. wait() returns -1
and sets errno to 4, "interrupted system call", and leaves the
variable that receives the child process's exit status as is.
NetHack has been relying on that exit status variable without
checking whether wait() returns success or failure.
This changes nethack's behavior when wait() fails. It is now
intentionally failing compress/uncompress (by init'ing the status
to 1) after it was briefly treated as success (due to a recent
commit that set status to 0 before calling wait()), after a long
time of accidentally failing due to not setting varible 'i' (was 2
after being used for another purpose). In addition to initializing
to failure prior to calling wait(), replace use of 'i' with a new
variable that is only used for this, possibly making the code more
comprehensible.
By the way, the issue with compress and uncompress failure when
running under control of gdb on OSX is not new. I don't use gdb
a lot and have gotten tired of rediscovering this misbehavior.
I realized that I put opts[] inside a block that would go out of
scope while it was still needed. When I went to fix that, I
discovered that it is already present where it ought to be. My
'experimentation' should have defined COMPRESS_OPTIONS sooner so
that the outer scope would see it.
This doesn't revert the previous commit because a couple of comments
and a bit of reformatting from it are still useful.
The consolidation of global variables from scattered source
files into decl.c and declared in decl.h was begun in 3.7.0.
Their placement in common files was done for centralized
initialization and potential re-initialization during a
"play again" scenario.
It wasn't really necessary for all of them to be housed in a
single huge structure to meet the "play again" requirement,
and the single huge structure has been a little unwieldy when
it comes to maintenance.
Following this commit, instead of one single extremely large structure
named 'g' to house all of the relocated global variables, they
are distributed into several ga through gz.
To make things easy for the developer, each variable is placed
into the struct corresponding to the starting letter of the variable.
That way, no lookup is required in order to know which struct houses
a particular variable, it is a simple match to the starting letter
for all the centralized global variables.
A global variable named 'amulets', would be found in ga.
ga.amulets
^ ^
A global varable named 'move', would be found in gm.
gm.moves
^ ^
A global variable named 'val_for_n_or_more' would be found in gv.
gv.val_for_n_or_more
^ ^
A global variable named 'youmonst' would be found in gy.
gy.youmonst
^ ^
Instead of using a compile-time macro to suppress inclusion of the
menu entry to show UNIX command-line usage in the help menu, use a
sysconf setting instead.
Default is HIDEUSAGE=0, to include the entry for command-line usage.
Set HIDEUSAGE=1 to exclude that. Does not affect 'nethack --usage'
if player actually has access to the command-line.
Instead of using index() macro defined to strchr, use C99 strchr.
Instead of using rindex() macro defined to strrchr, use C99 strrchr.
If you want to try building on a platform that doesn't offer those
two functions, these are available:
define NOT_C99 /* to make some non-C99 code available */
define NEED_INDEX /* to define a macro for index() */
define NEED_RINDX /* to define a macro for rindex() */
Two years ago I modified the parsing for [section] labels for the
config file's CHOOSE directive to allow end-of-line comments, but
the code used had a logic error (don't think I can blame it on
copy+paste). It looked for '#' after ']' but allowed anything--
rather than just spaces--in between.
"[section-name]abc#comment" would become "section-name" as if the
trailing junk hadn't been present. Parsing that should produce
"section-name]abc" and get rejected as invalid.
I hadn't ever used #saveoptions before and when I checked to see
whether the autounlock:none changes were being handled properly, I
discovered that options set via 'm O' weren't being handled at all.
This includes some miscellaneous reformatting of things noticed
while tracking down the problem.
Add a #saveoptions extended command, to allow saving configuration
settings from within the game. This is still highly experimental,
and gives plenty of warnings before asking to overwrite the file.
Lack of option saving is one of the biggest complaints new players
have, so this should help with it. More experienced players with
highly customized config file should not use this feature, as it
completely rewrites the file, removing all comments and non-config
lines.
Have the config error reporting routine check whether the message
it's delivering already has end-of-sentence punctuation instead of
adding that unconditionally.
One of the drivers of this change was that screen coordinates require a
type that can hold values greater than 127. Parameters to the window
port routines require a large type in order to be able to have values
a fair bit larger than COLNO and ROWNO passed to them, particularly for
their use to the right of the map window.
This splits the uses of xchar into 3 different situations, and adjusts
their type and size:
xchar
|
-----------------------
| | |
coordxy xint16 xint8
coordxy: Actual x or y coordinates for various things (moved to 16-bits).
xint16: Same data size as coordxy, but for non-coordinate use (16-bits).
xint8: There are only a few use cases initially, where it was very
plain to see that the variable could remain as 8-bits, rather
than be bumped to 16-bits. There are probably more such cases
that could be changed after additional review.
Note: This first changed all xchar variables to coordxy. Some were
reviewed and got changed to xint16 or xint8 when it became apparent that
their usage was not for coordinates.
This increments EDITLEVEL in patchlevel.h
Writing lua warnings to paniclog (coming soon; tested without the
garbage collection fix in order to have test data) could crash on
the last pair. Those are written after the 'nomakedefs' structure
had been freed so version_string was Null.
The NAO PANICLOG_FMT2 code triggered a warning about the test for
g.plname; it is array so will never be Null.
