Thanks for the review!

Adding folks involved with LTO. The problem is that stats emitted during the first round of compilation are likely to be overwritten by stats emitted during [Thin]LTO itself. Not touching it now to preserve the existing behavior. But wanted to inform you about this shortcoming in case it is important for you to fix.

Don't touch LTO pipeline to avoid breaking existing workflows.

Append to stats file when using CC_PRINT_INTERNAL_STAT and for LTO when reusing the same stats file.

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.

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?

Thanks for the review!

Thanks for the review, Bruno!

In D71734#4061228, @ChuanqiXu wrote:

LGTM generally. It'd better to mention this in the Potentially Breaking Changes section of ReleaseNotes.

setODRHash(0) for consistency, mention the change in Potentially Breaking Changes.

Thanks for the review!

Rebase and add assertion in ODRHash::AddRecordDecl.

Rebase the patch and trigger pre-commit checks.

Thanks for the review, Florian!

Ping. Also checked the change on a bunch of internal Objective-C/C code using modules - no new errors.

Ping.

Confirm that for me on macOS without the fix the test is failing every time, so the test seems to be totally sufficient.

Want to note that originally the issue was noticed with the modules because we serialize and deserialize PointerTypeLoc::starLoc and on deserialization we assert if the value is too big. With uninitialized memory the value can be too big sometimes and clang was failing intermittently. For testing don't use modules anymore and call PointerTypeLoc::getStarLoc directly to make it simpler and more reliable.

Doing this minor clean-up for D141424.

clang-format the change.

Thanks for the review!

Serialize/deserialize ObjCInterfaceTypeLoc::NameEndLoc as it is used in diagnostic about a mismatching superclass.

Rebase.

In D140002#4000233, @ChuanqiXu wrote:

Test case MergeLambdas3.cppm is unstable. Sometimes it passes but usually it fails.

Weird. What is the error message if it fails?

Test case MergeLambdas3.cppm is unstable. Sometimes it passes but usually it fails.

This is my first and pretty shallow review pass. Need to read the standard more thoroughly to be more useful. Others are welcome to chime in (and as I'll be on vacation are encouraged to chime in).

Abandoning this change as now it is split between D124286 and D140073.

Implement detecting and diagnosing duplicates during parsing based on ODR hash done during deserialization.

Superseded by D140073.

As I've mentioned CXXRecordDecl::dataPtr, the relevant code is https://github.com/llvm/llvm-project/blob/bc63a393262dd278d2f12153c30f8e6ea06a2450/clang/include/clang/AST/DeclCXX.h#L441-L442

If anybody is curious about anonymous structs/unions, the relevant change is D140055.

Rebase and use ODRDiagsEmitter. Add more tests.

From my experience, ShowInSystemHeader for -Rmodule-build is overall useful. I haven't heard from people who want no remarks from system headers and the rest is just hypothetical guesses that can go both ways (some people might like one option while some people might like another option).

Thanks for the review!

In D138859#3955806, @ChuanqiXu wrote:

And I think we should add tests for this. e.g., in the ASTImporterTests.cpp.

To help folks digging up the history, the code in driver was introduced in https://reviews.llvm.org/D11322 and the test case added in "test/Index/warning-flags.c" requires the change in "clang/tools/libclang/CIndex.cpp", that's why I don't have any extra test changes.

Ok, I was able to reproduce the error on macOS by adding -target x86_64-sie-ps5 to one of commands in "clang/test/ClangScanDeps/Inputs/no-werror.json" (wouldn't mind receiving PS5 DevKit, by the way). The problem is that Driver::BuildCompilation overwrites DiagnosticsEngine::IgnoreAllWarnings ignoring everything specified in DiagnosticOptions::IgnoreWarnings. Thus we still emit the warning despite asking in DiagnosticOptions not to. D138970 fixes this situation and relies on DiagnosticOptions to set diagnostic options.

In D138859#3954975, @erichkeane wrote:

The hash there isn't the problem, its that you're adding a field to Attr.td that isn't serialized in ASTWriter/ASTReader.

Thanks for the reviews!

In D138859#3954943, @erichkeane wrote:

I dont have a concern on this in general, but does it cause problems with modules built? I would think this flag needs to be written in ASTWriter/picked back up.

This patch builds on top of D135472 and aims to show that hashing attributes with TableGen works fine and doesn't have unexpected problems.

Ping.

Thanks for the quick review! During the commit I will also fix the typo s/there a no overrides/there are no overrides/

In D130327#3944188, @NancyWang2222 wrote:

@vsapsai hi Volodymyr Sapsai , test case clang/test/Modules/hidden-duplicates.m is failing on our llvm community AIX box https://lab.llvm.org/buildbot/#/builders/214/builds/4442/steps/6/logs/FAIL__Clang__hidden-duplicates_m , it gives exit code 70 without detail error message, can you help fix it as soon as possible. Thanks

Thanks for reviews! It was a long journey.

Should admit that approach with DiagOpts.IgnoreWarnings is pretty blunt but I'm not aware of any reasons to use something more elaborate, like stripping away all -Werror=... flags. So going with a simple change.

Thanks for the review!

Ping.

Ping.

Rebase and diagnose attribute mismatch for function parameters.

The issue is addressed in D133586.