Re: [PATCH COMMITTED] src: Fix NDEBUG compilation: avoid side-effects in assert

[Dan Čermák]

Hi list,

on a related note: how about re-defining `assert` to `(void)(cond)` for
NDEBUG builds, so that we don't get a plethora of unused variable
warnings now?

Also, are NDEBUG builds actually a good idea? I recall that when fuzzing
poke a lot of "crashes" that I found were just failed assertions. So
maybe we should refrain from supporting NDEBUG builds for now?


Cheers,

Dan

Eric Blake <address@hidden> writes:

> * src/ios.c (ios_close): Avoid side-effect inside assert.
> * src/pk-def.c (print_var_decl, print_fun_decl): Likewise.
> * src/pk-term.c (pk_printf): Likewise.
> * src/pkl-asm.c (pkl_asm_call): Likewise.
> * src/pkl-tab.y (pkl_register_dummies): Likewise.
> * src/pkl.c (pkl_detailed_location): Likewise.
> ---
>  ChangeLog     |  9 +++++++++
>  src/ios.c     |  4 +++-
>  src/pk-def.c  | 16 ++++++++++------
>  src/pk-term.c |  4 +++-
>  src/pkl-asm.c |  6 ++++--
>  src/pkl-tab.y |  4 +++-
>  src/pkl.c     |  7 +++++--
>  7 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index ba5a66ae..f8581455 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,12 @@
> +2020-02-26  Eric Blake  <address@hidden>
> +
> +     * src/ios.c (ios_close): Avoid side-effect inside assert.
> +     * src/pk-def.c (print_var_decl, print_fun_decl): Likewise.
> +     * src/pk-term.c (pk_printf): Likewise.
> +     * src/pkl-asm.c (pkl_asm_call): Likewise.
> +     * src/pkl-tab.y (pkl_register_dummies): Likewise.
> +     * src/pkl.c (pkl_detailed_location): Likewise.
> +
>  2020-02-26  Eric Blake  <address@hidden>
>
>       * etc/git.orderfile (ChangeLog): Always list first.
> diff --git a/src/ios.c b/src/ios.c
> index 2340bc88..4bdc80d6 100644
> --- a/src/ios.c
> +++ b/src/ios.c
> @@ -220,12 +220,14 @@ void
>  ios_close (ios io)
>  {
>    struct ios *tmp;
> +  int r;
>
>    /* XXX: if not saved, ask before closing.  */
>
>    /* Close the device operated by the IO space.
>       XXX: handle errors.  */
> -  assert (io->dev_if->close (io->dev));
> +  r = io->dev_if->close (io->dev);
> +  assert (r);
>
>    /* Unlink the IOS from the list.  */
>    assert (io_list != NULL); /* The list must contain at least one IO
> diff --git a/src/pk-def.c b/src/pk-def.c
> index b44a801b..f908945a 100644
> --- a/src/pk-def.c
> +++ b/src/pk-def.c
> @@ -41,14 +41,16 @@ print_var_decl (pkl_ast_node decl, void *data)
>
>    pkl_env compiler_env = pkl_get_env (poke_compiler);
>    pvm_env runtime_env = pvm_get_env (poke_vm);
> +  pkl_ast_node tmp;
>
>    /* XXX this is not needed.  */
>    if (PKL_AST_DECL_KIND (decl) != PKL_AST_DECL_KIND_VAR)
>      return;
>
> -  assert (pkl_env_lookup (compiler_env,
> -                          PKL_AST_IDENTIFIER_POINTER (decl_name),
> -                          &back, &over) != NULL);
> +  tmp = pkl_env_lookup (compiler_env,
> +                        PKL_AST_IDENTIFIER_POINTER (decl_name),
> +                        &back, &over);
> +  assert (tmp != NULL);
>
>    val = pvm_env_lookup (runtime_env, back, over);
>    assert (val != PVM_NULL);
> @@ -78,6 +80,7 @@ print_fun_decl (pkl_ast_node decl, void *data)
>    int back, over;
>
>    pkl_env compiler_env = pkl_get_env (poke_compiler);
> +  pkl_ast_node tmp;
>
>    /* Skip mappers, i.e. function declarations whose initials are
>       actually struct types, and not function literals.  */
> @@ -85,9 +88,10 @@ print_fun_decl (pkl_ast_node decl, void *data)
>    if (PKL_AST_CODE (func) != PKL_AST_FUNC)
>      return;
>
> -  assert (pkl_env_lookup (compiler_env,
> -                          PKL_AST_IDENTIFIER_POINTER (decl_name),
> -                          &back, &over) != NULL);
> +  tmp = pkl_env_lookup (compiler_env,
> +                        PKL_AST_IDENTIFIER_POINTER (decl_name),
> +                        &back, &over);
> +  assert (tmp != NULL);
>
>    /* Print the name and the type of the function.  */
>    pk_puts (PKL_AST_IDENTIFIER_POINTER (decl_name));
> diff --git a/src/pk-term.c b/src/pk-term.c
> index adf6c66d..4cdc4cd2 100644
> --- a/src/pk-term.c
> +++ b/src/pk-term.c
> @@ -108,9 +108,11 @@ pk_printf (const char *format, ...)
>  {
>    va_list ap;
>    char *str;
> +  int r;
>
>    va_start (ap, format);
> -  assert (vasprintf (&str, format, ap) != -1);
> +  r = vasprintf (&str, format, ap);
> +  assert (r != -1);
>    va_end (ap);
>
>    ostream_write_str (poke_ostream, str);
> diff --git a/src/pkl-asm.c b/src/pkl-asm.c
> index 04606833..621a9ea3 100644
> --- a/src/pkl-asm.c
> +++ b/src/pkl-asm.c
> @@ -1862,9 +1862,11 @@ pkl_asm_call (pkl_asm pasm, const char *funcname)
>  {
>    pkl_env compiler_env = pkl_get_env (pasm->compiler);
>    int back, over;
> +  pkl_ast_node tmp;
>
> -  assert (pkl_env_lookup (compiler_env, funcname,
> -                          &back, &over) != NULL);
> +  tmp = pkl_env_lookup (compiler_env, funcname,
> +                        &back, &over);
> +  assert (tmp != NULL);
>
>    pkl_asm_insn (pasm, PKL_INSN_PUSHVAR, back, over);
>    pkl_asm_insn (pasm, PKL_INSN_CALL);
> diff --git a/src/pkl-tab.y b/src/pkl-tab.y
> index cf63b3b7..d7ac3f4c 100644
> --- a/src/pkl-tab.y
> +++ b/src/pkl-tab.y
> @@ -160,6 +160,7 @@ pkl_register_dummies (struct pkl_parser *parser, int n)
>        char *name;
>        pkl_ast_node id;
>        pkl_ast_node decl;
> +      int r;
>
>        asprintf (&name, "@*UNUSABLE_OFF_%d*@", i);
>        id = pkl_ast_make_identifier (parser->ast, name);
> @@ -168,7 +169,8 @@ pkl_register_dummies (struct pkl_parser *parser, int n)
>                                  id, NULL /* initial */,
>                                  NULL /* source */);
>
> -      assert (pkl_env_register (parser->env, name, decl));
> +      r = pkl_env_register (parser->env, name, decl);
> +      assert (r);
>      }
>  }
>
> diff --git a/src/pkl.c b/src/pkl.c
> index 21d43f6e..658574f2 100644
> --- a/src/pkl.c
> +++ b/src/pkl.c
> @@ -512,9 +512,11 @@ pkl_detailed_location (pkl_ast ast, pkl_ast_loc loc,
>        int c;
>
>        off64_t cur_pos = ftello (fd);
> +      off64_t tmp;
>
>        /* Seek to the beginning of the file.  */
> -      assert (fseeko (fd, 0, SEEK_SET) == 0);
> +      tmp = fseeko (fd, 0, SEEK_SET);
> +      assert (tmp == 0);
>
>        while ((c = fgetc (fd)) != EOF)
>          {
> @@ -542,7 +544,8 @@ pkl_detailed_location (pkl_ast ast, pkl_ast_loc loc,
>          }
>
>        /* Restore the file position so parsing can continue.  */
> -      assert (fseeko (fd, cur_pos, SEEK_SET) == 0);
> +      tmp = fseeko (fd, cur_pos, SEEK_SET);
> +      assert (tmp == 0);
>      }
>
>    pk_puts ("\n");
> -- 
> 2.24.1

signature.asc
Description: PGP signature