Subject: bug#75451: scratch/igc: Enable CHECK_STRUCTS

From: Pip Cet <[email-masked]>
Date: Mon, 20 Jan 2025 08:09:32 +0000

"Paul Eggert" <[email-masked]> writes:

> On 2025-01-19 18:01, Stefan Kangas wrote:
>> In my attached patch,
>> there is one removal of 'default: eassume(false)'; how can I know if
>> it's worth keeping or not?
>
> There are a few reasons to have such a line. First, to pacify a GCC
> false alarm. Second, to make the code clearer to a human reader. Third
> (less likely), to generate more efficient code. If no reason applies I
> would omit such lines.

I think this approach is bad.  We should determine whether the switch is
meant to be exhaustive across an enum, and use a macro in this case, or
whether it isn't, and use another macro.

After that, we're done, and we'll have to wait for GCC to be fixed.  We
cannot and should not look at each individual case and decide
arbitrarily what to do.

GCC does not currently provide a way to define these macros: either you
pass -Wswitch-enum, and all switches over enum types produce warnings
(losing the ones you actually care about in a sea of false positives you
cannot fix), or you don't, and then we lose valuable warnings and
introduce bugs such as this one:

svg_css_length_to_pixels doesn't handle the RSVG_UNIT_CH case.  I don't
know at which version rsvg started to offer this unit; it's not
indicated in the header file.  The default case:

    default:
      /* We should never reach this.  */
      value = 0;

contains an incorrect comment (the header file warns that the enum is
incomplete and will be expanded), and incorrect code (0 is not a good
default value, and we should warn or return an error).

(I'm not a fan of statement attributes, but they're probably the best
solution here.  They were introduced for fallthrough, so I don't see why
they couldn't distinguish the two very different semantics of "default"
labels.  See
https://gcc.gnu.org/onlinedocs/gcc/Statement-Attributes.html)

>>    int type = XFIXNUM (dump_pop (&lreloc));
>>    reloc.emacs_offset = dump_off_from_lisp (dump_pop (&lreloc));
>>    dump_check_emacs_off (reloc.emacs_offset);
>> -  switch (type)
>> +  switch ((enum emacs_reloc_type) type)
>
> When it's easy, let's avoid casts like that as they're too powerful: you

I agree this cast should be avoided.  GCC incorrectly decided to
determine whether a switch is an enum switch or not based only on the
type of the switch value, not on the type of the case labels.  This is
fixable (see what I posted before) and should be fixed.

The correct code is the old version.  That GCC fails to produce a
warning is a GCC deficiency which needs to be fixed.

> can cast pointers by mistake. Here it's easy to omit the cast, and
> declare the local variable 'type' to be of type 'enum emacs_reloc_type'
> rather than of type 'int'. (I assume you know for other reasons that the
> value is in range for the enum - otherwise the cast wouldn't be valid
> either.) In other words, just change the first line to:
>
>     enum emacs_reloc_type type = XFIXNUM (dump_pop (&lreloc));

In this case, I agree this change should be made, but only because we
already have a variable.  If that weren't true, introducing one to work
around the GCC deficiency here is doing the compiler's job, and we
should avoid doing that if it's easier to make the compiler do it.

switch (XFIXNUM (lispval))
  {
    case ABC_A:
      return a;
    case ABC_B:
      return b;
    default:
      __attribute__ ((warn_if_covered_by_enum_case));
      eassume (0);
  }

should produce a warning (if the ABC enum contains A, B, and C).  We
don't have that attribute yet, so let's introduce a macro and wait for
it to be implemented.

Pip