In D144323#4204034, @ldionne wrote:

Can you please just rebase onto main and re-upload to give this one last CI spin? There's a Windows failure I don't understand. I think it's unrelated but rebasing and running the CI again should confirm that.

Rebasing

Make the unwind_ files private textual headers and revert the changes to them.

In D144322#4151757, @vsapsai wrote:

Sorry, forgot to ask the first time. This change is for Clang modules and not for C++20 modules, right? Asking because believe C++20 modules have standard-enforced module names.

In D144322#4151493, @vsapsai wrote:

This is a pretty big change and there are no tests added. I haven't thought how and if these changes can be tested. Have you considered how the automated testing should be done?

Fix some newly introduced module cycles.

Remove type_traits include from test_macros.h.
Rebase and fix conflicts.

In D144322#4140227, @dexonsmith wrote:

Great to see this making progress; looking forward to seeing the related workarounds removed from the Darwin SDKs.

What's the performance impact (for scanning/building/importing modules) of turning these all into top-level modules? If it's a significant regression, maybe there's a way to isolate the headers that are involved in the cycle with the SDK and the Clang headers from those that aren't, while still using only a handful of modules. And if it's a performance improvement in some cases (due to better parallelism or building less stuff), could be something to highlight.

Add a comment to cuchar to indicate why it's including __mbstate_t.h.

Run clang-format over the changed files.

Add a missing include to <tuple>

Found another way around the format cycles.

In D144322#4136898, @Mordante wrote:

I have not reviewed the entire patch.
Please update the commit message to describe why these changes are needed and what the benefit is. It looks some parts are wrong; removing headers for forward declarations, while the contents of the header are used, is wrong. It might compile due to transitive includes, but we really should include the proper headers.

Update the commit message with more details about the problem.

In D140250#4081133, @iana wrote:

In D140250#4081119, @rsmith wrote:

In D140250#4081102, @rsmith wrote:

Our builtin header stddef.h shouldn't be built as a module. It fundamentally needs to be treated as a textual header, because it consumes macros defined by the includer. The module map that we provide with our builtin headers does not cover stddef.h for that reason.

That said... the __has_feature(modules) checks throughout stddef.h look pretty wrong to me. They're probably working around the infelicities of the -fno-modules-local-submodule-visibility mode; maybe we can disable them when not in that mode?

It ends up being covered by Darwin.C.stddef on Apple platforms (due to some builtin shenanigans elsewhere in clang). Maybe we could make stddef.h be a textual header in a _Builtin_stddef module in clang and that would do the trick instead of splitting it into a bunch of different files.

In D140250#4081119, @rsmith wrote:

In D140250#4081102, @rsmith wrote:

Our builtin header stddef.h shouldn't be built as a module. It fundamentally needs to be treated as a textual header, because it consumes macros defined by the includer. The module map that we provide with our builtin headers does not cover stddef.h for that reason.

That said... the __has_feature(modules) checks throughout stddef.h look pretty wrong to me. They're probably working around the infelicities of the -fno-modules-local-submodule-visibility mode; maybe we can disable them when not in that mode?

In D140250#4081044, @aaron.ballman wrote:

In D140250#4081009, @iana wrote:

In D140250#4080990, @rsmith wrote:

Wait a second... if an OS wants only NULL, we already have a supported way of achieving that, which is compatible with GCC and glibc and other POSIX compilers -- define __need_NULL before including the header.

We shouldn't be providing internal headers with __mangled_names for OSes to include, those should be implementation details.

You can't do that with modules though, #define __need_NULL #include <stddef.h> doesn't work.

Also, from the patch summary: i.e. in POSIX headers that are required to define NULL but not the rest of stddef.h. -- I don't think __need_NULL works for this because it gets you NULL but it also gets you the rest of the stddef.h header, right?

In D140250#4080990, @rsmith wrote:

Wait a second... if an OS wants only NULL, we already have a supported way of achieving that, which is compatible with GCC and glibc and other POSIX compilers -- define __need_NULL before including the header.

We shouldn't be providing internal headers with __mangled_names for OSes to include, those should be implementation details.

Revert the unnecessary changes to unspecified-var-size.ll

In D140250#4077974, @iana wrote:

In D140250#4077956, @aaron.ballman wrote:

In D140250#4077826, @iana wrote:

In D140250#4076851, @aaron.ballman wrote:

I'd expect there to be a test under clang/test/Headers/ showing that this new header works as expected.

It's covered pretty well by stddefneeds.cpp already. I can add a new one if you want, but it'd pretty much be a copy/paste

I was thinking of one similar to that but includes the new header and *not* stddef.h to demonstrate that this works entirely standalone. WDYT?

Yeah ok sure

Add an explicit header test for __stddef_null.h

In D140250#4077956, @aaron.ballman wrote:

In D140250#4077826, @iana wrote:

In D140250#4076851, @aaron.ballman wrote:

I'd expect there to be a test under clang/test/Headers/ showing that this new header works as expected.

It's covered pretty well by stddefneeds.cpp already. I can add a new one if you want, but it'd pretty much be a copy/paste

I was thinking of one similar to that but includes the new header and *not* stddef.h to demonstrate that this works entirely standalone. WDYT?

In D140250#4076851, @aaron.ballman wrote:

I'd expect there to be a test under clang/test/Headers/ showing that this new header works as expected.

Fix the broken tests

Fix the debuginfo-generic-assignment-tracking-sroa test

Rebase, update diagnostic to include the full module name instead of just the top level.

I think this should be all we need, but I'm still running tests.

Add a module for stddef_null.h and give it the same special treatment as stddef_max_align_t.h.

This needs a bit more work, it looks like it's not enough to just add a header.

In D135345#3880719, @MaskRay wrote:

Good point about libunwind headers are usually installed in /usr/include , shared with other system headers.
Is there any information how Mach-O platforms use the modulemap file?
IIUC non-Mach-O platforms will probably not benefit from libunwind.modulemap.

In D135345#3880528, @ldionne wrote:

In D135345#3851537, @MaskRay wrote:

I think the default name module.modulemap (loaded by -fimplicit-module-maps) is more appropriate for the upstream and it helps layering check features in Clang.
The name libunwind.modulemap is indeed special and seems to platform specific. I don't know how to assess compact_unwind_encoding.modulemap and the MachO.* name instead of libunwind.*/unwind.*.

Does that mean that no library rooted at /usr/include can provide its own modulemap? For example, libc++ provides its own modulemap and calls it module.modulemap, but it can do that because it is rooted at /usr/include/c++/v1. If it were rooted at /usr/include, it would collide with any other system-provided module.modulemap file (arguably like a C standard library modulemap).

Yes that's correct. There can be only one owner per directory, and libunwind doesn't own /usr/include. All it can do is provide a module map that the owner can include from module.modulemap. That will be done by putting extern module libunwind "libunwind.modulemap" and extern module unwind "libunwind.modulemap" in the OS's /usr/include/module.modulemap.

Alright then, I'll bump this to the Apple fork. I don't know that there really is a great way to upstream this. If we call the top level module map file module.modulemap then that will install usr/include/module.modulemap which I don't think clang should do. Anything else (like libunwind.modulemap) will require OS/SDK coordination.

If this doesn't make sense in open source, just let me know and I'll move it to the Apple fork.

The buildable error looks like an infrastructure problem where Phabricator doesn't know the cmake/ninja command to build libunwind.

In D135345#3841584, @MaskRay wrote:

Can you state the motivation for this change?

In D132971#3759538, @Bigcheese wrote:

I'm fine with this change, but do we actually have a backwards compatibility policy anywhere in Clang? Would be good to know what range of SDKs a compiler release is expected to support.

In D131213#3702699, @ddcc wrote:

I missed line 19, yeah that makes sense. @iana is that ok with you?

That was the only one I saw from D130800, can someone double check me though?

Get rid of the extra parens

In D130060#3667467, @vitalybuka wrote:

() is not requirement
do you have committer access?

In D130060#3667463, @iana wrote:

In D130060#3667462, @vitalybuka wrote:

so (HAVE_RPC_XDR_H && !SANITIZER_APPLE) is just shorter

So why not a shorter version? Do I miss some benefits of this long expression?

Because I misread your comment the first time >.>

Update the platform checks to be the nicer shorter version.

In D130060#3667462, @vitalybuka wrote:

so (HAVE_RPC_XDR_H && !SANITIZER_APPLE) is just shorter

So why not a shorter version? Do I miss some benefits of this long expression?

In D130060#3667440, @vitalybuka wrote:

In D130060#3667367, @iana wrote:

In D130060#3667327, @vitalybuka wrote:

In D130060#3666880, @iana wrote:

In D130060#3666808, @vitalybuka wrote:

If OSX fixes HAVE_RPC_XDR_H=1 why not to fix mac handling of XDR here instead?

I didn't want to change the behavior on macOS, this part has never been on on that platform.

#if HAVE_RPC_XDR_H && !SANITIZER_APPLE
with TODO to fix for Apple?

@yln @wrotki added few recent Apple committers for opinion

Wouldn't it be best to match the sanitizer_platform_limits_posix.h guard? That's the crux of the bug anyway, the declaration and checks are guarded with totally different macros.

No. This file applies to FREEBSD as well
so it should be HAVE_RPC_XDR_H && ((SANITIZER_LINUX && !SANITIZER_ANDROID) || SANITIZER_FREEBSD)
but on all these platform HAVE_RPC_XDR_H will be set anyway
so (HAVE_RPC_XDR_H && !SANITIZER_APPLE) is just shorter

TODO is not needed as Apple does not intercept XDR

Update the platform checks.

In D130060#3667327, @vitalybuka wrote:

In D130060#3666880, @iana wrote:

In D130060#3666808, @vitalybuka wrote:

If OSX fixes HAVE_RPC_XDR_H=1 why not to fix mac handling of XDR here instead?

I didn't want to change the behavior on macOS, this part has never been on on that platform.

#if HAVE_RPC_XDR_H && !SANITIZER_APPLE
with TODO to fix for Apple?

@yln @wrotki added few recent Apple committers for opinion

That would be changing the struct __sanitizer_XDR declaration in sanitizer_platform_limits_posix.h to be HAVE_RPC_XDR_H instead of SANITIZER_LINUX && !SANITIZER_ANDROID. I think right now it doesn't check HAVE_RPC_XDR_H because it's not actually using anything from xdr.h.

In D130060#3666808, @vitalybuka wrote:

If OSX fixes HAVE_RPC_XDR_H=1 why not to fix mac handling of XDR here instead?

The checks were (I think erroneously) removed in D8698