A few comments, but they're all minor things or FIXMEs. I'm happy for this to land once they're addressed.

Thank you for the patch! This is certainly an improvement but I think there are still some cases where we compute the wrong range for ~ with this patch applied.

This is a bold direction but I like it a lot. Over to @aaron.ballman for final approval.

In D125919#3556418, @aaron.ballman wrote:

In D125919#3556319, @rjmccall wrote:

Now, the standard has chosen not to talk about _Atomic as a qualifier, I assume because there's a fair number of rules in the standard that assume that qualifiers can be freely added to pointers and don't change layout and so on, none of which apply to _Atomic. But those rules also don't apply to a large number of other extended qualifiers, like address spaces and the ARC ownership qualifiers and __ptrauth. The committee should probably just come to terms with the fact that it's the relatively easy-come-easy-go nature of the CVR qualifiers which is the special case. I've thought for awhile that Clang should really be representing _Atomic as a qualifier instead of having to treat AtomicType as a special case in a million places.

I'm not certain if we can get away with that. IIRC, Microsoft uses embedded locks on Windows in some circumstances. However, cl doesn't support _Atomic and so maybe we can get away with it on the assumption we have no other targets where the size/alignment would be different for an atomic type vs a non-atomic type?

OK, fair enough.

We perhaps can't or don't want to add test coverage for the "unexpected decl" case here. Can we add coverage for the "function template" case?

The underlying problem is basically wg21.link/cwg362 which has no concensus yet.

I'm very happy with how this patch is looking.

I've not looked at the test changes in any detail; please let me know if there's anything in there that deserves special attention.

I don't think we can live with the #define private public approach, not least because this violates the ODR and might lead to compile failures using modules as a result. As an alternative, how about:

In D125773#3525127, @ilya-biryukov wrote:

It looks like this particular change actually breaks standard compatibility as we also use the same parsing action and don't build a module separately on import.

[module.import]p5
A module-import-declaration that specifies a header-name H imports a synthesized header unit, which is a
translation unit formed by applying phases 1 to 7 of translation (5.2) to the source file or header nominated
by H, which shall not contain a module-declaration.

This direction looks good to me. Thanks!

Header modules are part of the C++20 standard (where they are called "header units"), and module maps are an intended way for Clang to provide this functionality in C++20 mode. I don't think turning this off by default in C++20 is the right forward-looking plan; rather, I think we should be thinking about moving towards header modules simply always being something that Clang can do, with no flag to control that.

• Put ""s around %s formats and add a length limit.

• Rebase.
• Reuse Clang's existing "guess a format specifier" logic.
• Format signed/unsigned/plain char as numbers.
• Add test for reference members.

• Add requested assert.

• Use printing policy more, and turn off anonymous tag locations.

• Respond to review comments.

• Update release notes

• Minor doc correction

• Fix example in documentation: the dump is terminated by a newline.

In D124221#3469251, @rsmith wrote:

I think we can address most of my concerns with __builtin_dump_struct without a new builtin, by incorporating things from this implementation

• Reimplement __builtin_dump_struct in Sema.

In D124221#3471645, @erichkeane wrote:

In D124221#3470550, @aaron.ballman wrote:

In D124221#3469251, @rsmith wrote:

• Take any callable as the printing function and pass it the fields with their correct types, so that a C++ implementation can dispatch based on the type and print the values of types that we don't hard-code (eg, we'd generate calls like user_dump_function("%s = %p", "field_name", &p->field_name);, and a smart C++ formatting function can preserve the field type and do something suitable when formatting a corresponding %p).

+1, though I'm curious what we want to do about things like bit-fields (do we want to alert the caller that it's a bit-field and what the width is?), anonymous bit-fields, base classes vs fields of class type, etc.

I suspect we'd want some sort of 'depth' parameter to that as well so we can handle the 'tabbing' correctly,

In D124221#3468706, @aaron.ballman wrote:

Thank you for looking into this! I've also thought that the __builtin_dump_struct implementation hasn't been quite as robust as it could be. However, the primary use case for that builtin has been for kernel folks to debug when they have no access to an actual debugger -- it being a super limited interface is actually a feature rather than a bug, in some ways.

In D124221#3467467, @erichkeane wrote:

I generally just question the usefulness of this. Despite its shortcomings, the 'dump struct' has the capability of runtime introspection into a type, whereas this seems to require that the user 'know' a significant amount about the type that they are introspecting (number of bases, number of fields, field TYPES).

• Fix some code that was making incorrect assumptions that PseudoObjectExpr is only used for ObjC properties.

• Some improvements.

I'm not entirely sure whether I want to pursue this -- I'd prefer to have only a single mechanism that works well in both C and C++, rather than this (which is flexible but not really usable in C) and __builtin_dump_struct (which is inflexible but works well for C) -- but I've not found a good way to expose this functionality that provides good support for both use cases yet. Thoughts on that are welcome.

In D119136#3463888, @cor3ntin wrote:

@rsmith @aaron.ballman Do you think it's worth resubmitting this patch with a look ahead of the mutable keyword (this seems to me a better strategy than other flimsy workaround suggested/tried), or would you rather wait for WG21 to come up with something?

In D119136#3463888, @cor3ntin wrote:

@rsmith @aaron.ballman Do you think it's worth resubmitting this patch with a look ahead of the mutable keyword (this seems to me a better strategy than other flimsy workaround suggested/tried), or would you rather wait for WG21 to come up with something?

In D123345#3463540, @thakis wrote:

I think your revert to fix the aix CI broke CI everywhere: http://45.33.8.238/linux/74239/step_7.txt

Please take a look and reland for now if it takes a while to fix.

In D123345#3463224, @daltenty wrote:

In D123345#3460639, @Jake-Egan wrote:

Hi, unfortunately there's a build failure on AIX: https://lab.llvm.org/buildbot/#/builders/214/builds/779/steps/9/logs/stdio. Could you take a look?

Hi, any feedback on the AIX break? Seems like this might be non-trivial to address (were on an older libc++ level), so perhaps this needs a revert till can be worked out.

In D123909#3462560, @aaron.ballman wrote:

This looks like a true positive to me, at least for the moment (Core is still trying to decide what to do about CWG2569 which may relax some restrictions in this area).

@MaskRay -- your revert was incorrect, please un-revert.

LGTM; two non-blocking comments and a question.

LGTM. Some comments for potential improvements, but I'd be OK with this landing as-is if you don't find any of them sufficiently motivating :)

In D123345#3457109, @rsmith wrote:

I'm working on a fix. I'll revert if it takes me more than a few minutes. If you'd prefer to not wait, please feel free to revert it yourself.

In D123345#3456964, @paulkirth wrote:

Hi, this is also breaking Fuchsia's clang CI builders (https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8816531831869640417/overview). If this will be hard to address, would you mind reverting until a patch is ready?