* Fix signal handler @ 2010-02-02 16:14 Markus Elfring 2010-02-02 20:58 ` Jeff King 0 siblings, 1 reply; 36+ messages in thread From: Markus Elfring @ 2010-02-02 16:14 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-02 16:14 Fix signal handler Markus Elfring @ 2010-02-02 20:58 ` Jeff King 2010-02-02 21:44 ` Markus Elfring 2010-02-09 18:01 ` Markus Elfring 0 siblings, 2 replies; 36+ messages in thread From: Jeff King @ 2010-02-02 20:58 UTC (permalink / raw) To: Markus Elfring; +Cc: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-02 20:58 ` Jeff King @ 2010-02-02 21:44 ` Markus Elfring 2010-02-02 22:32 ` Jeff King 2010-02-09 18:01 ` Markus Elfring 1 sibling, 1 reply; 36+ messages in thread From: Markus Elfring @ 2010-02-02 21:44 UTC (permalink / raw) To: Jeff King; +Cc: git > > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-02 21:44 ` Markus Elfring @ 2010-02-02 22:32 ` Jeff King 2010-02-03 10:20 ` Markus Elfring 0 siblings, 1 reply; 36+ messages in thread From: Jeff King @ 2010-02-02 22:32 UTC (permalink / raw) To: Markus Elfring; +Cc: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-02 22:32 ` Jeff King @ 2010-02-03 10:20 ` Markus Elfring 2010-02-03 10:29 ` Jeff King 0 siblings, 1 reply; 36+ messages in thread From: Markus Elfring @ 2010-02-03 10:20 UTC (permalink / raw) To: Jeff King; +Cc: git > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 10:20 ` Markus Elfring @ 2010-02-03 10:29 ` Jeff King 2010-02-03 11:55 ` Markus Elfring 0 siblings, 1 reply; 36+ messages in thread From: Jeff King @ 2010-02-03 10:29 UTC (permalink / raw) To: Markus Elfring; +Cc: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 10:29 ` Jeff King @ 2010-02-03 11:55 ` Markus Elfring 2010-02-03 13:12 ` Thomas Rast 2010-02-03 15:17 ` Jeff King 0 siblings, 2 replies; 36+ messages in thread From: Markus Elfring @ 2010-02-03 11:55 UTC (permalink / raw) To: Jeff King; +Cc: git > > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 11:55 ` Markus Elfring @ 2010-02-03 13:12 ` Thomas Rast 2010-02-03 15:46 ` Markus Elfring 2010-02-03 15:17 ` Jeff King 1 sibling, 1 reply; 36+ messages in thread From: Thomas Rast @ 2010-02-03 13:12 UTC (permalink / raw) To: Markus Elfring; +Cc: Jeff King, git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 13:12 ` Thomas Rast @ 2010-02-03 15:46 ` Markus Elfring 2010-02-03 15:52 ` Shawn O. Pearce 2010-02-03 15:53 ` Andreas Ericsson 0 siblings, 2 replies; 36+ messages in thread From: Markus Elfring @ 2010-02-03 15:46 UTC (permalink / raw) To: Thomas Rast; +Cc: Jeff King, git > > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 15:46 ` Markus Elfring @ 2010-02-03 15:52 ` Shawn O. Pearce 2010-02-03 15:53 ` Andreas Ericsson 1 sibling, 0 replies; 36+ messages in thread From: Shawn O. Pearce @ 2010-02-03 15:52 UTC (permalink / raw) To: Markus Elfring; +Cc: Thomas Rast, Jeff King, git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 15:46 ` Markus Elfring 2010-02-03 15:52 ` Shawn O. Pearce @ 2010-02-03 15:53 ` Andreas Ericsson 2010-02-03 16:24 ` Markus Elfring 1 sibling, 1 reply; 36+ messages in thread From: Andreas Ericsson @ 2010-02-03 15:53 UTC (permalink / raw) To: Markus Elfring; +Cc: Thomas Rast, Jeff King, git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 15:53 ` Andreas Ericsson @ 2010-02-03 16:24 ` Markus Elfring 2010-02-04 7:23 ` Andreas Ericsson 0 siblings, 1 reply; 36+ messages in thread From: Markus Elfring @ 2010-02-03 16:24 UTC (permalink / raw) To: Andreas Ericsson; +Cc: Thomas Rast, Jeff King, git > > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 16:24 ` Markus Elfring @ 2010-02-04 7:23 ` Andreas Ericsson 0 siblings, 0 replies; 36+ messages in thread From: Andreas Ericsson @ 2010-02-04 7:23 UTC (permalink / raw) To: Markus Elfring; +Cc: Thomas Rast, Jeff King, git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 11:55 ` Markus Elfring 2010-02-03 13:12 ` Thomas Rast @ 2010-02-03 15:17 ` Jeff King 2010-02-03 16:04 ` Markus Elfring 1 sibling, 1 reply; 36+ messages in thread From: Jeff King @ 2010-02-03 15:17 UTC (permalink / raw) To: Markus Elfring; +Cc: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 15:17 ` Jeff King @ 2010-02-03 16:04 ` Markus Elfring 2010-02-03 16:26 ` Bill Lear 0 siblings, 1 reply; 36+ messages in thread From: Markus Elfring @ 2010-02-03 16:04 UTC (permalink / raw) To: Jeff King; +Cc: git > > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-03 16:04 ` Markus Elfring @ 2010-02-03 16:26 ` Bill Lear 0 siblings, 0 replies; 36+ messages in thread From: Bill Lear @ 2010-02-03 16:26 UTC (permalink / raw) To: Markus Elfring; +Cc: Jeff King, git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-02 20:58 ` Jeff King 2010-02-02 21:44 ` Markus Elfring @ 2010-02-09 18:01 ` Markus Elfring 2010-02-09 23:49 ` Daniel Barkalow 2010-02-10 17:08 ` [PATCH] " Markus Elfring 1 sibling, 2 replies; 36+ messages in thread From: Markus Elfring @ 2010-02-09 18:01 UTC (permalink / raw) To: Jeff King; +Cc: git > > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: Fix signal handler 2010-02-09 18:01 ` Markus Elfring @ 2010-02-09 23:49 ` Daniel Barkalow 2010-02-10 17:08 ` [PATCH] " Markus Elfring 1 sibling, 0 replies; 36+ messages in thread From: Daniel Barkalow @ 2010-02-09 23:49 UTC (permalink / raw) To: Markus Elfring; +Cc: Jeff King, git 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* ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-09 18:01 ` Markus Elfring 2010-02-09 23:49 ` Daniel Barkalow @ 2010-02-10 17:08 ` Markus Elfring 2010-02-10 17:14 ` Shawn O. Pearce 2010-02-10 17:33 ` Jeff King 1 sibling, 2 replies; 36+ messages in thread From: Markus Elfring @ 2010-02-10 17:08 UTC (permalink / raw) To: Jeff King; +Cc: git [-- 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-10 17:08 ` [PATCH] " Markus Elfring @ 2010-02-10 17:14 ` Shawn O. Pearce 2010-02-10 17:35 ` Jeff King 2010-02-10 17:33 ` Jeff King 1 sibling, 1 reply; 36+ messages in thread From: Shawn O. Pearce @ 2010-02-10 17:14 UTC (permalink / raw) To: Markus Elfring; +Cc: Jeff King, git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-10 17:14 ` Shawn O. Pearce @ 2010-02-10 17:35 ` Jeff King 0 siblings, 0 replies; 36+ messages in thread From: Jeff King @ 2010-02-10 17:35 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: Markus Elfring, git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-10 17:08 ` [PATCH] " Markus Elfring 2010-02-10 17:14 ` Shawn O. Pearce @ 2010-02-10 17:33 ` Jeff King 2010-02-13 13:30 ` Markus Elfring 1 sibling, 1 reply; 36+ messages in thread From: Jeff King @ 2010-02-10 17:33 UTC (permalink / raw) To: Markus Elfring; +Cc: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-10 17:33 ` Jeff King @ 2010-02-13 13:30 ` Markus Elfring 2010-02-14 6:47 ` Jeff King 0 siblings, 1 reply; 36+ messages in thread From: Markus Elfring @ 2010-02-13 13:30 UTC (permalink / raw) To: Jeff King; +Cc: git > > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-13 13:30 ` Markus Elfring @ 2010-02-14 6:47 ` Jeff King 2010-02-14 10:19 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Jeff King @ 2010-02-14 6:47 UTC (permalink / raw) To: Markus Elfring; +Cc: git 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-14 6:47 ` Jeff King @ 2010-02-14 10:19 ` Junio C Hamano 2010-02-18 16:31 ` Markus Elfring 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2010-02-14 10:19 UTC (permalink / raw) To: Jeff King; +Cc: Markus Elfring, git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-14 10:19 ` Junio C Hamano @ 2010-02-18 16:31 ` Markus Elfring 2010-02-18 20:06 ` Junio C Hamano 0 siblings, 1 reply; 36+ messages in thread From: Markus Elfring @ 2010-02-18 16:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git > Other than that, your comments all looked very sensible. > Do you expect any more tweaks and fine-tuning for my update suggestion? Regards, Markus ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-18 16:31 ` Markus Elfring @ 2010-02-18 20:06 ` Junio C Hamano 2010-02-19 11:05 ` Markus Elfring 2010-02-22 12:10 ` [PATCH] Fix a " Markus Elfring 0 siblings, 2 replies; 36+ messages in thread From: Junio C Hamano @ 2010-02-18 20:06 UTC (permalink / raw) To: Markus Elfring; +Cc: Jeff King, git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix signal handler 2010-02-18 20:06 ` Junio C Hamano @ 2010-02-19 11:05 ` Markus Elfring 2010-02-22 12:10 ` [PATCH] Fix a " Markus Elfring 1 sibling, 0 replies; 36+ messages in thread From: Markus Elfring @ 2010-02-19 11:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix a signal handler 2010-02-18 20:06 ` Junio C Hamano 2010-02-19 11:05 ` Markus Elfring @ 2010-02-22 12:10 ` Markus Elfring 2010-02-22 18:31 ` Junio C Hamano 1 sibling, 1 reply; 36+ messages in thread From: Markus Elfring @ 2010-02-22 12:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git > 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 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix a signal handler 2010-02-22 12:10 ` [PATCH] Fix a " Markus Elfring @ 2010-02-22 18:31 ` Junio C Hamano 2010-02-23 8:55 ` Markus Elfring 0 siblings, 1 reply; 36+ messages in thread From: Junio C Hamano @ 2010-02-22 18:31 UTC (permalink / raw) To: Markus Elfring; +Cc: Jeff King, git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix a signal handler 2010-02-22 18:31 ` Junio C Hamano @ 2010-02-23 8:55 ` Markus Elfring 2010-02-23 9:10 ` Markus Elfring 2010-02-23 21:48 ` Junio C Hamano 0 siblings, 2 replies; 36+ messages in thread From: Markus Elfring @ 2010-02-23 8:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix a signal handler 2010-02-23 8:55 ` Markus Elfring @ 2010-02-23 9:10 ` Markus Elfring 2010-02-23 21:48 ` Junio C Hamano 1 sibling, 0 replies; 36+ messages in thread From: Markus Elfring @ 2010-02-23 9:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix a signal handler 2010-02-23 8:55 ` Markus Elfring 2010-02-23 9:10 ` Markus Elfring @ 2010-02-23 21:48 ` Junio C Hamano 2010-02-24 10:38 ` Markus Elfring 2010-02-24 11:08 ` Markus Elfring 1 sibling, 2 replies; 36+ messages in thread From: Junio C Hamano @ 2010-02-23 21:48 UTC (permalink / raw) To: Markus Elfring; +Cc: Jeff King, git 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". ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix a signal handler 2010-02-23 21:48 ` Junio C Hamano @ 2010-02-24 10:38 ` Markus Elfring 2010-02-24 10:51 ` Andreas Ericsson 2010-02-24 11:08 ` Markus Elfring 1 sibling, 1 reply; 36+ messages in thread From: Markus Elfring @ 2010-02-24 10:38 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix a signal handler 2010-02-24 10:38 ` Markus Elfring @ 2010-02-24 10:51 ` Andreas Ericsson 0 siblings, 0 replies; 36+ messages in thread From: Andreas Ericsson @ 2010-02-24 10:51 UTC (permalink / raw) To: Markus Elfring; +Cc: Junio C Hamano, Jeff King, git 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. ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH] Fix a signal handler 2010-02-23 21:48 ` Junio C Hamano 2010-02-24 10:38 ` Markus Elfring @ 2010-02-24 11:08 ` Markus Elfring 1 sibling, 0 replies; 36+ messages in thread From: Markus Elfring @ 2010-02-24 11:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git > 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 ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2010-02-24 11:08 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-02-02 16:14 Fix signal handler Markus Elfring 2010-02-02 20:58 ` Jeff King 2010-02-02 21:44 ` Markus Elfring 2010-02-02 22:32 ` Jeff King 2010-02-03 10:20 ` Markus Elfring 2010-02-03 10:29 ` Jeff King 2010-02-03 11:55 ` Markus Elfring 2010-02-03 13:12 ` Thomas Rast 2010-02-03 15:46 ` Markus Elfring 2010-02-03 15:52 ` Shawn O. Pearce 2010-02-03 15:53 ` Andreas Ericsson 2010-02-03 16:24 ` Markus Elfring 2010-02-04 7:23 ` Andreas Ericsson 2010-02-03 15:17 ` Jeff King 2010-02-03 16:04 ` Markus Elfring 2010-02-03 16:26 ` Bill Lear 2010-02-09 18:01 ` Markus Elfring 2010-02-09 23:49 ` Daniel Barkalow 2010-02-10 17:08 ` [PATCH] " Markus Elfring 2010-02-10 17:14 ` Shawn O. Pearce 2010-02-10 17:35 ` Jeff King 2010-02-10 17:33 ` Jeff King 2010-02-13 13:30 ` Markus Elfring 2010-02-14 6:47 ` Jeff King 2010-02-14 10:19 ` Junio C Hamano 2010-02-18 16:31 ` Markus Elfring 2010-02-18 20:06 ` Junio C Hamano 2010-02-19 11:05 ` Markus Elfring 2010-02-22 12:10 ` [PATCH] Fix a " Markus Elfring 2010-02-22 18:31 ` Junio C Hamano 2010-02-23 8:55 ` Markus Elfring 2010-02-23 9:10 ` Markus Elfring 2010-02-23 21:48 ` Junio C Hamano 2010-02-24 10:38 ` Markus Elfring 2010-02-24 10:51 ` Andreas Ericsson 2010-02-24 11:08 ` Markus Elfring
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.