Update several places where lazy lastseentyp[] might be an issue.
I think it isn't updated in a timely fashion when newsym() shows
a spot covered by an object or trap, but didn't manage to find any
cases where that caused a problem. This is more in the nature of
a precaution.
If MAKEDEFS_FILTER_NONASCII is defined (which config.h now does by
default), it will check data.base, rumors.*, and {various}.txt for
characters outside the range of ' ' through '~'. If it finds any, it
will warn about them and change them to '#'.
Tab handling is incomplete; the files that use tabs for indentation
will allow tabs anywhere, even though that's not wanted. That could
be fixed but doesn't seem particularly urgent. This is more about
spotting and repairing the special 3-char punctuation characters that
crept into data.base fairly recently.
For now, this will prevent the NONNULLxxx arguments from being
defined under a djgpp compiler or crosscompiler.
paxed reported a segfault under msdos:
nethack.exe
Exiting due to signal SIGSEGV
Page fault at eip=000c3f8c, error=0004
eax=00000000 ebx=00000000 ecx=00000000 edx=0000000f esi=00000000 edi=00000001
ebp=00589988 esp=00589970 program=C:\NH370\NETHACK.EXE
cs: sel=00a7 base=00400000 limit=0063ffff
ds: sel=00af base=00400000 limit=0063ffff
es: sel=00af base=00400000 limit=0063ffff
fs: sel=008f base=00001a20 limit=0000ffff
gs: sel=00bf base=00000000 limit=0010ffff
ss: sel=00af base=00400000 limit=0063ffff
App stack: [00589ba8..00389ba8] Exceptn stack: [00389af4..00387bb4]
Call frame traceback EIPs:
0x000c3f8c _read_config_file+19
0x0017619f _initoptions_finish+577
0x00176371 _initoptions+157
0x0025cec4 _pcmain+365
0x0025d8d9 _main+41
He was able to 'git bisect' to the macro definitions change,
and confirmed that the segfault no longer occurs after this commit.
There may be further investigation on this later.
Callgrind showed recalc_mapseen was three times more expensive (in terms
of instructions read) than anything else in our codebase. It was being
called in every vision change, re-evaluating the last seen map terrain
type for every map location in sight.
Remove updating the lastseen info in the vision code, and make a small
change so newsym() uses update_lastseentyp.
From my short tests, this seems to work correctly ...
Fix some of the extreme verbosity for null vs non-null triggered
by mklev.c. dungeon_branch() never returns Null.
'#include <assert.h>' should probably be moved out of multiple .c
files and into cstd.h or some such but this doesn't do that.
Checking the callers:
newsym() the use of see_with_infrared() is guarded by
} else if ((mon = m_at(x, y)) != 0 [...]
do_mgivenname() the use of see_with_infrared is guarded by !mtmp:
&& (!mtmp
|| (!sensemon(mtmp)
&& (!(cansee(cx, cy) || see_with_infrared(mtmp))
howmonseen(mon) dereferences mon in other places, so it would
segfault if mon were NULL; howmonseen has NONNULLARG1.
callers were checked:
domove_attackmon_at(mtmp, x, y, displaceu) has mtmp declared nonnull;
there are dereferences of mtmp in the first line of code in
the function.
In domove_core():
The 1st occurrence of is_safemon(mtmp) is guarded by if (mtmp) { }.
The 2nd occurrence of is_safemon(mtmp) is inside an if (mtmp) { } block.
The 3rd occurrence of is_safemon(mtmp) was just remediated by 987be7e8.
In lookaround():
The only occurrence of is_safemon(mtmp) is inside an
if ((mtmp = m_at(x, y)) != 0 [...] { } block.
In do_attack(mtmp), in uhitm.c:
The parameter is declared NONNULLARG1, and the 1st line of
code contains a dereference with mtmp->data, which would
segfault if mtmp were NULL.
Checking the callers:
toss_up() would have segfaulted prior to use of stone_missile() if obj were NULL.
thitu() now has a guard prior to use of stone_missile()
ohitmon() would have crashed from earlier dereference otmp->dknown if it were NULL,
otmp arg is declared nonnull
thitm() now has a guard prior to use of stone_missile().
hmon_hitmon_do_hit() null obj takes a different code path than the code path
using stone_missile(); comment asserting that added
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.
selection_getbounds() has a check and early return.
Initialization will ensure a known state if that early return
were ever taken.
This is an alternative approach to pr #1163.
The following were listed in extern.h as residing in makemon.c,
but they are actually in mon.c:
copy_mextra(struct monst *, struct monst *);
dealloc_mextra(struct monst *);
usmellmon(struct permonst *);
When trying to reproduce the wand of striking "interesting effect (0)"
report, I tried wishing for lava under the castle drawbridge. That
wasn't handling drawbridges properly. This fixes wishing for moat,
lava, ice, or floor at a drawbridge span location whether the bridge
is currently open of closed. It also allows wishing for room or floor
or ground at room spots; that hasn't had much testing.
Wishing for furniture, pool|moat|water, or lava at an ice location
wasn't cancelling any pending melt timer.
ice_descr() was declared as returning const but returns its non-const
output buffer argument. Change to 'char *' so that wizterrainwish()
can capitilize that output without jumping through any hoops.
After the drawbridge was destroyed, playing an instrument on the castle
level while knowing the tune continued to offer a chance to play it.
Then nothing interesting happened even if you were close enough to the
former bridge for it to have been useful prior to the destruction.
I think the hero could also be given the tune as a divine prayer boon
after bridge destruction but I didn't verify that. The player might
not know that the tune is no good anymore, but the hero's patron deity
should.
Hide some of the details about new Stealth.
Streamline mon_break_armor(). Move replicated bypass handling into
m_lose_armor(). Also, eliminate a 'goto'.
Donning elven boots while riding and not already stealthy, you'd get
the message "you walk quietly" when not walking at all. Instead of
just changing the message, make riding a non-flying steed block
stealth. Riding a flying steed (or one you take aloft with an amulet
of flying) does not. It would have been quite a bit simpler to have
made riding anything block stealth, but the hard part is done.
... and have more than 1 billed item, and using non-traditional
menustyle.
I opted to add an extra field to the bill struct, because
that made the code cleaner.
Breaks saves and bones.
During very early startup, Windows may not have loaded
the tty window procs yet, and it is running with safeprocs.
It will eventually load the tty stuff. If the currently
operating window port fails in can_set_perm_invent(),
try the check for WC_PERM_INVENT again explicitly on
the tty windowport.
Pull request from entrez: add 'nopick_dropped' option of not pick up
items dropped by hero via autopickup and 'pickup_stolen' to picup up
items stolen from hero via autopickup, bypassing pickup_types and
autopickup_exceptions like existing 'pickup_thrown'.
This fixes entry commit also fixes the description of pickup_stolen.
Closes#1140
Add pickup_stolen option to autopick items stolen from you by a nymph or
monkey, even if they don't match your normal autopickup settings.
Replace was_dropped, was_thrown with a 2-bit bitfield that can contain
values LOST_DROPPED, LOST_THROWN, and LOST_STOLEN (or 0), since they
should all be mutually exclusive anyway as they track the most recent
way the item left the hero's inventory.
[Rebase/merge conflict fixed up. PR]
This is based on a feature in UnNetHack (and I think some other variants
as well). If the hero intentionally drops an item with 'pickup_dropped'
disabled, don't autopick it back up when walking over that square again.
Typically when the player drops an item, it's because she doesn't want
it in her inventory any more, and this option stops autopickup from
defeating that goal (especially useful for tasks like stash management
without a container). Players have come up with workarounds to this
problem like toggling autopickup when approaching their stash pile or
adding name-based autopickup exceptions to allow them to exclude
individual items from autopickup, but this behavior should reduce the
need for those things.
I think 'pickup_dropped' is a little unfortunate because it suggests
equivalence to 'pickup_thrown' (i.e. any dropped items will be
automatically picked up regardless of autopickup exceptions). Calling
it something like 'nopick_dropped' might be better, but as far as I can
tell options cannot start with the word 'no' because it's interpreted as
a negation of the rest of the option name.
There were 6 brown 'd' monsters; move wolf and werewolf to grey,
and warg to black, as those colors had no canines.
The wolf tiles are already greyish; changed warg tile to be
slightly darker.
There are many red major demons, and hezrous and vrocks
now emit poison gas, so change the symbol color to green.
Also adjust vrock tiles to have green. The hezrou tiles
already are green.
Prior to this commit, there was no good way to deal with swarms of
weak, good-AC enemies using magic; trying to play a wizard as a
pure spellcaster would make bees and ants very difficult to deal
with (because they would usually dodge force bolts).
This commit adds a new spell designed to be very good against
swarms of weak enemies: "chain lightning", which does 2d6 lightning
damage to every monster around you. It has an initially short range,
but can chain from monster to monster to cover potentially large
distances (as long as none of the monsters en route are shock
resistant or tame/peaceful; the spell can't chain past shock
resistant monsters and avoids peacefuls). Monsters within one
space of the visible lightning bolts are affected. Unlike other
lightning effects, this one does only 2d6 damage, not enough to
blind affected monsters.
This commit breaks existing save and bones files (thus the
EDITLEVEL increase).
The same checks were being repeated for every damage type; this
sends them through two centralised functions (one for checking
whether an extrinsic blocks a specific instance of item destruction
and one for the enlightenment message), so that new mechanisms of
item destruction prevention will need to change only one point in
the code.
