Warnings/errors related to possibly clobbered variables

Warnings/errors related to possibly clobbered variables

[Dmitry Antipov]

With:

configure --prefix=/not/exists --enable-checking --enable-check-lisp-object-type --enable-gcc-warnings --with-x-toolkit=lucid

I got:

../../emacs/src/keyboard.c: In function ‘read_char’:
../../emacs/src/keyboard.c:2376:15: warning: variable ‘c’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
    Lisp_Object c;
                ^
But with:

configure --prefix=/not/exists --enable-checking --enable-check-lisp-object-type --enable-gcc-warnings --with-x-toolkit=no

the same warning is treated as fatal error:

../../emacs/src/keyboard.c: In function ‘read_char’:
../../emacs/src/keyboard.c:2376:15: error: variable ‘c’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered]
    Lisp_Object c;
                ^
cc1: all warnings being treated as errors

Why this is so?

Dmitry

Re: Warnings/errors related to possibly clobbered variables

[Andreas Schwab]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> configure --prefix=/not/exists --enable-checking --enable-check-lisp-object-type --enable-gcc-warnings --with-x-toolkit=lucid

  case $with_x_toolkit in
    lucid | athena | motif)
       # Old toolkits mishandle 'const'.
       nw="$nw -Wwrite-strings"
       ;;
    *)
       gl_WARN_ADD([-Werror], [WERROR_CFLAGS])
       ;;
  esac

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Warnings/errors related to possibly clobbered variables

[Dmitry Antipov]

On 01/13/2015 02:51 PM, Andreas Schwab wrote:

>    case $with_x_toolkit in
>      lucid | athena | motif)
>         # Old toolkits mishandle 'const'.
>         nw="$nw -Wwrite-strings"
>         ;;
>      *)
>         gl_WARN_ADD([-Werror], [WERROR_CFLAGS])
>         ;;
>    esac

Yes, but the question really was "how seriously we should treat
warnings about XXX which might be clobbered by YYY" :-).

Dmitry

Re: Warnings/errors related to possibly clobbered variables

[Eli Zaretskii]

> Date: Tue, 13 Jan 2015 15:12:52 +0300
> From: Dmitry Antipov <dmantipov@yandex.ru>
> Cc: eggert@cs.ucla.edu, emacs-devel@gnu.org
> 
> On 01/13/2015 02:51 PM, Andreas Schwab wrote:
> 
> >    case $with_x_toolkit in
> >      lucid | athena | motif)
> >         # Old toolkits mishandle 'const'.
> >         nw="$nw -Wwrite-strings"
> >         ;;
> >      *)
> >         gl_WARN_ADD([-Werror], [WERROR_CFLAGS])
> >         ;;
> >    esac
> 
> Yes, but the question really was "how seriously we should treat
> warnings about XXX which might be clobbered by YYY" :-).

Why not declare the offending variables 'volatile' and forget about
all this stuff?

Re: Warnings/errors related to possibly clobbered variables

[Paul Eggert]

Eli Zaretskii wrote:
> Why not declare the offending variables 'volatile' and forget about
> all this stuff?

When this topic last came up, I vaguely recall Stefan saying that we shouldn't 
declare local variables 'volatile' merely to work around GCC bugs, and that we 
should get GCC fixed instead.  I filed a bug report, but the bug has not been 
fixed yet; see:

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

Re: Warnings/errors related to possibly clobbered variables

[Stefan Monnier]

> Why not declare the offending variables 'volatile' and forget about
> all this stuff?

FWIW, `volatile' might not necessarily give us the behavior we
want/need.  IOW if the warning is legit, we should investigate and
decide exactly how to fix the problem (which might involve using
`volatile' or it might on the contrary involve making a copy of the
current value).

If the warning is not legit, then it's a problem in GCC and we shouldn't
do anything on our side other than report a bug.


        Stefan "-Werror is wrong"

Re: Warnings/errors related to possibly clobbered variables

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Dmitry Antipov <dmantipov@yandex.ru>,  schwab@suse.de,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Tue, 13 Jan 2015 14:43:11 -0500
> 
> > Why not declare the offending variables 'volatile' and forget about
> > all this stuff?
> 
> FWIW, `volatile' might not necessarily give us the behavior we
> want/need.

How so?  Are you saying that qualifying an automatic variable as
'volatile' might change the semantics of the program?

> IOW if the warning is legit, we should investigate and
> decide exactly how to fix the problem (which might involve using
> `volatile' or it might on the contrary involve making a copy of the
> current value).
> 
> If the warning is not legit, then it's a problem in GCC and we shouldn't
> do anything on our side other than report a bug.

The compiler is rarely wrong in these cases, especially if the warning
comes from several different versions of it.  Programs are much better
than people in examining control flow.  They don't understand the
hidden relations between the offending variable and other conditions,
so they can flag problems that don't really exist.  But they rarely
flag something that is clearly wrong.

IME, adding 'volatile' is generally TRT in these cases.  Plus, it's
very simple, and goes "by the book" (a.k.a. the C Standard).

Re: Warnings/errors related to possibly clobbered variables

[Stefan Monnier]

>> > Why not declare the offending variables 'volatile' and forget about
>> > all this stuff?
>> FWIW, `volatile' might not necessarily give us the behavior we
>> want/need.
> How so?  Are you saying that qualifying an automatic variable as
> 'volatile' might change the semantics of the program?

GCC's warning basically points out that the semantics of the program is
unclear, because when we come back from a longjmp we could either see
the latest value of the variable, or a value it had earlier when we did
the setjmp.

So using `volatile' chooses one of the two behaviors, and it might not
be the one we need.

> IME, adding 'volatile' is generally TRT in these cases.  Plus, it's
> very simple, and goes "by the book" (a.k.a. the C Standard).

FWIW, the last time I had to fight with this kind of interaction between
longjmp and local variables, I ended up installing the patch below to
fix the random crashes I was seeing: the problem is more subtle than
just "add `volatile'".


        Stefan


diff --git a/src/bytecode.c b/src/bytecode.c
index 0ea646a..f1bdfd9 100644
--- a/src/bytecode.c
+++ b/src/bytecode.c
@@ -501,7 +501,6 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
 		Lisp_Object args_template, ptrdiff_t nargs, Lisp_Object *args)
 {
   ptrdiff_t count = SPECPDL_INDEX ();
-  ptrdiff_t volatile count_volatile;
 #ifdef BYTE_CODE_METER
   int volatile this_op = 0;
   int prev_op;
@@ -509,14 +508,12 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
   int op;
   /* Lisp_Object v1, v2; */
   Lisp_Object *vectorp;
-  Lisp_Object *volatile vectorp_volatile;
 #ifdef BYTE_CODE_SAFE
-  ptrdiff_t volatile const_length;
-  Lisp_Object *volatile stacke;
-  ptrdiff_t volatile bytestr_length;
+  ptrdiff_t const_length;
+  Lisp_Object *stacke;
+  ptrdiff_t bytestr_length;
 #endif
   struct byte_stack stack;
-  struct byte_stack volatile stack_volatile;
   Lisp_Object *top;
   Lisp_Object result;
   enum handlertype type;
@@ -1122,9 +1119,6 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
 	    PUSH_HANDLER (c, tag, type);
 	    c->bytecode_dest = dest;
 	    c->bytecode_top = top;
-	    count_volatile = count;
-	    stack_volatile = stack;
-	    vectorp_volatile = vectorp;
 
 	    if (sys_setjmp (c->jmp))
 	      {
@@ -1135,12 +1129,11 @@ exec_byte_code (Lisp_Object bytestr, Lisp_Object vector, Lisp_Object maxdepth,
 		handlerlist = c->next;
 		PUSH (c->val);
 		CHECK_RANGE (dest);
-		stack = stack_volatile;
+		/* Might have been re-set by longjmp!  */
+		stack.byte_string_start = SDATA (stack.byte_string);
 		stack.pc = stack.byte_string_start + dest;
 	      }
 
-	    count = count_volatile;
-	    vectorp = vectorp_volatile;
 	    NEXT;
 	  }
 

Re: Warnings/errors related to possibly clobbered variables

[Paul Eggert]

On 01/13/2015 12:10 PM, Eli Zaretskii wrote:
> The compiler is rarely wrong in these cases

But it does appear to be wrong in this case.  That's why I filed a 
compiler bug, and the GCC maintainers seem to agree that it's a bug.

Long ago when I ran into this set of GCC bugs, I shrugged and added 
'volatile's to pacify the buggy GCCs.  Stefan removed some of them as 
part of some of his other fixes, but I suspect that a few 'volatile's 
are not necessary even today (except perhaps to pacify the GCC bug).

I say "set of GCC bugs" because I suspect there are more than one, some 
of which have been fixed, and some of which remain.