llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.35k stars 12.14k forks source link

Regression: <chrono> does not compile on macOS with `_XOPEN_SOURCE` defined to a value in [500, 699] #117630

Open nico opened 3 days ago

nico commented 3 days ago

Repro (minified from absl's time_zone_format.cc):

% cat chrono.cc 
#if !defined(_XOPEN_SOURCE) && !defined(__FreeBSD__) && !defined(__OpenBSD__)
#define _XOPEN_SOURCE 500  // Exposes definitions for SUSv2 (UNIX 98).
#endif

#include <chrono>
% clang++ -I../../buildtools/third_party/libc++ -D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_EXTENSIVE -nostdinc++ -isystem../../third_party/libc++/src/include  -c chrono.cc  -std=c++20
In file included from chrono.cc:5:
In file included from ../../third_party/libc++/src/include/chrono:969:
In file included from ../../third_party/libc++/src/include/__chrono/formatter.h:29:
In file included from ../../third_party/libc++/src/include/__chrono/ostream.h:36:
In file included from ../../third_party/libc++/src/include/__format/format_functions.h:21:
In file included from ../../third_party/libc++/src/include/__format/format_context.h:28:
In file included from ../../third_party/libc++/src/include/__locale:14:
In file included from ../../third_party/libc++/src/include/__locale_dir/locale_base_api.h:98:
In file included from ../../third_party/libc++/src/include/__locale_dir/support/apple.h:18:
../../third_party/libc++/src/include/__locale_dir/support/bsd_like.h:149:10: error: no member named 'wcsnrtombs_l' in the global namespace; did you mean '__wcsnrtombs'?
  149 |   return ::wcsnrtombs_l(__dest, __src, __nwc, __len, __ps, __loc);
      |          ^~
../../third_party/libc++/src/include/__locale_dir/support/bsd_like.h:148:1: note: '__wcsnrtombs' declared here
  148 | __wcsnrtombs(char* __dest, const wchar_t** __src, size_t __nwc, size_t __len, mbstate_t* __ps, __locale_t __loc) {
      | ^
../../third_party/libc++/src/include/__locale_dir/support/bsd_like.h:158:10: error: no member named 'mbsnrtowcs_l' in the global namespace; did you mean '__mbsnrtowcs'?
  158 |   return ::mbsnrtowcs_l(__dest, __src, __nms, __len, __ps, __loc);
      |          ^~
../../third_party/libc++/src/include/__locale_dir/support/bsd_like.h:157:1: note: '__mbsnrtowcs' declared here
  157 | __mbsnrtowcs(wchar_t* __dest, const char** __src, size_t __nms, size_t __len, mbstate_t* __ps, __locale_t __loc) {
      | ^

(We might be able to work around this by defining _XOPEN_SOURCE with a -D flag while compiling that file, haven't tried that yet.)

nico commented 3 days ago

Bisects to 1a187674a116518e3c79f21df40cdb2ddf5ca312

nico commented 3 days ago

Apparently broken when _XOPEN_SOURCE is in [500, 699].

ldionne commented 3 days ago

I'm having a look.

ldionne commented 3 days ago

So what seems to be happening here is that previously, the libc++ version of __wcsnrtombs was implemented as a macro defined to wcsnrtombs_l. However, since libc++ only uses __wcsnrtombs inside the dylib, the macro would never be expanded in user code, and so it didn't matter whether wcsnrtombs_l was available or not. After https://github.com/llvm/llvm-project/commit/1a187674a116518e3c79f21df40cdb2ddf5ca312, __wcsnrtombs is a proper function, which means that its definition must compile, and that doesn't work when wcsnrtombs_l isn't provided. _XOPEN_SOURCE = {500,600} causes that to happen, which fails the compilation.

ldionne commented 3 days ago

Reverting to a macro is not desirable, since it makes these headers non-modular and it's just a hack. Potential solutions:

  1. Only define __wcsnrtombs in the dylib, where we control what flags are used (so in particular we can say that compiling the dylib with _XOPEN_SOURCE=500 is not supported). We probably need to do the same thing for other functions?
  2. Say that using _XOPEN_SOURCE=500 is not supported when using libc++. I'm not sure how much impact this would have.

@nico Can you provide a bit more context around how you found out about this failure and why the code is using _XOPEN_SOURCE=500?

nico commented 3 days ago

Can you provide a bit more context around how you found out about this failure

The answer to this is always "I was trying to update libc++ in Chromium and some bots failed" :)

In this case, it was our macOS bots (using latest Xcode and SDK and whatnot). absl's time_zone_format.cc just does this: https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/time/internal/cctz/src/time_zone_format.cc;l=23?q=ime_zone_format.cc

#if defined(HAS_STRPTIME) && HAS_STRPTIME
#if !defined(_XOPEN_SOURCE) && !defined(__FreeBSD__) && !defined(__OpenBSD__)
#define _XOPEN_SOURCE 500  // Exposes definitions for SUSv2 (UNIX 98).
#endif
#endif

Why it does this, I don't know. I'm guessing 500 was the highest version when this was written, and it was apparently needed to get strptime() on some platforms.

We can add -D_XOPEN_SOURCE=700 while building that file in our build, but it looks like it might affect other projects too: https://grep.app/search?q=_XOPEN_SOURCE%20500&filter[lang][0]=C%2B%2B

ldionne commented 2 days ago

Ack. I think this version of _XOPEN_SOURCE should probably be updated, but I think I have a fix in the linked PR.

nico commented 2 days ago

(Some discussion if this is worth fixing at all over at https://github.com/llvm/llvm-project/pull/117764#issuecomment-2504097416)