bug#23640: 25.1.50; Getting rid of compiler warnings

bug#23640: 25.1.50; Getting rid of compiler warnings

[Ken Brown]

Building master on Cygwin with gcc-5.3.0 yields many "control reaches
end of non-void function" warnings.  What's the best way to get rid of
these?  Should I add return statements that will never be reached, like
this?

--- a/src/frame.c
+++ b/src/frame.c
@@ -263,6 +263,8 @@ See also `frame-live-p'.  */)
        return Qns;
      default:
        emacs_abort ();
+      /* Pacify compiler.  */
+      return Qnil;
      }
  }

Ken

bug#23640: 25.1.50; Getting rid of compiler warnings

[Eli Zaretskii]

> From: Ken Brown <kbrown@cornell.edu>
> Date: Sat, 28 May 2016 14:40:36 -0400
> 
> Building master on Cygwin with gcc-5.3.0 yields many "control reaches
> end of non-void function" warnings.  What's the best way to get rid of
> these?  Should I add return statements that will never be reached, like
> this?
> 
> --- a/src/frame.c
> +++ b/src/frame.c
> @@ -263,6 +263,8 @@ See also `frame-live-p'.  */)
>         return Qns;
>       default:
>         emacs_abort ();
> +      /* Pacify compiler.  */
> +      return Qnil;
>       }
>   }

emacs_abort is declared with _Noreturn, so how come GCC doesn't shut
up about "unreachable" code?

FWIW, I'm building master with GCC 5.3.0, and don't see these
warnings.

bug#23640: 25.1.50; Getting rid of compiler warnings

[Ken Brown]

On 5/28/2016 2:57 PM, Eli Zaretskii wrote:
>> From: Ken Brown <kbrown@cornell.edu>
>> Date: Sat, 28 May 2016 14:40:36 -0400
>>
>> Building master on Cygwin with gcc-5.3.0 yields many "control reaches
>> end of non-void function" warnings.  What's the best way to get rid of
>> these?  Should I add return statements that will never be reached, like
>> this?
>>
>> --- a/src/frame.c
>> +++ b/src/frame.c
>> @@ -263,6 +263,8 @@ See also `frame-live-p'.  */)
>>         return Qns;
>>       default:
>>         emacs_abort ();
>> +      /* Pacify compiler.  */
>> +      return Qnil;
>>       }
>>   }
>
> emacs_abort is declared with _Noreturn, so how come GCC doesn't shut
> up about "unreachable" code?

It looks like the problem is the definition of _Noreturn as a macro in 
config.h.  I'll have to figure out what's going on.

Ken

bug#23640: 25.1.50; Getting rid of compiler warnings

[Ken Brown]

On 5/28/2016 5:47 PM, Ken Brown wrote:
> On 5/28/2016 2:57 PM, Eli Zaretskii wrote:
>> emacs_abort is declared with _Noreturn, so how come GCC doesn't shut
>> up about "unreachable" code?
>
> It looks like the problem is the definition of _Noreturn as a macro in
> config.h.  I'll have to figure out what's going on.

That guess was wrong.  The problem turns out to be that lint is defined 
in config.h.  When lint is defined, Cygwin's <sys/cdefs.h> defines 
_Noreturn to be a macro with empty expansion.  I've raised the question 
on the Cygwin list 
(https://www.cygwin.com/ml/cygwin/2016-05/msg00374.html) as to whether 
that's a bug.

Ken

bug#23640: 25.1.50; Getting rid of compiler warnings

[Ken Brown]

On 5/29/2016 6:43 PM, Ken Brown wrote:
> On 5/28/2016 5:47 PM, Ken Brown wrote:
>> On 5/28/2016 2:57 PM, Eli Zaretskii wrote:
>>> emacs_abort is declared with _Noreturn, so how come GCC doesn't shut
>>> up about "unreachable" code?
>>
>> It looks like the problem is the definition of _Noreturn as a macro in
>> config.h.  I'll have to figure out what's going on.
>
> That guess was wrong.  The problem turns out to be that lint is defined
> in config.h.  When lint is defined, Cygwin's <sys/cdefs.h> defines
> _Noreturn to be a macro with empty expansion.  I've raised the question
> on the Cygwin list
> (https://www.cygwin.com/ml/cygwin/2016-05/msg00374.html) as to whether
> that's a bug.

The answer is that the Cygwin's <sys/cdefs.h> is taken from FreeBSD, so 
the problem will exist there too.  (I just checked the FreeBSD git repo 
and confirmed that the code in question is still there.)  So it looks 
like defining lint should be disabled on Cygwin and FreeBSD, at least. 
Or maybe it should only be enabled on platforms where it's known that it 
doesn't cause problems.

Paul, you're the one who introduced this.  What do you think?

Ken

bug#23640: 25.1.50; Getting rid of compiler warnings

[Ken Brown]

On 5/30/2016 7:39 AM, Ken Brown wrote:
> On 5/29/2016 6:43 PM, Ken Brown wrote:
>> On 5/28/2016 5:47 PM, Ken Brown wrote:
>>> On 5/28/2016 2:57 PM, Eli Zaretskii wrote:
>>>> emacs_abort is declared with _Noreturn, so how come GCC doesn't shut
>>>> up about "unreachable" code?
>>>
>>> It looks like the problem is the definition of _Noreturn as a macro in
>>> config.h.  I'll have to figure out what's going on.
>>
>> That guess was wrong.  The problem turns out to be that lint is defined
>> in config.h.  When lint is defined, Cygwin's <sys/cdefs.h> defines
>> _Noreturn to be a macro with empty expansion.  I've raised the question
>> on the Cygwin list
>> (https://www.cygwin.com/ml/cygwin/2016-05/msg00374.html) as to whether
>> that's a bug.
>
> The answer is that the Cygwin's <sys/cdefs.h> is taken from FreeBSD, so
> the problem will exist there too.  (I just checked the FreeBSD git repo
> and confirmed that the code in question is still there.)  So it looks
> like defining lint should be disabled on Cygwin and FreeBSD, at least.
> Or maybe it should only be enabled on platforms where it's known that it
> doesn't cause problems.
>
> Paul, you're the one who introduced this.  What do you think?

Another glitch: Removing the line in configure.ac that defines lint 
results in lots of 'may be used uninitialized' warnings.  That's because 
the IF_LINT macro now suppresses all the initializations that were 
previously added to avoid these warnings.

Question: Why bother with IF_LINT at all?  Why not just unconditionally 
initialize the variables that gcc complains about?

Ken

bug#23640: 25.1.50; Getting rid of compiler warnings

[Ken Brown]

On 5/30/2016 10:41 AM, Ken Brown wrote:
> Question: Why bother with IF_LINT at all?  Why not just unconditionally
> initialize the variables that gcc complains about?

If keeping IF_LINT is important, we could do something like this:
 
diff --git a/configure.ac b/configure.ac
index e88a3a9..f628c9a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -997,7 +997,14 @@ AC_DEFUN
     gl_WARN_ADD([-Wno-pointer-sign])
   fi
 
-  AC_DEFINE([lint], [1], [Define to 1 if the compiler is checking for lint.])
+  case $opsys in
+   cygwin | freebsd)
+    AC_DEFINE([gcc_warnings], [1], [Define to 1 if gcc warnings are enabled.])
+      ;;
+   *)
+    AC_DEFINE([lint], [1], [Define to 1 if the compiler is checking for lint.])
+      ;;
+  esac
   AC_DEFINE([GNULIB_PORTCHECK], [1], [enable some gnulib portability checks])
   AH_VERBATIM([GNULIB_PORTCHECK_FORTIFY_SOURCE],
   [/* Enable compile-time and run-time bounds-checking, and some warnings,
diff --git a/src/conf_post.h b/src/conf_post.h
index 5d3394f..987bcad 100644
--- a/src/conf_post.h
+++ b/src/conf_post.h
@@ -344,7 +344,7 @@ extern int emacs_setenv_TZ (char const *);
 #endif
 
 /* Use this to suppress gcc's `...may be used before initialized' warnings. */
-#ifdef lint
+#if defined lint || defined gcc_warnings
 /* Use CODE only if lint checking is in effect.  */
 # define IF_LINT(Code) Code
 #else

bug#23640: 25.1.50; Getting rid of compiler warnings

[Paul Eggert]

Ken Brown wrote:
> Why not just unconditionally initialize the variables that gcc complains about?

That could make the code harder to understand. Someone who sees "int i = 0;" can 
reasonably infer that the 0 is needed, i.e., that a later part of the code needs 
the 0. Someone who sees "int i IF_LINT (= 0);" should be under no such illusion.

The tradition of hijacking the symbol 'lint' for GCC purposes has been used for 
many years in several GNU packages. As the problem you mention affects only 
optionally-generated warnings on non-GNU platforms, I suspect nobody has cared 
until now about the issue. Anyway, I changed Emacs master to use the symbol 
GCC_LINT instead; this should avoid the collision in names. Please give it a try.

The FreeBSD and Cygwin uses of 'lint' violate the C Standard. For example:

#define lint 1
#include <stdio.h>
_Noreturn int x;

This program violates a constraint so the C Standard requires a compile-time 
diagnostic, but from what you say on FreeBSD and Cygwin the compiler misbehaves 
and silently ignores the _Noreturn. I see that HBBroeker is independently making 
a similar point on the Cygwin mailing list 
<https://www.cygwin.com/ml/cygwin/2016-05/msg00405.html>.

bug#23640: 25.1.50; Getting rid of compiler warnings

[Ken Brown]

On 5/30/2016 7:29 PM, Paul Eggert wrote:
> The tradition of hijacking the symbol 'lint' for GCC purposes has been
> used for many years in several GNU packages. As the problem you mention
> affects only optionally-generated warnings on non-GNU platforms, I
> suspect nobody has cared until now about the issue. Anyway, I changed
> Emacs master to use the symbol GCC_LINT instead; this should avoid the
> collision in names.

Thanks.  Closing.

Ken

bug#23640: 25.1.50; Getting rid of compiler warnings

[Andy Moreton]

On Mon 30 May 2016, Paul Eggert wrote:

> Ken Brown wrote:
>> Why not just unconditionally initialize the variables that gcc complains about?
>
> That could make the code harder to understand. Someone who sees "int i = 0;"
> can reasonably infer that the 0 is needed, i.e., that a later part of the code
> needs the 0. Someone who sees "int i IF_LINT (= 0);" should be under no such
> illusion.

Surely this makes the code easier to understand, since "int i = 0;"
immediately shows that the variable is always initialised, so there is
no need to check that it is initialised before use on all code paths
(and recheck after making any chages during maintenance).

Given that the performance cost of this is likely to be unmeasurably
small, the IF_LINT stuff is either unneccessary clutter, or hiding a
latent bug.

> The FreeBSD and Cygwin uses of 'lint' violate the C Standard. For example:
>
> #define lint 1
> #include <stdio.h>
> _Noreturn int x;
>
> This program violates a constraint so the C Standard requires a compile-time
> diagnostic, but from what you say on FreeBSD and Cygwin the compiler
> misbehaves and silently ignores the _Noreturn. I see that HBBroeker is
> independently making a similar point on the Cygwin mailing list
> <https://www.cygwin.com/ml/cygwin/2016-05/msg00405.html>.

That may be so, but any codebase has to deal with the world as it is,
not the ideal in one of the applicable standards.

    AndyM

bug#23640: 25.1.50; Getting rid of compiler warnings

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

I don't think we should clutter up programs with IF_LINT.
Instead, we could define a macro UNUSED with

#define UNUSED 0

and write

   int i = UNUSED;

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

bug#23640: 25.1.50; Getting rid of compiler warnings

[Paul Eggert]

Following up on the compatibility issue with 'lint' and '_Noreturn', I filed a 
FreeBSD bug report here:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=209877

bug#23640: 25.1.50; Getting rid of compiler warnings

[Paul Eggert]

> we could define a macro UNUSED with
>
> #define UNUSED 0
>
> and write
>
>    int i = UNUSED;

This wouldn't be enough to handle other cases, e.g., 'IF_LINT (= Qnil)' and 
'IF_LINT (volatile)'.

bug#23640: 25.1.50; Getting rid of compiler warnings

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

  > This wouldn't be enough to handle other cases, e.g., 'IF_LINT (= Qnil)' and 
  > 'IF_LINT (volatile)'.

A macro UNUSED_LISP could handle the first.

What is the reason for writing IF_LINT (volatile)
instead of just volatile?


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

bug#23640: 25.1.50; Getting rid of compiler warnings

[Paul Eggert]

On 06/01/2016 01:37 PM, Richard Stallman wrote:
> A macro UNUSED_LISP could handle the first.

Yes, we could have separate macros for each data type requiring 
syntactically different initializers. Something like this, say:

     int n UNUSED_0;
     Lisp_Object obj UNUSED_Qnil;

instead of the current:

     int n IF_LINT (= 0);
     Lisp_Object obj IF_LINT (= Qnil);

We could easily change the code in that way. Is it worth the trouble?

> What is the reason for writing IF_LINT (volatile)
> instead of just volatile?

Primarily, to tell the reader that the 'volatile' is not needed for 
correctness; it's present only to pacify a buggy compiler or lint 
checker. (In this case, it pacifies GCC; see GCC bug#54561.) It's 
basically the same reason the code uses 'IF_LINT (= 0)' rather than '= 0'.

bug#23640: 25.1.50; Getting rid of compiler warnings

[Andy Moreton]

On Wed 01 Jun 2016, Paul Eggert wrote:

> On 06/01/2016 01:37 PM, Richard Stallman wrote:
>> A macro UNUSED_LISP could handle the first.
>
> Yes, we could have separate macros for each data type requiring syntactically
> different initializers. Something like this, say:
>
>     int n UNUSED_0;
>     Lisp_Object obj UNUSED_Qnil;
>
> instead of the current:
>
>     int n IF_LINT (= 0);
>     Lisp_Object obj IF_LINT (= Qnil);
>
> We could easily change the code in that way. Is it worth the trouble?
>
>> What is the reason for writing IF_LINT (volatile)
>> instead of just volatile?
>
> Primarily, to tell the reader that the 'volatile' is not needed for
> correctness; it's present only to pacify a buggy compiler or lint checker. (In
> this case, it pacifies GCC; see GCC bug#54561.) It's basically the same reason
> the code uses 'IF_LINT (= 0)' rather than '= 0'.

Adding more macros that cruft up the source code to deal with a buggy
compiler warning is silly. If the warning is broken and does not
operate correctly, then do not enable that warning.

    AndyM

bug#23640: 25.1.50; Getting rid of compiler warnings

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

  > Yes, we could have separate macros for each data type requiring 
  > syntactically different initializers. Something like this, say:

  >      int n UNUSED_0;
  >      Lisp_Object obj UNUSED_Qnil;

Actually, one single macro UNUSED will work for integers
and Lisp_Object:

#define UNUSED = {0}

int n UNUSED;
Lisp_Object obj UNUSED;

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

bug#23640: 25.1.50; Getting rid of compiler warnings

[Paul Eggert]

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

On 06/02/2016 08:35 PM, Richard Stallman wrote:
> one single macro UNUSED will work for integers and Lisp_Object

Yes, that works, though UNUSED is a confusing name to attach to a 
variable, as the variable is actually used. A better name is UNINIT, 
indicating that the variable is uninitialized. Similarly, NONVOLATILE 
could indicate that a variable is not volatile, in cases where GCC would 
otherwise complain. The attached patch would suffice to omit all uses of 
the IF_LINT macro in the Emacs source, if that's a style you prefer.


[-- Attachment #2: 0001-Replace-IF_LINT-by-NONVOLATILE-and-UNINIT.patch --]
[-- Type: application/x-patch, Size: 21057 bytes --]

bug#23640: 25.1.50; Getting rid of compiler warnings

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

Your patch seems reasonable to me.

The reason I chose the name UNUSED is that I had in mind
it would stand for just the initial value (without the =).
But then I imitated the suggestion to include the =.
You're right that name UNUSED is not appropriate for that.

So I suggest

  char *ident_start = UNUSED;

and so on.  That should make it very clear, and the name
fits what it does.

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

bug#23640: 25.1.50; Getting rid of compiler warnings

[Paul Eggert]

Richard Stallman wrote:

>    char *ident_start = UNUSED;

That would set ident_start to zero even when GCC warnings are disabled, which 
would be a bit slower since ident_start does not need to be initialized. 
Unfortunately there is no syntax in C for a no-op initializer, so there's no way 
to define UNUSED so that it initializes ident_start when GCC warnings are 
enabled and leaves ident_start uninitialized otherwise.

bug#23640: 25.1.50; Getting rid of compiler warnings

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

  > That would set ident_start to zero even when GCC warnings are disabled, which 
  > would be a bit slower since ident_start does not need to be initialized. 

I guess so -- but is it a noticeable slowdown?  A priori, I doubt it.

If it is not significant, why worry about it?
Better to write the code in the clean way.

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

bug#23640: 25.1.50; Getting rid of compiler warnings

[Paul Eggert]

Richard Stallman wrote:
> I guess so -- but is it a noticeable slowdown?  A priori, I doubt it.

You're probably right. The major effect these days would be on cache pressure, 
which is tricky to measure.

> If it is not significant, why worry about it?
> Better to write the code in the clean way.

For me it's cleaner to think of UNINIT as an attribute for a declaration, 
instead of thinking of UNUSED as a special expression that neither transfers 
control nor has a side effect nor yields a value. It's merely a plus that UNINIT 
generates code that is a tiny bit smaller and faster.

bug#23640: 25.1.50; Getting rid of compiler warnings

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

  > For me it's cleaner to think of UNINIT as an attribute for a declaration, 
  > instead of thinking of UNUSED as a special expression that neither transfers 
  > control nor has a side effect nor yields a value. It's merely a plus that UNINIT 
  > generates code that is a tiny bit smaller and faster.

I won't shed any more bikes on this question.

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

bug#23640: 25.1.50; Getting rid of compiler warnings

[Paul Eggert]

> Surely this makes the code easier to understand, since "int i = 0;"
> immediately shows that the variable is always initialised

That is a superficial form of understanding, one that could be satisfied by 
initializing all variables whether they need it or not. If instead we make it 
clear that the initialization is present only to work around a GCC deficiency, 
the reader will get a less-shallow understanding.

> any codebase has to deal with the world as it is,
> not the ideal in one of the applicable standards.

Yes, and Emacs was changed to work around the Cygnus and FreeBSD glitches; see:

http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=cb379cbb7fc617f897e7dbc3ce45bf99ea3d1f87

 > the performance cost of this is likely to be unmeasurably small

Yes, it's primarily about making the code clearer, not performance.

GCC is gradually getting smarter in this area, and eventually I hope we can 
remove more IF_LINT instances. I removed several instances recently; see:

http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=237244bbd5ce753bcdf79634561de515bd76c687