* [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c @ 2019-08-24 6:14 Eli Zaretskii 2019-08-25 0:52 ` Paul Eggert 0 siblings, 1 reply; 26+ messages in thread From: Eli Zaretskii @ 2019-08-24 6:14 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel I'm sorry, but how is this variant more clear than the previous one? Please don't assume that I don't know about UNINIT or somehow forgot it existed. It is not true that the initializations are "unnecessary" in this case, just that when geometry is non-zero, x and y are initialized. None of that is clear from the UNINIT kludge. If we want this to be abundantly clear, we should have a comment there to the above effect. Clarify compiler-pacifier in frame.c * src/frame.c (Fx_parse_geometry): Pacify the compiler in a different way, so that the human reader can more easily see that the initializations are unnecessary. diff --git a/src/frame.c b/src/frame.c index 8ee8e42..330f98a 100644 --- a/src/frame.c +++ b/src/frame.c @@ -5327,7 +5327,7 @@ DEFUN ("x-parse-geometry", Fx_parse_geometry, Sx_parse_geometry, 1, 1, 0, On Nextstep, this just calls `ns-parse-geometry'. */) (Lisp_Object string) { - int geometry, x = 0, y = 0; + int geometry, x UNINIT, y UNINIT; unsigned int width, height; Lisp_Object result; ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-24 6:14 [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c Eli Zaretskii @ 2019-08-25 0:52 ` Paul Eggert 2019-08-25 7:13 ` Eli Zaretskii 0 siblings, 1 reply; 26+ messages in thread From: Paul Eggert @ 2019-08-25 0:52 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel [-- Attachment #1: Type: text/plain, Size: 575 bytes --] Eli Zaretskii wrote: > when geometry is non-zero, x and y are > initialized. And that means the initializations "x = 0, y = 0" were indeed unnecessary. The code works just fine without those initializations, because the values of x and y are not examined unless XParseGeometry has successfully set them. Newer GCC is smart enough to figure this out, at least with the default optimizations. Other platforms evidently aren't smart enough. > If we > want this to be abundantly clear, we should have a comment there to > the above effect. Sure, I installed the attached. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Clarify-Fx_parse_geometry-initialization.patch --] [-- Type: text/x-patch; name="0001-Clarify-Fx_parse_geometry-initialization.patch", Size: 1534 bytes --] From 84f1674ee8cbd83ea219595e9adec2d148946976 Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Sat, 24 Aug 2019 17:46:21 -0700 Subject: [PATCH] Clarify Fx_parse_geometry initialization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * src/frame.c (Fx_parse_geometry): Clarify why local init isn’t needed. --- src/frame.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/frame.c b/src/frame.c index 330f98aee1..cf38c85f09 100644 --- a/src/frame.c +++ b/src/frame.c @@ -5327,9 +5327,10 @@ DEFUN ("x-parse-geometry", Fx_parse_geometry, Sx_parse_geometry, 1, 1, 0, On Nextstep, this just calls `ns-parse-geometry'. */) (Lisp_Object string) { - int geometry, x UNINIT, y UNINIT; + /* x and y don't need initialization, as they are not accessed + unless XParseGeometry sets them. */ + int x UNINIT, y UNINIT; unsigned int width, height; - Lisp_Object result; CHECK_STRING (string); @@ -5337,9 +5338,9 @@ DEFUN ("x-parse-geometry", Fx_parse_geometry, Sx_parse_geometry, 1, 1, 0, if (strchr (SSDATA (string), ' ') != NULL) return call1 (Qns_parse_geometry, string); #endif - geometry = XParseGeometry (SSDATA (string), - &x, &y, &width, &height); - result = Qnil; + int geometry = XParseGeometry (SSDATA (string), + &x, &y, &width, &height); + Lisp_Object result = Qnil; if (geometry & XValue) { Lisp_Object element; -- 2.17.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-25 0:52 ` Paul Eggert @ 2019-08-25 7:13 ` Eli Zaretskii 2019-08-26 6:34 ` Paul Eggert 0 siblings, 1 reply; 26+ messages in thread From: Eli Zaretskii @ 2019-08-25 7:13 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > Cc: emacs-devel@gnu.org > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Sat, 24 Aug 2019 17:52:05 -0700 > > Newer GCC is smart enough to figure this out, at least with the default > optimizations. Other platforms evidently aren't smart enough. I believe the warning came from a compilation with GCC 9.2. > > If we > > want this to be abundantly clear, we should have a comment there to > > the above effect. > > Sure, I installed the attached. Thanks, but that wasn't enough, so I augmented it with a followup. Btw, I don't really like UNINIT because it only works in a Git repository. So it's a semi-kludgey semi-solution, not really better than an explicit initialization with a comment. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-25 7:13 ` Eli Zaretskii @ 2019-08-26 6:34 ` Paul Eggert 2019-08-26 7:54 ` Eli Zaretskii 0 siblings, 1 reply; 26+ messages in thread From: Paul Eggert @ 2019-08-26 6:34 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel >> Newer GCC is smart enough to figure this out, at least with the default >> optimizations. > I believe the warning came from a compilation with GCC 9.2. With default optimizations? In that case an UNINIT is called for indeed. Although it is a bit odd that I'm not seeing a problem with GCC 9.2 (x86-64, built for RHEL 7.6), the oddity could be due to other platform differences. > UNINIT ... only works in a Git repository. > So it's a semi-kludgey semi-solution, not really better > than an explicit initialization with a comment. UNINIT works outside of Git repositories, since it depends on GCC_LINT which can be set independently of whether one is in a Git repository. I regularly use this feature. And UNINIT allows one to use tools other than GCC to diagnose improper use of uninitialized variables, whereas an explicit initialization would prevent using these tools. I use this feature occasionally as well. Admittedly UNINIT is a bit of a kludge, but it's the best kludge we have in the area. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 6:34 ` Paul Eggert @ 2019-08-26 7:54 ` Eli Zaretskii 2019-08-26 8:15 ` Paul Eggert 0 siblings, 1 reply; 26+ messages in thread From: Eli Zaretskii @ 2019-08-26 7:54 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > Cc: emacs-devel@gnu.org > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Sun, 25 Aug 2019 23:34:25 -0700 > > > UNINIT ... only works in a Git repository. > > So it's a semi-kludgey semi-solution, not really better > > than an explicit initialization with a comment. > > UNINIT works outside of Git repositories, since it depends on GCC_LINT which can > be set independently of whether one is in a Git repository. That's not "working" in my book. The warnings will re-appear if one compiles outside of Git with suitable GCC options, so the solution is incomplete at best. > Admittedly UNINIT is a bit of a kludge, but it's the best kludge we have in the > area. An explicit initialization is a tad better, as it doesn't require any tinkering with obscure settings. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 7:54 ` Eli Zaretskii @ 2019-08-26 8:15 ` Paul Eggert 2019-08-26 9:47 ` Eli Zaretskii 0 siblings, 1 reply; 26+ messages in thread From: Paul Eggert @ 2019-08-26 8:15 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel Eli Zaretskii wrote: > The warnings will re-appear if one > compiles outside of Git with suitable GCC options, so the solution is > incomplete at best. We can't (and shouldn't try to) defend against people compiling Emacs with arbitrary-chosen GCC warning options, as there would be far too many false alarms. The best we can do is pacify GCC when a reasonable set of warning options is used, where "reasonable" is up to us (and is automated by 'configure'). >> Admittedly UNINIT is a bit of a kludge, but it's the best kludge we have in the >> area. > > An explicit initialization is a tad better, as it doesn't require any > tinkering with obscure settings. Neither should UNINIT require tinkering, if users employ default configuration settings. Explicit initialization uses plain C rather than the awkward UNINIT macro, and that is a plus for explicit initialization. However, that's a style issue, and for me it's outweighed by the technical advantage of aiding automated debugging tools that I use occasionally. For these tools it is helpful to say that a variable is not initialized, because that helps catch use-before-set errors. An explicit initialization would cause these use-before-set bugs to go uncaught by these debugging tools. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 8:15 ` Paul Eggert @ 2019-08-26 9:47 ` Eli Zaretskii 2019-08-26 15:21 ` Óscar Fuentes ` (2 more replies) 0 siblings, 3 replies; 26+ messages in thread From: Eli Zaretskii @ 2019-08-26 9:47 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > Cc: emacs-devel@gnu.org > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Mon, 26 Aug 2019 01:15:51 -0700 > > Eli Zaretskii wrote: > > > The warnings will re-appear if one > > compiles outside of Git with suitable GCC options, so the solution is > > incomplete at best. > > We can't (and shouldn't try to) defend against people compiling Emacs with > arbitrary-chosen GCC warning options, as there would be far too many false > alarms. It's not about us, it's about GCC's tendency to turn more and more warnings on by default. An explicit initialization with a comment explaining it is a small price to pay for clean compilation in arbitrary environments. > > An explicit initialization is a tad better, as it doesn't require any > > tinkering with obscure settings. > > Neither should UNINIT require tinkering, if users employ default configuration > settings. I call GCC_LINT "tinkering". > Explicit initialization uses plain C rather than the awkward UNINIT macro, and > that is a plus for explicit initialization. However, that's a style issue, and > for me it's outweighed by the technical advantage of aiding automated debugging > tools that I use occasionally. If this is just your personal stylistic preference, then I'd question whether we as a project should accept it. E.g., my stylistic preference is different; where does that leave us? > For these tools it is helpful to say that a variable is not > initialized, because that helps catch use-before-set errors. An > explicit initialization would cause these use-before-set bugs to go > uncaught by these debugging tools. What debugging tools can make a significant difference here, and how easy/practical is it to use them? I very much doubt they will catch every use-before-set bug anyway. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 9:47 ` Eli Zaretskii @ 2019-08-26 15:21 ` Óscar Fuentes 2019-08-26 16:13 ` Eli Zaretskii 2019-08-26 18:50 ` Paul Eggert 2019-08-26 23:17 ` Richard Stallman 2 siblings, 1 reply; 26+ messages in thread From: Óscar Fuentes @ 2019-08-26 15:21 UTC (permalink / raw) To: emacs-devel Eli Zaretskii <eliz@gnu.org> writes: >> For these tools it is helpful to say that a variable is not >> initialized, because that helps catch use-before-set errors. An >> explicit initialization would cause these use-before-set bugs to go >> uncaught by these debugging tools. > > What debugging tools can make a significant difference here, and how > easy/practical is it to use them? I very much doubt they will catch > every use-before-set bug anyway. Valgrind tracks each byte at the machine instruction level. Given the complexity of the Emacs C code, IMAO it is very useful to test for this type of nasty errors. I dislike hacks that pollute the source code, but on this case it is warranted. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 15:21 ` Óscar Fuentes @ 2019-08-26 16:13 ` Eli Zaretskii 2019-08-26 18:20 ` Óscar Fuentes 0 siblings, 1 reply; 26+ messages in thread From: Eli Zaretskii @ 2019-08-26 16:13 UTC (permalink / raw) To: Óscar Fuentes; +Cc: emacs-devel > > What debugging tools can make a significant difference here, and how > > easy/practical is it to use them? I very much doubt they will catch > > every use-before-set bug anyway. > > Valgrind tracks each byte at the machine instruction level. > > Given the complexity of the Emacs C code, IMAO it is very useful to test > for this type of nasty errors. I dislike hacks that pollute the source > code, but on this case it is warranted. Does valgrind know about UNINIT? I'd be surprised, since UNINIT is a source-level feature. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 16:13 ` Eli Zaretskii @ 2019-08-26 18:20 ` Óscar Fuentes 2019-08-26 18:39 ` Eli Zaretskii 0 siblings, 1 reply; 26+ messages in thread From: Óscar Fuentes @ 2019-08-26 18:20 UTC (permalink / raw) To: emacs-devel Eli Zaretskii <eliz@gnu.org> writes: >> > What debugging tools can make a significant difference here, and how >> > easy/practical is it to use them? I very much doubt they will catch >> > every use-before-set bug anyway. >> >> Valgrind tracks each byte at the machine instruction level. >> >> Given the complexity of the Emacs C code, IMAO it is very useful to test >> for this type of nasty errors. I dislike hacks that pollute the source >> code, but on this case it is warranted. > > Does valgrind know about UNINIT? I'd be surprised, since UNINIT is a > source-level feature. Valgrind knows nothing about UNINIT as it works with machine code, not with source code. But AFAIK that macro is conditionally defined to "=0" (for silencing bogus gcc warnings) or to nothing (for leaving the variable uninitalized at the declaration point). The later allows Valgrind to do a proper check. Simply changing int x; to int x = 0; just for silencing the gcc warning can hide a bug that Valgrind would detect otherwise. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 18:20 ` Óscar Fuentes @ 2019-08-26 18:39 ` Eli Zaretskii 2019-08-26 19:09 ` Paul Eggert 2019-08-26 19:15 ` Óscar Fuentes 0 siblings, 2 replies; 26+ messages in thread From: Eli Zaretskii @ 2019-08-26 18:39 UTC (permalink / raw) To: Óscar Fuentes; +Cc: emacs-devel > From: Óscar Fuentes <ofv@wanadoo.es> > Date: Mon, 26 Aug 2019 20:20:23 +0200 > > Valgrind knows nothing about UNINIT as it works with machine code, not > with source code. But AFAIK that macro is conditionally defined to "=0" > (for silencing bogus gcc warnings) or to nothing (for leaving the > variable uninitalized at the declaration point). The later allows > Valgrind to do a proper check. > > Simply changing > > int x; > > to > > int x = 0; > > just for silencing the gcc warning can hide a bug that Valgrind would > detect otherwise. This is backwards: it would mean we should use UNINIT all over the place just to be sure we will be able to spot some imaginary bugs by flipping a compiler switch. The initialization in this case was added because GCC flagged a potential use-before-define. After that, there's no more bug for Valgrind to find, so I see no reason to leave UNINIT around. I could understand using UNINIT (or something similar) when the developer has no idea what not initializing could cause. But this is not that case. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 18:39 ` Eli Zaretskii @ 2019-08-26 19:09 ` Paul Eggert 2019-08-26 19:15 ` Óscar Fuentes 1 sibling, 0 replies; 26+ messages in thread From: Paul Eggert @ 2019-08-26 19:09 UTC (permalink / raw) To: emacs-devel Eli Zaretskii wrote: > This is backwards: it would mean we should use UNINIT all over the > place just to be sure we will be able to spot some imaginary bugs by > flipping a compiler switch. That's not what UNINIT is for. It should be used only to pacify GCC. If GCC is happy with plain 'int x;' then UNINIT should not be used. Furthermore, these use-before-set bugs are not "imaginary". In the typical case where GCC's -Wmaybe-uninitialized diagnostics are not false alarms, they are quite useful in spotting and fixing use-before-set bugs. I've fixed many that way myself, typically in my working copy as I develop code. Indeed, 'configure' enables -Wmaybe-uninitialized precisely because its utility outweighs its cost of a few false alarms and resulting need to use UNINIT or unnecessary initializations. UNINIT can improve debugging, whereas unnecessary initializations can make the code look nicer. There's a tradeoff here, and different developers have different preferences of course. Developers who don't use tools that can take advantage of UNINIT may not appreciate or value its advantages. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 18:39 ` Eli Zaretskii 2019-08-26 19:09 ` Paul Eggert @ 2019-08-26 19:15 ` Óscar Fuentes 2019-08-26 19:33 ` Eli Zaretskii 1 sibling, 1 reply; 26+ messages in thread From: Óscar Fuentes @ 2019-08-26 19:15 UTC (permalink / raw) To: emacs-devel Eli Zaretskii <eliz@gnu.org> writes: >> From: Óscar Fuentes <ofv@wanadoo.es> >> Date: Mon, 26 Aug 2019 20:20:23 +0200 >> >> Valgrind knows nothing about UNINIT as it works with machine code, not >> with source code. But AFAIK that macro is conditionally defined to "=0" >> (for silencing bogus gcc warnings) or to nothing (for leaving the >> variable uninitalized at the declaration point). The later allows >> Valgrind to do a proper check. >> >> Simply changing >> >> int x; >> >> to >> >> int x = 0; >> >> just for silencing the gcc warning can hide a bug that Valgrind would >> detect otherwise. > > This is backwards: it would mean we should use UNINIT all over the > place just to be sure we will be able to spot some imaginary bugs by > flipping a compiler switch. UNINIT should only be used to avoid bogus warnings, *actual* bogus warnings, not potential ones. > The initialization in this case was added because GCC flagged a > potential use-before-define. After that, there's no more bug for > Valgrind to find, so I see no reason to leave UNINIT around. Let's suppose that the warning was correct and the hacker was wrong when judged it bogus. Adding the initialization silences the warning *and* Valgrind, while the UNINIT trick would cause Valgrind to complain (when Valgrind examines an Emacs executable built with the options that cause UNINIT to be empty.) Or suppose that the warning is indeed bogus now. By adding the initialization you are precluding Valgrind to find a bug if some future code change introduces it. > I could understand using UNINIT (or something similar) when the > developer has no idea what not initializing could cause. But this is > not that case. One of the most important things of the mental model of a C/C++ hacker is the declare->initialize->use->dispose sequence of any storage. If the developer is unsure about where to initialize a variable, he must stop and think. The declaration should do the initialization only when it is the right thing, that means that the variable must have that value from that point. A variable shouldn't be assigned before the point where the information it is supposed to store is available. (Sorry if that sounds as if I were giving a lesson, that's not my intention at all. I'm sure that you know what I explained by heart, but it seems to me that you are not fully aware of the fact that some tools do provide reliable detection of certain conditions that the compilers tried half-assedly to detect for ages only to annoy experienced hackers and force them to use workarounds that now are counterproductive.) ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 19:15 ` Óscar Fuentes @ 2019-08-26 19:33 ` Eli Zaretskii 2019-08-26 19:49 ` Óscar Fuentes 2019-08-26 22:33 ` Paul Eggert 0 siblings, 2 replies; 26+ messages in thread From: Eli Zaretskii @ 2019-08-26 19:33 UTC (permalink / raw) To: Óscar Fuentes; +Cc: emacs-devel > From: Óscar Fuentes <ofv@wanadoo.es> > Date: Mon, 26 Aug 2019 21:15:17 +0200 > > > The initialization in this case was added because GCC flagged a > > potential use-before-define. After that, there's no more bug for > > Valgrind to find, so I see no reason to leave UNINIT around. > > Let's suppose that the warning was correct and the hacker was wrong when > judged it bogus. The warning was correct, and I didn't decide it was bogus. I added the initialization because the warning was NOT bogus. Please see the code and the comment. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 19:33 ` Eli Zaretskii @ 2019-08-26 19:49 ` Óscar Fuentes 2019-08-26 22:33 ` Paul Eggert 1 sibling, 0 replies; 26+ messages in thread From: Óscar Fuentes @ 2019-08-26 19:49 UTC (permalink / raw) To: emacs-devel Eli Zaretskii <eliz@gnu.org> writes: >> Let's suppose that the warning was correct and the hacker was wrong when >> judged it bogus. > > The warning was correct, and I didn't decide it was bogus. I added > the initialization because the warning was NOT bogus. Please see the > code and the comment. Fair enough. I was not discussing the specific case, but the general practice. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 19:33 ` Eli Zaretskii 2019-08-26 19:49 ` Óscar Fuentes @ 2019-08-26 22:33 ` Paul Eggert 2019-08-27 6:12 ` Eli Zaretskii 1 sibling, 1 reply; 26+ messages in thread From: Paul Eggert @ 2019-08-26 22:33 UTC (permalink / raw) To: emacs-devel Eli Zaretskii wrote: >> Let's suppose that the warning was correct and the hacker was wrong when >> judged it bogus. > The warning was correct, and I didn't decide it was bogus. I added > the initialization because the warning was NOT bogus. No, actually that GCC warning was a false alarm, in that the initialization was not needed: even without the initialization the C code has well-defined behavior because no uninitialized object is ever used. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 22:33 ` Paul Eggert @ 2019-08-27 6:12 ` Eli Zaretskii 2019-08-27 7:28 ` Paul Eggert 0 siblings, 1 reply; 26+ messages in thread From: Eli Zaretskii @ 2019-08-27 6:12 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Mon, 26 Aug 2019 15:33:22 -0700 > > Eli Zaretskii wrote: > >> Let's suppose that the warning was correct and the hacker was wrong when > >> judged it bogus. > > The warning was correct, and I didn't decide it was bogus. I added > > the initialization because the warning was NOT bogus. > > No, actually that GCC warning was a false alarm, in that the initialization was > not needed: even without the initialization the C code has well-defined behavior > because no uninitialized object is ever used. I think you didn't read the code of XParseGeometry (the one that is executed on MS-Windows) well enough, if you think GCC gave a false alarm. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-27 6:12 ` Eli Zaretskii @ 2019-08-27 7:28 ` Paul Eggert 2019-08-27 8:02 ` Eli Zaretskii 0 siblings, 1 reply; 26+ messages in thread From: Paul Eggert @ 2019-08-27 7:28 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel Eli Zaretskii wrote: > I think you didn't read the code of XParseGeometry (the one that is > executed on MS-Windows) well enough, if you think GCC gave a false > alarm. I just now reread that code in current master and still see only a false alarm. If that function returns a mask where (mask & XValue) != 0, then *x must have been set by this statement: 5298 if (mask & XValue) 5299 *x = clip_to_bounds (INT_MIN, tempX, INT_MAX); So, if GCC warns about the use of x in the calling code: 5342 int geometry = XParseGeometry (SSDATA (string), 5343 &x, &y, &width, &height); 5344 Lisp_Object result = Qnil; 5345 if (geometry & XValue) 5346 { 5347 Lisp_Object element; 5348 5349 if (x >= 0 && (geometry & XNegative)) ... then GCC is giving a false alarm: x must be initialized in line 5349 if (geometry & XValue) is nonzero in line 5345. If my reasoning is wrong, can you give the path through the function and its caller where x is used as an uninitialized variable? Because if there is such a path, we shouldn't be marking x with UNINIT; UNINIT is only for pacifying false alarms. Similarly for y. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-27 7:28 ` Paul Eggert @ 2019-08-27 8:02 ` Eli Zaretskii 2019-08-27 9:28 ` Paul Eggert 0 siblings, 1 reply; 26+ messages in thread From: Eli Zaretskii @ 2019-08-27 8:02 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > Cc: emacs-devel@gnu.org > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Tue, 27 Aug 2019 00:28:58 -0700 > > 5342 int geometry = XParseGeometry (SSDATA (string), > 5343 &x, &y, &width, &height); > 5344 Lisp_Object result = Qnil; > 5345 if (geometry & XValue) > 5346 { > 5347 Lisp_Object element; > 5348 > 5349 if (x >= 0 && (geometry & XNegative)) > > ... then GCC is giving a false alarm: x must be initialized in line 5349 if > (geometry & XValue) is nonzero in line 5345. How do you know whether (geometry & XValue) is nonzero in each relevant use case? And how should GCC know that? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-27 8:02 ` Eli Zaretskii @ 2019-08-27 9:28 ` Paul Eggert 2019-08-27 10:15 ` Eli Zaretskii 0 siblings, 1 reply; 26+ messages in thread From: Paul Eggert @ 2019-08-27 9:28 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel Eli Zaretskii wrote: > How do you know whether (geometry & XValue) is nonzero in each > relevant use case? By reading the source code. Although it's simple enough, here's a very detailed analysis to make sure we're on the same page. In Fx_parse_geometry, every use of the local variable x must be preceded by an initialization of x, because every use is guarded by (geometry & XValue) and this test succeeds only when x is initialized. The initialization is done not directly by Fx_parse_geometry, but by a function that Fx_parse_geometry calls. This sort of thing is common. E.g., image_create_bitmap_from_file does this: 490 unsigned int width, height; ... 517 result = XReadBitmapFile (FRAME_X_DISPLAY (f), FRAME_X_DRAWABLE (f), 518 filename, &width, &height, &bitmap, &xhot, &yhot); 519 if (result != BitmapSuccess) 520 return -1; ... 528 dpyinfo->bitmaps[id - 1].height = height; 529 dpyinfo->bitmaps[id - 1].width = width; Here too there is no path through the code where 'width' and 'height' can be used before being set, because every use of 'width' and 'height' is guarded by (result == BitmapSuccess) and this test succeeds only when 'width' and 'height' are initialized. Hence image_create_bitmap_from_file need not initialize 'width' and 'height' in line 490, since XReadBitmapFile initializes them when it returns BitmapSuccess. Also, 'width' and 'height' should not be marked with UNINIT here since GCC does not warn here. GCC's lack of warning here does not mean that GCC knows that width and height are initialized: all it means is that GCC's heuristics for warnings do not elicit a warning here. There are two reasons that omitting UNINIT causes GCC to warn about Fx_parse_geometry but not image_create_bitmap_from_file. First, under MS-Windows GCC has access to the source code of the initialization function XParseGeometry that Fx_parse_geometry calls, whereas under X Windows GCC lacks access to the source code of the initialization function XReadBitmapFile that image_create_bitmap_from_file calls (I am assuming typical builds without fancy link-time optimization). Second, GCC does not fully grok the source code of MS-Windows XParseGeometry; GCC gets lost and thinks that there's a path through Fx_parse_geometry + XParseGeometry that will use Fx_parse_geometry's x without initializing it, even though there is no such path. > And how should GCC know that? GCC could use the same sort of reasoning I used. And perhaps some future version of GCC will do that. But in the meantime GCC gives a false alarm, so UNINIT is warranted here. UNINIT is precisely for this sort of situation: the programmer knows that a variable need not be initialized, but GCC falsely warns about the lack of initialization. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-27 9:28 ` Paul Eggert @ 2019-08-27 10:15 ` Eli Zaretskii 2019-08-27 12:05 ` Paul Eggert 0 siblings, 1 reply; 26+ messages in thread From: Eli Zaretskii @ 2019-08-27 10:15 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > Cc: emacs-devel@gnu.org > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Tue, 27 Aug 2019 02:28:42 -0700 > > There are two reasons that omitting UNINIT causes GCC to warn about > Fx_parse_geometry but not image_create_bitmap_from_file. First, under MS-Windows > GCC has access to the source code of the initialization function XParseGeometry > that Fx_parse_geometry calls, whereas under X Windows GCC lacks access to the > source code of the initialization function XReadBitmapFile that > image_create_bitmap_from_file calls (I am assuming typical builds without fancy > link-time optimization). Second, GCC does not fully grok the source code of > MS-Windows XParseGeometry; GCC gets lost and thinks that there's a path through > Fx_parse_geometry + XParseGeometry that will use Fx_parse_geometry's x without > initializing it, even though there is no such path. Do you have a guess why GCC might get lost in that code? > > And how should GCC know that? > > GCC could use the same sort of reasoning I used. Which reasoning is that? You haven't presented your reasoning for XParseGeometry, AFAICT. You presented reasoning for some other code, which you consider similar. > And perhaps some future version of GCC will do that. But in the > meantime GCC gives a false alarm, so UNINIT is warranted here. > > UNINIT is precisely for this sort of situation: the programmer knows that a > variable need not be initialized, but GCC falsely warns about the lack of > initialization. No, UNINIT is for you to be able to use your tools of choice, and perhaps also to cater to your personal stylistic preferences. I consider an initialization with a comment explaining why to be a better alternative, whether the warning is real or a false alarm. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-27 10:15 ` Eli Zaretskii @ 2019-08-27 12:05 ` Paul Eggert 2019-08-27 12:43 ` Eli Zaretskii 0 siblings, 1 reply; 26+ messages in thread From: Paul Eggert @ 2019-08-27 12:05 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel [-- Attachment #1: Type: text/plain, Size: 2142 bytes --] Eli Zaretskii wrote: > Do you have a guess why GCC might get lost in that code? Sure, lots of guesses. GCC may have a limit on how many variables it's willing to analyze before it gives up. Or maybe it has a limit on the complexity of the mask being used. Or maybe it's just using range and oddity analyses and that works only up to two-bit masks. (I should say that GCC is "lost" only in the sense that it's issuing bogus warnings; it's still generating correct code.) To illustrate, please see the attached file maybe-uninitialized.c. When I compile it with 'gcc -O2 -Wmaybe-uninitialized -S maybe-uninitialized.c' on Fedora 30 x86-64, GCC warns that w, x, and y might be used uninitialized in bar4. It doesn't warn about h in bar4 even though h is initialized using the same strategy as the others. GCC also doesn't warn about bar2's locals even though bar 2 uses the same strategy as bar4. All the warnings GCC issues are false alarms, and these false alarms come from bar4 (with 4 variables) rather than from bar2 (with only 2). >>> And how should GCC know that? >> >> GCC could use the same sort of reasoning I used. > > Which reasoning is that? You haven't presented your reasoning for > XParseGeometry, AFAICT. You presented reasoning for some other code, > which you consider similar. I presented the reasoning GCC's bogus warning about XParseGeometry's caller in this earlier email: https://lists.gnu.org/r/emacs-devel/2019-08/msg00531.html If that presentation wasn't clear enough I can go into it in more detail; just let me know which parts weren't clear. Perhaps maybe-uninitialized.c will help explain things too. > UNINIT is for you to be able to use your tools of choice, and > perhaps also to cater to your personal stylistic preferences. It's not just me using those tools. And UNINIT is not my stylistic preference: I don't like using UNINIT and when variables need not be initialized I'd rather just not initialize them (this is the longstanding tradition in GNU and UNIX code). The only reason for UNINIT is that it's more useful for preventing and catching bugs than its alternatives are. [-- Attachment #2: maybe-uninitialized.c --] [-- Type: text/x-csrc, Size: 932 bytes --] extern int v; extern int bar2 (void); extern int bar4 (void); int v; static int foo2 (int *x, int *y) { int mask = 0; if (v & 1) { mask = 1; *x = v; } if (v & 2) { mask |= 2; *y = v; } return mask; } int bar2 (void) { int sum = 0; int x, y; int mask = foo2 (&x, &y); if (mask & 1) sum += x; if (mask & 2) sum += y; return sum; } static int foo4 (int *x, int *y, int *w, int *h) { int mask = 0; if (v & 1) { mask = 1; *x = v; } if (v & 2) { mask |= 2; *y = v; } if (v & 4) { mask |= 4; *w = v; } if (v & 8) { mask |= 8; *h = v; } return mask; } int bar4 (void) { int sum = 0; int x, y, w, h; int mask = foo4 (&x, &y, &w, &h); if (mask & 1) sum += x; if (mask & 2) sum += y; if (mask & 4) sum += w; if (mask & 8) sum += h; return sum; } ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-27 12:05 ` Paul Eggert @ 2019-08-27 12:43 ` Eli Zaretskii 0 siblings, 0 replies; 26+ messages in thread From: Eli Zaretskii @ 2019-08-27 12:43 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Tue, 27 Aug 2019 05:05:33 -0700 > Cc: emacs-devel@gnu.org > > Eli Zaretskii wrote: > > > Do you have a guess why GCC might get lost in that code? > > Sure, lots of guesses. Well, I don't think this discussion is going anywhere useful, so let's end it. > > UNINIT is for you to be able to use your tools of choice, and > > perhaps also to cater to your personal stylistic preferences. > > It's not just me using those tools. And UNINIT is not my stylistic preference: I > don't like using UNINIT and when variables need not be initialized I'd rather > just not initialize them (this is the longstanding tradition in GNU and UNIX > code). The only reason for UNINIT is that it's more useful for preventing and > catching bugs than its alternatives are. We clearly disagree here. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 9:47 ` Eli Zaretskii 2019-08-26 15:21 ` Óscar Fuentes @ 2019-08-26 18:50 ` Paul Eggert 2019-08-26 18:56 ` Eli Zaretskii 2019-08-26 23:17 ` Richard Stallman 2 siblings, 1 reply; 26+ messages in thread From: Paul Eggert @ 2019-08-26 18:50 UTC (permalink / raw) To: Eli Zaretskii; +Cc: emacs-devel Eli Zaretskii wrote: > It's not about us, it's about GCC's tendency to turn more and more > warnings on by default. GCC does not enable this particular warning by default, and it's not likely ever to do so given the high rate of false alarms. People run into warnings like this because Emacs 'configure' enables them, and Emacs 'configure' is our responsibility. > I call GCC_LINT "tinkering". Developers and builders should not need to tinker in the typical case, if the default settings are OK. And developers using unusual tools will likely need to specify special arguments to 'configure' or 'make' anyway, so that's OK. > What debugging tools can make a significant difference here, and how > easy/practical is it to use them? I very much doubt they will catch > every use-before-set bug anyway. Valgrind is one. There are also proprietary tools such as Coverity, QA-C, and Oracle Studio. Although they don't catch every use-before-set bug, they catch enough to be useful and they are practical to use. Admittedly they might not be *trivial* to use, but they aren't that hard and they don't (or at least shouldn't) require changing the Emacs source code. Valgrind works with Emacs master as-is, if you configure Valgrind and Emacs correctly and avoid having Emacs call library functions that rely on undefined behavior (Gtk has some, unfortunately, so I typically use -nw when using Valgrind to debug Emacs). I have used Valgrind to find and fix uninitialized-memory bugs in Emacs that would have been very difficult to find simply by using GDB or by staring at the code. As far as I know Valgrind is the best free tool for debugging some types of low-level errors that GCC does not diagnose, and this includes some significant classes of use-before-set bugs. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 18:50 ` Paul Eggert @ 2019-08-26 18:56 ` Eli Zaretskii 0 siblings, 0 replies; 26+ messages in thread From: Eli Zaretskii @ 2019-08-26 18:56 UTC (permalink / raw) To: Paul Eggert; +Cc: emacs-devel > Cc: emacs-devel@gnu.org > From: Paul Eggert <eggert@cs.ucla.edu> > Date: Mon, 26 Aug 2019 11:50:23 -0700 > > > What debugging tools can make a significant difference here, and how > > easy/practical is it to use them? I very much doubt they will catch > > every use-before-set bug anyway. > > Valgrind is one. Valgrind is irrelevant to this case. To sum this up, please in the future don't "fix" my code by putting UNINIT where I decided to use an explicit initialization. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c 2019-08-26 9:47 ` Eli Zaretskii 2019-08-26 15:21 ` Óscar Fuentes 2019-08-26 18:50 ` Paul Eggert @ 2019-08-26 23:17 ` Richard Stallman 2 siblings, 0 replies; 26+ messages in thread From: Richard Stallman @ 2019-08-26 23:17 UTC (permalink / raw) To: Eli Zaretskii; +Cc: eggert, emacs-devel [[[ To any NSA and FBI agents reading my email: please consider ]]] [[[ whether defending the US Constitution against all enemies, ]]] [[[ foreign or domestic, requires you to follow Snowden's example. ]]] > It's not about us, it's about GCC's tendency to turn more and more > warnings on by default. I am concerned about that tendency, since it goes against the decision I made at the outset. Would people like to tell me, off the list, specifics of this? -- Dr Richard Stallman President, Free Software Foundation (https://gnu.org, https://fsf.org) Internet Hall-of-Famer (https://internethalloffame.org) ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-08-27 12:43 UTC | newest] Thread overview: 26+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-24 6:14 [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c Eli Zaretskii 2019-08-25 0:52 ` Paul Eggert 2019-08-25 7:13 ` Eli Zaretskii 2019-08-26 6:34 ` Paul Eggert 2019-08-26 7:54 ` Eli Zaretskii 2019-08-26 8:15 ` Paul Eggert 2019-08-26 9:47 ` Eli Zaretskii 2019-08-26 15:21 ` Óscar Fuentes 2019-08-26 16:13 ` Eli Zaretskii 2019-08-26 18:20 ` Óscar Fuentes 2019-08-26 18:39 ` Eli Zaretskii 2019-08-26 19:09 ` Paul Eggert 2019-08-26 19:15 ` Óscar Fuentes 2019-08-26 19:33 ` Eli Zaretskii 2019-08-26 19:49 ` Óscar Fuentes 2019-08-26 22:33 ` Paul Eggert 2019-08-27 6:12 ` Eli Zaretskii 2019-08-27 7:28 ` Paul Eggert 2019-08-27 8:02 ` Eli Zaretskii 2019-08-27 9:28 ` Paul Eggert 2019-08-27 10:15 ` Eli Zaretskii 2019-08-27 12:05 ` Paul Eggert 2019-08-27 12:43 ` Eli Zaretskii 2019-08-26 18:50 ` Paul Eggert 2019-08-26 18:56 ` Eli Zaretskii 2019-08-26 23:17 ` Richard Stallman
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git 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.