[llvm-branch-commits] [clang] [libcxx] [clang] Preserve Qualifiers and type sugar in TemplateNames (PR #93433)

[Ilya Biryukov via llvm-branch-commits]

https://github.com/ilya-biryukov approved this pull request.

Went through the changes one more time and they definitely LGTM.
See the two small comments left from my side about documentation and release 
notes, I think they're worth fixing before landing this. Otherwise, let's ship 
it!

https://github.com/llvm/llvm-project/pull/93433
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

I am happy to take a look, but will run out of time for it today. I will 
definitely do it tomorrow, though.

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

https://github.com/ilya-biryukov edited 
https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

https://github.com/ilya-biryukov commented:

Thanks for the change! I have done a round of review and left a few suggestion 
and also have a bunch of questions. I am sorry if some of them may look too 
obvious, I did try to dig into the code where I could, but Clang's 
serialization is complicated and some things are easier researched through a 
conversation than through looking at code.  Please bear with me, I promise to 
get better in the upcoming reviews.

Here are a few extra question that came to mind too.

Question 1: Do we have estimates on how much bigger the PCMs become? Did you 
observer any negative performance impact?

I would expect `TypeID`s to be much more common than decls, and the 
corresponding size increases to be more significant. We've already lost ~6% 
from DeclID change, I am slightly worried the types are going to be a bigger 
hit.

Question 2: could you explain why we the PCM in your example was changing 
before? Was there some base offset inherited from the PCM files we depended on?

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -7650,6 +7647,16 @@ ModuleFile *ASTReader::getOwningModuleFile(GlobalDeclID 
ID) const {
   return &getModuleManager()[ModuleFileIndex - 1];
 }
 
+ModuleFile *ASTReader::getOwningModuleFile(TypeID ID) const {
+  if (ID < NUM_PREDEF_TYPE_IDS)
+return nullptr;
+
+  uint64_t ModuleFileIndex = ID >> 32;

ilya-biryukov wrote:

NIT: maybe we could define helper functions to extract the `ModuleFileIndex`, 
type index and qualifiers from `TypeID`?

It would help the readability a bit (it's not too hard to remember how to do 
these operations, but it's a bit of code duplication and definitely makes 
reading a bit more involved)

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -70,38 +71,53 @@ using DeclID = DeclIDBase::DeclID;
 
 /// An ID number that refers to a type in an AST file.
 ///
-/// The ID of a type is partitioned into two parts: the lower
+/// The ID of a type is partitioned into three parts:
+/// - the lower
 /// three bits are used to store the const/volatile/restrict

ilya-biryukov wrote:

```suggestion
///   three bits are used to store the const/volatile/restrict
```

NIT: indent by 2 spaces to align the second line of the bullet item with its 
first letters on the first line.

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -7100,14 +7084,25 @@ TypeSourceInfo *ASTRecordReader::readTypeSourceInfo() {
   return TInfo;
 }
 
+std::pair
+ASTReader::translateTypeIDToIndex(serialization::TypeID ID) const {
+  unsigned Index =
+  (ID & llvm::maskTrailingOnes(32)) >> Qualifiers::FastWidth;
+
+  ModuleFile *OwningModuleFile = getOwningModuleFile(ID);
+  assert(OwningModuleFile &&
+ "untranslated type ID or local type ID shouldn't be in TypesLoaded");
+  return {OwningModuleFile, OwningModuleFile->BaseTypeIndex + Index};
+}
+
 QualType ASTReader::GetType(TypeID ID) {
   assert(ContextObj && "reading type with no AST context");
   ASTContext &Context = *ContextObj;
 
   unsigned FastQuals = ID & Qualifiers::FastMask;
-  unsigned Index = ID >> Qualifiers::FastWidth;
 
-  if (Index < NUM_PREDEF_TYPE_IDS) {
+  if (uint64_t Index = ID >> Qualifiers::FastWidth;

ilya-biryukov wrote:

Higher bits still represent the module file index here, right?
Could you clarify why it's correct to compare with `NUM_PREDEF_TYPE_IDS` 
without first extracting those bits?

Are the higher bits of predefined types always `0`?
(If this was done on top of results `translateTypeIDToIndex`, it would be very 
clear that the code is correct, not sure if there are reasons to postpone this 
call).

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -3262,17 +3262,18 @@ void ASTWriter::WritePragmaDiagnosticMappings(const 
DiagnosticsEngine &Diag,
 /// Write the representation of a type to the AST stream.
 void ASTWriter::WriteType(QualType T) {
   TypeIdx &IdxRef = TypeIdxs[T];
-  if (IdxRef.getIndex() == 0) // we haven't seen this type before.
+  if (IdxRef.getValue() == 0) // we haven't seen this type before.

ilya-biryukov wrote:

NIT: For the purpose of improving readability, I wonder if there is a better 
name than `getValue`? Would something like `getSerializedValue()` or 
`getGlobalIndex()` be appropriate here?

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -6659,13 +6655,22 @@ void ASTWriter::MacroRead(serialization::MacroID ID, 
MacroInfo *MI) {
 }
 
 void ASTWriter::TypeRead(TypeIdx Idx, QualType T) {
-  // Always take the highest-numbered type index. This copes with an 
interesting
+  // Always take the type index that comes in later module files.
+  // This copes with an interesting
   // case for chained AST writing where we schedule writing the type and then,
   // later, deserialize the type from another AST. In this case, we want to
-  // keep the higher-numbered entry so that we can properly write it out to
+  // keep the just writing entry so that we can properly write it out to
   // the AST file.
   TypeIdx &StoredIdx = TypeIdxs[T];
-  if (Idx.getIndex() >= StoredIdx.getIndex())
+
+  // Ignore it if the type comes from the current being written module file.
+  unsigned ModuleFileIndex = StoredIdx.getModuleFileIndex();
+  if (ModuleFileIndex == 0 && StoredIdx.getValue())
+return;
+
+  // Otherwise, keep the highest ID since the module file comes later has
+  // higher module file indexes.
+  if (Idx.getValue() >= StoredIdx.getValue())

ilya-biryukov wrote:

```suggestion 
  if (Idx.getModuleFileIndex() >= StoredIdx.getModuleFileIndex())
```
This seems to align better with what the comment for the code are saying. If 
this does not work, I might be missing something.

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -70,38 +71,53 @@ using DeclID = DeclIDBase::DeclID;
 
 /// An ID number that refers to a type in an AST file.
 ///
-/// The ID of a type is partitioned into two parts: the lower
+/// The ID of a type is partitioned into three parts:
+/// - the lower
 /// three bits are used to store the const/volatile/restrict
-/// qualifiers (as with QualType) and the upper bits provide a
-/// type index. The type index values are partitioned into two
+/// qualifiers (as with QualType).
+/// - the upper 29 bits provide a type index in the corresponding
+/// module file.
+/// - the upper 32 bits provide a module file index.
+///
+/// The type index values are partitioned into two
 /// sets. The values below NUM_PREDEF_TYPE_IDs are predefined type
 /// IDs (based on the PREDEF_TYPE_*_ID constants), with 0 as a
 /// placeholder for "no type". Values from NUM_PREDEF_TYPE_IDs are
 /// other types that have serialized representations.
-using TypeID = uint32_t;
+using TypeID = uint64_t;
 
 /// A type index; the type ID with the qualifier bits removed.
+/// Keep structure alignment 32-bit since the blob is assumed as 32-bit
+/// aligned.
 class TypeIdx {
+  uint32_t ModuleFileIndex = 0;
   uint32_t Idx = 0;
 
 public:
   TypeIdx() = default;
-  explicit TypeIdx(uint32_t index) : Idx(index) {}
+  explicit TypeIdx(uint32_t Idx) : ModuleFileIndex(0), Idx(Idx) {}

ilya-biryukov wrote:

Could we explicitly pass 0 to the ModuleFileIndex instead? It's very uncommon 
to have defaulted parameters at the start of the parameter list rather than at 
the end.

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -6659,13 +6655,22 @@ void ASTWriter::MacroRead(serialization::MacroID ID, 
MacroInfo *MI) {
 }
 
 void ASTWriter::TypeRead(TypeIdx Idx, QualType T) {
-  // Always take the highest-numbered type index. This copes with an 
interesting
+  // Always take the type index that comes in later module files.
+  // This copes with an interesting
   // case for chained AST writing where we schedule writing the type and then,
   // later, deserialize the type from another AST. In this case, we want to
-  // keep the higher-numbered entry so that we can properly write it out to
+  // keep the just writing entry so that we can properly write it out to

ilya-biryukov wrote:

```suggestion
  // keep the entry from a later module so that we can properly write it out to
```

NIT: the "just writing" sounds weird and there is a reference to the fact it's 
being written later in the line anyway.

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -6659,13 +6655,22 @@ void ASTWriter::MacroRead(serialization::MacroID ID, 
MacroInfo *MI) {
 }
 
 void ASTWriter::TypeRead(TypeIdx Idx, QualType T) {
-  // Always take the highest-numbered type index. This copes with an 
interesting
+  // Always take the type index that comes in later module files.
+  // This copes with an interesting
   // case for chained AST writing where we schedule writing the type and then,
   // later, deserialize the type from another AST. In this case, we want to
-  // keep the higher-numbered entry so that we can properly write it out to
+  // keep the just writing entry so that we can properly write it out to
   // the AST file.
   TypeIdx &StoredIdx = TypeIdxs[T];
-  if (Idx.getIndex() >= StoredIdx.getIndex())
+
+  // Ignore it if the type comes from the current being written module file.

ilya-biryukov wrote:

Could you explain why we need this special case now and we didn't need it 
before the change?
What has changed? Do we have tests for it?

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -70,38 +71,53 @@ using DeclID = DeclIDBase::DeclID;
 
 /// An ID number that refers to a type in an AST file.
 ///
-/// The ID of a type is partitioned into two parts: the lower
+/// The ID of a type is partitioned into three parts:
+/// - the lower
 /// three bits are used to store the const/volatile/restrict
-/// qualifiers (as with QualType) and the upper bits provide a
-/// type index. The type index values are partitioned into two
+/// qualifiers (as with QualType).
+/// - the upper 29 bits provide a type index in the corresponding

ilya-biryukov wrote:

```suggestion
/// - the next 29 bits provide a type index in the corresponding

NIT: maybe update the wording? Upon the first read I thought it's referring to 
the 29 highest bit and only after reading the next bullet item I have realized 
it's referring to the 29 bits after the initial 3 lower bits.

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -12,44 +12,44 @@
 // RUN: -fmodule-file=Module=%t/Module.pcm -emit-llvm -o - | FileCheck 
%t/Object.cppm
 
 // Test again with reduced BMI.
-// RUN: rm -rf %t
-// RUN: mkdir -p %t
-// RUN: split-file %s %t
+// RUNX: rm -rf %t

ilya-biryukov wrote:

are these leftovers from previous versions of the patch that need to be 
reverted?

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Ilya Biryukov via llvm-branch-commits]

@@ -7392,27 +7388,28 @@ QualType ASTReader::GetType(TypeID ID) {
   return TypesLoaded[Index].withFastQualifiers(FastQuals);
 }
 
-QualType ASTReader::getLocalType(ModuleFile &F, unsigned LocalID) {
+QualType ASTReader::getLocalType(ModuleFile &F, TypeID LocalID) {

ilya-biryukov wrote:

Could we clarify what `LocalID` means here in a comment somewhere?
IIUC, the `ModuleFileIndex` there is an index into `F.TransitiveImports` rather 
than into a "global" module manager.

Previously, we had distinction between `unsigned` in parameters to this 
function and `TypeID` as a return type.
Maybe we should keep this here and accept `uint64_t` to avoid confusing the 
common global module file index and a local file index?

https://github.com/llvm/llvm-project/pull/92511
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

> @ilya-biryukov, could we have another try of this PR on your end?

Sorry for missing this, we will rerun the testing and get back to you.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

> In the meantime, could you rebase the patch on top of current ToT?

I thought that it should not have any merge conflicts and it turned out to be 
almost true, the only conflict I encountered was in `CMakeLists.txt`, feel free 
to use this squashed and rebased commit for testing from my fork: 
https://github.com/ilya-biryukov/llvm-project/commit/e6329b9390445369c537aa6aa5a5cd64ba27e19c


https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

We've hit an error during testing:
```cpp
In module '...stl_cc_library':
.../src/libcxx/include/__memory/allocator.h:128:60: error: 
'std::allocator>::deallocate' from 
module '...stl_cc_library./cxx17/vector' is not present in definition of 
'std::allocator>' provided earlier
  128 |   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void 
deallocate(_Tp* __p, size_t __n) _NOEXCEPT {
  |^
.../src/libcxx/include/__memory/allocator.h:94:28: note: definition has no 
member 'deallocate'
   94 | class _LIBCPP_TEMPLATE_VIS allocator : private 
__non_trivial_if::value, allocator<_Tp> > {
  |^

```

I will try to share a reproducer, but just like the last time, this might take 
quite some time because it only shows up with code that builds many modules 
before eventually producing this error.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

We'll pick this up, I'll get back with the testing results.
(Sorry for the delayed response)

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Btw, if I don't respond timely to these requests and you need an urgent 
reaction, feel free to ping me via email at ibiryu...@google.com.
We heavily use Clang header modules internally at Google and we are really 
interested in helping to discover and prevent the module bugs as early as 
possible. And we also really appreciate your work on improving and optimizing 
the modules implementation!

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Sorry for the delay, I had an emergency and took a week off.
We have hit some form of OOM (an infinite loop or some exponential computation 
allocating memory?)  on the following small example (ready as a test case, also 
in [this 
commit](https://github.com/ilya-biryukov/llvm-project/commit/6e2f55a1d1fa90b325ca2891c2a101bc79a32b9d)):

```cpp
// RUN: rm -rf %t
// RUN: mkdir -p %t
// RUN: split-file %s %t
//
// RUN: %clang_cc1 -std=c++20 %t/type_traits.cppm -emit-module-interface -o 
%t/type_traits.pcm
// RUN: %clang_cc1 -std=c++20 %t/test.cpp -fprebuilt-module-path=%t -verify

//--- type_traits.cppm
export module type_traits;

export template 
constexpr bool is_pod_v = __is_pod(T);

//--- test.cpp
import type_traits;
// Base is either void or wrapper.
template  struct wrapper : Base {};
template <> struct wrapper {};

// wrap<0>::type is wrapper, wrap<1>::type is wrapper>,
// and so on.
template 
struct wrap {
  template 
  using type = wrapper::template type>;
};

template <>
struct wrap<0> {
  template 
  using type = wrapper;
};

inline constexpr int kMaxRank = 40;
template 
using rank = typename wrap::template type;
using rank_selector_t = rank<40>;

static_assert(is_pod_v, "Must be POD");
```

There might be more problems as this one has been caught early and blocked 
further progress.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Thanks for fixing it quickly, we'll try the new patch and get back with the 
results.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

We have hit quite a few issues when trying this out. I have spent a day trying 
to reduce to a small repro that I can share, but haven't not fully succeeded 
yet, I will continue tomorrow.
Still wanted to share the error descriptions in case that would allow to make 
progress in the meanwhile.

* Crash with the following assertion failure:

```
assert.h assertion failed at 
third_party/llvm/llvm-project/clang/include/clang/AST/DeclCXX.h:464 in struct 
DefinitionData &clang::CXXRecordDecl::data() const: DD && "queried property of 
class with no definition"
*** Check failure stack trace: ***
@ 0x55fb619aaea4  absl::log_internal::LogMessage::SendToLog()
@ 0x55fb619aad04  absl::log_internal::LogMessage::Flush()
@ 0x55fb619ab209  
absl::log_internal::LogMessageFatal::~LogMessageFatal()
@ 0x55fb6199a3e4  __assert_fail
@ 0x55fb5bd8d400  clang::CXXRecordDecl::data()
@ 0x55fb5e24de0d  clang::CXXBasePaths::lookupInBases()
@ 0x55fb5e24e388  clang::CXXBasePaths::lookupInBases()
@ 0x55fb5e24d55c  clang::CXXRecordDecl::lookupInBases()
@ 0x55fb5da8bd60  clang::Sema::LookupQualifiedName()
@ 0x55fb5dc747f3  clang::Sema::CheckTypenameType()
@ 0x55fb5de19e72  clang::TreeTransform<>::TransformDependentNameType()
@ 0x55fb5ddee35c  clang::TreeTransform<>::TransformType()
@ 0x55fb5ddedd3f  clang::TreeTransform<>::TransformType()
@ 0x55fb5dded94d  clang::Sema::SubstType()
@ 0x55fb5dc62c9f  SubstDefaultTemplateArgument()
@ 0x55fb5dc58761  clang::Sema::CheckTemplateArgumentList()
@ 0x55fb5dc5720b  clang::Sema::CheckTemplateIdType()
@ 0x55fb5de19086  
clang::TreeTransform<>::TransformTemplateSpecializationType()
@ 0x55fb5de2115d  
clang::TreeTransform<>::TransformTemplateSpecializationType()
@ 0x55fb5ddee280  clang::TreeTransform<>::TransformType()
@ 0x55fb5de1dc1a  clang::TreeTransform<>::TransformElaboratedType()
@ 0x55fb5ddee3c4  clang::TreeTransform<>::TransformType()
@ 0x55fb5ddedd3f  clang::TreeTransform<>::TransformType()
@ 0x55fb5dded94d  clang::Sema::SubstType()
@ 0x55fb5dc62c9f  SubstDefaultTemplateArgument()
@ 0x55fb5dc62632  clang::Sema::SubstDefaultTemplateArgumentIfAvailable()
@ 0x55fb5dd7056b  clang::Sema::FinishTemplateArgumentDeduction()
@ 0x55fb5dde5796  llvm::function_ref<>::callback_fn<>()
@ 0x55fb5d420dcf  clang::Sema::runWithSufficientStackSpace()
@ 0x55fb5dd729f4  clang::Sema::DeduceTemplateArguments()
@ 0x55fb5dbc00d5  clang::Sema::AddTemplateOverloadCandidate()
@ 0x55fb5da7065d  ResolveConstructorOverload()
@ 0x55fb5da52dd8  TryConstructorInitialization()
@ 0x55fb5da50e7a  TryDefaultInitialization()
@ 0x55fb5da4e6b1  clang::InitializationSequence::InitializeFrom()
@ 0x55fb5d72c3ed  CollectFieldInitializer()
@ 0x55fb5d72a972  clang::Sema::SetCtorInitializers()
@ 0x55fb5d72ef65  clang::Sema::ActOnDefaultCtorInitializers()
@ 0x55fb5d2401b0  clang::Parser::ParseLexedMethodDef()
@ 0x55fb5d23e77a  clang::Parser::ParseLexedMethodDefs()
@ 0x55fb5d1b50e0  clang::Parser::ParseCXXMemberSpecification()
@ 0x55fb5d1b2744  clang::Parser::ParseClassSpecifier()
@ 0x55fb5d1d8ef0  clang::Parser::ParseDeclarationSpecifiers()
@ 0x55fb5d15ea4a  clang::Parser::ParseDeclOrFunctionDefInternal()
@ 0x55fb5d15e5b6  clang::Parser::ParseDeclarationOrFunctionDefinition()
@ 0x55fb5d15d2ea  clang::Parser::ParseExternalDeclaration()
@ 0x55fb5d1a8c73  clang::Parser::ParseInnerNamespace()
@ 0x55fb5d1a7e4f  clang::Parser::ParseNamespace()
@ 0x55fb5d1d2bde  clang::Parser::ParseDeclaration()
@ 0x55fb5d15ceb6  clang::Parser::ParseExternalDeclaration()
@ 0x55fb5d1a8c73  clang::Parser::ParseInnerNamespace()
@ 0x55fb5d1a7e4f  clang::Parser::ParseNamespace()
@ 0x55fb5d1d2bde  clang::Parser::ParseDeclaration()
@ 0x55fb5d15ceb6  clang::Parser::ParseExternalDeclaration()
@ 0x55fb5d15b04b  clang::Parser::ParseTopLevelDecl()
@ 0x55fb5d154d2e  clang::ParseAST()
@ 0x55fb5cea31e3  clang::FrontendAction::Execute()
@ 0x55fb5ce1c62d  clang::CompilerInstance::ExecuteAction()
@ 0x55fb5bd4a20e  clang::ExecuteCompilerInvocation()
@ 0x55fb5bd3e106  cc1_main()
@ 0x55fb5bd3b9a6  ExecuteCC1Tool()
```

* Invalid errors about absence or presence of certain members in 
std::pointer_traits and other types:

```
third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/pointer_traits.h:161:3:
 error: 'std::pointer_traits::pointer_to' 
from module '/
/third_party/crosstool/v18/stable:stl_cc_library.third_party/stl/cxx17/optional'
 is not present in definition of 'std::pointer_traits' provided earlier
  161 |   pointer_to(__conditional_t::value, __nat, 
elemen

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Sorry, still struggling to get a small repro. The build graphs we have are 
quite large, unfortunately.
Did any of the stack traces or error message I posted help to find certain 
problems? Or is there no hope until we get a smaller repro?

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

> Can you export all files into a standalone reproducer? I might be able to 
> reduce an example.

Not really, this is why it's taking so long. Our infrastructure in that space 
is lacking, the issue is that the root case is not in one compilation step, but 
rather in some of the dependencies and the dependency graph for any of those 
problems is really large.

We will get you a reproducer, please bear with us. Sorry for taking a long time.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

After endless hours and recompilations, I finally have this to share.
It could probably be smaller, but I decided to share this as soon as it crashed 
outside of our monorepo. 

[module.tgz](https://github.com/llvm/llvm-project/files/15223606/module.tgz)
^^ If you run build.sh with Clang 16, it builds. However, it fails with the 
current patch. 
See out/errors for a list of errors I get on my machine. 
This breaks with libstdc++ 13, but also latest libc++ (which is what we use in 
our monorepo).

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [libcxx] [clang] Preserve Qualifiers and type sugar in TemplateNames (PR #93433)

[Ilya Biryukov via llvm-branch-commits]

@@ -9304,7 +9299,8 @@ TemplateName 
ASTContext::getAssumedTemplateName(DeclarationName Name) const {
 TemplateName ASTContext::getQualifiedTemplateName(NestedNameSpecifier *NNS,
   bool TemplateKeyword,
   TemplateName Template) const 
{
-  assert(NNS && "Missing nested-name-specifier in qualified template name");

ilya-biryukov wrote:

Does that mean we will start creating `QualifiedTemplateName` with a nullptr 
qualifier?
That's a big change in contract and there is quite a bit of code that does not 
check for null when accessing this member.

Could you explain a bit why we need this change? What are we trying to 
represent?
Are there any alternative AST nodes we could use or even add?

https://github.com/llvm/llvm-project/pull/93433
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [libcxx] [clang] Preserve Qualifiers and type sugar in TemplateNames (PR #93433)

[Ilya Biryukov via llvm-branch-commits]

@@ -804,6 +804,8 @@ Bug Fixes to AST Handling
 - Clang now properly preserves ``FoundDecls`` within a ``ConceptReference``. 
(#GH82628)
 - The presence of the ``typename`` keyword is now stored in 
``TemplateTemplateParmDecl``.
 - Fixed malformed AST generated for anonymous union access in templates. 
(#GH90842)
+- Improved preservation of qualifiers and sugar in TemplateNames, including

ilya-biryukov wrote:

NIT: maybe say "template names". Alternatively, put TemplateName into double 
backticks if you are referring to the particular class.

https://github.com/llvm/llvm-project/pull/93433
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [libcxx] [clang] Preserve Qualifiers and type sugar in TemplateNames (PR #93433)

[Ilya Biryukov via llvm-branch-commits]

https://github.com/ilya-biryukov commented:

The change makes handling of template names more uniform, which overall looks 
like a great change.

However, we are changing the contract of `QualifiedTemplateName` (it can have a 
null qualifier now) and this seems both counter-intuitive from the API design 
perspective and potentially adding bugs in various places where the qualifier 
is used without checking for null.
I am not necessarily saying that's wrong, but maybe we need a different name 
for the class or a different node for the case where there is no qualifier.
We could discuss this in the relevant comment thread.

https://github.com/llvm/llvm-project/pull/93433
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [libcxx] [clang] Preserve Qualifiers and type sugar in TemplateNames (PR #93433)

[Ilya Biryukov via llvm-branch-commits]

https://github.com/ilya-biryukov edited 
https://github.com/llvm/llvm-project/pull/93433
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [libcxx] [clang] Preserve Qualifiers and type sugar in TemplateNames (PR #93433)

[Ilya Biryukov via llvm-branch-commits]

@@ -9304,7 +9299,8 @@ TemplateName 
ASTContext::getAssumedTemplateName(DeclarationName Name) const {
 TemplateName ASTContext::getQualifiedTemplateName(NestedNameSpecifier *NNS,
   bool TemplateKeyword,
   TemplateName Template) const 
{
-  assert(NNS && "Missing nested-name-specifier in qualified template name");

ilya-biryukov wrote:

Makes total sense, this change to the contract of the class seems very 
reasonable.
I have also went through all usages and we don't see to have any uses left that 
don't check for null in return of `getQualifier`.

Could we update the documentation of the class? It currently says (see below) 
that the name must always be qualified, mentioning that it may also be 
qualified only with `template` keyword without the nested name specifier.

```cpp
/// Represents a template name that was expressed as a
/// qualified name.
///
/// This kind of template name refers to a template name that was
/// preceded by a nested name specifier, e.g., \c std::vector. Here,
/// the nested name specifier is "std::" and the template name is the
/// declaration for "vector". The QualifiedTemplateName class is only
/// used to provide "sugar" for template names that were expressed
/// with a qualified name, and has no semantic meaning. In this
/// manner, it is to TemplateName what ElaboratedType is to Type,
/// providing extra syntactic sugar for downstream clients.
```

https://github.com/llvm/llvm-project/pull/93433
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Sorry for disappearing, I had a holiday, vacation and some unplanned 
interruptions over the last week and the start of this week. I have made really 
good progress and the amount of code that I need to dig through is quite 
manageable.

I should have a repro for you this week.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Here is the reproducer. Like before, builds at head and fails with the patch 
applied. After unpacking the archive just run
```
$ CLANGPP= ./build.sh
```

Note the comment in `build.sh` about the system libc++ required for it (libc++ 
17 worked, 15 and 16 did not).
[containers.tgz](https://github.com/user-attachments/files/16728578/containers.tgz)

Let me know if this reproduces for you.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Thanks for fixing the problem.

@alexfh and another person who was running these investigations before is on 
vacation until next week. I will ask if someone else can do this for them, but 
I wouldn't be surprised that it's involved enough that we may need to wait 
until next week.

Sorry about the long waiting times, but I still wanted to share so that folks 
are aware of the timelines.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

We did manage to run another round of testing and it fails, with somewhat 
familiar module-related issues:

```cpp
[third_party/absl/container/internal/compressed_tuple.h:250]:24: error: 
'absl::container_internal::CompressedTuple>>::get' from module 
'//third_party/absl/container:compressed_tuple.third_party/absl/container/internal/compressed_tuple.h'
 is not present in definition of 
'absl::container_internal::CompressedTuple>>' provided earlier
  250 |   constexpr ElemT&& get() && {
```

I am progressing towards a reproducer, hope to share something early next week.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Just wanted to give a short update that I'm still on it, but it takes longer 
than I initially anticipated.
I've got entangled into a few spaghetti-like dependencies and untangling them 
takes a bit of time... I should be much closer now, hope to share something 
tomorrow.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

I got to a small reproducer that only uses STL, but it only produces an error 
in our environment and if I try it with this patch, the error goes away.
I am probably missing something subtle, will dig deeper tomorrow.

Sorry for another delay.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

I had to replicate what our build was doing very thoroughly, the exact PCMs 
each build action receives were important.
Here's the repro: 
[maps.tgz](https://github.com/user-attachments/files/16992332/maps.tgz). Just 
like last time, it works without the patch and starts failing with the patch.


https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Sorry for loosing track of this, we'll run another round of tests and get back 
to you.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

There were some merge conflicts with main, I've managed to resolve them to 
this: https://github.com/ilya-biryukov/llvm-project/tree/pr83237_merged

I am now trying to bootstrap the compiler with that version, but it would be 
useful if someone rebased this PR against main, in case I've made mistakes 
during the merging process and the bootstrap/testing will fail because of this.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

We've hit only one correctness issue that we don't yet have a reproducer for, 
but it gives this error:
```shell
Eigen/.../plugins/CommonCwiseBinaryOps.inc:47:29: error: inline function 
'Eigen::operator*' is not defined [-Werror,-Wundefined-inline]
```
I'll be on vacation for the next two weeks, @usx95 may be able to provide a 
reproducer in the meanwhile.
Maybe you'll have ideas on why it's happening without the reproducer too.

More importantly, we have seen quite a few compile actions becoming much slower 
and causing timeouts.
@alexfh is trying to profile the compile actions to find which functions are 
the bottleneck and will report the results here.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

> @vgvassilev @ilya-biryukov @alexfh If I read correctly, the only blocker 
> issue is the above reported performance issue. And I tried to split partial 
> specialization from the full specialization table to avoid merging tables 
> again and again. Could you please take another round of testing? Thanks in 
> ahead.

@alexfh is on vacation until tomorrow, but we've already asked him to run 
another round of testing.
Thanks for fixing the issue!

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

> > > Sorry, could you provide the hash id for the commit that avoid the 
> > > warning?
> > 
> > 
> > I tried running this on head and unfortunately it reproduces on head as 
> > well :( So this looks like a sleeper issue which now also gets triggered by 
> > this PR in a non-reduced version in our codebase. Looks like it is not rare 
> > with module-related issues where most issues remain silent due to lazy 
> > deserialization in large codebases. Would it be possible for you to take a 
> > look at this issue or provide pointers ?
> 
> Got it. I am not sure if I had the time. But given this is not related to 
> this PR, and it sounds like a regression issue triggered by other commits. 
> Maybe it might be helpful to find that commit to understand the issue better.

+1, I think that fixing the underlying issue to unblock this might be the 
easiest path. Reducing and checking at every step that compiler from HEAD 
succeeds and compiler with this commit fails will likely take more time than 
actually fixing it.
@usx95 would you mind looking at the underlying issue? I'm also happy to help 
dig through it.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Thanks for bearing with us. The latest round of correctness testing came out 
positive. We have had one sleeper issue (the venerable "inline function 
undefined") pop up in the leaf of the build graph, but we managed to work 
around it.

@alexfh is still running the benchmarks to see which effect it has on the 
compilation times. But we have not seen new timeouts during correctness 
testing, and it's already a very positive sign.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

> So the meaning of stacked PR in this series of patches is pretty 
> questionable. So I feel it is better to merge these PRs into a single commit 
> so that it is easier to be cherry-picked and reverted in my experience

+100, this is definitely the right call.
... and I wish we had a better way to do stacked PRs, this is definitely one of 
the downgrades compared to Phabricator. 

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

https://github.com/ilya-biryukov approved this pull request.


https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Ilya Biryukov via llvm-branch-commits]

ilya-biryukov wrote:

Once again, thanks for bearing with us and addressing all the issues.
The latest version seems both correct and does not cause performance 
regressions.
Let's land this!

PS please note that the resolution of our compile-time profiling instruments is 
not that great, we might not notice even something like a 10% regression in 
compile times.

https://github.com/llvm/llvm-project/pull/83237
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits

[llvm-branch-commits] [clang] release/20.x: [clang] Do not infer lifetimebound for functions with void return type (#131997) (PR #133997)

[Ilya Biryukov via llvm-branch-commits]

https://github.com/ilya-biryukov approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/133997
___
llvm-branch-commits mailing list
llvm-branch-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits