There was a transcription error in the comments in cstd.h for
the standard list of header files, where only the description
remained for <stdlib.h>, not the name of the file itself.
Remove several extraneous inclusions of the standard C99 headers.
Tested on the following afterwards:
Linux (using hints/linux.370) including tty, curses, qt6, and X11
macOS (using hints/macOS.370) including tty, curses, qt5, and X11
Windows MSYS2 using sys/windows/GNUmakefile
Windows Visual Studio using sys/windows/Makefile.nmake
msdos cross-compile on Ubuntu using djgpp cross-compiler
This build command will include line number info, gdb.exe or nhgdb.bat in the package:
make CROSS_TO_MSDOS=1 WANT_DEBUG=1 package
This build command will not include line number info, gdb.exe or nhgdb.bat in the package:
make CROSS_TO_MSDOS=1 package
The WANT_DEBUG=1 will cause the cross-compile to include line
number information in the NetHack executable, useful for
backtraces and gdb debugging sessions.
How a developer can use the optional deploy-to-dosbox target:
make CROSS_TO_MSDOS=1 WANT_DEBUG=1 dosbox=/mnt/c/dosbox deploy-to-dosbox
where dosbox= points to the directory which will be mounted for
your drive in dosbox
THe deploy-to-dosbox recipe ensures that a target copy of gdb.exe
ends up alongside nethack.exe at the target, including:
- placing the source code that gdb requires on the target
in the nhsrc subfolder.
- an nhgdb.bat that supplies the right switches to gdb
for locating the NetHack sources.
gcc has recognized various "magic comments" for white-listing
occurrences of implicit fallthrough in switch statements for
a long time:
The range and shape of "falls through" comments accepted are
contingent upon the level of the warning. (The default level is =3.)
-Wimplicit-fallthrough=0 disables the warning altogether.
-Wimplicit-fallthrough=1 treats any kind of comment as a "falls through" comment.
-Wimplicit-fallthrough=2 essentially accepts any comment that contains something
that matches (case insensitively) "falls?[ \t-]*thr(ough|u)" regular expression.
-Wimplicit-fallthrough=3 case sensitively matches a wide range of regular
expressions, listed in the GCC manual. E.g., all of these are accepted:
/* Falls through. */
/* fall-thru */
/* Else falls through. */
/* FALLTHRU */
/* ... falls through ... */
etc.
-Wimplicit-fallthrough=4 also, case sensitively matches a range of regular
expressions but is much more strict than level =3.
-Wimplicit-fallthrough=5 doesn't recognize any comments.
Plenty of other compilers did not recognize the gcc comment convention,
and up until now the compiler warning for detecting unintended
fallthrough had to be suppressed on other compilers. That's because the code
in NetHack has been relying on the gcc approach, and only the gcc approach.
The C23 standard introduces an attribute [[fallthrough]] for the
functionality, when implicit fallthrough warnings have been enabled.
Several popular compilers already support that, or a very similar attribute
style approach, today, even ahead of their C23 support:
C compiler whitelist approach
--------------------------- -------------------------------------
C23 conforming compilers [[fallthrough]]
clang versions supporting
standards prior to
C23 __attribute__((__fallthrough__))
Microsoft Visual Studio
since VS 2022 17.4.
The warning C5262 controls
whether the implict
fallthrough is detected and
warned about with
/std:clatest. [[fallthrough]]
This adds support to NetHack for the attribute approach by inserting a
macro FALLTHROUGH to the existing cases that require white-listing, so
other compilers can analyze things too.
The definition of the FALLTHROUGH macro is controlled in include/tradstdc.h.
The gcc comment approach has also been left in place at this time.
...some of them, at any rate. We shut off (1) warnings provoked by Matt
Bishop's "mn" macro package, and (2) warnings spuriously emitted by
groff 1.23 and previous when "-wall" (or "-ww") is specified.
Also update explanatory comments.
NOSTATICCORE - an option for Linux to get a better backtrace
updatedepend - an easier alternative to "make depend"
- .gitignore: ignore new temporary .*.c files
- hints/linux.370: add NOSTATICCORE and instructions
- Makefile.src:
-- If NOSTATICCORE defined, run core .c files through awk to
remove static attribute from functions.
-- add updatedepend target and related bits
new .h files: hacklib.h selvar.h stairs.h
new .c files: calendar.c, getpos.c, report.c, selvar.c, stairs.c,
strutil.c, wizcmds.c
cleanup of hacklib.c and mdlib.c
hacklib contains functions that do not have to link with the core
relocate wiz commands from cmd.c to wizcmds.c
relocate CRASHREPORT stuff to report.c
relocate getpos stuff from do_name.c to getpos.c
remove temporary struct definition from extern.h
cross-compile PRE-section split into cross-pre1.370 and cross-pre2.370
Windows sys/windows/Makefile.nmake and sys/windows/Makefile.mingw32 and
visual studio project file updates
Unix sys/unix/Makefile.src, sys/unix/Makefile.utl
populate selvar.c and selvar.h
build on MS-DOS (not cross-compile) Makefile updates
for sys/msdos/Makefile.GCC (untested)
vms updates for above (untested)
Compile-time option SELF_RECOVER was implemented for Windows;
add it to unix systems too, with it being on by default when
compiling with the linux hints file.
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?)
Add distinct potential steps into Makefile.top for putting
sysconf into place, instead of appending the steps for doing so
to the generic POSTINSTALL.
SYSCONFINSTALL is used for 'make install' and unconditionally
copies sys/unix/sysconf to the install directory.
SYSCONFENSURE is used for 'make update' and only copies
sys/unix/sysconf to the installation directory if it doesn't
already exist.
The initial trigger for revisiting this was because of new reports
that cp from (GNU coreutils) 9.4 is now issuing a warning during
NetHack builds. The warning pertains to deprecated use of the '-n'
switch with cp, which is used to only copy the file if the target
does not exist.
After this update, the shell is used to make that determination,
and if the file doesn't exist, cp is now invoked on Linux without
the '-n' switch.
For the macOS hints file, macOS.370, cp wasn't being used for sysconf
anyway, a utility script was being invoked and this doesn't change
that. macOS.370 was updated to remain in sync with linux.370 and
Makefile.top regarding the use of SYSCONFINSTALL and SYSCONFENSURE.
WANT_SOURCE_INSTALL=1 can be specified when compiling to build NetHack
within the repo/source tree, rather than in the normal system install
location. I find it useful for development on OSX, but it wasn't
present in any other hints files, so I've been using a custom hints file
on Linux for a while that adds the same option. The code is based on
the macOS.370 hints file.
Before using this updated packaging you will need
to do the following (one time):
sh sys/msdos/fetch-cross-compiler.sh
And you'll need to update your Makefiles as follows.
On Linux:
sh sys/msdos/setup.sh sys/unix/hints/linux.370
or on macOS;
sh sys/msdos/setup.sh sys/unix/hints/macOS.370
Create the msdos package with:
make CROSS_TO_MSDOS=1 package
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.
