You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2006/01/17 00:34:05 UTC
Re: svn commit: r18040 - in trunk/subversion: libsvn_delta
libsvn_fs_fs libsvn_ra_dav libsvn_ra_svn libsvn_repos libsvn_subr
libsvn_wc svn tests/libsvn_delta
dlr@tigris.org writes:
> Author: dlr
> Date: Tue Jan 10 13:20:20 2006
> New Revision: 18040
>
> Modified:
> trunk/subversion/libsvn_delta/svndiff.c
> trunk/subversion/libsvn_fs_fs/fs_fs.c
> trunk/subversion/libsvn_fs_fs/revs-txns.c
> trunk/subversion/libsvn_ra_dav/session.c
> trunk/subversion/libsvn_ra_svn/client.c
> trunk/subversion/libsvn_repos/load.c
> trunk/subversion/libsvn_subr/config_file.c
> trunk/subversion/libsvn_wc/adm_files.c
> trunk/subversion/svn/lock-cmd.c
> trunk/subversion/tests/libsvn_delta/random-test.c
>
> Log:
> Cleanse GCC 4.0.1 compilation warnings.
Only recent gcc's that give these warnings, I think it's a combination
of '-funit-at-a-time' being enabled, which allows the optimiser to
look into the body of static function calls, together with the
optimiser not being aware that svn_error_create always returns
non-NULL. However humans know about svn_error_create and we can see
that the "may be used" never happens and so the initialisation is
unnecessary. The drawback of the unnecessary initialisation is that
it might make it harder for humans to understand the code and it
definitely defeats valgrind which can detect such uses if they get
tested.
It's possible to stop gcc reporting these warnings with
'-Wno-uninitialized' although that will also stop the warnings that
are not 'false alarms'. It's possible to change the optimiser with
'-fno-unit-at-a-time' but that might reduce the optimisation. I don't
know if we can tell the optimiser about the non-NULL return of
svn_error_create, but I think not.
Maybe we are stuck with these initialisations, but it does make me a
little uneasy. I realise that you want to get rid of the warnings, a
warning free build is a very good thing, but it's possible that the
cure is worse than the disease.
> * subversion/libsvn_wc/adm_files.c
> (svn_wc_ensure_adm2): Initialize local variable "exists_already" to
> FALSE.
>
> * subversion/libsvn_subr/config_file.c
> (svn_config__parse_file): Initialize "ctx.ungotten_char" to NUL.
>
> * subversion/tests/libsvn_delta/random-test.c
> (random_combine_test): Initialize local variable "seed" to 0.
>
> * subversion/libsvn_repos/load.c
> (svn_repos_parse_dumpstream2): Initialize local variable "version"
> to SVN_REPOS_DUMPFILE_FORMAT_VERSION,
Is SVN_REPOS_DUMPFILE_FORMAT_VERSION a good value? Since the
initialization is unnecessary the actual value doesn't matter at
present, but were the code to change then falsely claiming to have a
valid version might be a bad idea. Something "invalid" such as
INT_MIN or -1 might be better.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Suppressing GCC's "uninitialised" warnings
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Čibej <br...@xbc.nu> writes:
> and b) surely a future version of GCC will do better in this
> case, especially if someone reports the bug to them (hint, hint!)
"Do better" probably requires GCC to do whole program optimisation.
Humans can determine that the warnings are "wrong" only by using the
knowledge that svn_error_create will never return NULL. GCC doesn't
emit the warnings if it has the same information, but generally the
definition of svn_error_create isn't even in the same library, let
alone the same translation unit:
$ gcc -Wall -O2 -c z.c
z.c: In function 'bar':
z.c:9: warning: 'i' is used uninitialized in this function
$ gcc -Wall -O2 -c -DVISIBLE z.c
$ cat z.c
typedef struct svn_error_t { int i; } svn_error_t;
extern svn_error_t *svn_error_create(void);
static svn_error_t *foo(int *p) { return svn_error_create(); }
svn_error_t *bar(void)
{
int i;
svn_error_t *err = foo(&i);
if (err) return err;
if (i) return svn_error_create();
return 0;
}
#ifdef VISIBLE
svn_error_t *svn_error_create(void) { static svn_error_t d; return &d; }
#endif
I'm not exactly sure what constitues the GCC "bug" beyond the fact
that the warning is a false alarm, and false alarms are why there is a
-Wno-initialized flag. I suppose we could ask for a way to label the
function declaration, __attribute__(nonullreturn) perhaps?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Suppressing GCC's "uninitialised" warnings [was: svn commit:
r18040 ...]
Posted by Branko Čibej <br...@xbc.nu>.
Julian Foad wrote:
> Daniel Rall wrote:
>>
>> I didn't realize that CFLAGS already contained -Wall. It's not
>> explicitly added by our configure process, but I see it in Makefile.
>> Perhaps it's inherited from httpd or APR.
>>
>> Using CFLAGS instead of an override which always turns off that type
>> of warning is easy enough (see below), but it seemed less appropriate
>> given the situation (since people can still easily shoot themself in
>> the foot). What do you think?
>
> This warning only seems to occur in this situation (the r18040
> situation: passing "&var" to a local function which either initialises
> "var" or returns an error) if the option "-funit-at-a-time" is given,
> as Philip mentioned. Furthermore it is only a problem when "-Werror"
> is also given. But note well that the same warning can be produced in
> other situations, such as simple use of the value of a variable before
> it is initialised.
>
> I think maybe trying to suppress "uninitialised" warnings is trying to
> be too clever. It's nice to provide a warning-free build when invoked
> with just the default options, but if someone is adding extra flags it
> is up to them to specify the flags they need to get the effect they want.
>
> Suppressing all "uninitialised" warnings, even when "-funit-at-a-time"
> and "-Werror" are not given, may hurt more people than it helps.
> Therefore I think we should not do this.
I agree. If for no other reason, then because a) there are lots of
versions of GCC out there, only some of which are affected by this
problem; and b) surely a future version of GCC will do better in this
case, especially if someone reports the bug to them (hint, hint!)
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Suppressing GCC's "uninitialised" warnings [was: svn commit: r18040
...]
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Rall wrote:
>
> I didn't realize that CFLAGS already contained -Wall. It's not
> explicitly added by our configure process, but I see it in Makefile.
> Perhaps it's inherited from httpd or APR.
>
> Using CFLAGS instead of an override which always turns off that type
> of warning is easy enough (see below), but it seemed less appropriate
> given the situation (since people can still easily shoot themself in
> the foot). What do you think?
This warning only seems to occur in this situation (the r18040 situation:
passing "&var" to a local function which either initialises "var" or returns an
error) if the option "-funit-at-a-time" is given, as Philip mentioned.
Furthermore it is only a problem when "-Werror" is also given. But note well
that the same warning can be produced in other situations, such as simple use
of the value of a variable before it is initialised.
I think maybe trying to suppress "uninitialised" warnings is trying to be too
clever. It's nice to provide a warning-free build when invoked with just the
default options, but if someone is adding extra flags it is up to them to
specify the flags they need to get the effect they want.
Suppressing all "uninitialised" warnings, even when "-funit-at-a-time" and
"-Werror" are not given, may hurt more people than it helps. Therefore I think
we should not do this.
- Julian
> [[[
> Disable GCC 4.0.1's inconsistent "uninitialized" warnings, which
> otherwise cause build failures when using its '-Wall -Werror' flags.
> A follow-up to the r18040/r18161 commit/revert.
>
> * configure.in
> (CFLAGS): Add new -Wno-uninitialized compilation flag onto the end
> of this argument list when the compiler (CC) is gcc.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18040 - in trunk/subversion: libsvn_delta libsvn_fs_fs libsvn_ra_dav libsvn_ra_svn libsvn_repos libsvn_subr libsvn_wc svn tests/libsvn_delta
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 18 Jan 2006, Philip Martin wrote:
> Daniel Rall <dl...@collab.net> writes:
>
> > On Wed, 18 Jan 2006, Philip Martin wrote:
> >
> >> Daniel Rall <dl...@collab.net> writes:
> >>
> >> > * configure.in
> >> > (OVERRIDE_CFLAGS): Add new substitution used when the compiler (CC)
> >> > is gcc, tacking "-Wno-uninitialized" onto the end of the
> >> > compilation flags. This additional substitution is necessary
> >> > because GCC 4.0.1 takes argument ordering of such flags into
> >> > consideration, and the flags may need to be positioned after
> >> > EXTRA_CFLAGS to take affect.
> >>
> >> Why should -Wno-uninitialized override EXTRA_CFLAGS? I thought that
> >> EXTRA_CFLAGS was *intended* to override the other CFLAGS.
> >
> > Because the command-line positioning of gcc's compilation flags
> > impacts their interpretation, '-Wall -Werror -Wno-uninitialized'
> > works, while '-Wno-uninitialized -Wall -Werror' fails (I assume
> > because -Wall clobbers -Wno-uninitialized).
>
> I know that, but I don't see why that means -Wno-uninitialized in the
> Makefile has to override EXTRA_CFLAGS. EXTRA_CFLAGS is the way a user
> can override whatever is in the Makefile, you are making it impossible
> to override -Wno-uninitialized.
If EXTRA_CFLAGS includes -Wall, any -Wno-uninitialized flag added to
CFLAGS will be ignored by GCC, and result in compilation warnings.
This is especially annoying if EXTRA_CFLAGS includes -Werror, which
turns those warnings into build failures.
Given that Subversion's source code, when compiled using some version
of GCC, will always generate seemingly spurious warnings, it didn't
seem unreasonable to supress those warnings (regardless of
EXTRA_CFLAGS).
> If configure is going to put both -Wall and -Wno-uninitialized in the
> Makefile then it needs to ensure that they are in the right order, but
> they should both go before EXTRA_CFLAGS.
I didn't realize that CFLAGS already contained -Wall. It's not
explicitly added by our configure process, but I see it in Makefile.
Perhaps it's inherited from httpd or APR.
Using CFLAGS instead of an override which always turns off that type
of warning is easy enough (see below), but it seemed less appropriate
given the situation (since people can still easily shoot themself in
the foot). What do you think?
[[[
Disable GCC 4.0.1's inconsistent "uninitialized" warnings, which
otherwise cause build failures when using its '-Wall -Werror' flags.
A follow-up to the r18040/r18161 commit/revert.
* configure.in
(CFLAGS): Add new -Wno-uninitialized compilation flag onto the end
of this argument list when the compiler (CC) is gcc.
Suggested by: philip
lundblad
]]]
Index: configure.in
===================================================================
--- configure.in (revision 18163)
+++ configure.in (working copy)
@@ -694,6 +694,16 @@
# ==== Miscellaneous bits ====================================================
+if test "$CC" = "gcc" ; then
+ # Turn off compilation warning known to cause problems with
+ # Subversion's source and GCC 4.0.1.
+ if echo "$CFLAGS" | grep '\-Wall' > /dev/null; then
+ if echo "$CFLAGS" | grep -v '\-Wno-uninitialized' > /dev/null; then
+ CFLAGS="$CFLAGS -Wno-uninitialized"
+ fi
+ fi
+fi
+
dnl Since this is used only on Unix-y systems, define the path separator as '/'
AC_DEFINE_UNQUOTED(SVN_PATH_LOCAL_SEPARATOR, '/',
[Defined to be the path separator used on your local filesystem])
Re: svn commit: r18040 - in trunk/subversion: libsvn_delta
libsvn_fs_fs libsvn_ra_dav libsvn_ra_svn libsvn_repos libsvn_subr
libsvn_wc svn tests/libsvn_delta
Posted by Philip Martin <ph...@codematters.co.uk>.
Daniel Rall <dl...@collab.net> writes:
> On Wed, 18 Jan 2006, Philip Martin wrote:
>
>> Daniel Rall <dl...@collab.net> writes:
>>
>> > * configure.in
>> > (OVERRIDE_CFLAGS): Add new substitution used when the compiler (CC)
>> > is gcc, tacking "-Wno-uninitialized" onto the end of the
>> > compilation flags. This additional substitution is necessary
>> > because GCC 4.0.1 takes argument ordering of such flags into
>> > consideration, and the flags may need to be positioned after
>> > EXTRA_CFLAGS to take affect.
>>
>> Why should -Wno-uninitialized override EXTRA_CFLAGS? I thought that
>> EXTRA_CFLAGS was *intended* to override the other CFLAGS.
>
> Because the command-line positioning of gcc's compilation flags
> impacts their interpretation, '-Wall -Werror -Wno-uninitialized'
> works, while '-Wno-uninitialized -Wall -Werror' fails (I assume
> because -Wall clobbers -Wno-uninitialized).
I know that, but I don't see why that means -Wno-uninitialized in the
Makefile has to override EXTRA_CFLAGS. EXTRA_CFLAGS is the way a user
can override whatever is in the Makefile, you are making it impossible
to override -Wno-uninitialized.
If configure is going to put both -Wall and -Wno-uninitialized in the
Makefile then it needs to ensure that they are in the right order, but
they should both go before EXTRA_CFLAGS.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18040 - in trunk/subversion: libsvn_delta libsvn_fs_fs libsvn_ra_dav libsvn_ra_svn libsvn_repos libsvn_subr libsvn_wc svn tests/libsvn_delta
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 18 Jan 2006, Philip Martin wrote:
> Daniel Rall <dl...@collab.net> writes:
>
> > * configure.in
> > (OVERRIDE_CFLAGS): Add new substitution used when the compiler (CC)
> > is gcc, tacking "-Wno-uninitialized" onto the end of the
> > compilation flags. This additional substitution is necessary
> > because GCC 4.0.1 takes argument ordering of such flags into
> > consideration, and the flags may need to be positioned after
> > EXTRA_CFLAGS to take affect.
>
> Why should -Wno-uninitialized override EXTRA_CFLAGS? I thought that
> EXTRA_CFLAGS was *intended* to override the other CFLAGS.
Because the command-line positioning of gcc's compilation flags
impacts their interpretation, '-Wall -Werror -Wno-uninitialized'
works, while '-Wno-uninitialized -Wall -Werror' fails (I assume
because -Wall clobbers -Wno-uninitialized).
--
Daniel Rall
Re: svn commit: r18040 - in trunk/subversion: libsvn_delta
libsvn_fs_fs libsvn_ra_dav libsvn_ra_svn libsvn_repos libsvn_subr
libsvn_wc svn tests/libsvn_delta
Posted by Philip Martin <ph...@codematters.co.uk>.
Daniel Rall <dl...@collab.net> writes:
> * configure.in
> (OVERRIDE_CFLAGS): Add new substitution used when the compiler (CC)
> is gcc, tacking "-Wno-uninitialized" onto the end of the
> compilation flags. This additional substitution is necessary
> because GCC 4.0.1 takes argument ordering of such flags into
> consideration, and the flags may need to be positioned after
> EXTRA_CFLAGS to take affect.
Why should -Wno-uninitialized override EXTRA_CFLAGS? I thought that
EXTRA_CFLAGS was *intended* to override the other CFLAGS.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18040 - in trunk/subversion: libsvn_delta libsvn_fs_fs libsvn_ra_dav libsvn_ra_svn libsvn_repos libsvn_subr libsvn_wc svn tests/libsvn_delta
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 17 Jan 2006, Peter N. Lundblad wrote:
> On Tue, 17 Jan 2006, Philip Martin wrote:
>
> > definitely defeats valgrind which can detect such uses if they get
> > tested.
> >
> > It's possible to stop gcc reporting these warnings with
> > '-Wno-uninitialized' although that will also stop the warnings that
> > are not 'false alarms'. It's possible to change the optimiser with
> > '-fno-unit-at-a-time' but that might reduce the optimisation. I don't
> > know if we can tell the optimiser about the non-NULL return of
> > svn_error_create, but I think not.
> >
> > Maybe we are stuck with these initialisations, but it does make me a
> ...
>
> As I indicated in another mail, I think disabling this warning is the
> solution. Having valgrind indicate uninitialized data is much more
> valuable than relying on this unreliable GCC warning (even though valgrind
> is not used as much as it should). The worst thing is having to
> initialize "out parameters" to an arbitrary value before calling a
> function.
>
> And, since this is most often a false alarm, the natural reaction after
> some time will be to just add an initializer, possibly missing a real
> error.
I reverted r18040 in r18161. Based on your feedback, I propose the
following patch to our build system to account for GCC's
idiosyncrasies WRT Subversion's source code.
[[[
Disable GCC 4.0.1's inconsistent "uninitialized" warnings, which
otherwise cause build failures when using its '-Wall -Werror' flags.
A follow-up to the r18040/r18161 commit/revert.
* Makefile.in
(CFLAGS): Append "configure" substitution OVERRIDE_CFLAGS after
EXTRA_CFLAGS.
* configure.in
(OVERRIDE_CFLAGS): Add new substitution used when the compiler (CC)
is gcc, tacking "-Wno-uninitialized" onto the end of the
compilation flags. This additional substitution is necessary
because GCC 4.0.1 takes argument ordering of such flags into
consideration, and the flags may need to be positioned after
EXTRA_CFLAGS to take affect.
Suggested by: philip
lundblad
]]]
Index: Makefile.in
===================================================================
--- Makefile.in (revision 18161)
+++ Makefile.in (working copy)
@@ -127,7 +127,7 @@
MKDIR = @MKDIR@
# The EXTRA_ parameters can be used to pass extra flags at 'make' time.
-CFLAGS = @CFLAGS@ $(EXTRA_CFLAGS)
+CFLAGS = @CFLAGS@ $(EXTRA_CFLAGS) @OVERRIDE_CFLAGS@
### A few of the CFLAGS (e.g. -Wmissing-prototypes, -Wstrict-prototypes,
### -Wmissing-declarations) are not valid for C++, and should be somehow
### suppressed (but they may come from httpd or APR).
Index: configure.in
===================================================================
--- configure.in (revision 18162)
+++ configure.in (working copy)
@@ -694,6 +694,19 @@
# ==== Miscellaneous bits ====================================================
+if test "$CC" = "gcc" ; then
+ # Turn off compilation warning known to cause problems with
+ # Subversion's source and GCC 4.0.1.
+ if echo "$CFLAGS" | grep -v '\-Wno-uninitialized' > /dev/null; then
+ dnl We use OVERRIDE_CFLAGS instead of vanilla CFLAGS (positioned
+ dnl after $(EXTRA_CFLAGS) in Makefile.in), because argument
+ dnl ordering matters to GCC, and our supplied flags must take
+ dnl precedence.
+ OVERRIDE_CFLAGS='-Wno-uninitialized'
+ AC_SUBST(OVERRIDE_CFLAGS)
+ fi
+fi
+
dnl Since this is used only on Unix-y systems, define the path separator as '/'
AC_DEFINE_UNQUOTED(SVN_PATH_LOCAL_SEPARATOR, '/',
[Defined to be the path separator used on your local filesystem])
Re: svn commit: r18040 - in trunk/subversion: libsvn_delta libsvn_fs_fs
libsvn_ra_dav libsvn_ra_svn libsvn_repos libsvn_subr libsvn_wc svn
tests/libsvn_delta
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 17 Jan 2006, Philip Martin wrote:
> definitely defeats valgrind which can detect such uses if they get
> tested.
>
> It's possible to stop gcc reporting these warnings with
> '-Wno-uninitialized' although that will also stop the warnings that
> are not 'false alarms'. It's possible to change the optimiser with
> '-fno-unit-at-a-time' but that might reduce the optimisation. I don't
> know if we can tell the optimiser about the non-NULL return of
> svn_error_create, but I think not.
>
> Maybe we are stuck with these initialisations, but it does make me a
...
As I indicated in another mail, I think disabling this warning is the
solution. Having valgrind indicate uninitialized data is much more
valuable than relying on this unreliable GCC warning (even though valgrind
is not used as much as it should). The worst thing is having to
initialize "out parameters" to an arbitrary value before calling a
function.
And, since this is most often a false alarm, the natural reaction after
some time will be to just add an initializer, possibly missing a real
error.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org