C warning in MSYS2 build

C warning in MSYS2 build

[Richard Copley]

Hi,

One more warning crept into the MSYS2 build after the recent update to
the system headers/libraries:

make[1]: Entering directory '/c/projects/emacs/src'
  CC       indent.o
indent.c: In function 'scan_for_column':
indent.c:69:10: warning: potential null pointer dereference [-Wnull-dereference]
   return 0;
          ^
indent.c:69:10: warning: potential null pointer dereference [-Wnull-dereference]
indent.c:69:10: warning: potential null pointer dereference [-Wnull-dereference]
indent.c:69:10: warning: potential null pointer dereference [-Wnull-dereference]
make[1]: Leaving directory '/c/projects/emacs/src'

Re: C warning in MSYS2 build

[Eli Zaretskii]

> From: Richard Copley <rcopley@gmail.com>
> Date: Thu, 5 Oct 2017 18:44:43 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>
> 
> One more warning crept into the MSYS2 build after the recent update to
> the system headers/libraries:
> 
> make[1]: Entering directory '/c/projects/emacs/src'
>   CC       indent.o
> indent.c: In function 'scan_for_column':
> indent.c:69:10: warning: potential null pointer dereference [-Wnull-dereference]
>    return 0;
>           ^
> indent.c:69:10: warning: potential null pointer dereference [-Wnull-dereference]
> indent.c:69:10: warning: potential null pointer dereference [-Wnull-dereference]
> indent.c:69:10: warning: potential null pointer dereference [-Wnull-dereference]
> make[1]: Leaving directory '/c/projects/emacs/src'

Thanks, but it isn't new.  You already reported this very warning in

  http://lists.gnu.org/archive/html/emacs-devel/2017-09/msg00478.html

and I already said here:

  http://lists.gnu.org/archive/html/emacs-devel/2017-09/msg00427.html

that I thought it was a GCC bug.  Paul seemed to confirm that in

  http://lists.gnu.org/archive/html/emacs-devel/2017-09/msg00493.html

I have no idea how to fix this, except by removing -Wnull-dereference
from the warning flags.

Re: C warning in MSYS2 build

[Paul Eggert]

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

On 10/05/2017 11:54 AM, Eli Zaretskii wrote:
> Thanks, but it isn't new.

In my experience this kind of warning can appear and vanish almost at 
whim: compiling at a different optimization levels, or changing 
compilation in some other way, can cause GCC to omit or include the 
warning. So possibly Richard wasn't getting the warning a day ago, but 
got the warning after changing from -O2 to -O0 (or whatever), or by 
upgrading GCC.

If it's a major annoyance we could disable --Wnull-dereference for 
indent.c. I hope that's not needed, though.

Come to think of it, last year I filed a GCC bug report about a similar 
problem with lib-src/etags.c and -Wnull-dereference, here:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71157

The GCC bug is not fixed yet. etags.c confused GCC by using an 
assignment inside an 'if' expression, which is contrary to the usual GNU 
style, and last year I worked around the GCC bug by changing etags.c to 
use a cleaner style. I notice that indent.c also has an assignment 
inside an 'if' expression that is relevant to these warnings. Richard, 
does it help to recode indent.c to use the usual GNU style, as in the 
attached patch? If so, let's do that instead. (Perhaps we should do that 
anyway....)


[-- Attachment #2: indent.diff --]
[-- Type: text/x-patch, Size: 761 bytes --]

diff --git a/src/indent.c b/src/indent.c
index 26507b5eb5..79841a3d2c 100644
--- a/src/indent.c
+++ b/src/indent.c
@@ -76,15 +76,17 @@ buffer_display_table (void)
 static int
 character_width (int c, struct Lisp_Char_Table *dp)
 {
-  Lisp_Object elt;
-
   /* These width computations were determined by examining the cases
      in display_text_line.  */
 
   /* Everything can be handled by the display table, if it's
      present and the element is right.  */
-  if (dp && (elt = DISP_CHAR_VECTOR (dp, c), VECTORP (elt)))
-    return ASIZE (elt);
+  if (dp)
+    {
+      Lisp_Object elt = DISP_CHAR_VECTOR (dp, c);
+      if (VECTORP (elt))
+	return ASIZE (elt);
+    }
 
   /* Some characters are special.  */
   if (c == '\n' || c == '\t' || c == '\015')

Re: C warning in MSYS2 build

[Richard Copley]

On 5 October 2017 at 20:32, Paul Eggert <eggert@cs.ucla.edu> wrote:
> On 10/05/2017 11:54 AM, Eli Zaretskii wrote:
>>
>> Thanks, but it isn't new.
>
> In my experience this kind of warning can appear and vanish almost at whim:
> compiling at a different optimization levels, or changing compilation in
> some other way, can cause GCC to omit or include the warning. So possibly
> Richard wasn't getting the warning a day ago, but got the warning after
> changing from -O2 to -O0 (or whatever), or by upgrading GCC.
>
> If it's a major annoyance we could disable --Wnull-dereference for indent.c.
> I hope that's not needed, though.
>
> Come to think of it, last year I filed a GCC bug report about a similar
> problem with lib-src/etags.c and -Wnull-dereference, here:
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71157
>
> The GCC bug is not fixed yet. etags.c confused GCC by using an assignment
> inside an 'if' expression, which is contrary to the usual GNU style, and
> last year I worked around the GCC bug by changing etags.c to use a cleaner
> style. I notice that indent.c also has an assignment inside an 'if'
> expression that is relevant to these warnings. Richard, does it help to
> recode indent.c to use the usual GNU style, as in the attached patch? If so,
> let's do that instead. (Perhaps we should do that anyway....)

Thanks Eli and Paul.
The warnings don't change with that patch.
FWIW it seems to me that the results of the buffer_display_table calls
in c_string_width and lisp_string_width are dereferenced unchecked,
in character.c.

Re: C warning in MSYS2 build

[Paul Eggert]

On 10/05/2017 01:08 PM, Richard Copley wrote:
> FWIW it seems to me that the results of the buffer_display_table calls
> in c_string_width and lisp_string_width are dereferenced unchecked,
> in character.c.

I don't see how those two functions dereference the results. Although 
they do pass results to char_width, char_width has a run-time check that 
prevents dereferencing the results when null.

Re: C warning in MSYS2 build

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

  > The GCC bug is not fixed yet. etags.c confused GCC by using an 
  > assignment inside an 'if' expression, which is contrary to the usual GNU 
  > style, and last year I worked around the GCC bug by changing etags.c to 
  > use a cleaner style. I notice that indent.c also has an assignment 
  > inside an 'if' expression that is relevant to these warnings. Richard, 
  > does it help to recode indent.c to use the usual GNU style, as in the 
  > attached patch?

I don't understand the question.

An assignment inside an if condition is somewhat ugly,
and moving the assignment out of the code is in general
a good thing for clarity.  So by all means do it.
If that works around a GCC bug, that is good -- but the bug
still needs to be fixed.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: C warning in MSYS2 build

[Richard Copley]

On 6 October 2017 at 16:16, Richard Stallman <rms@gnu.org> wrote:
> [[[ 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. ]]]
>
>   > The GCC bug is not fixed yet. etags.c confused GCC by using an
>   > assignment inside an 'if' expression, which is contrary to the usual GNU
>   > style, and last year I worked around the GCC bug by changing etags.c to
>   > use a cleaner style. I notice that indent.c also has an assignment
>   > inside an 'if' expression that is relevant to these warnings. Richard,
>   > does it help to recode indent.c to use the usual GNU style, as in the
>   > attached patch?
>
> I don't understand the question.

Paul was asking me to test the attached patch.

> An assignment inside an if condition is somewhat ugly,
> and moving the assignment out of the code is in general
> a good thing for clarity.  So by all means do it.

That's not the proposal. You're free to read the patch if you wish.

> If that works around a GCC bug, that is good -- but the bug
> still needs to be fixed.

Indeed, but fortunately the impact is low.