GNU bug report logs - #75451
scratch/igc: Enable CHECK_STRUCTS

Previous Next

Full log

Message #97 received at 75451 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> protonmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: Gerd Möllmann <gerd.moellmann <at> gmail.com>,
 Gerd Moellmann <gerd <at> gnu.org>, Paul Eggert <eggert <at> cs.ucla.edu>,
 75451 <at> debbugs.gnu.org
Subject: Re: bug#75451: scratch/igc: Enable CHECK_STRUCTS
Date: Mon, 20 Jan 2025 20:32:53 +0000

"Stefan Kangas" <stefankangas <at> gmail.com> writes:

> Pip Cet <pipcet <at> protonmail.com> writes:
>
>> Using "break" instead of "return" is impossible:
>>
>> * "default: eassume (0);"  means you lose compiler warnings if the enum is
>> expanded.  -Wswitch-enum generates the compiler warnings, but they're
>> lost in a sea of false positives because sometimes we cannot switch over
>> an enum exhaustively.
>>
>> * "default: emacs_abort ();" will generate code to handle the "impossible"
>> case
>>
>> * Omitting the default statement will make GCC treat the fall-through case
>> as reachable
>>
>> This is because this code:
>>
>> #include <stdio.h>
>>
>> enum three { A, B, C };
>>
>> int main (void)
>> {
>>   volatile enum three four = C + 1;
>>   volatile const char *bad = 0;
>>   switch (four) {
>>   case A:
>>   case B:
>>   case C:
>>     printf ("%s\n", "good");
>>     break;
>>   default:
>>     printf ("%s\n", bad);
>>   }
>>   return 0;
>> }
>>
>> is treated by GCC as valid code and the "bad" printf cannot be
>> determined to be unreachable.
>
> I have no reason to doubt that you are correct, but what are the
> consequences for us, in concrete terms, that make you say that "using
> 'break' instead of 'return' is impossible"?  Given that most programs

I'm sorry, I meant that it's impossible to replace the version that
works with one that doesn't while maintaining the right set of compiler
warnings.  Of course the code will work fine no matter which option you
use: sometimes we'll get warnings about missed cases, sometimes we
won't; sometimes, an unnecessary extra case will be generated, sometimes
it won't.

> run just fine despite the shortcomings and/or bugs you have identified
> in GCC, I think that saying using the switch statement in the normal way
> is "impossible" might be overstating the case.

It certainly is overstating the case, and I'm sorry I wasn't clearer.

What is hard (not impossible) is to get the right warnings from GCC: if
you want your switch statement to be exhaustive, you shouldn't include a
default case; if you don't have a default case, the fall-through path
will be reachable, as far as GCC is concerned, and prompt additional
warnings; if you fix that by adding "default: eassume(0);", you lose the
exhaustiveness warning; if you fix that by adding -Wswitch=enum, you get
too many false positives for enums that aren't even controlled by Emacs;
if you fix that by adding #pragma statements around all the places that
depend on external enums which might change, you have set a lingering
trap for future developers, as #pragmas tend to move away from their
intended region and disable more warnings than you want them to.  (See
bug#71744 / commit 1282714da55cd4bbc1c7f2e49edeb43503427e5e for a bug
made harder to discover by a #pragma that no longer made sense.)

After all this, you still don't get any warnings if you use enum values
in the labels but the switch parameter has an int type: if a new glyph
type is added to enum glyph_type, x_draw_glyph_string won't warn about
it.

(Yes, dispextern.h should use ENUM_BF, will come up with a patch, but
GCC should *also* have warned about the code in x_draw_glyph_string
before that).

> My apologies if I'm missing the point that you're making here.

You're not.  I was getting overexcited and forgot to carefully check
whether pragmas can be made to work.  They're ugly, but right now it
looks like the answer is "yes, if you use ENUM_BF; if you don't, tough
luck".

Thanks for calling me out on this.

> BTW, has this been reported to the GCC developers?

I've only just become aware of how bad it is.  I'll report it in due
time, after searching for previous issues.  The switch ((int) x) { case
ENUM_A: ... } thing was easy enough to fix, and I have a vague feeling
that someone out there is mixing different enums in the same switch
statement and warning about that will help them.

A related issue I don't fully understand yet is why
extern int flag; int main (void)
{
  int ret; // uninitialized
  switch (flag)
  {
    case 0: ret = 1;
  }
  return ret;
}

doesn't appear to produce a warning with -Wuninitialized or
-Wmaybe-uninitialized.

(-Wnull-dereference is even worse: at -O1, gcc doesn't complain about

extern int *flag;
int main (void)
{
  int ret; // uninitialized
  switch (*flag)
  {
    case 0: return 1;
  }
  return ret + *(int *)0;
}

in any way that I can find, and returns 1 unconditionally, ignoring the
flag entirely.  I'll report that one first, I guess).

>> Let's introduce macros for the different cases, and use the same macro
>> in the same case, until GCC offers a fix.
>
> I'm with Gerd here, and not too excited about the prospect of yet
> another macro.  I'd rather do without the additional warning, and just
> have it crash at runtime instead, like we do now.

rsvg doesn't crash, it'll just fail to display the image, IIUC.

> But are we sure that installing my patch would make things worse?

It would make things better!  I disagreed with the "think carefully
about each enum switch and choose between options, then hardcode that
one option in the code so no one can ever see the bugs you decided not
to warn about" approach, particularly if it amounts to leaving
detectable bugs for others to find.

However we decide to do this, there are only two semantically different
valid enum switches: an exhaustive one, and one that cannot be
exhaustive because we don't control the enum, or because we do but it
has too many members for us to list.

If we decide that the former (which should be the much more common case)
doesn't require a default: label, your patch is fine; we'll just have to
make that case work with GCC so it warns about non-exhaustive switches,
and doesn't generate the new spurious warnings I'm seeing here:

pdumper.c:4561:6: warning: ‘mem_prot’ may be used uninitialized [-Wmaybe-uninitialized]
 4561 |   if (mem_prot != PROT_NONE)
      |      ^

> If I'm reading Paul correctly, he's saying that a static check is
> more reliable.

I'll have to reread.  With my GCC, I don't believe that's true: where
the old code will abort, the new code will produce spurious warnings in
some cases, and fall through and leave things uninitialized without
warning about them in others, IIUC.

Pip

Previous Next

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