You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Erik Huelsmann <eh...@gmail.com> on 2005/11/20 13:23:34 UTC
[wc-propcaching] Use of uninitialized variable (libsvn_wc/props.c)
I get a gcc warning about base_props being used uninitialized in
svn_wc__load_props.
I propose this patch to fix it:
Index: subversion/libsvn_wc/props.c
===================================================================
--- subversion/libsvn_wc/props.c (revision 17455)
+++ subversion/libsvn_wc/props.c (working copy)
@@ -387,11 +387,14 @@
if (base_props_p)
*base_props_p = base_props;
}
+ else
+ base_props = NULL;
if (props_p)
{
if (has_propcaching && ! entry->prop_mods && entry->has_props)
- *props_p = apr_hash_copy (pool, base_props);
+ *props_p = base_props
+ ? apr_hash_copy (pool, base_props) : apr_hash_make (pool);
else if (! has_propcaching || entry->has_props)
{
const char *prop_path;
bye,
Erik.
Re: Shutting up warnings (was: Re: [wc-propcaching] Use of uninitialized variable (libsvn_wc/props.c))
Posted by Erik Huelsmann <eh...@gmail.com>.
> > if (props_p)
> > {
> > if (has_propcaching && ! entry->prop_mods && entry->has_props)
> > - *props_p = apr_hash_copy (pool, base_props);
> > + *props_p = base_props
> > + ? apr_hash_copy (pool, base_props) : apr_hash_make (pool);
>
> If I'm not missing something (which might well be true, since I wrote this
> code), base_props can never be NULL here. I think it is confusing to add
> code that will never be executed. (An assert is better if that's desired.)
Hmm. Sorry, ofcourse you're right there.
>
>
> > else if (! has_propcaching || entry->has_props)
> > {
> > const char *prop_path;
bye,
Erik.
Re: Shutting up warnings
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 22 Nov 2005, Julian Foad wrote:
> Peter N. Lundblad wrote:
> > On Mon, 21 Nov 2005, Julian Foad wrote:
> >
> > Here we go. The problem seems to be that if you pass a variable by
> > reference to a function whihc is expected to fill in the variable value,
> > the compiler is pesimistic about this.
>
> > I can go fixing these if people
> > don't object. I'm not opposed to shutting up warnings per se. I more want
> > us to resolve the question before going on and doing this in particular
> > cases.
>
> I think it would definitely be wrong to initialise these variables just to
> avoid the warnings. It would be illogical since they are always being
> initialised by the existing code, and it could hide genuine errors as has been
> pointed out before.
>
I agree with this and think we should either be consistent in adding code
to silence these warnings or not doing it. Shutting warnings only for gcc3
is pointless.
> This is clearly the kind of warning that is suitable only for occasional use
> (when somebody feels like checking all of the reported possible problems), or
> perhaps for use on code where output parameters are specially marked so the
> compiler knows that's what they are. Just like the "Function argument is
> unused" warning that was discussed a few weeks ago.
>
Yes, it's not useful to us in routine builds.
> The same applies in theory to the case that I just told Eric he should "fix".
> (What's the word I need? "Indulge the compiler"?) I think I have just made up
> my mind that we should _not_ generally add code just to avoid these speculative
> kinds of warnings. Erik's particular case was the only remaining instance of a
> particular kind, so the benefit-to-clutter ratio was high. (I imagined several
> instances of that kind had existed and been "fixed", but I'm not sure.)
>
The problem with Erik's fix was that it adds code to set a value to NULL
that will never be used. That's confusing to me and that was why I reacted
in the first place.
So the conclusion seems to be that we shouldn't care about GCC's
uninitialized warnings, at least not until GCC gets smarter in this
regard. Did I interpret your comments correct, and do others agrre to
this?
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Shutting up warnings
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Mon, 21 Nov 2005, Julian Foad wrote:
>
>>Peter, for my interest, could you show me a few warnings of this type from gcc
>>4.0.2? I'd like to have a look at our code in those places, and try to see how
>>it differs.
>
> Here we go. The problem seems to be that if you pass a variable by
> reference to a function whihc is expected to fill in the variable value,
> the compiler is pesimistic about this.
Thanks. Yes, that seems to be what's new in GCC 4. GCC 3's "-Wuninitialized"
option doesn't act on variables addressed by reference (or structures, or
various other things).
> I can go fixing these if people
> don't object. I'm not opposed to shutting up warnings per se. I more want
> us to resolve the question before going on and doing this in particular
> cases.
I think it would definitely be wrong to initialise these variables just to
avoid the warnings. It would be illogical since they are always being
initialised by the existing code, and it could hide genuine errors as has been
pointed out before.
This is clearly the kind of warning that is suitable only for occasional use
(when somebody feels like checking all of the reported possible problems), or
perhaps for use on code where output parameters are specially marked so the
compiler knows that's what they are. Just like the "Function argument is
unused" warning that was discussed a few weeks ago.
The same applies in theory to the case that I just told Eric he should "fix".
(What's the word I need? "Indulge the compiler"?) I think I have just made up
my mind that we should _not_ generally add code just to avoid these speculative
kinds of warnings. Erik's particular case was the only remaining instance of a
particular kind, so the benefit-to-clutter ratio was high. (I imagined several
instances of that kind had existed and been "fixed", but I'm not sure.)
- Julian
> ../1.3.x/subversion/libsvn_delta/svndiff.c: In function 'decode_window':
> ../1.3.x/subversion/libsvn_delta/svndiff.c:339: warning: 'op.action_code' may be used uninitialized in this function
> ../1.3.x/subversion/libsvn_delta/svndiff.c:339: warning: 'op.offset' may be used uninitialized in this function
> ../1.3.x/subversion/libsvn_delta/svndiff.c:411: warning: 'ninst' may be used uninitialized in this function
[...]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Shutting up warnings
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 21 Nov 2005, Julian Foad wrote:
> Peter, for my interest, could you show me a few warnings of this type from gcc
> 4.0.2? I'd like to have a look at our code in those places, and try to see how
> it differs.
>
Here we go. The problem seems to be that if you pass a variable by
reference to a function whihc is expected to fill in the variable value,
the compiler is pesimistic about this. I can go fixing these if people
don't object. I'm not opposed to shutting up warnings per se. I more want
us to resolve the question before going on and doing this in particular
cases.
../1.3.x/subversion/libsvn_delta/svndiff.c: In function 'decode_window':
../1.3.x/subversion/libsvn_delta/svndiff.c:339: warning: 'op.action_code' may be used uninitialized in this function
../1.3.x/subversion/libsvn_delta/svndiff.c:339: warning: 'op.offset' may be used uninitialized in this function
../1.3.x/subversion/libsvn_delta/svndiff.c:411: warning: 'ninst' may be used uninitialized in this function
../1.3.x/subversion/libsvn_delta/svndiff.c: In function 'read_window_header':
../1.3.x/subversion/libsvn_delta/svndiff.c:658: warning: 'c' may be used uninitialized in this function
../1.3.x/subversion/libsvn_delta/svndiff.c:639: warning: 'c' may be used uninitialized in this function
../1.3.x/subversion/libsvn_delta/svndiff.c:639: warning: 'c' may be used uninitialized in this function
../1.3.x/subversion/libsvn_delta/svndiff.c:639: warning: 'c' may be used uninitialized in this function
../1.3.x/subversion/libsvn_delta/svndiff.c:639: warning: 'c' may be used uninitialized in this function
../1.3.x/subversion/libsvn_subr/path.c: In function 'svn_path_cstring_to_utf8':
../1.3.x/subversion/libsvn_subr/path.c:1236: warning: 'path_is_utf8' may be used uninitialized in this function
../1.3.x/subversion/libsvn_subr/path.c: In function 'svn_path_cstring_from_utf8':
../1.3.x/subversion/libsvn_subr/path.c:1219: warning: 'path_is_utf8' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/fs_fs.c: In function 'svn_fs_fs__rev_get_root':
../1.3.x/subversion/libsvn_fs_fs/fs_fs.c:1142: warning: 'root_id' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/fs_fs.c: In function 'rep_read_contents':
../1.3.x/subversion/libsvn_fs_fs/fs_fs.c:1483: warning: 'cwindow' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/fs_fs.c: In function 'read_representation':
../1.3.x/subversion/libsvn_fs_fs/fs_fs.c:1252: warning: 'rep_args' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/fs_fs.c: In function 'commit_body':
../1.3.x/subversion/libsvn_fs_fs/fs_fs.c:3852: warning: 'start_node_id' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/fs_fs.c:3852: warning: 'start_copy_id' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/revs-txns.c: In function 'svn_fs_fs__get_txn_ids':
../1.3.x/subversion/libsvn_fs_fs/revs-txns.c:131: warning: 'txn' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/tree.c: In function 'find_youngest_copyroot':
../1.3.x/subversion/libsvn_fs_fs/tree.c:2748: warning: 'path_parent' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/tree.c:2748: warning: 'path_parent' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/tree.c:2748: warning: 'path_parent' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/tree.c:2748: warning: 'path_parent' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/tree.c:2748: warning: 'path_parent' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/tree.c:2748: warning: 'path_parent' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/tree.c:2748: warning: 'path_parent' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_fs/tree.c:2748: warning: 'path_parent' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_base/revs-txns.c: In function 'svn_fs_base__set_rev_prop':
../1.3.x/subversion/libsvn_fs_base/revs-txns.c:243: warning: 'txn_id' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_base/revs-txns.c:242: warning: 'txn' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_base/revs-txns.c: In function 'svn_fs_base__rev_get_root':
../1.3.x/subversion/libsvn_fs_base/revs-txns.c:118: warning: 'txn' may be used uninitialized in this function
../1.3.x/subversion/libsvn_fs_base/revs-txns.c: In function 'txn_body_revision_proplist':
../1.3.x/subversion/libsvn_fs_base/revs-txns.c:178: warning: 'txn' may be used uninitialized in this function
../1.3.x/subversion/libsvn_repos/load.c: In function 'svn_repos_parse_dumpstream2':
../1.3.x/subversion/libsvn_repos/load.c:486: warning: 'version' may be used uninitialized in this function
../1.3.x/subversion/libsvn_repos/load.c:528: warning: 'actual_prop_length' may be used uninitialized in this function
../1.3.x/subversion/libsvn_ra_svn/client.c: In function 'ra_svn_open':
../1.3.x/subversion/libsvn_ra_svn/client.c:616: warning: 'args' may be used uninitialized in this function
../1.3.x/subversion/libsvn_ra_svn/client.c:614: warning: 'conn' may be used uninitialized in this function
../1.3.x/subversion/libsvn_wc/adm_files.c: In function 'svn_wc_ensure_adm2':
../1.3.x/subversion/libsvn_wc/adm_files.c:1119: warning: 'exists_already' may be used uninitialized in this function
../1.3.x/subversion/libsvn_ra_dav/session.c: In function 'svn_ra_dav__open':
../1.3.x/subversion/libsvn_ra_dav/session.c:665: warning: 'timeout' may be used uninitialized in this function
../1.3.x/subversion/libsvn_ra_dav/session.c:666: warning: 'debug' may be used uninitialized in this function
../1.3.x/subversion/libsvn_ra_dav/session.c: In function 'svn_ra_dav__lock':
../1.3.x/subversion/libsvn_ra_dav/session.c:1200: warning: 'lock' may be used uninitialized in this function
../1.3.x/subversion/clients/cmdline/lock-cmd.c: In function 'svn_cl__lock':
../1.3.x/subversion/clients/cmdline/lock-cmd.c:85: warning: 'comment' may be used uninitialized in this function
../1.3.x/subversion/svnadmin/main.c: In function 'subcommand_dump':
../1.3.x/subversion/svnadmin/main.c:581: warning: 'stdout_stream' may be used uninitialized in this function
../1.3.x/subversion/svnadmin/main.c: In function 'subcommand_load':
../1.3.x/subversion/svnadmin/main.c:671: warning: 'stdin_stream' may be used uninitialized in this function
../1.3.x/subversion/svnserve/serve.c: In function 'diff':
../1.3.x/subversion/svnserve/serve.c:1209: warning: 'versus_path' may be used uninitialized in this function
../1.3.x/subversion/svnserve/serve.c: In function 'switch_cmd':
../1.3.x/subversion/svnserve/serve.c:1166: warning: 'switch_path' may be used uninitialized in this function
../1.3.x/subversion/svnserve/serve.c: In function 'link_path':
../1.3.x/subversion/svnserve/serve.c:483: warning: 'fs_path' may be used uninitialized in this function
../1.3.x/subversion/tests/libsvn_delta/random-test.c: In function 'random_combine_test':
../1.3.x/subversion/tests/libsvn_delta/random-test.c:512: warning: 'seed' may be used uninitialized in this function
...
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Shutting up warnings
Posted by Julian Foad <ju...@btopenworld.com>.
Erik Huelsmann wrote:
> On 11/21/05, Julian Foad <ju...@btopenworld.com> wrote:
>
>>>On Sun, 20 Nov 2005, Erik Huelsmann wrote:
>>>
>>>>I get a gcc warning about base_props being used uninitialized in
>>>>svn_wc__load_props.
>>
>>Erik, if you "make clean" and compile the whole of Subversion, how many of that
>>type of warning do you get? Which gcc?
>
> After my last commit to trunk? Or were you talking about the branch?
Either; I just meant roughly how many in the whole of our source code.
> Trunk: 0 warnings with EXTRA_CFLAGS='-Wall -O2'
> wc-propcaching: 1 warning (the one I sent the patch about) with the same flags.
In that case I'd say "fix" it (silence it in the neatest way you can find).
This might not do any good for gcc v4 users, but it does for gcc v3 users.
> GCC version:
>
> [erik@bartje trunk]$ gcc -v
> gcc version 3.2.2 20030222 (Red Hat Linux 3.2.2-5)
Come to think of it, I'm getting the same (gcc 3.3.5, those and a few more
warnings enabled).
Peter, for my interest, could you show me a few warnings of this type from gcc
4.0.2? I'd like to have a look at our code in those places, and try to see how
it differs.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Shutting up warnings
Posted by Erik Huelsmann <eh...@gmail.com>.
On 11/21/05, Julian Foad <ju...@btopenworld.com> wrote:
> Peter N. Lundblad wrote:
> > On Sun, 20 Nov 2005, Erik Huelsmann wrote:
> >
> >>I get a gcc warning about base_props being used uninitialized in
> >>svn_wc__load_props.
>
> Erik, if you "make clean" and compile the whole of Subversion, how many of that
> type of warning do you get? Which gcc?
After my last commit to trunk? Or were you talking about the branch?
Trunk: 0 warnings with EXTRA_CFLAGS='-Wall -O2'
wc-propcaching: 1 warning (the one I sent the patch about) with the same flags.
GCC version:
[erik@bartje trunk]$ gcc -v
Reading specs from /usr/lib/gcc-lib/i386-redhat-linux/3.2.2/specs
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
--infodir=/usr/share/info --enable-shared --enable-threads=posix
--disable-checking --with-system-zlib --enable-__cxa_atexit
--host=i386-redhat-linux
Thread model: posix
gcc version 3.2.2 20030222 (Red Hat Linux 3.2.2-5)
Bye,
Erik.
Re: Shutting up warnings
Posted by Julian Foad <ju...@btopenworld.com>.
Peter N. Lundblad wrote:
> On Sun, 20 Nov 2005, Erik Huelsmann wrote:
>
>>I get a gcc warning about base_props being used uninitialized in
>>svn_wc__load_props.
Erik, if you "make clean" and compile the whole of Subversion, how many of that
type of warning do you get? Which gcc?
> With gcc 4.0.2 I gets a lot of such warnings all over the code. The topic
> of shutting up warnings has been up for discussion before and some people
> have objected because just setting a value to NULL (or some other bogus
> value) might just hide real bugs. Said more as a general comment to
> current threads. The below doesn't necessarily do that. Problem is that
> different compilers seem to produce this warning in different situations.
> In this case, the warning is bogus, AFAICT.
>
> > I propose this patch to fix it:
>
>
>>Index: subversion/libsvn_wc/props.c
>>===================================================================
>>--- subversion/libsvn_wc/props.c (revision 17455)
>>+++ subversion/libsvn_wc/props.c (working copy)
>>@@ -387,11 +387,14 @@
>> if (base_props_p)
>> *base_props_p = base_props;
>> }
>>+ else
>>+ base_props = NULL;
>>
>> if (props_p)
>> {
>> if (has_propcaching && ! entry->prop_mods && entry->has_props)
>>- *props_p = apr_hash_copy (pool, base_props);
>>+ *props_p = base_props
>>+ ? apr_hash_copy (pool, base_props) : apr_hash_make (pool);
>
> If I'm not missing something (which might well be true, since I wrote this
> code), base_props can never be NULL here. I think it is confusing to add
> code that will never be executed. (An assert is better if that's desired.)
Yes, it's definitely bad to add that second part of the patch if you know
"base_props" is never null there. (Yes, "assert" is better, but hardly
worthwhile here.[1])
The first part, making sure to initialise the variable (which could be done
either as shown or, less obtrusively, at its declaration) is a tough call. My
current feeling is that if there are lots of these cases as you say, then it's
probably not worth adding any code to shut up the warning: the ugliness would
outweigh the benefit. Disable the warning instead; it's one of those warnings
that is only useful for certain styles of coding.
- Julian
[1] "assert" is hardly worthwhile because the pointer is soon dereferenced here
anyway. The potential benefit of "assert" is that it's like documentation: the
programmer declares that the pointer is intended to be non-null. However,
dereferencing a pointer is really just as good a declaration of intent.
There's little reason for a reader to believe that the "assert" is any more
correct and up-to-date than the subsequent code that uses the pointer.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Shutting up warnings (was: Re: [wc-propcaching] Use of uninitialized
variable (libsvn_wc/props.c))
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 20 Nov 2005, Erik Huelsmann wrote:
> I get a gcc warning about base_props being used uninitialized in
> svn_wc__load_props.
With gcc 4.0.2 I gets a lot of such warnings all over the code. The topic
of shutting up warnings has been up for discussion before and some people
have objected because just setting a value to NULL (or some other bogus
value) might just hide real bugs. Said more as a general comment to
current threads. The below doesn't necessarily do that. Problem is that
different compilers seem to produce this warning in different situations.
In this case, the warning is bogus, AFAICT.
> I propose this patch to fix it:
> Index: subversion/libsvn_wc/props.c
> ===================================================================
> --- subversion/libsvn_wc/props.c (revision 17455)
> +++ subversion/libsvn_wc/props.c (working copy)
> @@ -387,11 +387,14 @@
> if (base_props_p)
> *base_props_p = base_props;
> }
> + else
> + base_props = NULL;
>
> if (props_p)
> {
> if (has_propcaching && ! entry->prop_mods && entry->has_props)
> - *props_p = apr_hash_copy (pool, base_props);
> + *props_p = base_props
> + ? apr_hash_copy (pool, base_props) : apr_hash_make (pool);
If I'm not missing something (which might well be true, since I wrote this
code), base_props can never be NULL here. I think it is confusing to add
code that will never be executed. (An assert is better if that's desired.)
> else if (! has_propcaching || entry->has_props)
> {
> const char *prop_path;
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org