Make precompiled headers reproducible by switching OpenCL extension to std::map

[riccibruno]

Summary

As discussed in https://reviews.llvm.org/D60379 let's use std::map to store OpenCL extensions data structure.

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

[lebedev.ri]

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

You could add a test that contains several entries in OpenCLTypeExtMap and several entries in OpenCLDeclExtMap.
In that test, write te PCH, and then apply llvm-bcanalyzer to that PCH. It should dump it the bitcode in textual dump.
And then simply apply FileCheck to that textual dump, with CHECK lines requiring the specific order of these entries.
If the order is not deterministic in PCH, then that test would (should!) fail sporadically.
At least that is my guess.

[Anastasia]

In D60778#1468825, @lebedev.ri wrote:

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

You could add a test that contains several entries in OpenCLTypeExtMap and several entries in OpenCLDeclExtMap.
In that test, write te PCH, and then apply llvm-bcanalyzer to that PCH. It should dump it the bitcode in textual dump.
And then simply apply FileCheck to that textual dump, with CHECK lines requiring the specific order of these entries.
If the order is not deterministic in PCH, then that test would (should!) fail sporadically.
At least that is my guess.

Ok I could do that, but I guess it can then fail on the commits that didn't actually trigger the issue. Would it not be a problem?

[lebedev.ri]

In D60778#1470152, @Anastasia wrote:

In D60778#1468825, @lebedev.ri wrote:

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

You could add a test that contains several entries in OpenCLTypeExtMap and several entries in OpenCLDeclExtMap.
In that test, write te PCH, and then apply llvm-bcanalyzer to that PCH. It should dump it the bitcode in textual dump.
And then simply apply FileCheck to that textual dump, with CHECK lines requiring the specific order of these entries.
If the order is not deterministic in PCH, then that test would (should!) fail sporadically.
At least that is my guess.

Ok I could do that, but I guess it can then fail on the commits that didn't actually trigger the issue. Would it not be a problem?

Why would it fail if this fixes the issue?
Re randomness - see https://reviews.llvm.org/D59401#change-QMkBH7328Dyv

[riccibruno]

Mmm. I hope I am not missing something obvious, but how is this actually fixing the issue ? I don't see why the order between the Type * and between the Decl * should be deterministic (I think they will be when they are part of the same slab in the allocator, but I don't see why the slab ordering should be stable).

[lebedev.ri]

In D60778#1470181, @riccibruno wrote:

Mmm. I hope I am not missing something obvious, but how is this actually fixing the issue ? I don't see why the order between the Type * and between the Decl * should be deterministic (I think they will be when they are part of the same slab in the allocator, but I don't see why the slab ordering should be stable).

Oh. Duh.
Can a custom comparator be provided for these std::map, equivalent to the one in https://reviews.llvm.org/D60379 ?
Something like getTypeID(LHS.first->getCanonicalTypeInternal()) < getTypeID(RHS.first->getCanonicalTypeInternal()) ?

[riccibruno]

Maybe, but is there an equivalent to getTypeID for declarations ?

[Anastasia]

In D60778#1470176, @lebedev.ri wrote:

In D60778#1470152, @Anastasia wrote:

In D60778#1468825, @lebedev.ri wrote:

Not sure how to test this though? I guess we can trust that std::map is always sorted just as it says in its description.

You could add a test that contains several entries in OpenCLTypeExtMap and several entries in OpenCLDeclExtMap.
In that test, write te PCH, and then apply llvm-bcanalyzer to that PCH. It should dump it the bitcode in textual dump.
And then simply apply FileCheck to that textual dump, with CHECK lines requiring the specific order of these entries.
If the order is not deterministic in PCH, then that test would (should!) fail sporadically.
At least that is my guess.

Ok I could do that, but I guess it can then fail on the commits that didn't actually trigger the issue. Would it not be a problem?

Why would it fail if this fixes the issue?

But once something is changed and it's no longer deterministic, we might not be able to detect this necessarily on the offending commit with such test?

Re randomness - see https://reviews.llvm.org/D59401#change-QMkBH7328Dyv

[Anastasia]

In D60778#1470188, @riccibruno wrote:

Maybe, but is there an equivalent to getTypeID for declarations ?

getDeclID is used in the first patch...

although now I am thinking may be original patch is a better approach?

[riccibruno]

What about something like D60835 ?

[lebedev.ri]

Abandon this?
Sorry for this fruitless effort.

[riccibruno]

Closing this since the issue was fixed in r358674.

include/clang/Sema/Sema.h

ClassTemplateDecl *lookupCoroutineTraits(SourceLocation KwLoc,
                                            SourceLocation FuncLoc);
 
   //===--------------------------------------------------------------------===//
   // OpenCL extensions.
   //
 private:
   std::string CurrOpenCLExtension;
   /// Extensions required by an OpenCL type.
-  llvm::DenseMap<const Type*, std::set<std::string>> OpenCLTypeExtMap;
+  std::map<const Type*, std::set<std::string>> OpenCLTypeExtMap;
   /// Extensions required by an OpenCL declaration.
-  llvm::DenseMap<const Decl*, std::set<std::string>> OpenCLDeclExtMap;
+  std::map<const Decl*, std::set<std::string>> OpenCLDeclExtMap;
 public:
   llvm::StringRef getCurrentOpenCLExtension() const {
     return CurrOpenCLExtension;
   }
 
   /// Check if a function declaration \p FD associates with any
   /// extensions present in OpenCLDeclExtMap and if so return the
   /// extension(s) name(s).

include/clang/Serialization/ASTReader.h

private:
   };
   llvm::SmallVector<PragmaPackStackEntry, 2> PragmaPackStack;
   llvm::SmallVector<std::string, 2> PragmaPackStrings;
 
   /// The OpenCL extension settings.
   OpenCLOptions OpenCLExtensions;
 
   /// Extensions required by an OpenCL type.
-  llvm::DenseMap<const Type *, std::set<std::string>> OpenCLTypeExtMap;
+  std::map<const Type *, std::set<std::string>> OpenCLTypeExtMap;
 
   /// Extensions required by an OpenCL declaration.
-  llvm::DenseMap<const Decl *, std::set<std::string>> OpenCLDeclExtMap;
+  std::map<const Decl *, std::set<std::string>> OpenCLDeclExtMap;
 
   /// A list of the namespaces we've seen.
   SmallVector<uint64_t, 4> KnownNamespaces;
 
   /// A list of undefined decls with internal linkage followed by the
   /// SourceLocation of a matching ODR-use.
   SmallVector<uint64_t, 8> UndefinedButUsed;