Re: master 8c81818673a 6/7: Tune volatile in read_char

[Pip Cet <pipcet@protonmail.com>]

"Andrea Corallo" <acorallo@gnu.org> writes:

> Paul Eggert <eggert@cs.ucla.edu> writes:
>
>> branch: master
>> commit 8c81818673ae9ff788c6e65fb90984f327b27964
>> Author: Paul Eggert <eggert@cs.ucla.edu>
>> Commit: Paul Eggert <eggert@cs.ucla.edu>
>>
>>     Tune volatile in read_char
>>
>>     * src/keyboard.c (read_char): Optimize access to a local volatile.
>
> Hi Paul,
>
> this change building with "--enable-checking=all
> --enable-check-lisp-object-type" on my system (GCC 14
> x86_64-pc-linux-gnu) is introducing:
>
> "keyboard.c:2520:15: warning: variable ‘c’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]"

Well, GCC is late to the party, we already found that bug ;-)

I think GCC, by its own logic, is right about this one. It doesn't know
that local_getcjmp is local (and hasn't escaped), so it would be, in
theory, possible for a signal handler to call 'longjmp' before
c_volatile is initialized.  And since GCC does not distinguish between
the two branches following a 'setjmp', it might end up using the value
of 'c' which might hypothetically have been clobbered.

Of course there are many reasons that can't actually happen, but the GCC
limitations are well known and we can work around them:

diff --git a/src/keyboard.c b/src/keyboard.c
index 0d3506bc59b..0992fab653b 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2752,7 +2752,7 @@ read_char (int commandflag, Lisp_Object map,
      it *must not* be in effect when we call redisplay.  */
 
   specpdl_ref jmpcount = SPECPDL_INDEX ();
-  Lisp_Object volatile c_volatile;
+  Lisp_Object volatile c_volatile = c;
   if (sys_setjmp (local_getcjmp))
     {
       c = c_volatile;
@@ -2800,7 +2800,6 @@ read_char (int commandflag, Lisp_Object map,
       goto non_reread;
     }
 
-  c_volatile = c;
 #if GCC_LINT && __GNUC__ && !__clang__
   /* This useless assignment pacifies GCC 14.2.1 x86-64
      <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=21161>.  */

Does that help? It does here, but these warnings are highly dependent on
the precise GCC version and optimization flags used, which is why I
still think we should default to -Werror=clobbered to catch distributors
performing LTO builds and such.

Pip