Disable -Wimplicit-fallthrough warning for Modules/expat

[sobolevn]

Bug report

Right now compilers produce a lot of warnings for vendored modules:

• https://github.com/python/cpython/actions/runs/11341852046/job/31541004707#step:8:248

But, there's not much we can do about them, since they are vendored. For example, siphash has explicit fall-through comments:

cpython/Modules/expat/siphash.h

Lines 235 to 243 in 92af191

	case 7:
	b |= (uint64_t)H->buf[6] << 48;
	/* fall through */
	case 6:
	b |= (uint64_t)H->buf[5] << 40;
	/* fall through */
	case 5:
	b |= (uint64_t)H->buf[4] << 32;
	/* fall through */

But, it is still reported:

 In file included from ./Modules/expat/xmlparse.c:121:
./Modules/expat/siphash.h:238:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  case 6:
  ^
./Modules/expat/siphash.h:238:3: note: insert '__attribute__((fallthrough));' to silence this warning
  case 6:
  ^
  __attribute__((fallthrough)); 
./Modules/expat/siphash.h:238:3: note: insert 'break;' to avoid fall-through
  case 6:
  ^
  break; 
./Modules/expat/siphash.h:241:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  case 5:
  ^
./Modules/expat/siphash.h:241:3: note: insert '__attribute__((fallthrough));' to silence this warning
  case 5:
  ^
  __attribute__((fallthrough)); 
./Modules/expat/siphash.h:241:3: note: insert 'break;' to avoid fall-through
  case 5:
  ^
  break; 
./Modules/expat/siphash.h:244:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  case 4:
  ^
./Modules/expat/siphash.h:244:3: note: insert '__attribute__((fallthrough));' to silence this warning
  case 4:
  ^
  __attribute__((fallthrough)); 
./Modules/expat/siphash.h:244:3: note: insert 'break;' to avoid fall-through
  case 4:
  ^
  break; 
./Modules/expat/siphash.h:247:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  case 3:
  ^
./Modules/expat/siphash.h:247:3: note: insert '__attribute__((fallthrough));' to silence this warning
  case 3:
  ^
  __attribute__((fallthrough)); 
./Modules/expat/siphash.h:247:3: note: insert 'break;' to avoid fall-through
  case 3:
  ^
  break; 
./Modules/expat/siphash.h:250:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  case 2:
  ^
./Modules/expat/siphash.h:250:3: note: insert '__attribute__((fallthrough));' to silence this warning
  case 2:
  ^
  __attribute__((fallthrough)); 
./Modules/expat/siphash.h:250:3: note: insert 'break;' to avoid fall-through
  case 2:
  ^
  break; 
./Modules/expat/siphash.h:253:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  case 1:
  ^
./Modules/expat/siphash.h:253:3: note: insert '__attribute__((fallthrough));' to silence this warning
  case 1:
  ^
  __attribute__((fallthrough)); 
./Modules/expat/siphash.h:253:3: note: insert 'break;' to avoid fall-through
  case 1:
  ^
  break; 
./Modules/expat/siphash.h:256:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  case 0:
  ^
./Modules/expat/siphash.h:256:3: note: insert 'break;' to avoid fall-through
  case 0:
  ^
  break; 
./Modules/expat/xmlparse.c:1944:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  default:
  ^
./Modules/expat/xmlparse.c:1944:3: note: insert '__attribute__((fallthrough));' to silence this warning
  default:
  ^
  __attribute__((fallthrough)); 
./Modules/expat/xmlparse.c:1944:3: note: insert 'break;' to avoid fall-through
  default:
  ^
  break; 
./Modules/expat/xmlparse.c:2067:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
  default:
  ^
./Modules/expat/xmlparse.c:2067:3: note: insert '__attribute__((fallthrough));' to silence this warning
  default:
  ^
  __attribute__((fallthrough)); 
./Modules/expat/xmlparse.c:2067:3: note: insert 'break;' to avoid fall-through
  default:
  ^
  break; 
./Modules/expat/xmlparse.c:2096:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
    default:; /* should not happen */
    ^
./Modules/expat/xmlparse.c:2096:5: note: insert '__attribute__((fallthrough));' to silence this warning
    default:; /* should not happen */
    ^
    __attribute__((fallthrough)); 
./Modules/expat/xmlparse.c:2096:5: note: insert 'break;' to avoid fall-through
    default:; /* should not happen */
    ^
    break; 
./Modules/expat/xmlparse.c:2292:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
    default:;
    ^
./Modules/expat/xmlparse.c:2292:5: note: insert '__attribute__((fallthrough));' to silence this warning
    default:;
    ^
    __attribute__((fallthrough)); 
./Modules/expat/xmlparse.c:2292:5: note: insert 'break;' to avoid fall-through
    default:;
    ^
    break; 
./Modules/expat/xmlparse.c:4864:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
    case XML_ROLE_ENTITY_PUBLIC_ID:
    ^
./Modules/expat/xmlparse.c:4864:5: note: insert '__attribute__((fallthrough));' to silence this warning
    case XML_ROLE_ENTITY_PUBLIC_ID:
    ^
    __attribute__((fallthrough)); 
./Modules/expat/xmlparse.c:4864:5: note: insert 'break;' to avoid fall-through
    case XML_ROLE_ENTITY_PUBLIC_ID:
    ^
    break; 
./Modules/expat/xmlparse.c:5192:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
    case XML_ROLE_ENTITY_SYSTEM_ID:
    ^
./Modules/expat/xmlparse.c:5192:5: note: insert '__attribute__((fallthrough));' to silence this warning
    case XML_ROLE_ENTITY_SYSTEM_ID:
    ^
    __attribute__((fallthrough)); 
./Modules/expat/xmlparse.c:5192:5: note: insert 'break;' to avoid fall-through
    case XML_ROLE_ENTITY_SYSTEM_ID:
    ^
    break; 
./Modules/expat/xmlparse.c:6050:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
    case XML_TOK_ATTRIBUTE_VALUE_S:
    ^
./Modules/expat/xmlparse.c:6050:5: note: insert '__attribute__((fallthrough));' to silence this warning
    case XML_TOK_ATTRIBUTE_VALUE_S:
    ^
    __attribute__((fallthrough)); 
./Modules/expat/xmlparse.c:6050:5: note: insert 'break;' to avoid fall-through
    case XML_TOK_ATTRIBUTE_VALUE_S:
    ^
    break; 
./Modules/expat/xmlparse.c:6303:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
    case XML_TOK_DATA_NEWLINE:
    ^
./Modules/expat/xmlparse.c:6303:5: note: insert '__attribute__((fallthrough));' to silence this warning
    case XML_TOK_DATA_NEWLINE:
    ^
    __attribute__((fallthrough)); 
./Modules/expat/xmlparse.c:6303:5: note: insert 'break;' to avoid fall-through
    case XML_TOK_DATA_NEWLINE:
    ^
    break; 

My opinion is that we only should show actionable warnings, that we can actually fix.

CC @hugovk

[hugovk]

cc @nohlson @sethmlarson

[rruuaanng]

In fact, We can add -Wno-implicit-fallthrough to suppress it, Because this warning only means that the case statement has no break.
(Although this will result in no warning being triggered if the user writes a statement without a break).

[sobolevn]

@rruuaanng

We can add -Wno-implicit-fallthrough to suppress it

I think that we should not add anything here, because we are working with a vendored module that cannot be modified (without a very strong reason to).

Although this will result in no warning being triggered if the user writes a statement without a break

Again, developers should not touch vendored libs without a very strong reason. Fixing such a warning is not good enough reason to do so.

[sethmlarson]

I'm in agreement that we shouldn't be warning on code we don't control. Our current setup for warning tooling applies the warning to all code covered by CFLAGS_NODIST, so even if the warnings don't cause CI to fail they still show up in logs. I don't know if there's an easy way to ignore whole directories that we vendor at the configure level to not apply warnings to those files?

[erlend-aasland]

I don't know if there's an easy way to ignore whole directories that we vendor at the configure level to not apply warnings to those files?

We don't have such functionality in configure.ac, but you should be fine with adding -Wno-implicit-fallthrough after -Wimplicit-fallthrough. I don't remember from the top of my head if LIBEXPAT_CFLAGS are added before or after CFLAGS_NODIST, but if LIBEXPAT_CFLAGS are added post CFLAGS_NODIST, getting rid of those warnings for libexpat should be pretty trivial¹.

PoC hack

diff --git a/Makefile.pre.in b/Makefile.pre.in
index 07c8a4d2014..fa9faad0c9b 100644
--- a/Makefile.pre.in
+++ b/Makefile.pre.in
@@ -1351,7 +1351,7 @@ $(LIBMPDEC_A): $(LIBMPDEC_OBJS)
 
 ##########################################################################
 # Build static libexpat.a
-LIBEXPAT_CFLAGS=@LIBEXPAT_CFLAGS@ $(PY_STDMODULE_CFLAGS) $(CCSHARED)
+LIBEXPAT_CFLAGS=@LIBEXPAT_CFLAGS@ $(PY_STDMODULE_CFLAGS) -Wno-implicit-fallthrough $(CCSHARED)
 
 Modules/expat/xmlparse.o: $(srcdir)/Modules/expat/xmlparse.c $(LIBEXPAT_HEADERS) $(PYTHON_HEADERS)
        $(CC) -c $(LIBEXPAT_CFLAGS) -o $@ $(srcdir)/Modules/expat/xmlparse.c

Footnotes

1. a trivial, but dirty, hack in Makefile.pre.in, that is ↩