Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 30 Nov 2012 10:39:23 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Konstantin Belousov <kostikbel@gmail.com>
Cc:        svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Eitan Adler <eadler@FreeBSD.org>, Jilles Tjoelker <jilles@stack.nl>
Subject:   Re: svn commit: r243665 - head/sbin/dump
Message-ID:  <20121130100508.R1242@besplex.bde.org>
In-Reply-To: <20121129205729.GN3013@kib.kiev.ua>
References:  <201211290516.qAT5GoT5098709@svn.freebsd.org> <20121129120147.GE3013@kib.kiev.ua> <20121129162412.GD29338@stack.nl> <20121129205729.GN3013@kib.kiev.ua>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 29 Nov 2012, Konstantin Belousov wrote:

> On Thu, Nov 29, 2012 at 05:24:12PM +0100, Jilles Tjoelker wrote:
>> On Thu, Nov 29, 2012 at 02:01:47PM +0200, Konstantin Belousov wrote:
>>> On Thu, Nov 29, 2012 at 05:16:50AM +0000, Eitan Adler wrote:
>>>> Log:
>>>>   Mark non-returning function as such
>>
>>>>   PR:		bin/172978
>>>>   Approved by:	cperciva
>>>>   MFC after:	3 days
>>
>>>> Modified:
>>>>   head/sbin/dump/dump.h
>>
>>>> Modified: head/sbin/dump/dump.h
>>>> ==============================================================================
>>>> --- head/sbin/dump/dump.h	Thu Nov 29 03:48:39 2012	(r243664)
>>>> +++ head/sbin/dump/dump.h	Thu Nov 29 05:16:50 2012	(r243665)
>>>> @@ -121,7 +121,7 @@ void	trewind(void);
>>>>  void	writerec(char *dp, int isspcl);
>>>>
>>>>  void	Exit(int status) __dead2;
>>>> -void	dumpabort(int signo);
>>>> +void	dumpabort(int signo) __dead2;
>>>>  void	dump_getfstab(void);
>>>>
>>>>  char	*rawname(char *cp);
>>
>>> What is the goal of this change ?

It seems reasonable.  The function is not static, so compilers can't always
see that it is no-return.  But I think most older uses of __dead2 for
static functions are like uses of the register keyword.

>> This has been discussed before in
>> http://lists.freebsd.org/pipermail/freebsd-current/2011-January/022179.html
>> and the rest of the thread. The result there seems that it is the least
>> bad thing to add __dead2 even on static functions that do not return to
>> reduce compiler/analyzer warnings and improve the generated code with
>> GCC 4.2.1 and GCC 4.5.
> But we are already in the shiny new world ? We should not care about so
> antique and unsupported compilers any more ?
> Or, at least, the commit message should have said that we do.
>
> I suppose that the worry about slightly unefficient code in the dump(8) would
> be good April 1 subject.
>>
>>> It is arguably backward. There is absolutely no use to mark signal
>>> handler as __dead2, and all other uses do not benefit from the
>>> redundand declaration.
>>
>> Although most of the uses of dumpabort() are in the same file as its
>> definition, GCC 4.2.1 is too dumb to detect its noreturn property
>> automatically.
>>
>> Furthermore, some of the uses are in separate files. In particular,
>> __dead2 on dumpabort() is required to mark or detect quit() as noreturn.
>>
>>> Also, being quite removed from the function definition, there is a
>>> chance that some future modification would make the attribute a lie.
>>
>> Both GCC and Clang generate a warning by default if it appears possible
>> for a noreturn function to return, so as long as -Werror remains enabled
>> it should be safe enough.
>
> The annotation is very much like comment which rephrase the code and thus is
> useless. But in this case, the attribute annotation does has a chance of
> changing the compiler behaviour.
>
> IMO, such changes shall not be done, and the already accumulated nonsense
> needs to be reverted.

The only problem that I see is that the change obfuscates the actual bugs:
- dumpabort() is a normal function that is used as a signal handler
- the use of dumpabort() as a signal handler is not just a style bug, since
   it is very async-signal unsafe and thus cannot be used as a (non-broken)
   signal handler.

Similar bugs were fixed in the less-critical application dd(1).  Even in
4.4BSD, the signal handler summaryx() was safe (it used and still uses
a combination of snprintf() and write()), and the only fix needed was
to chane exit() to _exit() in the signal handler terminate().  dumpabort()
does much more than exit() in some cases, so it is harder to fix.  dd was
presumably more correct since signals occur in normal operation so races
in the signal handling would have been more obvious.

I'm not happy with some other fixes for unsafe signal handlers.  The ones
in ping and top are still broken AFAIK.  Just setting a flag in the signal
and checking the in the main loop doesn't work if any syscall is restarted.
In top, the read() syscall is restarted, so top is broken for signal that
occur in its read() loop (e.g., for responding to the prompt for the 's'
command).  So SA_RESTART should be kept clear for most signals.  But then
every syscall has to check for EINTR.  This is a burden and is not done
by top, although it is probably manageable for top (there aren't any (?)
syscalls other than the one in the read() loop to check). ping is more
careful and keeps SA_RESTART clear, but it is still broken when the resolver
library restarts.  Libraries in general might not handle the EINTR's from
!SA_RESTART properly, but in the resolver library the problem is sort of
the opposite: at least in very old versions that use select(), EINTR is
normal but the resolvier library doesn't want to return for EINTR, so it
restarts at the library level and thus the main loop in ping is never
reached in another way.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121130100508.R1242>