Re: master a99ba59aa02: ; * src/pdumper.c (dump_do_fixup): Pacify GCC.

[Pip Cet]

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

>> Date: Thu, 30 Jan 2025 09:58:49 +0000
>> From: Pip Cet <pipcet@protonmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>, Stefan Kangas <stefankangas@gmail.com>, 
>> Paul Eggert <eggert@cs.ucla.edu>
>>
>> "Eli Zaretskii" <eliz@gnu.org> writes:
>>
>> > branch: master
>> > commit a99ba59aa02ef8cfd314737950b6cd8d97015925
>> > Author: Eli Zaretskii <eliz@gnu.org>
>> > Commit: Eli Zaretskii <eliz@gnu.org>
>> >
>> >     ; * src/pdumper.c (dump_do_fixup): Pacify GCC.
>>
>> > ---
>> >  src/pdumper.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/src/pdumper.c b/src/pdumper.c
>> > index 9f0447eb5aa..bfa790b963a 100644
>> > --- a/src/pdumper.c
>> > +++ b/src/pdumper.c
>> > @@ -3990,7 +3990,7 @@ dump_do_fixup (struct dump_context *ctx,
>> >    Lisp_Object arg = dump_pop (&fixup);
>> >    eassert (NILP (fixup));
>> >    dump_seek (ctx, dump_fixup_offset);
>> > -  intptr_t dump_value;
>> > +  intptr_t dump_value UNINIT;
>>
>> Can we please stop rewriting every single switch statement again and
>> again, each time attempting to solve one problem which will make
>> another, worse one, pop up instead?
>
> The above commit didn't rewrite the switch, it just shuts up the

Correct.  It's just one of many, many commits we'll see if we decide on
this problematic way of getting GCC warnings at the cost of correctness,
performance, and loss of run-time warnings.

> compiler which evidently cannot figure out that dump_value is not
> initialized only in the cases where dump_write is false, which means
> dump_write is not called, and the value of dump_value is not used.

THat's incorrect.  If type is 0x5a, for example, do_write will be true,
dump_value will be uninitialized, and dump_write will be called with a
pointer to the uninitialized dump_value.

>> UNINIT, IMHO, is to be used in situations where GCC fails to discover
>> that a code path to a usage of the variable lacking an initializer is
>> impossible.
>
> That's the situation here.

No, it's not: a variable of type enum X can legally have values that
aren't in the enum.  The compiler needs to assume it does to produce
correct code.

>> This is a very different case.  By removing the default case in the
>> switch statement, we made the code path which falls through the switch
>> statement without initializing dump_value reachable.  There's not even a
>> hint to the compiler that this is an unlikely code path, so the
>> resulting code will be significantly worse than what would have been
>> possible before.
>
> The default case was removed before the above commit, and not by me.

Correct.  This commit demonstrated the consequence of getting enum
switch statements wrong; in this case, it made you use UNINIT
incorrectly.

>> Can we please just go back to the "default: emacs_abort();" we had
>> before?
>
> I don't mind going back.  Stefan?

I'll prepare and post (not push!) a patch to see what that would look
like.  But, Stefan (and others), feel perfectly free to ignore the patch
and discuss the issue in English first, if that's better :-)

Pip