You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2017/12/11 17:17:45 UTC
1.10 task: error leaks
The release branch creation checkpoints calls for checking for error
leaks, so here's the output of tools/dev/warn-unused-result.sh (after
removing the __attribute__ from svn_error__malfunction()):
[[[
subversion/libsvn_delta/branch.c:485:11: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_element__tree_set(branch->priv->element_tree, old_eid, NULL);
^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
subversion/libsvn_delta/branch.c:486:11: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_element__tree_set(branch->priv->element_tree, new_eid, element);
^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
subversion/libsvn_delta/branch.c:975:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_element__tree_set(branch->priv->element_tree, eid, element);
^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3 warnings generated.
subversion/libsvn_delta/element.c:465:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_element__tree_set(new_subtree, new_subtree->root_eid,
^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/libsvn_subr/cache_config.c:164:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err
^
1 warning generated.
subversion/libsvn_subr/io.c:2426:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err
^
1 warning generated.
subversion/libsvn_subr/stream.c:1471:9: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_checksum_ctx_reset(btn->read_ctx);
^~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~
subversion/libsvn_subr/stream.c:1474:9: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_checksum_ctx_reset(btn->write_ctx);
^~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~
2 warnings generated.
subversion/libsvn_fs_fs/low_level.c:673:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_stream_puts(stream, "\n");
^~~~~~~~~~~~~~~ ~~~~~~~~~~~~
subversion/libsvn_fs_fs/low_level.c:890:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err
^
2 warnings generated.
subversion/libsvn_fs_fs/transaction.c:3857:26: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err
^
subversion/libsvn_fs_fs/transaction.c:3873:26: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err
^
2 warnings generated.
subversion/libsvn_fs_fs/verify.c:856:26: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err2
^
1 warning generated.
subversion/libsvn_fs_x/changes.c:187:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
append_change(changes, APR_ARRAY_IDX(list, i, svn_fs_x__change_t *));
^~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/libsvn_fs_x/low_level.c:365:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err
^
subversion/libsvn_fs_x/low_level.c:1134:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_stream_puts(stream, "\n");
^~~~~~~~~~~~~~~ ~~~~~~~~~~~~
2 warnings generated.
subversion/libsvn_fs_x/transaction.c:1262:11: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_io_dir_make(txn_dir, APR_OS_DEFAULT, iterpool);
^~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/libsvn_fs_x/verify.c:787:26: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err2
^
1 warning generated.
subversion/libsvn_client/merge.c:368:18: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err
^
1 warning generated.
subversion/libsvn_client/patch.c:2148:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_stream_printf(target->reject_stream,
^~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/libsvn_wc/adm_ops.c:418:20: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
__attribute__((warn_unused_result)) svn_error_t *err
^
1 warning generated.
subversion/libsvn_wc/wc_db.c:16528:15: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
lock_remove_txn(queue->wcroot, cqi->local_relpath, work_item,
^~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/svn/conflict-callbacks.c:1657:11: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_cmdline_fprintf(stderr, iterpool, "%s\n",
^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/svn/notify.c:213:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_hash_keys(conflicted_paths, all_conflicts, result_pool);
^~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/svnfsfs/load-index-cmd.c:150:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_stream_readline(input, &line, "\n", &eol, iterpool);
^~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/tests/libsvn_repos/authz-test.c:280:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
print_user_rights(NULL, NULL, 0, &authz->anon_rights, pool);
^~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
subversion/tests/libsvn_repos/authz-test.c:282:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
print_user_rights(NULL, NULL, 0, &authz->authn_rights, pool);
^~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
subversion/tests/libsvn_client/conflicts-test.c:543:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
sbox_wc_move(b, move_src_path, new_dir_path);
^~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/tests/libsvn_repos/dump-load-test.c:84:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_stream_close(stream);
^~~~~~~~~~~~~~~~ ~~~~~~
subversion/tests/libsvn_repos/dump-load-test.c:134:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_stream_close(stream);
^~~~~~~~~~~~~~~~ ~~~~~~
2 warnings generated.
subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c:1634:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_hash_write2(props, svn_stream_from_stringbuf(hash_rep, pool), "END",
^~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/tests/libsvn_subr/mergeinfo-test.c:1765:6: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_rangelist_to_string(&tmp_string, rangelist, pool);
^~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
subversion/tests/libsvn_subr/priority-queue-test.c:128:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
verify_empty_queue(queue);
^~~~~~~~~~~~~~~~~~ ~~~~~
subversion/tests/libsvn_subr/priority-queue-test.c:157:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
verify_empty_queue(queue);
^~~~~~~~~~~~~~~~~~ ~~~~~
subversion/tests/libsvn_subr/priority-queue-test.c:217:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
verify_empty_queue(queue);
^~~~~~~~~~~~~~~~~~ ~~~~~
3 warnings generated.
tools/client-side/svnconflict/svnconflict.c:366:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_cmdline_printf(pool, "text-conflict\n");
^~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~
tools/client-side/svnconflict/svnconflict.c:371:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_cmdline_printf(pool, "prop-conflict: %s\n", propname);
^~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/client-side/svnconflict/svnconflict.c:383:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_cmdline_printf(pool, "tree-conflict: %s %s\n",
^~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/client-side/svnconflict/svnconflict.c:404:7: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_cmdline_printf(pool, "%d: %s\n", id, label);
^~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
4 warnings generated.
tools/dev/svnmover/svnmover.c:2439:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_stringbuf_from_stream(&text, src, 0, scratch_pool);
^~~~~~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~
tools/dev/svnmover/svnmover.c:2779:3: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
svn_element__tree_set(from_subtree->tree, from_subtree->tree->root_eid,
^~~~~~~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 warnings generated.
]]]
I think we should enable this warning by default in maintainer mode in
supporting compilers, but that's a separate discussion to just fixing
these error leaks in trunk (& 1.10.x).
Cheers,
Daniel
Re: 1.10 task: error leaks
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Mon, 11 Dec 2017 19:51 +0000:
> Daniel Shahaf wrote:
> > I think we should enable this warning by default in maintainer mode in
> > supporting compilers, but that's a separate discussion to just fixing
> > these error leaks in trunk (& 1.10.x).
>
> +1. I was going to say that, as soon as I saw this long list.
>
> Anyone want to make it happen?
We already have an instance of __attribute__((warn_unused_result)) in
mod_dav_svn, not guarded by any #ifdef or anything, so I think we don't need to
bother with compiler checks (AC_TRY_COMPILE() and friends), but rather, we can
simply add that attribute unconditionally.
Of course, then we'd have a new problem: we might declare or define a function
without the __attribute__ annotation. It would be best if we could tell our C
compilers, "_Any_ function that returns svn_error_t* should be treated as
having the __attribute__((warn_unused_result)) annotation". Is that possible?
> (I volunteer to do some of this too. In the role of Release Manager I
> need to be asking for volunteers for as much as possible, so please
> don't anyone take offence at my continuing to do so.)
None taken; it should be a team effort. Thanks for leading it.
Cheers,
Daniel
Re: 1.10 task: error leaks
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> The release branch creation checkpoints calls for checking for error
> leaks, so here's the output of tools/dev/warn-unused-result.sh (after
> removing the __attribute__ from svn_error__malfunction()):
>
> [about 41 instances]
Thank you, Daniel!
Anyone volunteering to fix them?
> I think we should enable this warning by default in maintainer mode in
> supporting compilers, but that's a separate discussion to just fixing
> these error leaks in trunk (& 1.10.x).
+1. I was going to say that, as soon as I saw this long list.
Anyone want to make it happen?
(I volunteer to do some of this too. In the role of Release Manager I
need to be asking for volunteers for as much as possible, so please
don't anyone take offence at my continuing to do so.)
- Julian
Re: 1.10 task: error leaks
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Fri, 15 Dec 2017 19:51 +0000:
> r1818320.
>
> That's the lot.
Thanks, Julian!
Re: 1.10 task: error leaks
Posted by Julian Foad <ju...@apache.org>.
>> subversion/tests/libsvn_repos/authz-test.c:280:5:
>> print_user_rights(NULL, NULL, 0, &authz->anon_rights, pool);
>> subversion/tests/libsvn_repos/authz-test.c:282:5:
>> print_user_rights(NULL, NULL, 0, &authz->authn_rights, pool);
>> subversion/tests/libsvn_client/conflicts-test.c:543:7:
>> sbox_wc_move(b, move_src_path, new_dir_path);
>> subversion/tests/libsvn_repos/dump-load-test.c:84:3:
>> svn_stream_close(stream);
>> subversion/tests/libsvn_repos/dump-load-test.c:134:3:
>> svn_stream_close(stream);
>> subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c:1634:3:
>> svn_hash_write2(props, svn_stream_from_stringbuf(hash_rep, pool),
>> "END",
>> subversion/tests/libsvn_subr/mergeinfo-test.c:1765:6:
>> svn_rangelist_to_string(&tmp_string, rangelist, pool);
>> subversion/tests/libsvn_subr/priority-queue-test.c:128:3:
>> verify_empty_queue(queue);
>> subversion/tests/libsvn_subr/priority-queue-test.c:157:3:
>> verify_empty_queue(queue);
>> subversion/tests/libsvn_subr/priority-queue-test.c:217:3:
>> verify_empty_queue(queue);
>> tools/client-side/svnconflict/svnconflict.c:366:5:
>> svn_cmdline_printf(pool, "text-conflict\n");
>> tools/client-side/svnconflict/svnconflict.c:371:7:
>> svn_cmdline_printf(pool, "prop-conflict: %s\n", propname);
>> tools/client-side/svnconflict/svnconflict.c:383:7:
>> svn_cmdline_printf(pool, "tree-conflict: %s %s\n",
>> tools/client-side/svnconflict/svnconflict.c:404:7:
>> svn_cmdline_printf(pool, "%d: %s\n", id, label);
>> tools/dev/svnmover/svnmover.c:2439:5:
>> svn_stringbuf_from_stream(&text, src, 0, scratch_pool);
r1818318.
And on running the script I found a few more, calling a 'notify_func'
through a pointer...
subversion/libsvn_fs_fs/pack.c:2070
subversion/libsvn_fs_fs/pack.c:2125
subversion/libsvn_fs_fs/pack.c:2135
subversion/libsvn_fs_x/pack.c:2207
subversion/libsvn_fs_x/pack.c:2261
r1818320.
That's the lot.
- Julian
Re: 1.10 task: error leaks
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> The release branch creation checkpoints calls for checking for error
> leaks, so here's the output of tools/dev/warn-unused-result.sh
[...]
> ...: warning: 'warn_unused_result' attribute only applies to functions, methods, and classes [-Wignored-attributes]
This kind of warning is when the script inappropriately puts the
attribute on a variable declaration, so we can ignore these.
> subversion/libsvn_subr/stream.c:1471:9:
> svn_checksum_ctx_reset(btn->read_ctx);
> subversion/libsvn_subr/stream.c:1474:9:
> svn_checksum_ctx_reset(btn->write_ctx);
These are the only two callers of this new-for-1.10 API:
/**
* Reset an existing checksum @a ctx to initial state.
* @see svn_checksum_ctx_create()
*
* @since New in 1.10.
*/
svn_error_t *
svn_checksum_ctx_reset(svn_checksum_ctx_t *ctx);
One possible solution would be to drop the error return.
I have taken the more conservative option of adding SVN_ERR() around the
calls, in r1818307.
> subversion/libsvn_client/patch.c:2148:7:
> svn_stream_printf(target->reject_stream,
r1818309.
> subversion/libsvn_fs_fs/low_level.c:673:5:
> svn_stream_puts(stream, "\n");
> subversion/libsvn_fs_x/changes.c:187:5:
> append_change(changes, APR_ARRAY_IDX(list, i, svn_fs_x__change_t *));
> subversion/libsvn_fs_x/low_level.c:1134:5:
> svn_stream_puts(stream, "\n");
> subversion/libsvn_fs_x/transaction.c:1262:11:
> svn_io_dir_make(txn_dir, APR_OS_DEFAULT, iterpool);
> subversion/libsvn_wc/wc_db.c:16528:15:
> lock_remove_txn(queue->wcroot, cqi->local_relpath, work_item,
> subversion/svn/conflict-callbacks.c:1657:11:
> svn_cmdline_fprintf(stderr, iterpool, "%s\n",
> subversion/svn/notify.c:213:3:
> svn_hash_keys(conflicted_paths, all_conflicts, result_pool);
> subversion/svnfsfs/load-index-cmd.c:150:7:
> svn_stream_readline(input, &line, "\n", &eol, iterpool);
r1818310.
> subversion/tests/libsvn_repos/authz-test.c:280:5:
> print_user_rights(NULL, NULL, 0, &authz->anon_rights, pool);
> subversion/tests/libsvn_repos/authz-test.c:282:5:
> print_user_rights(NULL, NULL, 0, &authz->authn_rights, pool);
> subversion/tests/libsvn_client/conflicts-test.c:543:7:
> sbox_wc_move(b, move_src_path, new_dir_path);
> subversion/tests/libsvn_repos/dump-load-test.c:84:3:
> svn_stream_close(stream);
> subversion/tests/libsvn_repos/dump-load-test.c:134:3:
> svn_stream_close(stream);
> subversion/tests/libsvn_fs_fs/fs-fs-pack-test.c:1634:3:
> svn_hash_write2(props, svn_stream_from_stringbuf(hash_rep, pool), "END",
> subversion/tests/libsvn_subr/mergeinfo-test.c:1765:6:
> svn_rangelist_to_string(&tmp_string, rangelist, pool);
> subversion/tests/libsvn_subr/priority-queue-test.c:128:3:
> verify_empty_queue(queue);
> subversion/tests/libsvn_subr/priority-queue-test.c:157:3:
> verify_empty_queue(queue);
> subversion/tests/libsvn_subr/priority-queue-test.c:217:3:
> verify_empty_queue(queue);
> tools/client-side/svnconflict/svnconflict.c:366:5:
> svn_cmdline_printf(pool, "text-conflict\n");
> tools/client-side/svnconflict/svnconflict.c:371:7:
> svn_cmdline_printf(pool, "prop-conflict: %s\n", propname);
> tools/client-side/svnconflict/svnconflict.c:383:7:
> svn_cmdline_printf(pool, "tree-conflict: %s %s\n",
> tools/client-side/svnconflict/svnconflict.c:404:7:
> svn_cmdline_printf(pool, "%d: %s\n", id, label);
> tools/dev/svnmover/svnmover.c:2439:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
> svn_stringbuf_from_stream(&text, src, 0, scratch_pool);
Still to do.
- Julian
Re: 1.10 task: error leaks
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
[...]
> svn_element__tree_set(branch->priv->element_tree, old_eid, NULL);
> svn_element__tree_set(branch->priv->element_tree, new_eid, element);
> svn_element__tree_set(branch->priv->element_tree, eid, element);
> svn_element__tree_set(new_subtree, new_subtree->root_eid,
[...]
I fixed those in r1817814 by removing the unused 'svn_error_t *' return
type.
(This reminds me the 'element' stuff and 'tools/dev/svnmover' is still
in trunk... unused, unfinished. I wonder if I should remove it or if I
need to do anything with it before 1.10.)
- Julian