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

[Pip Cet <pipcet@protonmail.com>]

"Eli Zaretskii" <eliz@gnu.org> writes:

>> From: Andrea Corallo <acorallo@gnu.org>
>> Cc: emacs-devel@gnu.org, eliz@gnu.org, Stefan Kangas <stefankangas@gmail.com>
>> Date: Mon, 19 Aug 2024 10:43:00 -0400
>>
>> Paul Eggert <eggert@cs.ucla.edu> writes:
>>
>> > On 2024-08-18 00:39, Andrea Corallo wrote:
>> >> Has this optimization any measurable effect?
>> >
>> > On my platform (Ubuntu 24.04 LTS, x86-64, gcc -O2) it shrinks the size
>> > of read_char's machine code by 5%.
>>
>> Don't know what the other maintainers think about this but FWIW I don't
>> like this change (and similar ones).  Our codebase is already
>> sufficiently tricky and convoluted, complexifying code for no observable
>> improvements should be IMO out of our goals.
>
> I tend to agree.  Especially given that the rationale for this
> juggling and why exactly we assign and re-assign these two variables
> in those particular places is nowhere to be found, neither in the
> source code nor in the commit log message.

Could we measure the performance impact of making all stack variables in
functions that call setjmp() volatile?

As I pointed out in the bug thread, gcc's -Wclobbered warning doesn't
actually complain about all violations of the C standard, it only
complains about registers that will definitely be clobbered in a
specific build.  As it's unpredictable which build options distributors
will use (in the case of this bug, the combination was "-flto=auto 
-fexceptions".  I don't even know whether -flto=auto results in
reproducible builds!), I think we should do one of these things:

1. make all automatic variables in setjmp()-calling functions volatile,
perhaps with one or two well-documented exceptions
2. default to -Werror=clobbered, which will break distributors' builds
rather than letting them ship broken Emacs versions (as Arch is doing).
3. adapt the GCC analyzer (-fanalyzer, find a machine with enough RAM to
run it on Emacs...) to report ALL instances of potentially clobbered
variables, as it has enough information to do so.

(3) is an "interesting" project, meaning that I looked at the analyzer
code today and have no clue where to even start doing it. (2) is
problematic because distributors will simply remove the -Werror.  So (1)
is our best choice, I'm afraid.

We can add a comment deploring how C pretends setjmp() is an ordinary
function; GCC and clang on POSIX systems don't (and can't) treat it that
way, and on Windows systems things are even more complicated.

Maybe we can even add -Wsetjmp-with-any-nonvolatiles to GCC...

Pip