GNU bug report logs - #75964
Switching the Emacs build to -Wswitch-enum in src/

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 75964 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com>
Subject: bug#75964: Switching the Emacs build to -Wswitch-enum in src/
Date: Sat, 01 Feb 2025 09:02:57 +0000

"Paul Eggert" <eggert <at> cs.ucla.edu> writes:

> On 2025-01-31 03:05, Pip Cet wrote:
>> This GCC version is also patched to "infer" a switch type
>
> Suppose the inference is wrong: how does one easily pacify the modified
> GCC? Is it something like "case +PVEC_NORMAL:"?

I haven't submitted a patch yet, because I wasn't sure of such details.
In this case, yes, case +PVEC_NORMAL would work, but I had to test this :-)

> Suppose one case has an enum type and another case has some other type:
> is that an error?

I have code to make mixing several enums and mixing enums and integer
types an error, but the second is in use in Emacs, so that switch would
be a bit useless, and extending enums by starting a second enum starting
with the last value of the first one is also quite common.

>
>> +    case PVEC_FREE: eassume (0);
>> +    default: eassume (0);
>
> Must this be written that way? Suppose I do this:
>
>     case PVEC_FREE:
>     default:
>       eassume (false);

> Does that also pacify the modified GCC? Also, does emacs_abort () also
> pacify the modified GCC?

Absolutely.  You can also handle the default case, of course, it just
has to be there in addition to one case per enumerated value.

> Perhaps we should define a new macro eunreachable () that expands to
> eassume (false). eunreachable : unreachable :: eassert : assert. This
> might be a good idea regardless of anything else we do.

I was thinking the same thing!

I was going to propose emacs_unreachable (), but using the

emacs_abort : emacs_unreachable :: eassert : eassume

diagram.

>> alloc.c was a simple case because the enums are all internal
>> to Emacs and unlikely to change drastically without people touching the
>> code anyway.  image.c is very different in that regard.
>
> How does that sort of thing pan out?

Not sure I understand the question.  Right now, image people extend
their enums, we don't handle the new cases, the bug goes unnoticed until
it bites the user.

>> +  ws="$ws -Wswitch-enum"
>
> This won't work with the latest stable GCC, as it will cause false
> positives. We need a way to have Emacs use -Wswitch-enum only with the
> proposed patch installed.

While modifying GCC seems (to me) important, the behavior of unmodified
GCC is to produce some of the warnings we want, and I'm testing my patch
with both versions.

The difference is that modified GCC warns about some additional cases.
Most importantly, many enum values are stored in plain integer bitfields
rather than ENUM_BF ones, and the modified GCC warns about those.
(struct glyph::type is an example).  While that should be fixed by
turning them into ENUM_BF, we get warnings in the meantime.

> Perhaps the new GCC behavior should be enabled
> by a new GCC option (not -Wswitch-enum), so that it's easy to test
> whether the proposed GCC behavior is available.

I believe that applies to the "mismatched enum" code (two different
enums/enums and ints), but simply inferring that a switch is an enum
switch because all of its case labels are seems safe to me.  But I'll
think about it some more.

>> +ALL_CFLAGS = ${BASE_CFLAGS} ${PROFILING_CFLAGS} ${LDFLAGS} ${CPPFLAGS} ${CFLAGS} ${PACIFYING_CFLAGS}
>>  CPP_CFLAGS = ${BASE_CFLAGS} ${PROFILING_CFLAGS} ${CPPFLAGS} ${CFLAGS}
>>  o
>>  ALL_CXXFLAGS = $(filter-out ${NON_CXX_CFLAGS},${BASE_CFLAGS}) \
>>    ${PROFILING_CFLAGS} ${LDFLAGS} ${CPPFLAGS} ${CFLAGS} ${CXXFLAGS} ${HAIKU_CFLAGS}
>>
>> +unfixed_Wswitch_obj += ctags${EXEEXT}
>> +unfixed_Wswitch_obj += etags${EXEEXT}
>
> This sort of thing can be confusing. Do we need it at all? That is, once
> the fixes are in the C source files we shouldn't need to change the
> Makefile.

That was my initial reason for making it so unfixed_Wswitch_obj would
eventually be empty.

> Alternatively, perhaps it'd be better to use a pragma in the (I hope)
> rare places we need to pacify GCC.

I did propose that as an alternative, yes.

> E.g., how we currently treat
> -Wanalyzer-null-dereference in four .c files.

I've been bitten by "old" #pragmas disabling "bogus" warnings which
turned out not to be bogus before, and I think the #pragma syntax is
horrible, so I'd prefer the case +A: approach.

But the important thing is we start doing this to some files, not how we
fix what are likely to be two or three problematic files.  I'm totally
open to suggestions there.

The modified GCC is in no way necessary to switch to switch-enum; it
does catch some more cases, but those cases caught by plain old GCC are
those we should fix first.

The only effect of the modified GCC is that if we try working around the
current GCC's behavior by casting the switch value, that won't work with
the modified GCC.

Thank you so much for those comments!  Please do let me know if there's
anything else I can help with!

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.