[Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

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;

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Paul Eggert]

[-- 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

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Paul Eggert]

>> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Paul Eggert]

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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Óscar Fuentes]

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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> > 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Óscar Fuentes]

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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Paul Eggert]

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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Óscar Fuentes]

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.)

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Óscar Fuentes]

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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Paul Eggert]

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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Paul Eggert]

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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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?

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Paul Eggert]

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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Paul Eggert]

[-- 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;
}

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Paul Eggert]

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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Eli Zaretskii]

> 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.

Re: [Emacs-diffs] master 6cd5678: Clarify compiler-pacifier in frame.c

[Richard Stallman]

[[[ 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)