attribute warn_unused_result

attribute warn_unused_result

[Eli Zaretskii]

fencepost.gnu.org was upgraded to GCC 4.4.3-4ubuntu5 and glibc 2.11,
and that combination causes GCC to emit several warnings like these
while compiling Emacs:

  sysdep.c: In function 'sys_subshell':
  sysdep.c:550: warning: ignoring return value of 'chdir', declared with attribute warn_unused_result
  sysdep.c:581: warning: ignoring return value of 'write', declared with attribute warn_unused_result
  fileio.c: In function 'Fcopy_file':
  fileio.c:1968: warning: ignoring return value of 'fchown', declared with attribute warn_unused_result

Do we care about these warnings?  There's significant controversy
about them (e.g., the warnings about `write' are when we write an
error message to stderr, in which case there's nothing useful one can
do with the return value), so I'm not sure we should care.

If we do care about this, we should fix the code; if not, we should
either add "-Wno-unused-result" to the compilation switches, or use
`#pragma GCC diagnostic ignored "-Wunused-result"' in the affected
source files.

Re: attribute warn_unused_result

[Stefan Monnier]

> Do we care about these warnings?  There's significant controversy
> about them (e.g., the warnings about `write' are when we write an
> error message to stderr, in which case there's nothing useful one can
> do with the return value), so I'm not sure we should care.

I think the right thing to do is to adjust the code so as to make it
clear to the compiler that we thought about the issue and decided that
we really do want to ignore the return value.

So, for each such case, we should think about it and if we indeed want
to ignore the return value, we should put an explicit cast to that
effect (which should hopefully be understood by gcc to silence the
warning).


        Stefan

Re: attribute warn_unused_result

[Paul Eggert]

On 02/03/11 10:53, Stefan Monnier wrote:

> So, for each such case, we should think about it and if we indeed want
> to ignore the return value, we should put an explicit cast to that
> effect (which should hopefully be understood by gcc to silence the
> warning).

Gnulib's ignore-value module is designed for that.  I installed
the following: it suppresses the first warning in Eli's list that really
ought to be suppressed.  We can suppress the others as they
come up.  (I'm omitting auto-regenerated changes in this email.)


=== modified file 'ChangeLog'
--- ChangeLog	2011-01-31 23:54:50 +0000
+++ ChangeLog	2011-02-03 19:26:58 +0000
@@ -1,3 +1,9 @@
+2011-02-03  Paul Eggert  <eggert@cs.ucla.edu>
+
+	allow C code to suppress warnings about ignored return values
+	* Makefile.in (GNULIB_MODULES): Add ignore-value.
+	* configure, lib/Makefile.in, lib/gnulib.mk, m4/gl-comp.m4: Regenerate.
+
 2011-01-31  Chong Yidong  <cyd@stupidchicken.com>
 
 	* configure.in: Test existence of xaw3d library, not just the

=== modified file 'Makefile.in'
--- Makefile.in	2011-01-30 23:34:18 +0000
+++ Makefile.in	2011-02-03 19:14:20 +0000
@@ -330,7 +330,7 @@
 # Update modules from gnulib, for maintainers, who should have it in
 # $(gnulib_srcdir) (relative to $(srcdir) and should have build tools
 # as per $(gnulib_srcdir)/DEPENDENCIES.
-GNULIB_MODULES = dtoastr getopt-gnu mktime strftime
+GNULIB_MODULES = dtoastr getopt-gnu ignore-value mktime strftime
 GNULIB_TOOL_FLAGS = \
  --import --no-changelog --no-vc-files --makefile-name=gnulib.mk
 sync-from-gnulib: $(gnulib_srcdir)

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2011-02-03 13:46:03 +0000
+++ src/ChangeLog	2011-02-03 19:25:14 +0000
@@ -1,3 +1,16 @@
+2011-02-03  Paul Eggert  <eggert@cs.ucla.edu>
+
+	allow C code to suppress warnings about ignored return values
+
+	We need to go through the code and for each such warning, either
+	fix the code to pay attention to the returned value, or tell GCC
+	that we really do want to ignore the returned value.  Here is one
+	example of how to do the latter.
+	* sysdep.c: Include <ignore-value.h>.
+	(sys_subshell): Suppress an undesirable warning about not checking
+	the returned value of 'write', as there's nothing useful one can
+	do with that returned value.
+
 2011-02-03  Jan Djärv  <jan.h.d@swipnet.se>
 
 	* xterm.c (x_connection_closed): Remove all calls that calls

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2011-01-25 04:08:28 +0000
+++ src/sysdep.c	2011-02-03 19:16:45 +0000
@@ -31,6 +31,8 @@
 #endif /* HAVE_LIMITS_H */
 #include <unistd.h>
 
+#include <ignore-value.h>
+
 #include "lisp.h"
 #include "sysselect.h"
 #include "blockinput.h"
@@ -263,7 +265,7 @@
 init_baud_rate (int fd)
 {
   int emacs_ospeed;
- 
+
   if (noninteractive)
     emacs_ospeed = 0;
   else
@@ -578,7 +580,7 @@
 	write (1, "Can't execute subshell", 22);
 #else   /* not WINDOWSNT */
       execlp (sh, sh, (char *) 0);
-      write (1, "Can't execute subshell", 22);
+      ignore_value (write (1, "Can't execute subshell", 22));
       _exit (1);
 #endif  /* not WINDOWSNT */
 #endif /* not MSDOS */
@@ -3058,4 +3060,3 @@
 }
 
 #endif	/* !defined (WINDOWSNT) */
-

Re: attribute warn_unused_result

[Chad Brown]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]


On Feb 3, 2011, at 11:33 AM, Paul Eggert wrote:
> 
> Gnulib's ignore-value module is designed for that.  I installed
> the following:

..and it broke the W32 and nextstep builds (at least) again.

I appreciate the ideas behind the gnulib inclusion, but it seems to be causing
a lot more broken builds than we saw before.  What can we do to fix this?

I get the general impression via the 50+ message thread about DOS port
compatibility that the gnulib people are mostly unwilling to make efforts
to avoid breaking the emacs build.  Is this the case, or are we simply seeing
bad luck?

So far, the features of gnulib have not seemed to be worth the trouble.

*Chad

[-- Attachment #2: Type: text/html, Size: 1101 bytes --]

Re: attribute warn_unused_result

[Eli Zaretskii]

> From: Chad Brown <yandros@MIT.EDU>
> Date: Thu, 3 Feb 2011 12:42:35 -0800
> Cc: Paul Eggert <eggert@cs.ucla.edu>
> 
> On Feb 3, 2011, at 11:33 AM, Paul Eggert wrote:
> > 
> > Gnulib's ignore-value module is designed for that.  I installed
> > the following:
> 
> ..and it broke the W32 and nextstep builds (at least) again.

The GNU/Linux build is also broken.

Paul, you didn't install the ignore-value.h header itself.  OTOH, you
did commit unrelated changes to texinfo.tex.  Some bzr snafu?

Re: attribute warn_unused_result

[Paul Eggert]

On 02/03/11 13:30, Eli Zaretskii wrote:
> Paul, you didn't install the ignore-value.h header itself.  OTOH, you
> did commit unrelated changes to texinfo.tex.  Some bzr snafu?

Those were two separate commits to my branch, that happened
to be merged at the same time into the trunk.  The changes
weren't completely unrelated, as they both resulted from the
same sync from gnulib.  In the future I'll try to split these
things up better.

On 02/03/11 13:47, Lennart Borgman wrote:
> So what is wrong here, Paul?

Before, I was pretty cautious, and checked out everything
separately, doing a complete build and "make dist" and so forth,
before committing a change.

However, I was told I was being overly conservative
and inefficient, that this caution was getting in the way
making it convenient to do the Windows port,
and that it's better to just do a bzr merge
without the final check.  So I started doing that today;
but I broke things because I forgot to do a "bzr add"
before the merge.

Perhaps I should go back to being cautious.  :-)

Re: attribute warn_unused_result

[Lennart Borgman]

On Thu, Feb 3, 2011 at 10:58 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
>
> On 02/03/11 13:47, Lennart Borgman wrote:
>> So what is wrong here, Paul?
>
> Before, I was pretty cautious, and checked out everything
> separately, doing a complete build and "make dist" and so forth,
> before committing a change.
>
> However, I was told I was being overly conservative
> and inefficient, that this caution was getting in the way
> making it convenient to do the Windows port,
> and that it's better to just do a bzr merge
> without the final check.  So I started doing that today;
> but I broke things because I forgot to do a "bzr add"
> before the merge.
>
> Perhaps I should go back to being cautious.  :-)


My impression is that you have not been too cautious. Rather I believe
you might have underestimated the problem with complexity. This is
quite human. Experience help, but it can perhaps be best understood as
a logical/mathematical problem.

Could you please really consider the alternative of doing this things
on a separate branch?

It will be no big problem for me checking out such a branch and just
compiling it. (And it is easy providing a script for others to use
doing the same thing.)

Re: attribute warn_unused_result

[Eli Zaretskii]

> Date: Thu, 03 Feb 2011 13:58:40 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Chad Brown <yandros@MIT.EDU>, emacs-devel@gnu.org
> 
> Before, I was pretty cautious, and checked out everything
> separately, doing a complete build and "make dist" and so forth,
> before committing a change.
> 
> However, I was told I was being overly conservative
> and inefficient, that this caution was getting in the way
> making it convenient to do the Windows port,
> and that it's better to just do a bzr merge
> without the final check.  So I started doing that today;
> but I broke things because I forgot to do a "bzr add"
> before the merge.
> 
> Perhaps I should go back to being cautious.  :-)

Cautious, yes, but I don't think extreme measures such as "make dist"
are required (and how would that tell you that texinfo.tex is being
part of the merge commit, anyway?).

Here's what I normally do after I finish development (and testing) of
some change on a branch, and before I merge onto the trunk:

  . Run "bzr status".

  . Review every file marked "modified", "added", "removed",
    "renamed", and "kind changed", and make sure all of them are meant
    to be part of the changeset.

  . Review every file marked "unknown", and make sure none of them
    should be "bzr add"ed.

  . Make sure that every directory that has any of the above files has
    also a ChangeLog that is "modified" and is part of the changeset.

  . Run "bzr diff -rsubmit:", and review the diffs to make sure the
    changes I'm about to merge are what I expect.

  . If several local commits to the branch were done since the last
    merge, run "bzr log --line" to see their commit messages, and make
    sure all of them are indeed parts of a single changeset.

  . Merge, run "configure && make", test, and make sure everything
    builds and works on the trunk as well.

This procedure takes only a few moments, and is very efficient in
catching mistakes before they are propagated to the remote
repository.  In some rare cases, the remote repository gets commits
while I build and test the merged code, and I need a "bzr up" before
committing.  But the changes are usually in unrelated places, so no
additional testing is required, and I can proceed with an immediate
commit of my own.  (To eliminate almost all possible conflicts in
ChangeLog files, I use the changelog_merge plugin.)

IOW, it isn't about being over-cautious, it's about using efficiently
the tools we have at hand.  Granted, I wasn't born with the above
knowledge; it took some time to learn and get used to these
facilities.  I post it here in the hope this procedure will be useful
to others.

Re: attribute warn_unused_result

[Chong Yidong]

Eli Zaretskii <eliz@gnu.org> writes:

> Here's what I normally do after I finish development (and testing) of
> some change on a branch, and before I merge onto the trunk:
>
>   . Run "bzr status".
>
>   . Review every file marked "modified", "added", "removed",
>     "renamed", and "kind changed", and make sure all of them are meant
>     to be part of the changeset.
> ...
>
>   . Run "bzr diff -rsubmit:", and review the diffs to make sure the
>     changes I'm about to merge are what I expect.
> ...

Or, use VC-dir together with `C-x v D' (vc-root-diff).

Re: attribute warn_unused_result

[Paul Eggert]

On 02/03/11 12:42, Chad Brown wrote:

> ..and it broke the W32 and nextstep builds (at least) again.

Sorry, I forgot to add the new file lib/ignore-value.h.
That should be fixed now.

On 02/03/11 13:16, Eli Zaretskii wrote:

> And is butt-ugly, IMO.

I don't like ignore-value either.  I'd rather that we simply
tell GCC never to generate those bogus warnings anywhere.
However, as I understand it, that's not so easily done.

By the way, I forgot to say, I audited Emacs last month for
every instance of these warnings under GNU/Linux, and fixed
all the code where the warnings were legitimate.  This was
bzr commit 102952, dated January 23.  All the remaining
warnings (on GNU/Linux, anyway) were therefore bogus, in
my opinion, though it wouldn't hurt to have other people
check that.

Re: attribute warn_unused_result

[Eli Zaretskii]

> Date: Thu, 03 Feb 2011 13:40:21 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Emacs-Devel devel <emacs-devel@gnu.org>, Eli Zaretskii <eliz@gnu.org>
> 
> On 02/03/11 12:42, Chad Brown wrote:
> 
> > ..and it broke the W32 and nextstep builds (at least) again.
> 
> Sorry, I forgot to add the new file lib/ignore-value.h.
> That should be fixed now.
> 
> On 02/03/11 13:16, Eli Zaretskii wrote:
> 
> > And is butt-ugly, IMO.
> 
> I don't like ignore-value either.  I'd rather that we simply
> tell GCC never to generate those bogus warnings anywhere.
> However, as I understand it, that's not so easily done.

The -Wno-unused-result compiler options looks easy to me, and is IMO
cleaner than using ignore-value.h.  Heck, even the corresponding
pragma in the affected source files looks cleaner.

The only advantage of ignore-value.h is that it will work with
compilers other than GCC (if some of them also insist on this
warning).

Re: attribute warn_unused_result

[Paul Eggert]

On 02/04/2011 12:41 AM, Eli Zaretskii wrote:
> The -Wno-unused-result compiler options looks easy to me, and is IMO
> cleaner than using ignore-value.h.  Heck, even the corresponding
> pragma in the affected source files looks cleaner.

Unfortunately those approaches shut off all instances
of the warnings, even the useful ones.  I wouldn't have
found the eight bugs fixed in BZR 102952 if I hadn't
had those warnings.

Like I said, though, I don't much like ignore_value,
and either of the above approaches would also be fine
with me.

Re: attribute warn_unused_result

[Stefan Monnier]

> I don't like ignore-value either.

Shouldn't a cast to void be sufficient to tell gcc to not report
a particular case?
If so, there's no need to drag in gnulib's ignore_value.


        Stefan

Re: attribute warn_unused_result

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Chad Brown <yandros@MIT.EDU>,  Eli Zaretskii <eliz@gnu.org>,  Emacs-Devel devel <emacs-devel@gnu.org>
> Date: Fri, 04 Feb 2011 16:05:18 -0500
> 
> > I don't like ignore-value either.
> 
> Shouldn't a cast to void be sufficient to tell gcc to not report
> a particular case?

No, it isn't.  The warning remains.

Re: attribute warn_unused_result

[Lennart Borgman]

On Thu, Feb 3, 2011 at 9:42 PM, Chad Brown <yandros@mit.edu> wrote:
>
> On Feb 3, 2011, at 11:33 AM, Paul Eggert wrote:
>
> Gnulib's ignore-value module is designed for that.  I installed
> the following:
>
> ..and it broke the W32 and nextstep builds (at least) again.


So what is wrong here, Paul? We have suggested that you make a
separate branch for this. Why don't you do that?

Re: attribute warn_unused_result

[Stefan Monnier]

>> Gnulib's ignore-value module is designed for that.  I installed
>> the following:
>> ..and it broke the W32 and nextstep builds (at least) again.
> So what is wrong here, Paul? We have suggested that you make a
> separate branch for this. Why don't you do that?

AFAIK this last change introduced a bug, just like any other commit can
(and sometimes does) introduce a bug because some simple omission that's
easily fixed.  I.e. it's very different from the other cases where he
knew before that his change was going to break the w32 build.
IIUC people are reacting too strongly to this last problem because it
just happens to follow the other ones, but really it's a completely
different case.  I myself break the build on a regular basis as well, as
you all know too well.

BTW, I'd still like to see someone setup a build farm that can be used
to keep track of "the last revision that bootstraps", so people who
don't want to bump into such problems can follow that branch instead
of trunk.


        Stefan

Re: attribute warn_unused_result

[Lennart Borgman]

On Fri, Feb 4, 2011 at 10:08 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>>> Gnulib's ignore-value module is designed for that.  I installed
>>> the following:
>>> ..and it broke the W32 and nextstep builds (at least) again.
>> So what is wrong here, Paul? We have suggested that you make a
>> separate branch for this. Why don't you do that?
>
> AFAIK this last change introduced a bug, just like any other commit can
> (and sometimes does) introduce a bug because some simple omission that's
> easily fixed.  I.e. it's very different from the other cases where he
> knew before that his change was going to break the w32 build.
> IIUC people are reacting too strongly to this last problem because it
> just happens to follow the other ones, but really it's a completely
> different case.  I myself break the build on a regular basis as well, as
> you all know too well.

Yes, I realized that. A bit too late.

It would have been much easier to realize that with a little bit more
feedback. Mind reading is difficult face to face and even a little bit
more difficult over the net.


> BTW, I'd still like to see someone setup a build farm that can be used
> to keep track of "the last revision that bootstraps", so people who
> don't want to bump into such problems can follow that branch instead
> of trunk.

For me that would indeed be good especially when I need to test some
patch (which happens too often, actually).

Re: attribute warn_unused_result

[Eli Zaretskii]

> From: Lennart Borgman <lennart.borgman@gmail.com>
> Date: Fri, 4 Feb 2011 22:15:36 +0100
> Cc: Chad Brown <yandros@mit.edu>, Emacs-Devel devel <emacs-devel@gnu.org>,
> 	Paul Eggert <eggert@cs.ucla.edu>
> 
> It would have been much easier to realize that with a little bit more
> feedback. Mind reading is difficult face to face and even a little bit
> more difficult over the net.

I see no need for mind reading.  If you looked at the report displayed
by "bzr up" or "bzr log", you'd have realized that the file about
which the compiler complained was not part of the commit, which should
have told you what was the nature of the problem.

IOW, before you make a decision, look at the facts at hand.

Re: attribute warn_unused_result

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 04 Feb 2011 16:08:55 -0500
> Cc: Chad Brown <yandros@mit.edu>, Emacs-Devel devel <emacs-devel@gnu.org>,
> 	Paul Eggert <eggert@cs.ucla.edu>
> 
> BTW, I'd still like to see someone setup a build farm that can be used
> to keep track of "the last revision that bootstraps", so people who
> don't want to bump into such problems can follow that branch instead
> of trunk.

+1

Joakim said he is going to set that up, but that it will take him some
time.

Re: attribute warn_unused_result

[Andy Moreton]

On Thu 03 Feb 2011, Chad Brown wrote:

> On Feb 3, 2011, at 11:33 AM, Paul Eggert wrote:
>> 
>> Gnulib's ignore-value module is designed for that.  I installed the
>> following:
>
> ..and it broke the W32 and nextstep builds (at least) again.
>
> I appreciate the ideas behind the gnulib inclusion, but it seems to be
> causing a lot more broken builds than we saw before.  What can we do
> to fix this?

1) Prevail upon the developers in question to work in a more consensual
style and publicise changes that will break non-POSIX builds *before*
installing them.

2) Work on fixing the w32 build system so it is less hand-crafted and
makes more use of the POSIX build machinery. Given the number of
different environments and toolchains used to build for win32 targets,
this is tricky.

> I get the general impression via the 50+ message thread about DOS port
> compatibility that the gnulib people are mostly unwilling to make
> efforts to avoid breaking the emacs build.  Is this the case, or are
> we simply seeing bad luck?
>
> So far, the features of gnulib have not seemed to be worth the
> trouble.

As long as it does not restrict portability of emacs to platforms and
toolchains that gnulib want to support then there is a little temporary
pain and some longer term benefits.

If the two projects have divergent views on which platforms and
toolchains to support then that would indeed be a problem.

    AndyM

Re: attribute warn_unused_result

[Paul Eggert]

On 02/03/11 14:08, Andy Moreton wrote:
> If the two projects have divergent views on which platforms and
> toolchains to support then that would indeed be a problem.

So far, the only point of divergence that we've identified
is over 8+3 file name support.  The gnulib folks have made it
reasonably clear that they don't want to be restricted
to an 8+3 filename namespace for their source file names.

Right now this is not a problem with gnulib and Emacs,
since the file names imported from gnulib happen not to clash.
It is a problem with some other files in the Emacs trunk, though.
I've proposed to address this by modifying the MS-DOS build
procedure rather than by renaming the files; see
<http://lists.gnu.org/archive/html/emacs-devel/2011-01/msg01182.html>.
The goal is to allow non-MS-DOS developers to stop worrying about the
8+3 problem, for names of files imported from gnulib
and for other similar file names.

Re: attribute warn_unused_result

[Eli Zaretskii]

> Date: Thu, 03 Feb 2011 11:33:03 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> On 02/03/11 10:53, Stefan Monnier wrote:
> 
> > So, for each such case, we should think about it and if we indeed want
> > to ignore the return value, we should put an explicit cast to that
> > effect (which should hopefully be understood by gcc to silence the
> > warning).
> 
> Gnulib's ignore-value module is designed for that.

And is butt-ugly, IMO.

Re: attribute warn_unused_result

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Thu, 03 Feb 2011 13:53:25 -0500
> 
> > Do we care about these warnings?  There's significant controversy
> > about them (e.g., the warnings about `write' are when we write an
> > error message to stderr, in which case there's nothing useful one can
> > do with the return value), so I'm not sure we should care.
> 
> I think the right thing to do is to adjust the code so as to make it
> clear to the compiler that we thought about the issue and decided that
> we really do want to ignore the return value.

You can't do that, not with this warning.

> So, for each such case, we should think about it and if we indeed want
> to ignore the return value, we should put an explicit cast to that
> effect (which should hopefully be understood by gcc to silence the
> warning).

It isn't.

Re: attribute warn_unused_result

[Paul Eggert]

On 02/03/11 13:14, Eli Zaretskii wrote:
>> From: Stefan Monnier <monnier@iro.umontreal.ca>

>> I think the right thing to do is to adjust the code so as to make it
>> clear to the compiler that we thought about the issue and decided that
>> we really do want to ignore the return value.
> 
> You can't do that, not with this warning.

Actually, you can do it.  In the current trunk, the warning is
ignored for one particular case, documented by wrapping the
call in question by ignore_value (CALL).  This is in
src/sysdep.c, line 583.

We can wrap the other calls as needed.  I did just one call
for now, to illustrate the technique.

Re: attribute warn_unused_result

[Eli Zaretskii]

> Date: Thu, 03 Feb 2011 16:57:14 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: Stefan Monnier <monnier@iro.umontreal.ca>, emacs-devel@gnu.org
> 
> On 02/03/11 13:14, Eli Zaretskii wrote:
> >> From: Stefan Monnier <monnier@iro.umontreal.ca>
> 
> >> I think the right thing to do is to adjust the code so as to make it
> >> clear to the compiler that we thought about the issue and decided that
> >> we really do want to ignore the return value.
> > 
> > You can't do that, not with this warning.
> 
> Actually, you can do it.  In the current trunk, the warning is
> ignored for one particular case, documented by wrapping the
> call in question by ignore_value (CALL).  This is in
> src/sysdep.c, line 583.

I know, but that's not what I meant, and I imagine that's not what
Stefan meant, either.  It is a truism that you can avoid the warning
by adding a call to another function that is not declared with the
warn_unused_result attribute.  What I meant was that it seems to be
impossible to tell the compiler not to emit this warning by the
"usual" C technique of using a cast.

Btw, isn't it ironic that gnulib, a GNU project, and all other GNU
projects that use gnulib, need to jump through the hoops to work
around misfeatures in glibc, one of the most important and core GNU
projects?

Re: attribute warn_unused_result

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

Eli> Btw, isn't it ironic that gnulib, a GNU project, and all other GNU
Eli> projects that use gnulib, need to jump through the hoops to work
Eli> around misfeatures in glibc, one of the most important and core GNU
Eli> projects?

I think this problem is caused by a change in Ubuntu.  IIRC, it adds
-D_FORTIFY_SOURCE=2 by default.  With ordinary glibc and gcc, you must
ask for this setting, it is not the default.  See
https://wiki.ubuntu.com/CompilerFlags.

Tom

Re: attribute warn_unused_result

[Eli Zaretskii]

> From: Tom Tromey <tromey@redhat.com>
> Cc: Paul Eggert <eggert@cs.ucla.edu>, monnier@iro.umontreal.ca,
>         emacs-devel@gnu.org
> Date: Fri, 04 Feb 2011 08:50:45 -0700
> 
> I think this problem is caused by a change in Ubuntu.  IIRC, it adds
> -D_FORTIFY_SOURCE=2 by default.

Yep, looks like that, thanks.  I see this in "gcc -dumpspecs":

   %{!D_FORTIFY_SOURCE:%{!D_FORTIFY_SOURCE=*:%{!U_FORTIFY_SOURCE:-D_FORTIFY_SOURCE=2}}}

> With ordinary glibc and gcc, you must ask for this setting, it is
> not the default.  See https://wiki.ubuntu.com/CompilerFlags.

Except that this page says -D_FORTIFY_SOURCE=2 is in effect with -O2
or higher, but I also see the warning with -O1.  Only -O0 disables it.

Anyway, am I the only one who uses this version of Ubuntu?  If so,
sorry for the noise, and please disregard.  I will find some way to
get rid of it.

Re: attribute warn_unused_result

[Tom Tromey]

Eli> Except that this page says -D_FORTIFY_SOURCE=2 is in effect with -O2
Eli> or higher, but I also see the warning with -O1.  Only -O0 disables it.

I think that page is probably wrong.

In my libc's features.h, I see:

#if defined _FORTIFY_SOURCE && _FORTIFY_SOURCE > 0 \
    && defined __OPTIMIZE__ && __OPTIMIZE__ > 0
[...]

__OPTIMIZE__ is set to 1 if any optimization level > 0 is used.  I don't
think headers can distinguish the levels.  It could probably be done in
GCC specs, but I didn't look to see if the Ubuntu change attempts this.

Tom

Re: attribute warn_unused_result

[Paul Eggert]

On 02/04/11 08:38, Eli Zaretskii wrote:
> Anyway, am I the only one who uses this version of Ubuntu? 

No, I have observed the problem when I built Emacs on Ubuntu,
and it is a real annoyance.  This was on Ubuntu 10.10, the current
stable version.

By the way, I just now looked at one of the remaining instances
of this warning, a call to chdir whose return value is ignored,
and I now think it's a bug.  The bug is hard to trigger, but I'm
pretty sure I can exploit it to (say) unexpectly remove files in
one's home directory.

Is there ever a good reason to ignore the return value from
chdir?  If not, we should do a sweep of the code and
fix all of these.

Re: attribute warn_unused_result

[Eli Zaretskii]

> Date: Fri, 04 Feb 2011 16:11:36 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org
> 
> Is there ever a good reason to ignore the return value from
> chdir?

IMO, only if the chdir is part of handling a fatal error.

Re: attribute warn_unused_result

[Paul Eggert]

On 02/05/2011 08:35 AM, Chong Yidong wrote:
> If you fix this, please fix it in the emacs-23 branch (assuming the bug
> exists there), so that it will be included in the 23.3 release.

I could do that, but three things.  First, I've never installed
into the emacs-23 branch and am a bit worried about messing with that
if it is about to be released.  Second, I just now installed a fix for
mainline platforms into the trunk (see below) but don't know how
to fix similar problems on Windows, DOS, or NextStep.

Third, the problem doesn't occur in normal operation on
mainline platforms (on RHEL 5.5 I could reproduce it with temacs,
but not with a normal dumped emacs), so it may not be urgent
to put it into emacs-23.

don't ignore chdir failure
* sysdep.c (sys_subshell) [!defined DOS_NT]: Diagnose chdir
failure and exit.
(sys_subshell) [defined DOS_NT]: Mark with a FIXME the two
remaining unchecked chdir calls in this function; some DOS/NT
expert needs to fix them.
* emacs.c (main): Mark with a FIXME the unchecked chdir calls
in this function; some NextStep expert needs to fix them.
=== modified file 'src/emacs.c'
--- src/emacs.c	2011-01-31 08:12:52 +0000
+++ src/emacs.c	2011-02-06 01:23:24 +0000
@@ -1296,6 +1296,8 @@
 #ifdef NS_IMPL_COCOA
       if (skip_args < argc)
         {
+	  /* FIXME: Do the right thing if getenv returns NULL, or if
+	     chdir fails.  */
           if (!strncmp(argv[skip_args], "-psn", 4))
             {
               skip_args += 1;

=== modified file 'src/sysdep.c'
--- src/sysdep.c	2011-02-03 19:29:35 +0000
+++ src/sysdep.c	2011-02-06 01:23:24 +0000
@@ -548,8 +548,13 @@
 	sh = "sh";

       /* Use our buffer's default directory for the subshell.  */
-      if (str)
-	chdir ((char *) str);
+      if (str && chdir ((char *) str) != 0)
+	{
+#ifndef DOS_NT
+	  ignore_value (write (1, "Can't chdir\n", 12));
+	  _exit (1);
+#endif
+	}

       close_process_descs ();	/* Close Emacs's pipes/ptys */

@@ -567,7 +572,7 @@
 	    setenv ("PWD", str, 1);
 	  }
 	st = system (sh);
-	chdir (oldwd);
+	chdir (oldwd);	/* FIXME: Do the right thing on chdir failure.  */
 	if (epwd)
 	  putenv (old_pwd);	/* restore previous value */
       }
@@ -575,7 +580,7 @@
 #ifdef  WINDOWSNT
       /* Waits for process completion */
       pid = _spawnlp (_P_WAIT, sh, sh, NULL);
-      chdir (oldwd);
+      chdir (oldwd);	/* FIXME: Do the right thing on chdir failure.  */
       if (pid == -1)
 	write (1, "Can't execute subshell", 22);
 #else   /* not WINDOWSNT */

Re: attribute warn_unused_result

[Eli Zaretskii]

> Date: Sat, 05 Feb 2011 17:34:49 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: emacs-devel@gnu.org
> 
> On 02/05/2011 08:35 AM, Chong Yidong wrote:
> > If you fix this, please fix it in the emacs-23 branch (assuming the bug
> > exists there), so that it will be included in the 23.3 release.
> 
> I could do that, but three things.  First, I've never installed
> into the emacs-23 branch and am a bit worried about messing with that
> if it is about to be released.

With bzr, each branch is in its own directory, preferably under the
shared repository (i.e., under the same parent as the trunk directory
and your feature branch).  The same command "bzr branch" that you
issued to clone the trunk will work for the branch, except that the
URL is different:

  bzr+ssh://eliz@bzr.savannah.gnu.org/emacs/emacs-23/

> Second, I just now installed a fix for mainline platforms into the
> trunk (see below) but don't know how to fix similar problems on
> Windows, DOS, or NextStep.

MS-DOS uses GCC, so it will work with lib/ignore-value.h.

MS-Windows already works with lib/ignore-value.h (your change in
sysdep.c already used it).  So there's no need to use #ifndef DOS_NT
in this case.

Re: attribute warn_unused_result

[Paul Eggert]

On 02/05/2011 08:07 PM, Eli Zaretskii wrote:
> MS-Windows already works with lib/ignore-value.h (your change in
> sysdep.c already used it).  So there's no need to use #ifndef DOS_NT
> in this case.

I don't see how the following patch could possibly work on Windows.
Although it would compile, surely it would break things badly if
the call to chdir () fails on Windows.  Have you tested it for
that case?

=== modified file 'src/sysdep.c'
--- src/sysdep.c        2011-02-06 01:25:41 +0000                               
+++ src/sysdep.c        2011-02-06 06:57:13 +0000                               
@@ -550,10 +550,8 @@ sys_subshell (void)
       /* Use our buffer's default directory for the subshell.  */              
       if (str && chdir ((char *) str) != 0)                                    
        {                                                                       
-#ifndef DOS_NT                                                                 
          ignore_value (write (1, "Can't chdir\n", 12));                        
          _exit (1);                                                            
-#endif                                                                         
        }                                                                       
                                                                                
       close_process_descs ();  /* Close Emacs's pipes/ptys */                  

Re: attribute warn_unused_result

[Eli Zaretskii]

> Date: Sat, 05 Feb 2011 23:04:04 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: cyd@stupidchicken.com, emacs-devel@gnu.org
> 
> On 02/05/2011 08:07 PM, Eli Zaretskii wrote:
> > MS-Windows already works with lib/ignore-value.h (your change in
> > sysdep.c already used it).  So there's no need to use #ifndef DOS_NT
> > in this case.
> 
> I don't see how the following patch could possibly work on Windows.

I don't see why not.

> Although it would compile, surely it would break things badly if
> the call to chdir () fails on Windows.

I'm not sure what you mean.  The only thing I could imagine is that
perhaps the call to `_exit' should be avoided on DOS_NT.  If that's
not what you meant, could you please tell what problem(s) did you have
in mind?  Perhaps I'm missing something important here.

Re: attribute warn_unused_result

[Paul Eggert]

On 02/06/2011 02:21 AM, Eli Zaretskii wrote:

> perhaps the call to `_exit' should be avoided on DOS_NT.

Yes, because it'll make the Emacs top level exit on
Windows.

The patch I installed does not change the behavior on
Windows, so the Windows-related bugs in the resulting
code are bugs that were already present.  They can be
fixed by someone with expertise in Windows and desire to
fix it (I expect that's you :-) when time permits.

Re: attribute warn_unused_result

[Eli Zaretskii]

> Date: Sun, 06 Feb 2011 10:58:59 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: cyd@stupidchicken.com, emacs-devel@gnu.org
> 
> The patch I installed does not change the behavior on
> Windows, so the Windows-related bugs in the resulting
> code are bugs that were already present.  They can be
> fixed by someone with expertise in Windows and desire to
> fix it (I expect that's you :-) when time permits.

I actually question the resulting behavior on Posix platforms: AFAIU,
Emacs will (almost silently) do nothing.  Is that a good UI?  Should
we perhaps signal an error instead?

Re: attribute warn_unused_result

[Paul Eggert]

On 02/06/2011 11:27 AM, Eli Zaretskii wrote:
> I actually question the resulting behavior on Posix platforms: AFAIU,
> Emacs will (almost silently) do nothing.  Is that a good UI?  Should
> we perhaps signal an error instead?

Emacs can't merely signal an error there, since it's in the child.

My recent change causes Emacs to report chdir failure the same way
it reports execlp failure.  If the former behavior isn't right,
the latter isn't either; and I expect that the latter behavior has
been there for decades.

For modern mainstream platforms the issue doesn't come up, as
this code is exercised only on weird POSIXish platforms that
can't suspend Emacs.  I last used a platform like that in
the 1980s, and I expect they're very low priority now.

For Windows things may well be different, but that
difference can be put into #ifdef DOS_NT code.

Re: attribute warn_unused_result

[Eli Zaretskii]

> Date: Sun, 06 Feb 2011 12:11:35 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: cyd@stupidchicken.com, emacs-devel@gnu.org
> 
> On 02/06/2011 11:27 AM, Eli Zaretskii wrote:
> > I actually question the resulting behavior on Posix platforms: AFAIU,
> > Emacs will (almost silently) do nothing.  Is that a good UI?  Should
> > we perhaps signal an error instead?
> 
> Emacs can't merely signal an error there, since it's in the child.

It can if we decide that it's TRT.

> My recent change causes Emacs to report chdir failure the same way
> it reports execlp failure.  If the former behavior isn't right,
> the latter isn't either; and I expect that the latter behavior has
> been there for decades.

I guess the probability of chdir to fail is not very high.

> For modern mainstream platforms the issue doesn't come up, as
> this code is exercised only on weird POSIXish platforms that
> can't suspend Emacs.  I last used a platform like that in
> the 1980s, and I expect they're very low priority now.

If there are no such platforms, perhaps all that function should
become DOS_NT only.

Re: attribute warn_unused_result

[Stefan Monnier]

> impossible to tell the compiler not to emit this warning by the
> "usual" C technique of using a cast.

Looks like a bug in gcc.  I'd be surprised if they accept this
qualification, but really what's the point of emitting a warning when
the user explicitly put a cast to void?

> Btw, isn't it ironic that gnulib, a GNU project, and all other GNU
> projects that use gnulib, need to jump through the hoops to work
> around misfeatures in glibc, one of the most important and core GNU
> projects?

I don't see how/why you'd consider it a misfeature of glibc.
The warning comes from gcc, AFAICT, and is generally a good one, it's
just that gcc should understand the standard way in C to say that you
want to discard a value.


        Stefan

Re: attribute warn_unused_result

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  emacs-devel@gnu.org
> Date: Fri, 04 Feb 2011 16:14:03 -0500
> 
> > impossible to tell the compiler not to emit this warning by the
> > "usual" C technique of using a cast.
> 
> Looks like a bug in gcc.  I'd be surprised if they accept this
> qualification, but really what's the point of emitting a warning when
> the user explicitly put a cast to void?

You can google about this: the GCC maintainers explicitly think this
isn't a bug, but a feature.  They say that the warning was provided to
uncover the cases where lazy programmers cast function calls to void
to avoid diagnostics, without considering the implications.  They say
that if you don't want this warning, you should not declare functions
with this attribute.

> I don't see how/why you'd consider it a misfeature of glibc.

See above.  It's glibc headers that declare the functions with this
attribute.

However, Tom pointed out that glibc only does that if the compiler is
invoked with -D_FORTIFY_SOURCE=2 or higher, so it's an optional
feature as far as glibc is concerned.  Ubuntu turns this option on,
because its specs for GCC include -D_FORTIFY_SOURCE=2, so it's
actually an issue with Ubuntu, not with glibc.

Personally, I think it's wrong to force _FORTIFY_SOURCE on all the
users of a general-purpose system, but that's me.

Re: attribute warn_unused_result

[Stefan Monnier]

>> > impossible to tell the compiler not to emit this warning by the
>> > "usual" C technique of using a cast.
>> Looks like a bug in gcc.  I'd be surprised if they accept this
>> qualification, but really what's the point of emitting a warning when
>> the user explicitly put a cast to void?
> You can google about this: the GCC maintainers explicitly think this
> isn't a bug, but a feature.  They say that the warning was provided to
> uncover the cases where lazy programmers cast function calls to void
> to avoid diagnostics, without considering the implications.

That's brain dead: lazy programmers don't bother to add a `void'.

> They say that if you don't want this warning, you should not declare
> functions with this attribute.

That's also brain dead: declaring functions with this attribute is
adding good information, so it should be done wherever possible and
should not come with any negative consequences such as "when you really
don't care about the result, then please obfuscate your code, oh and
keep an eye on it, because a future gcc version may try and be even more
clever in uncovering code obfuscation, so you'll have to obfuscate it
even more to avoid the warning".

Anyway... Richard, could you try and get the gcc people to their senses?

>> I don't see how/why you'd consider it a misfeature of glibc.
> See above.  It's glibc headers that declare the functions with this
> attribute.

And it's a good thing they do.

> Personally, I think it's wrong to force _FORTIFY_SOURCE on all the
> users of a general-purpose system, but that's me.

I think the wrong part is to force users to obfuscate their code.


        Stefan