- add nhl_pcall_handle() to wrap all nhl_pcall calls that didn't check
return value and either panic() or impossible()
- add --loglua (unix only) to dump Lua memory and steps info to livelog
- remove old logging
- set memory and step limits on all Lua VMs
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.
In the rare event that
make fetch-lua
is not working because the primary Lua site is not available,
provide another target
make fetch-lua-mirror
that uses the official lua mirror site documented at:
https://www.lua.org/mirrors.html
where calling term_start_color(NO_COLOR) would have the same
effect as calling term_end_color().
That change only included a change for termcap, but not any of
the NO_TERMS configurations. (NO_TERMS is defined for an
implementation where termcap is not used).
This attempts to make sys/msdos/video.c and sys/windows/consoletty.c
honor the change.
The msdos change has not yet been tested.
No attempt was made to alter the term_start_color() implementations
within the outdated tree.
Allow 'make USE_MANDOC=1 distrib' to be used for generating
the doc/*.txt files for systems that have mandoc and may not have groff.
(macOS Ventura doesn't include groff in the OS).
USE_MANDOC=1 is not restricted only to macOS.
Have hints/macOS.370 specify the /usr/share/man/man6 directory for the macOS
man pages, so that 'make manpages' will target the correct directory on
that platform.
add a 'distrib' target to the top level Makefile so that the
distribution doc/*.txt files can be created via 'make distrib'
from the top directory. It utilizes the existing 'distrib'
target in the doc/Makefile.
Have hints/linux.370 specify the correct directory for Linux
man pages of /usr/share/man/man6, so that 'sudo make manpages'
works on that platform,
Some changes to achieve the following MAN2TXT commands...
If groff version is 1.23 or greater:
nroff -man -Tascii -P -cbou
If groff version is less than 1.23:
nroff -man -Tascii -c | col -b
If non-groff nroff:
nroff -man | col -b
Closes#1153 again.
Following a commit for Issue #1153, g-branden-robinson commented:
> Mac OS X stayed on _groff_ 1.19.2 for over a decade (presumably due to
> _groff_ 1.20 adopting to GNU GPLv3), until finally dropping _groff_
> altogether for macOS Ventura (2022).
>
> There _has_ been an interface change in that time. The [`-P` option I
> advised about is new to _groff_ 1.23.0 (July 2023)]
> (https://git.savannah.gnu.org/cgit/groff.git/tree/NEWS?h=1.23.0#n86).
> [...]
>
> There is a significant number of _groff_ users via Homebrew (enough that
> we hear from them occasionally via bug reports). Some of these have
> upgraded to 1.23.0 via that mechanism.
> [...]
>
> `nroff -` is not necessary with any _nroff_ known to me; like many other
> Bell Labs Unix programs, it reads from the standard input stream by default
> if not given any operands.
Action taken:
1. Remove the unnecessary ' -' from the nroff command in Makefile.doc.
2. In the misc.370 file containing make snippets to include, test whether
groff >= 1.23, and only insert the -P option for 1.23 or greater.
rename hints/include/multiw-3.370 to hints/include/misc.370
keep the portable nroff options in sys/unix/Makefile.doc,
and relocate the non-portable bits to a variable defined
in sys/unix/hints/include/misc.370
This assumes that the groff options are compatible between
Linux and macOS implementations of groff.
If that turns out not to be the case, this bit:
ifneq "$(NROFFISGROFF)" "" # It's groff
# add the groff-specific plain text flags
MORE_MAN2TXT_FLAGS += -Tascii -P -cbou
endif
should relocate from sys/unix/hints/include/misc.370
to sys/unix/hints/linux.370 and sys/unix/hints/macOS.370,
immediately following the inclusion of misc.370, and the
appropriate platform-specific groff options can be
adjused in whichever of those appropriately needs it.
Closes#1153
Rebuild doc/*.txt (other than Guidebook) with 'nroff -man -c -Tascii'
rather than just 'nroff -man' to prevent it generating non-ASCII
characters for hyphen, aprostrophe, single and double quotes, long
dash, possibly others. Also preprocess with '--grep-define ALLDOCS'
so that a few parts of the text don't end up being specific to the
local configuration.
The next time they're rebuilt they'll probably be subject to the
ping-pong effect of inserting padding spaces for justification
(alternating right-to-left vs left-to-right as intended but starting
with different parity so lots of gratuitous changes) since my quite
old version of groff triggers that for Guidebook.txt.
linux.370 and macOS.370 pass the name of the hints file
and the list of included files, that they use, to Makefile.check.
The name of the hints file is in variable HINTSFILE, the
definition of which is now inserted by setup.sh
The list of hints include files is in variable HINTSINCLFILES.
../lib/pdcursesmod/dos/../common/dosutil.c:36:15: warning: comparison of integer expressions of different signedness: 'long int' and 'long unsigned int' [-Wsign-compare]
36 | while( ms > MAX_NAP_SPAN)
| ^
It isn't something that we can actually resolve within NetHack,
so just suppress the submodule build warning.
This commit will trigger the CI to carry out a test of the build.
There could be some follow-up after the results.
