bug#71744: 29.4; SIGSEGV during completion-at-point in lsp-mode with corfu and cape

[Pip Cet via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

"Paul Eggert" <eggert@cs.ucla.edu> writes:
> On 2024-08-16 08:08, Pip Cet wrote:
>> /* Work around GCC bug 54561.  */
>> #if GNUC_PREREQ (4, 3, 0)
>> # pragma GCC diagnostic ignored "-Wclobbered"
>> #endif
>>
>> which means we won't get any warnings at all about such bugs.
>>
>> However, even with that part removed, I don't get a compiler warning
>> about 'c'.
>
> With that part removed I got a warning about 'kb', not 'c'. The 'kb'

Oh, that's interesting! Can you let me know which GCC version that was,
and which compiler flags?

I don't get that diagnostic here.

> warning was a false positive, the 'c' warning a false negative. The most
> likely explanation, I think, is that GCC got confused and put the wrong
> identifier name in its diagnostic. This would help to explain a lot of
> the -Wclobbered false positives we've gotten over the years, which has
> caused us to ignore -Wclobbered.

Hmm. I've looked at the (current) GCC code, and I don't see how it got
confused, but I'll look again :-)

>> What's weird about this is that the store at +5463 that's causing our
>> problem isn't necessary, and neither is the load at +5479. The variable
>> in question, 'kb', is not marked volatile, so it seems strange to me it's
>> stored, then loaded, unnecessarily.
>
> My guess is that GCC internally coalesced the two variables, and got
> confused because one was clobbered and the other was not, and issued a
> warning that it clobbered the variable but mistakenly said 'kb' not 'c'.
> The C standard entitles GCC to clobber 'c' so this isn't wrong code,
> it's just a bad (a *really* bad) diagnostic.

I agree. It is, however, suspiciously badly-optimized code, and it only
happens with -flto here...

Note that, by my reading of the source code, GCC warns only about
variables that it actually clobbered, not variables that might have been
clobbered by another compiler obeying the C standard differently. Maybe
the best way to deal with that is to default to making "clobbered" an
error, not a warning.

>> Paul, do you agree that we should simply mark 'c' as volatile, or do you
>> think this needs further investigation on the GCC side (because of the
>> lack of a -Wclobbered warning being issued, or because of the weird
>> code)?
>
> We definitely should mark 'c' as volatile, or do something equivalent,
> because the Emacs code violates the C standard.
>
> Also, if my hypothesis of the mistaken identifier is correct, we should
> stop ignoring -Wclobbered, and instead pacify GCC without using
> -Wclobbered. That way, when we screw up in setjmp related code, we'll
> get *some* warning that *something* is being clobbered, and can investigate.
>
> To do that, I installed the attached patches into the Emacs master
> branch. While looking into this I noticed several uses of clobbered
> variables (i.e., violations of the C standard) and fixed them. Patches
> 0001-0004 fix these similar bugs elsewhere. Patch 0005 implements your
> suggestion and should fix Bug#71744. Patch 0006 is a minor performance
> improvement over Patch 0005.
>
> Please give these patches a try. Patch 0005 is simple and if it works
> for the original bug reporter we should be able to close the bug report
> as fixed.

Thank you very much! This seems like the best course of action.

Pip