aaron.ballman added a comment.

In D157331#4654265 <https://reviews.llvm.org/D157331#4654265>, @mstorsjo wrote:

> This change broke building a recent version of gettext. Gettext uses gnulib 
> for polyfilling various utility functions. Since not long ago, these 
> functions internally use `<stdckdint.h>`, 
> https://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/malloca.c?id=ef5a4088d9236a55283d1eb576f560aa39c09e6f.
>  If the toolchain doesn't provide the header, gnulib provides a fallback 
> version of its own: 
> https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/stdckdint.in.h
>
> Now since Clang provides it since this commit, gnulib doesn't provide their 
> own fallback, but goes on to use `ckd_add`. This fails with Clang's 
> `<stdckdint.h>` implementation, since gnulib isn't built in C23 mode but 
> still wants to use `<stdckdint.h>`:
>
>   i686-w64-mingw32-clang -DHAVE_CONFIG_H -DEXEEXT=\".exe\" -I. 
> -I../../../gettext-runtime/gnulib-lib -I..  -I../intl 
> -I../../../gettext-runtime/intl -DDEPENDS_ON_LIBICONV=1 
> -DDEPENDS_ON_LIBINTL=1 
> -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include  -Wno-cast-qual 
> -Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef 
> -Wno-unused-function -Wno-unused-parameter -Wno-float-conversion 
> -Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits 
> -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include -g -O2 -c -o 
> libgrt_a-malloca.o `test -f 'malloca.c' || echo 
> '../../../gettext-runtime/gnulib-lib/'`malloca.c
>   ../../../gettext-runtime/gnulib-lib/malloca.c:53:8: error: call to 
> undeclared function 'ckd_add'; ISO C99 and later do not support implicit 
> function declarations [-Wimplicit-function-declaration]
>      53 |   if (!ckd_add (&nplus, n, plus) && !xalloc_oversized (nplus, 1))
>         |        ^
>   1 error generated.
>   make: *** [Makefile:2246: libgrt_a-malloca.o] Error 1
>
> It seems like GCC's version of this header exposes the functionality even if 
> compiled in an older language mode: 
> https://github.com/gcc-mirror/gcc/blob/f019251ac9be017aaf3c58f87f43d88b974d21cf/gcc/ginclude/stdckdint.h
>
> Would you consider changing the Clang version to do the same? Otherwise we 
> would need to convince gnulib to not try to use/polyfill this header unless 
> gnulib itself is compiled in C23 mode.

Well that's a fun situation. The identifiers exposed by the header are not in a 
reserved namespace and they're macros, which is usually not something we'd want 
to expose in older language modes because of the risk of breaking code. But in 
this case, there's an explicit opt-in to a brand-new header file so it 
shouldn't break code to expose the contents in older language modes. But I 
expect there to be changes to this file in C2y, and those additions could break 
code if we're not careful.

CC @jrtc27 @jyknight @efriedma for opinions from others, but my thinking is 
that we should expose the contents in earlier language modes. We support the 
builtin in those modes, and the user is explicitly asking for the header to be 
included -- it seems pretty rare for folks to want the header to be empty, 
especially given that `__has_include` will report "sure do!". I don't think 
there's a conformance issue here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157331/new/

https://reviews.llvm.org/D157331

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to