Fix signal handler

Fix signal handler

[Markus Elfring]

Hello,

The function "early_output" that is set as a signal handler by the
function "setup_early_output" contains a simple looking instruction.
http://git.kernel.org/?p=git/git.git;a=blob;f=builtin-log.c;h=8d16832f7e9483f7903009459a72efc39e267c98;hb=HEAD#l173

A global variable gets a function pointer assigned.
http://git.kernel.org/?p=git/git.git;a=blob;f=revision.h;h=a14deefc252bd641fba5e16f7859b4a985a72578;hb=HEAD#l138

I find that this approach does not fit to standard rules because the
data type "sig_atomic_t" is the only type that can be safely used for
global write access in signal handlers.
https://www.securecoding.cert.org/confluence/display/seccode/SIG31-C.+Do+not+access+or+modify+shared+objects+in+signal+handlers

Would you like to change any details in the design of your software
because of this issue to avoid undefined behaviour?

Regards,
Markus

Re: Fix signal handler

[Jeff King]

On Tue, Feb 02, 2010 at 05:14:23PM +0100, Markus Elfring wrote:

> The function "early_output" that is set as a signal handler by the
> function "setup_early_output" contains a simple looking instruction.
> [...]
> A global variable gets a function pointer assigned.
> [...]
> I find that this approach does not fit to standard rules because the
> data type "sig_atomic_t" is the only type that can be safely used for
> global write access in signal handlers.

No, it's not a sig_atomic_t, but it is assignment of a single function
pointer that is properly declared as volatile. Is this actually a
problem on any known system?

If you want to nit-pick, there are much worse cases. For example, in
diff.c, we do quite a bit of work in remove_tempfile_on_signal. It
assumes that char* assignment is atomic, but nothing is even marked as
volatile. But again, is this actually a problem on any system?

You will find that most git developers care about real problems that can
be demonstrated on real systems. Standards can be a useful guide, but
they can be too loose (e.g., we run on some non-POSIX systems) as well
as too restrictive. What matters is what actually runs in practice.

If you can demonstrate a practical problem and provide a patch, then I
am sure people would be happy to read it.

-Peff

Re: Fix signal handler

[Markus Elfring]

>
> No, it's not a sig_atomic_t, but it is assignment of a single function
> pointer that is properly declared as volatile. Is this actually a
> problem on any known system?
>   

Is it guaranteed to work on all supported software environments that an
address can be atomically set?


> If you want to nit-pick, there are much worse cases. For example, in
> diff.c, we do quite a bit of work in remove_tempfile_on_signal.
>   

Thanks that you point out another open issue.


> It assumes that char* assignment is atomic, but nothing is even marked as
> volatile. But again, is this actually a problem on any system?
>   

Would you like to provide software implementations that work by design?

Regards,
Markus

Re: Fix signal handler

[Jeff King]

On Tue, Feb 02, 2010 at 10:44:37PM +0100, Markus Elfring wrote:

> > No, it's not a sig_atomic_t, but it is assignment of a single function
> > pointer that is properly declared as volatile. Is this actually a
> > problem on any known system?
> 
> Is it guaranteed to work on all supported software environments that an
> address can be atomically set?

I think you are missing my point. We are not coding to a set of
standards that provide guarantees. We are coding to a practical set of
real-world implementations that people try to run git on and produce bug
reports for. I do not think anyone on this list could even enumerate a
complete a list of the "supported software environments" of git.

We try to be conservative about portability issues. Some things are
obviously wrong. But other things may violate the letter of some
standards, and yet work in practice on all of the platforms people are
interested in running git on.

I don't think anyone here is much interested in whether there is any
sort of guarantee on a particular construct working. What we do care
about is whether there is an actual problem on some platform that enough
people care about to justify rewriting the code to handle it.

So to answer your question, I honestly don't know. The code may well be
broken on common platforms and it is simply a race condition that has
never come up. But I do know that it has not been a common source of bug
reports, which makes me not want to spend time investigating it when
nobody has demonstrated its incorrectness beyond mentioning a standards
document.  Especially when that time could be better spent fixing other
bugs.

-Peff

Re: Fix signal handler

[Markus Elfring]

> I don't think anyone here is much interested in whether there is any
> sort of guarantee on a particular construct working.

That is a pity. - I would expect that professional software development
will build on working specifications instead of potentially undefined
behaviour.


> So to answer your question, I honestly don't know. The code may well
> be broken on common platforms and it is simply a race condition that
> has never come up. But I do know that it has not been a common source
> of bug reports, which makes me not want to spend time investigating
> it when nobody has demonstrated its incorrectness beyond mentioning
> a standards document.
>   

Thanks for your clarification.

I find that programming errors in this area might be hard to identify
from the outside because resulting race conditions and deadlocks fall
into the symptom category of heisenbugs, don't they?
How many software developers do deal with the nasty design details for
signal handler implementations correctly?

Regards,
Markus

Re: Fix signal handler

[Jeff King]

On Wed, Feb 03, 2010 at 11:20:30AM +0100, Markus Elfring wrote:

> > I don't think anyone here is much interested in whether there is any
> > sort of guarantee on a particular construct working.
> 
> That is a pity. - I would expect that professional software development
> will build on working specifications instead of potentially undefined
> behaviour.

I think it is simply impractical. It's not that we're ignoring a
specification, it's that there _isn't_ a concrete specification for the
set of systems we're interested in.

> > So to answer your question, I honestly don't know. The code may well
> > be broken on common platforms and it is simply a race condition that
> > has never come up. But I do know that it has not been a common source
> > of bug reports, which makes me not want to spend time investigating
> > it when nobody has demonstrated its incorrectness beyond mentioning
> > a standards document.
> [...]
> I find that programming errors in this area might be hard to identify
> from the outside because resulting race conditions and deadlocks fall
> into the symptom category of heisenbugs, don't they?

Yes, they can be hard to identify from the outside. But if you are
interested in addressing the situation, I am suggesting that the first
step would be to demonstrate that there in fact _is_ a race condition,
and it is not simply some theoretical problem.

-Peff

Re: Fix signal handler

[Markus Elfring]

>
> I think it is simply impractical.

I have got the opposite opinion.


> It's not that we're ignoring a specification, it's that there _isn't_
> a concrete specification for the set of systems we're interested in.
>   

I have got doubts on your view. Known specifications are available for
POSIX and corresponding programming languages like C and C++. I know
that they have got open issues on their own because a few important
wordings are not as precise and clear as you might prefer.

For which software environments do you miss programming standards?

How many efforts would you like to spend on conditional compilation for
"special" platforms?


>
> But if you are interested in addressing the situation, I am suggesting
> that the first step would be to demonstrate that there in fact _is_ a
> race condition, and it is not simply some theoretical problem.
>   

I try to point out this open issue once more.

Can you imagine any unwanted results if the desired address can not be
atomically set in a way that fulfils requirements for signal handler
implementations?
Can it happen that the assigned function pointer will become a dangling
pointer because of word-tearing?


Another "dangerous" use case:
Do you know if any function is called during the execution in a signal
handling context that is not async-signal-safe like "fprintf()"?

Regards,
Markus

Re: Fix signal handler

[Thomas Rast]

On Wednesday 03 February 2010 12:55:51 Markus Elfring wrote:
> [Jeff King wrote:]
> >
> > I think it is simply impractical.
> 
> I have got the opposite opinion.

You've been wasting two regular contributors' (Avery and Peff) time on
issues they point out are impractical to fix, while demonstrating that
you have enough standards and C knowledge to do the fix yourself.

So why don't you post patches (either fixes or testcases exhibiting
the issue) instead of more mails containing the same points?  This is,
after all, an open source project.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: Fix signal handler

[Jeff King]

On Wed, Feb 03, 2010 at 12:55:51PM +0100, Markus Elfring wrote:

> > It's not that we're ignoring a specification, it's that there _isn't_
> > a concrete specification for the set of systems we're interested in.
> 
> I have got doubts on your view. Known specifications are available for
> POSIX and corresponding programming languages like C and C++. I know
> that they have got open issues on their own because a few important
> wordings are not as precise and clear as you might prefer.
> 
> For which software environments do you miss programming standards?

Off the top of my head, I have seen mention of git running on Linux,
{Free,Net,Open}BSD, Solaris 7-10, OpenSolaris, AIX 5.x and 6.x, HP-UX,
Windows, SCO OpenServer, and SCO UnixWare. Not all of those are POSIX,
and I am not sure even if they were that we could (or would want to)
stick to a strict subset of POSIX. If all of those systems allow less
strict behavior, then what is the problem in taking advantage of it if
it makes development or maintenance of the code easier?

Ditto for C. We mostly stick to C89, but there are many parts of the C89
standard where behavior may be implementation defined or even undefined,
but in practice work just fine. I am not interested in spending a lot of
effort working around those issues just to meet the letter of the
standard if there is no practical system on which it matters.

> How many efforts would you like to spend on conditional compilation for
> "special" platforms?

If I haven't made that clear, _I_ don't want to spend any effort. If
_you_ are concerned about it, feel free to make a patch. If your patch
is not too intrusive, and especially if you can demonstrate that it is a
problem on a real-world system, then I think your patch would be
considered for inclusion upstream.

If you are not willing to spend any effort on these problems, and
instead are trying to direct _my_ priorities, then I have no interest in
listening to what you have to say (and I suspect that goes for the rest
of the regular git developers, too).

-Peff

Re: Fix signal handler

[Markus Elfring]

>
> So why don't you post patches (either fixes or testcases exhibiting
> the issue) instead of more mails containing the same points?
>   

I try to get a feeling about acceptance for update suggestions before
they can be expressed in the target programming language.
The "correct" wording in the source code might become more work than you
might agree with.

Regards,
Markus

Re: Fix signal handler

[Shawn O. Pearce]

Markus Elfring <Markus.Elfring@web.de> wrote:
> >
> > So why don't you post patches (either fixes or testcases exhibiting
> > the issue) instead of more mails containing the same points?
> >   
> 
> I try to get a feeling about acceptance for update suggestions before
> they can be expressed in the target programming language.

We've been burned too many times by people who drive-by and demand
we fix X, without showing proof that X is a problem, or offering
a patch to resolve whatever X they claim is an issue.  Each such
time burns existing well-known contributor time.

We've also been burned too many times by well-known contributors
posting "Hey, we should do Y" and then never actually writing the
code themselves.  I know, I've done it.

Pie-in-the-sky discussions serve no purpose, and just waste
everyone's time.


If its worthwhile, write the damn code, and share it.  If its not
worth your time to write the code in order to propose the idea,
its not worth our time to listen.

I've never kill-filed _ANYONE_ on this mailing list before.  You are
>.< this close to making me go figure out how to setup a kill file
on my domain just so I can stop receving all emails from you.


-- 
Shawn.

Re: Fix signal handler

[Andreas Ericsson]

On 02/03/2010 04:46 PM, Markus Elfring wrote:
> 
>>
>> So why don't you post patches (either fixes or testcases exhibiting
>> the issue) instead of more mails containing the same points?
>>
> 
> I try to get a feeling about acceptance for update suggestions before
> they can be expressed in the target programming language.

The general feeling on this list is that patches are listened to, no
matter how foul they are, and you will get a (hopefully) polite
rejection if it is considered useless because it addresses a problem
that doesn't exist.

Suggestions that others do a lot of work is generally not listened
to.

> The "correct" wording in the source code might become more work than you
> might agree with.
> 

Since you're the only one really interested in this, you'll be the
one doing the work. If you do that work well and submit your patch(es)
by the git patch submission standards, screening them for useless
code churn is but a moments work.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: Fix signal handler

[Markus Elfring]

>
> If your patch is not too intrusive, and especially if you can demonstrate
> that it is a problem on a real-world system, then I think your patch would
> be considered for inclusion upstream.
>   

I have got the feeling that my corresponding update suggestion (in
source code form) would become intrusive to some degree. If I do not get
an indication that issues from word-tearing in signal handlers is a
mentionable problem here, I assume that your acceptance is low for
potential fixes from every software developer (including me).

Regards,
Markus

Re: Fix signal handler

[Markus Elfring]

>
> The general feeling on this list is that patches are listened to, no
> matter how foul they are, and you will get a (hopefully) polite
> rejection if it is considered useless because it addresses a problem
> that doesn't exist.
>   

I hope that a healthy balance will be found between correct software
design, development and quick "hacking". There might also be more
efforts if too many patches will be rejected just because the suggested
and planned changes were not discussed before.

Would you like to get an acknowledgement for signal handler problems
from people in other discussion groups like "comp.programming.threads"?

Regards,
Markus

Re: Fix signal handler

[Bill Lear]

On Wednesday, February 3, 2010 at 17:04:12 (+0100) Markus Elfring writes:
>> If your patch is not too intrusive, and especially if you can demonstrate
>> that it is a problem on a real-world system, then I think your patch would
>> be considered for inclusion upstream.
>>   
>I have got the feeling that my corresponding update suggestion (in
>source code form) would become intrusive to some degree. If I do not get
>an indication that issues from word-tearing in signal handlers is a
>mentionable problem here, I assume that your acceptance is low for
>potential fixes from every software developer (including me).

Do the words "insufferable !@#$!%%$" mean anything to you?  I mean,
seriously, you are WELCOME TO SUBMIT A PATCH FOR THIS.  Do it!  Go
ahead, write it!  We would love to see it.  Fix things, make things
better!  Don't continue to whine and waste people's time.  ALL PATCHES
ARE INTRUSIVE.  JUST SUBMIT A PATCH!


Bill

Re: Fix signal handler

[Andreas Ericsson]

On 02/03/2010 05:24 PM, Markus Elfring wrote:
> 
>>
>> The general feeling on this list is that patches are listened to, no
>> matter how foul they are, and you will get a (hopefully) polite
>> rejection if it is considered useless because it addresses a problem
>> that doesn't exist.
>>
> 
> I hope that a healthy balance will be found between correct software
> design, development and quick "hacking".

What's considered a "healthy balance" varies from person to person
though.

> There might also be more
> efforts if too many patches will be rejected just because the suggested
> and planned changes were not discussed before.
> 

Perhaps, but those wasted efforts would have been yours, not ours. Sorry,
but the sad fact is that unless you're willing to "fix" this (which you
should show by submitting patches), it's entirely uninteresting to even
discuss it. We're all busy folks here, and we do not have unlimited time
on our hands to masturbate mentally when most of us realize that it's
not a practical approach to blindly follow standards.

> Would you like to get an acknowledgement for signal handler problems
> from people in other discussion groups like "comp.programming.threads"?
> 

No. I have no interest in theoretical best practices for a program that
has no known issues with its use of multiple threads.

I do have interest in bug-reports about my favourite scm and patches to
make it better. So far you have only shown a willingness to discuss
possible problems instead of real-world ones. I've mentally marked you
down as "the threads theory troll". To remedy that sad title, you should
deliver some patches pointing out and fixing problems in the sources.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: Fix signal handler

[Markus Elfring]

>
> If you can demonstrate a practical problem and provide a patch, then I
> am sure people would be happy to read it.
>   

I need a few further clarifications on this issue to choose a potential fix.

I have noticed that the variable "show_early_output" gets a value
assigned only at a few places in the source code. I wonder that the set
pointer is only used by the function "limit_list" to call the function
"log_show_early" on demand.
http://git.kernel.org/?p=git/git.git;a=blob;f=revision.c;h=3ba6d991f6e9789949c314c2981dfc6b208a6f66;hb=HEAD#l683

I find that a simple flag would be sufficient. I see no need to handle
different function pointers here. Do any objections exist to achieve the
same effect with the data type "sig_atomic_t"?

Regards,
Markus

Re: Fix signal handler

[Daniel Barkalow]

On Tue, 9 Feb 2010, Markus Elfring wrote:

> 
> >
> > If you can demonstrate a practical problem and provide a patch, then I
> > am sure people would be happy to read it.
> >   
> 
> I need a few further clarifications on this issue to choose a potential fix.
> 
> I have noticed that the variable "show_early_output" gets a value
> assigned only at a few places in the source code. I wonder that the set
> pointer is only used by the function "limit_list" to call the function
> "log_show_early" on demand.
> http://git.kernel.org/?p=git/git.git;a=blob;f=revision.c;h=3ba6d991f6e9789949c314c2981dfc6b208a6f66;hb=HEAD#l683
> 
> I find that a simple flag would be sufficient. I see no need to handle
> different function pointers here. Do any objections exist to achieve the
> same effect with the data type "sig_atomic_t"?

In that particular instance, there's actually a comment that says it uses 
an int (which is almost certainly what sig_atomic_t is, but sig_atomic_t 
might not be defined on some actual platforms). Making the code match the 
comment, at least, would be good.

In particular, function pointers are more likely than other pointers to be 
not a machine word. I'm pretty sure that an IA64 machine could potentially 
have a race with a small window here.

As to whether to use int (as the comment says) or sig_atomic_t, I don't 
really have any idea which would have fewer problems.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] Fix signal handler

[Markus Elfring]

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

Hello,

How do Git software developers think about the appended update suggestion?
Would you like to integrate such adjustments into your source code
repository?

Regards,
Markus

[-- Attachment #2: 0001-Fix-a-signal-handler.patch --]
[-- Type: text/x-patch, Size: 3457 bytes --]

>From c37d8dafef11168d8302d40c8d1453943a058d95 Mon Sep 17 00:00:00 2001
From: Markus Elfring <Markus.Elfring@web.de>
Date: Wed, 10 Feb 2010 17:05:45 +0100
Subject: [PATCH] Fix a signal handler

A global flag can only be set by a signal handler in a portable way if it has got the data type "sig_atomic_t". The previously used assignment of a function pointer in the function "early_output" was moved to another variable in the function "setup_early_output".
The involved software design details were also mentioned on the mailing list.
---
 builtin-log.c |   12 +++---------
 revision.c    |   14 ++++++--------
 revision.h    |    3 ++-
 3 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 8d16832..358c98b 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -123,7 +123,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
 
 static struct itimerval early_output_timer;
 
-static void log_show_early(struct rev_info *revs, struct commit_list *list)
+extern void log_show_early(struct rev_info *revs, struct commit_list *list)
 {
 	int i = revs->early_output;
 	int show_header = 1;
@@ -170,20 +170,14 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
 
 static void early_output(int signal)
 {
-	show_early_output = log_show_early;
+	show_early_output = 1;
 }
 
 static void setup_early_output(struct rev_info *rev)
 {
 	struct sigaction sa;
 
-	/*
-	 * Set up the signal handler, minimally intrusively:
-	 * we only set a single volatile integer word (not
-	 * using sigatomic_t - trying to avoid unnecessary
-	 * system dependencies and headers), and using
-	 * SA_RESTART.
-	 */
+	early_output_function = &log_show_early;
 	memset(&sa, 0, sizeof(sa));
 	sa.sa_handler = early_output;
 	sigemptyset(&sa.sa_mask);
diff --git a/revision.c b/revision.c
index 3ba6d99..62402fb 100644
--- a/revision.c
+++ b/revision.c
@@ -13,7 +13,8 @@
 #include "decorate.h"
 #include "log-tree.h"
 
-volatile show_early_output_fn_t show_early_output;
+sig_atomic_t show_early_output = 0;
+show_early_output_fn_t early_output_function = NULL;
 
 char *path_name(const struct name_path *path, const char *name)
 {
@@ -654,7 +655,6 @@ static int limit_list(struct rev_info *revs)
 		struct commit_list *entry = list;
 		struct commit *commit = list->item;
 		struct object *obj = &commit->object;
-		show_early_output_fn_t show;
 
 		list = list->next;
 		free(entry);
@@ -680,12 +680,10 @@ static int limit_list(struct rev_info *revs)
 		date = commit->date;
 		p = &commit_list_insert(commit, p)->next;
 
-		show = show_early_output;
-		if (!show)
-			continue;
-
-		show(revs, newlist);
-		show_early_output = NULL;
+		if (show_early_output) {
+			(*early_output_function)(revs, newlist);
+			show_early_output = 0;
+		}
 	}
 	if (revs->cherry_pick)
 		cherry_pick_list(newlist, revs);
diff --git a/revision.h b/revision.h
index a14deef..93a8ffc 100644
--- a/revision.h
+++ b/revision.h
@@ -135,7 +135,8 @@ struct rev_info {
 
 /* revision.c */
 typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *);
-extern volatile show_early_output_fn_t show_early_output;
+extern show_early_output_fn_t early_output_function;
+extern sig_atomic_t show_early_output;
 
 extern void init_revisions(struct rev_info *revs, const char *prefix);
 extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def);
-- 
1.6.6.1

Re: [PATCH] Fix signal handler

[Shawn O. Pearce]

Markus Elfring <Markus.Elfring@web.de> wrote:
> How do Git software developers think about the appended update suggestion?
> Would you like to integrate such adjustments into your source code
> repository?

Finally, a concrete patch we can comment on!
 
> Subject: [PATCH] Fix a signal handler
> 
> A global flag can only be set by a signal handler in a portable way if it has got the data type "sig_atomic_t". The previously used assignment of a function pointer in the function "early_output" was moved to another variable in the function "setup_early_output".
> The involved software design details were also mentioned on the mailing list.

Please line wrap your commit messages at ~70 characters per line.
This improves readability when reading the messages with tools like
`git log` and `gitk` where the lines aren't reflowed.

Please read Documentation/SubmittingPatches and add a Signed-off-by
line if you agree to the Developer's Certificate of Origin.


> +	early_output_function = &log_show_early;
...
> -volatile show_early_output_fn_t show_early_output;
> +sig_atomic_t show_early_output = 0;
> +show_early_output_fn_t early_output_function = NULL;
...
> +		if (show_early_output) {
> +			(*early_output_function)(revs, newlist);
> +			show_early_output = 0;
> +		}

The function pointer isn't necessary.  AFAIK its only called in
this one call site.  So you can make a direct reference to the
log_show_early function.

-- 
Shawn.

Re: [PATCH] Fix signal handler

[Jeff King]

On Wed, Feb 10, 2010 at 06:08:43PM +0100, Markus Elfring wrote:

> A global flag can only be set by a signal handler in a portable way if
> it has got the data type "sig_atomic_t". The previously used
> assignment of a function pointer in the function "early_output" was
> moved to another variable in the function "setup_early_output".
>
> The involved software design details were also mentioned on the
> mailing list.

Keep in mind commit messages will be read much later through "git log"
and the like.  Mentioning the mailing list is usually not very helpful
there. It is usually a good idea instead to summarize what was said on
the list for later readers of the commit (though in this case, I think
your first paragraph really says everything that needs to be said).

> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -123,7 +123,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
>  
>  static struct itimerval early_output_timer;
>  
> -static void log_show_early(struct rev_info *revs, struct commit_list *list)
> +extern void log_show_early(struct rev_info *revs, struct commit_list *list)

Why does this need to become extern? It looks like we are still just
assigning the function pointer from within this file.

> -volatile show_early_output_fn_t show_early_output;
> +sig_atomic_t show_early_output = 0;
> +show_early_output_fn_t early_output_function = NULL;

Good. I was worried from the above s/static/extern/ that you were going
to make log_show_early the only possible early output function, but the
way you did it is definitely the right way.

Overall, this change looks sane to me. You still haven't provided any
evidence that this is a problem in practice, but these changes are not
particularly cumbersome, so it is probably better to be on the safe
side.

-Peff

Re: [PATCH] Fix signal handler

[Jeff King]

On Wed, Feb 10, 2010 at 09:14:06AM -0800, Shawn O. Pearce wrote:

> > +	early_output_function = &log_show_early;
> ...
> > -volatile show_early_output_fn_t show_early_output;
> > +sig_atomic_t show_early_output = 0;
> > +show_early_output_fn_t early_output_function = NULL;
> ...
> > +		if (show_early_output) {
> > +			(*early_output_function)(revs, newlist);
> > +			show_early_output = 0;
> > +		}
> 
> The function pointer isn't necessary.  AFAIK its only called in
> this one call site.  So you can make a direct reference to the
> log_show_early function.

I disagree. The original intent was to decrease coupling between the
library-like revision walker and the actual log command. We aren't using
that flexibility now, but I don't see any reason to decrease it
(especially since it is so easy to keep it).

-Peff

Re: [PATCH] Fix signal handler

[Markus Elfring]

>
> Why does this need to become extern?

How do you think about to stress the detail that the function
"log_show_early" is called by the function "limit_list" from an other
translation unit.



>
> Overall, this change looks sane to me.

How are the chances to get the update suggestion into the public Git
repository?


> You still haven't provided any evidence that this is a problem in practice,
> but these changes are not particularly cumbersome, so it is probably better
> to be on the safe side.
>   

It is a matter of safety if all implementation details of the source
code conform to well-known programming standards.

Regards,
Markus

Re: [PATCH] Fix signal handler

[Jeff King]

On Sat, Feb 13, 2010 at 02:30:45PM +0100, Markus Elfring wrote:

> > Why does this need to become extern?
> 
> How do you think about to stress the detail that the function
> "log_show_early" is called by the function "limit_list" from an other
> translation unit.

Stress to whom? The keyword "extern" tells something to the linker, but
the linker doesn't care (and in fact making it extern introduces cruft
into the global namespace). If you want to tell the user, a comment
would be appropriate, but I don't think it is necessary. It is not hard
to see that the only use is assigning to an extern function pointer.

> > Overall, this change looks sane to me.
> 
> How are the chances to get the update suggestion into the public Git
> repository?

You would have a better chance if you followed the directions in
SubmittingPatches, including sending it to the maintainer, including
your patch inline, and wrapping your commit message.

-Peff

Re: [PATCH] Fix signal handler

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You would have a better chance if you followed the directions in
> SubmittingPatches, including sending it to the maintainer, including
> your patch inline, and wrapping your commit message.

Please do not encourage a patch sent to the maintainer while it is still
under active discussion and refinement, aka "not quite ready yet".

Other than that, your comments all looked very sensible.

Thanks.

Re: [PATCH] Fix signal handler

[Markus Elfring]

> Other than that, your comments all looked very sensible.
>   

Do you expect any more tweaks and fine-tuning for my update suggestion?

Regards,
Markus

Re: [PATCH] Fix signal handler

[Junio C Hamano]

Markus Elfring <Markus.Elfring@web.de> writes:

>> Other than that, your comments all looked very sensible.
>
> Do you expect any more tweaks and fine-tuning for my update suggestion?

Are you asking me if _I_ expect?  How would I be able to read your mind to
see if you will decide to send a polished update?

Of are you asking me if I'd apply your patch if you send a polished update,
and asking me to decide it before seeing the patch?

Sorry, but I am confused.

Re: [PATCH] Fix signal handler

[Markus Elfring]

> How would I be able to read your mind to see if you will decide to send a polished update?
>   

You indicated that my patch and the discussion about it was "not quite
ready yet" so far. I try to collect further suggestions for the next
refinement.

Regards,
Markus

Re: [PATCH] Fix a signal handler

[Markus Elfring]

> Of are you asking me if I'd apply your patch if you send a polished update,
> and asking me to decide it before seeing the patch?

Would you like to pick this source code adjustment up?

Regards,
Markus

---
From e138904a08ceaf469fa2f4d0ec87b5891be14760 Mon Sep 17 00:00:00 2001
From: Markus Elfring <Markus.Elfring@web.de>
Date: Mon, 22 Feb 2010 11:53:35 +0100
Subject: [PATCH] Fix a signal handler

A global flag can only be set by a signal handler in a portable way
if it has got the data type "sig_atomic_t". The previously used assignment
of a function pointer in the function "early_output" was moved to another
variable in the function "setup_early_output".

The involved software design details were also mentioned on the mailing list.

Signed-off-by: Markus Elfring <Markus.Elfring@web.de>

---
 builtin-log.c |   10 ++--------
 revision.c    |   14 ++++++--------
 revision.h    |    3 ++-
 3 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index e0d5caa..beccf7f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -170,20 +170,14 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
 
 static void early_output(int signal)
 {
-	show_early_output = log_show_early;
+	show_early_output = 1;
 }
 
 static void setup_early_output(struct rev_info *rev)
 {
 	struct sigaction sa;
 
-	/*
-	 * Set up the signal handler, minimally intrusively:
-	 * we only set a single volatile integer word (not
-	 * using sigatomic_t - trying to avoid unnecessary
-	 * system dependencies and headers), and using
-	 * SA_RESTART.
-	 */
+	early_output_function = &log_show_early;
 	memset(&sa, 0, sizeof(sa));
 	sa.sa_handler = early_output;
 	sigemptyset(&sa.sa_mask);
diff --git a/revision.c b/revision.c
index 3ba6d99..62402fb 100644
--- a/revision.c
+++ b/revision.c
@@ -13,7 +13,8 @@
 #include "decorate.h"
 #include "log-tree.h"
 
-volatile show_early_output_fn_t show_early_output;
+sig_atomic_t show_early_output = 0;
+show_early_output_fn_t early_output_function = NULL;
 
 char *path_name(const struct name_path *path, const char *name)
 {
@@ -654,7 +655,6 @@ static int limit_list(struct rev_info *revs)
 		struct commit_list *entry = list;
 		struct commit *commit = list->item;
 		struct object *obj = &commit->object;
-		show_early_output_fn_t show;
 
 		list = list->next;
 		free(entry);
@@ -680,12 +680,10 @@ static int limit_list(struct rev_info *revs)
 		date = commit->date;
 		p = &commit_list_insert(commit, p)->next;
 
-		show = show_early_output;
-		if (!show)
-			continue;
-
-		show(revs, newlist);
-		show_early_output = NULL;
+		if (show_early_output) {
+			(*early_output_function)(revs, newlist);
+			show_early_output = 0;
+		}
 	}
 	if (revs->cherry_pick)
 		cherry_pick_list(newlist, revs);
diff --git a/revision.h b/revision.h
index a14deef..93a8ffc 100644
--- a/revision.h
+++ b/revision.h
@@ -135,7 +135,8 @@ struct rev_info {
 
 /* revision.c */
 typedef void (*show_early_output_fn_t)(struct rev_info *, struct commit_list *);
-extern volatile show_early_output_fn_t show_early_output;
+extern show_early_output_fn_t early_output_function;
+extern sig_atomic_t show_early_output;
 
 extern void init_revisions(struct rev_info *revs, const char *prefix);
 extern int setup_revisions(int argc, const char **argv, struct rev_info *revs, const char *def);
-- 
1.7.0

Re: [PATCH] Fix a signal handler

[Junio C Hamano]

Markus Elfring <Markus.Elfring@web.de> writes:

>> Of are you asking me if I'd apply your patch if you send a polished update,
>> and asking me to decide it before seeing the patch?
>
> Would you like to pick this source code adjustment up?
>
> Regards,
> Markus
>
> ---
>>From e138904a08ceaf469fa2f4d0ec87b5891be14760 Mon Sep 17 00:00:00 2001

It would be preferred to remove everything above this line when you send
patches.

> From: Markus Elfring <Markus.Elfring@web.de>
> Date: Mon, 22 Feb 2010 11:53:35 +0100
> Subject: [PATCH] Fix a signal handler

Please be a bit more careful when coming up with what Subject: says; it
will be the only piece of information to tell the reader of output from
"git shortlog" what the commit made from this patch is about.  E.g.

    Subject: [PATCH] log --early-output: signal handler pedantic fix

> A global flag can only be set by a signal handler in a portable way
> if it has got the data type "sig_atomic_t". The previously used assignment
> of a function pointer in the function "early_output" was moved to another
> variable in the function "setup_early_output".

The first sentence gives the basis of your argument (so that people can
decide to agree or disagree with you).  Then you describe why you think
the code you are changing is wrong --- oops, that part is missing --- and
what you did based on the above two observations --- oops, that part is
missing, too --- and then any additional info.

I'd phrase the above like this:

    The behavior is undefined if the signal handler refers to any object
    other than errno with static storage duration other than by assigning
    a value to a static storage duration variable of type "volatile
    sig_atomic_t", but the existing code updates a variable that holds a
    pointer to a function (i.e. not a sigatomic_t variable).

    Change it to only set a flag, and adjust the callsite that calls the
    early-output function to check it.

and that would be sufficiently clear without saying anything else.

> diff --git a/builtin-log.c b/builtin-log.c
> index e0d5caa..beccf7f 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -170,20 +170,14 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)
>  
>  static void early_output(int signal)
>  {
> -	show_early_output = log_show_early;
> +	show_early_output = 1;
>  }
>  
>  static void setup_early_output(struct rev_info *rev)
>  {
>  	struct sigaction sa;
>  
> -	/*
> -	 * Set up the signal handler, minimally intrusively:
> -	 * we only set a single volatile integer word (not
> -	 * using sigatomic_t - trying to avoid unnecessary
> -	 * system dependencies and headers), and using
> -	 * SA_RESTART.
> -	 */
> +	early_output_function = &log_show_early;

Your proposed log message also needs to make a good counter-argument why
the above "we purposely avoid using sigatomic_t --- it is not worth the
hassle of having to deal with systems that lack this type in practice" is
worried too much, and it now is sensible to assume that everybody has
sigatomic_t these days to allow us do "the right thing".  It can be just
as simple as 'Output from "git grep sigatomic_t" indicates that we are
already using it.' but you need to say something, as this comment you are
removing makes it clear that it was not a bug by mistake or ignorance, but
instead was a deliberate choice.

> diff --git a/revision.c b/revision.c
> index 3ba6d99..62402fb 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -13,7 +13,8 @@
>  #include "decorate.h"
>  #include "log-tree.h"
>  
> -volatile show_early_output_fn_t show_early_output;
> +sig_atomic_t show_early_output = 0;
> +show_early_output_fn_t early_output_function = NULL;

According to POSIX, "s-e-o" has to be "volatile sig_atomic_t".  Also we do
not explicitly initialize bss variables to zero or NULL.

Thanks.

Re: [PATCH] Fix a signal handler

[Markus Elfring]

>     Subject: [PATCH] log --early-output: signal handler pedantic fix

I would prefer a correct and portable approach here instead of a "pedantic" one.
  ;-)


> I'd phrase the above like this:

It might be that the suggested commit message was too terse.


>     The behavior is undefined if the signal handler refers to any object
>     other than errno with static storage duration other than by assigning
>     a value to a static storage duration variable

I would not repeat the specification of undefined behaviour if a reference to a
standard like POSIX will be sufficient.


> and that would be sufficiently clear without saying anything else.

It seems that we have got different opinions about the clarity of signal handling.


> Your proposed log message also needs to make a good counter-argument why
> the above "we purposely avoid using sigatomic_t --- it is not worth the
> hassle of having to deal with systems that lack this type in practice" is
> worried too much, and it now is sensible to assume that everybody has
> sigatomic_t these days to allow us do "the right thing".

This data type is actually not used (because an underscore is missing in the
name).   ;-)


> It can be just as simple as 'Output from "git grep sigatomic_t" indicates
> that we are already using it.' but you need to say something, as this
> comment you are removing makes it clear that it was not a bug by mistake
> or ignorance, but instead was a deliberate choice.

Should I really add to the log message that there is another user for it like
the source file "progress.c"?


> According to POSIX, "s-e-o" has to be "volatile sig_atomic_t".

How do you think about informations from a discussion on a topic like 'Is
"volatile sig_atomic_t" redundant'?
http://groups.google.de/group/comp.lang.c/browse_frm/thread/da3118a2d2c0737c/718dc093b83e03f8?#718dc093b83e03f8


> Also we do not explicitly initialize bss variables to zero or NULL.

If we would like to insist on the implementation of a strictly conforming
program, the source code should be restructured even more.
https://www.securecoding.cert.org/confluence/display/seccode/SIG31-C.+Do+not+access+or+modify+shared+objects+in+signal+handlers

The variable "show_early_output" should be moved to the source file
"builtin-log.c" where it will become "static". Other means would be needed to
transfer corresponding state changes to the function "path_name".

Regards,
Markus

Re: [PATCH] Fix a signal handler

[Markus Elfring]

> Other means would be needed to transfer corresponding state changes to the function "path_name".

I'm sorry for a potential confusion. - The function "limit_list" is the intended
call site so far.

Regards,
Markus

Re: [PATCH] Fix a signal handler

[Junio C Hamano]

Markus Elfring <Markus.Elfring@web.de> writes:

>> According to POSIX, "s-e-o" has to be "volatile sig_atomic_t".
>
> How do you think about informations from a discussion on a topic like 'Is
> "volatile sig_atomic_t" redundant'?
> http://groups.google.de/group/comp.lang.c/browse_frm/thread/da3118a2d2c0737c/718dc093b83e03f8?#718dc093b83e03f8

Honestly I don't care; you are the one who are interested in being
pedantic, and you are welcome wasting your time on that endeavor.  Don't
ask me to waste my time by joining your mental masturbation.

>> Also we do not explicitly initialize bss variables to zero or NULL.
>
> If we would like to insist on the implementation of a strictly conforming
> program,...

We don't.

The thing is, we do not like to insist any such thing.  We are practical
bunch who are interested in getting git work well on real platforms used
by real people.  Portability across platforms people care about is one of
the goals and standard conformance for us is merely a tool to achieve it.

Name one platform you tried to port git to and had trouble with because
the platform did not initialize variables in bss segment to zero, or
perhaps on that platfor NULL had a bitpattern different from all zero, and
after you initialized them explicitly to zero or NULL, you managed to make
everything work perfectly.

Name one platform you actually got a segfault in the early-output codepath
on it, because a function pointer on that platform is not of an atomic
type, and the assignment from show_early_output to show done in
limit_list() picked up a pointer half-written by the signal handler, and
we ended up calling a garbage address, and you managed to make everything
work perfectly with your fix.

Just name one.

Standard conformance by itself is never a goal for us, unless it helps to
solve real world problems.  And until you understand that, you wouldn't
understand why this patch deserves to be labelled with "pedantic fix".

Re: [PATCH] Fix a signal handler

[Markus Elfring]

> Name one platform you tried to port git to and had trouble with because
> the platform did not initialize variables in bss segment to zero, or
> perhaps on that platfor NULL had a bitpattern different from all zero, and
> after you initialized them explicitly to zero or NULL, you managed to make
> everything work perfectly.
> 
> Name one platform you actually got a segfault in the early-output codepath
> on it, because a function pointer on that platform is not of an atomic
> type, and the assignment from show_early_output to show done in
> limit_list() picked up a pointer half-written by the signal handler, and
> we ended up calling a garbage address, and you managed to make everything
> work perfectly with your fix.

Thanks for your feedback.

Which is the name for this specific software environment where the "unexpected"
behaviour was observed?

Does the mentioned improvement justify the integration of my intermediate update
suggestion that works without a "static" flag so far into your source code
repository?

Regards,
Markus

Re: [PATCH] Fix a signal handler

[Andreas Ericsson]

On 02/24/2010 11:38 AM, Markus Elfring wrote:
>> Name one platform you tried to port git to and had trouble with because
>> the platform did not initialize variables in bss segment to zero, or
>> perhaps on that platfor NULL had a bitpattern different from all zero, and
>> after you initialized them explicitly to zero or NULL, you managed to make
>> everything work perfectly.
>>
>> Name one platform you actually got a segfault in the early-output codepath
>> on it, because a function pointer on that platform is not of an atomic
>> type, and the assignment from show_early_output to show done in
>> limit_list() picked up a pointer half-written by the signal handler, and
>> we ended up calling a garbage address, and you managed to make everything
>> work perfectly with your fix.
> 
> Thanks for your feedback.
> 
> Which is the name for this specific software environment where the "unexpected"
> behaviour was observed?
> 

There isn't one. He was asking you to provide a bugreport for a system where
this behaviour was observed to prove that your fix isn't pedantic. Returning
the question does not help.

> Does the mentioned improvement justify the integration of my intermediate update
> suggestion that works without a "static" flag so far into your source code
> repository?
> 

Wait and find out. I consider it useless codechurn since it's not fixing any
real-world problem, but it's not intrusive enough for me to care much either
way, so as long as it doesn't break anything I don't care either way. That's
the big problem, really. You're asking a lot of people to spend a lot of time
on something that we, over and over, have told you we're not interested in
unless you can prove you're solving a real problem.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: [PATCH] Fix a signal handler

[Markus Elfring]

> Just name one.

I do not know a concrete software platform so far where the misused assignment
of the function pointer in the signal handler implementation results in
unexpected effects that are directly noticeable like segmentation faults.

Regards,
Markus