nethack-gwaa-kiwi/nethack
1
0
Fork 0
Code Pull Requests Activity

augment include/extern.h with nonnull arg info

Browse Source 
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.
This commit is contained in:
nhmall
2023-12-14 20:06:03 -05:00
parent ae83a430c6
commit 978ec6a3a7
6 changed files with 1568 additions and 1361 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2822
include/extern.h
View File

File diff suppressed because it is too large Load Diff

4
include/obj.h
View File

@@ -419,7 +419,9 @@ struct obj {
#define is_mines_prize(o) ((o)->o_id == gc.context.achieveo.mines_prize_oid)
#define is_soko_prize(o) ((o)->o_id == gc.context.achieveo.soko_prize_oid)
#define is_art(o,art) ((o) && (o)->oartifact == (art))
/* is_art() is now a function in artifact.c */
/* #define is_art(o,art) ((o) && (o)->oartifact == (art)) */
#define u_wield_art(art) is_art(uwep, art)
/* mummy wrappings are more versatile sizewise than other cloaks */

54
include/tradstdc.h
View File

@@ -361,12 +361,40 @@ typedef genericptr genericptr_t; /* (void *) or (char *) */
#endif
#endif
#if __GNUC__ >= 5
#define NONNULL __attribute__((returns_nonnull))
#ifndef NONNULLS_DEFINED
#define DO_DEFINE_NONNULLS
#endif /* !NONNULLS_DEFINED */
/* #pragma message is available */
#define NH_PRAGMA_MESSAGE 1
#endif
#endif
#if defined(__clang__) && !defined(DO_DEFINE_NONNULLS)
#define DO_DEFINE_NONNULLS
#endif
#if defined(DO_DEFINE_NONNULLS) && !defined(NONNULLS_DEFINED)
#define NONNULL __attribute__((returns_nonnull))
#define NONNULLPTRS __attribute__((nonnull))
#define NONNULLARG1 __attribute__((nonnull (1)))
#define NONNULLARG2 __attribute__((nonnull (2)))
#define NONNULLARG3 __attribute__((nonnull (3)))
#define NONNULLARG4 __attribute__((nonnull (4)))
#define NONNULLARG5 __attribute__((nonnull (5)))
#define NONNULLARG7 __attribute__((nonnull (7))) /* for bhit() */
#define NONNULLARG12 __attribute__((nonnull (1, 2)))
#define NONNULLARG123 __attribute__((nonnull (1, 2, 3)))
#define NONNULLARG13 __attribute__((nonnull (1, 3)))
#define NONNULLARG14 __attribute__((nonnull (1, 4))) /* for query_category */
#define NONNULLARG134 __attribute__((nonnull (1, 3, 4))) /* for do_stone_mon */
#define NONNULLARG145 __attribute__((nonnull (1, 4, 5))) /* find_roll_to_hit */
#define NONNULLARG17 __attribute__((nonnull (1, 7))) /* for askchain() */
#define NONNULLARG24 __attribute__((nonnull (2, 4))) /* query_objlist() */
#define NONNULLARG45 __attribute__((nonnull (4, 5))) /* do_screen_descri... */
#define NONNULLS_DEFINED
#undef DO_DEFINE_NONNULLS
#endif /* __clang__ && !NONNULLS_DEFINED */
#ifdef _MSC_VER
#ifndef ATTRNORETURN
#define ATTRNORETURN __declspec(noreturn)
@@ -390,9 +418,29 @@ typedef genericptr genericptr_t; /* (void *) or (char *) */
#ifndef NORETURN
#define NORETURN
#endif
#ifndef NONNULL
#ifndef NONNULLS_DEFINED
#define NONNULL
#endif
#define NONNULLPTRS
#define NONNULLARG1
#define NONNULLARG2
#define NONNULLARG3
#define NONNULLARG4
#define NONNULLARG5
#define NONNULLARG7
#define NONNULLARG12
#define NONNULLARG123
#define NONNULLARG13
#define NONNULLARG14
#define NONNULLARG134
#define NONNULLARG145
#define NONNULLARG17
#define NONNULLARG24
#define NONNULLARG45
#define NONNULLS_DEFINED
#endif /* NONNULLS_DEFINED */
#ifndef NO_NONNULLS
#define NO_NONNULLS
#endif /* NO_NONNULLS */
/*
* Allow gcc and clang to catch the use of non-C99 functions that

9
src/artifact.c
View File

@@ -2523,4 +2523,13 @@ has_magic_key(struct monst *mon) /* if null, hero assumed */
return (struct obj *) 0;
}
/* #define is_art(o,art) ((o) && (o)->oartifact == (art)) */
boolean
is_art(struct obj *obj, int art)
{
if (obj && obj->oartifact == art)
return TRUE;
return FALSE;
}
/*artifact.c*/

39
src/files.c
View File

@@ -219,13 +219,14 @@ static void cnf_parser_init(struct _cnf_parser_state *parser);
static void cnf_parser_done(struct _cnf_parser_state *parser);
static void parse_conf_buf(struct _cnf_parser_state *parser,
boolean (*proc)(char *arg));
/* next one is in extern.h; why here too? */
boolean parse_conf_str(const char *str, boolean (*proc)(char *arg));
static boolean parse_conf_file(FILE *fp, boolean (*proc)(char *arg));
static void parseformat(int *, char *);
static FILE *fopen_wizkit_file(void);
static void wizkit_addinv(struct obj *);
boolean proc_wizkit_line(char *buf);
void read_wizkit(void);
void read_wizkit(void); /* in extern.h; why here too? */
static FILE *fopen_sym_file(void);
#ifdef SELF_RECOVER
@@ -497,20 +498,18 @@ const int bei = 1;
void
zero_nhfile(NHFILE *nhfp)
{
if (nhfp) {
nhfp->fd = -1;
nhfp->mode = COUNTING;
nhfp->structlevel = FALSE;
nhfp->fieldlevel = FALSE;
nhfp->addinfo = FALSE;
nhfp->bendian = IS_BIGENDIAN();
nhfp->fpdef = (FILE *) 0;
nhfp->fplog = (FILE *) 0;
nhfp->fpdebug = (FILE *) 0;
nhfp->count = 0;
nhfp->eof = FALSE;
nhfp->fnidx = 0;
}
nhfp->fd = -1;
nhfp->mode = COUNTING;
nhfp->structlevel = FALSE;
nhfp->fieldlevel = FALSE;
nhfp->addinfo = FALSE;
nhfp->bendian = IS_BIGENDIAN();
nhfp->fpdef = (FILE *) 0;
nhfp->fplog = (FILE *) 0;
nhfp->fpdebug = (FILE *) 0;
nhfp->count = 0;
nhfp->eof = FALSE;
nhfp->fnidx = 0;
}
static NHFILE *
@@ -534,12 +533,10 @@ free_nhfile(NHFILE *nhfp)
void
close_nhfile(NHFILE *nhfp)
{
if (nhfp) {
if (nhfp->structlevel && nhfp->fd != -1)
(void) nhclose(nhfp->fd), nhfp->fd = -1;
zero_nhfile(nhfp);
free_nhfile(nhfp);
}
if (nhfp->structlevel && nhfp->fd != -1)
(void) nhclose(nhfp->fd), nhfp->fd = -1;
zero_nhfile(nhfp);
free_nhfile(nhfp);
}
void

1
sys/unix/hints/include/compiler.370
View File

@@ -63,6 +63,7 @@ CFLAGS+=-Wimplicit-int
CFLAGS+=-Wmissing-prototypes
CFLAGS+=-Wold-style-definition
CFLAGS+=-Wstrict-prototypes
CFLAGS+=-Wnonnull
#detection of clang vs gcc
CCISCLANG := $(shell echo `$(CC) --version` | grep clang)