[PATCH] D120874: [C++20] [Modules] Use '-' as the separator of partitions when searching in filesystems

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

I was was asked to chime in to assess whether this patch could be a problem for
the prebuilt-implicit clang modules workflow. No problem here.
With prebuilt modules, the output file name has to be specified manually. So the
mapping does not change the existing requirement of managing the file names to
avoid collissions.
Even in the future with implicit modules - as noted by others - `:` and `-` are
not allowed in the clang module name in the `.modulemap`, so there cannot be any
conflict.

A couple comments on the way. Take them with a grain of salt, since I don't much
about how this is going to be used or what stage of the work this is.

I see the mapping is only applied on the "prebuilt" code path, and not the
"implicit" and "prebuilt implicit" code paths. I suppose at some point, module
generation will be implicit. So any particular handling of `ModuleName` to be
appended to the filesystem path will need to be done on different code paths. So
I would already abstract this away in a helper.

Without much of an informed opinion, I find @iains makes a valid point. Does
this kind of mapping (semantic module name to name being used for filesystem
search/storage) belong at a higher level ? Maybe that's to be answered or worked
on later as well.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120874/new/

https://reviews.llvm.org/D120874

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: [modules] Use `HashBuilder` and `MD5` for the module hash.

[Alexandre Rames via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG655bea4226b4: [modules] Use `HashBuilder` and `MD5` for the 
module hash. (authored by arames).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

Files:
  clang/include/clang/Basic/ObjCRuntime.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/include/clang/Serialization/ModuleFileExtension.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/TestModuleFileExtension.cpp
  clang/lib/Frontend/TestModuleFileExtension.h
  clang/lib/Serialization/ModuleFileExtension.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Support/VersionTuple.h

Index: llvm/include/llvm/Support/VersionTuple.h
===
--- llvm/include/llvm/Support/VersionTuple.h
+++ llvm/include/llvm/Support/VersionTuple.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/Support/HashBuilder.h"
 #include 
 #include 
 
@@ -164,6 +165,12 @@
 return llvm::hash_combine(VT.Major, VT.Minor, VT.Subminor, VT.Build);
   }
 
+  template 
+  friend void addHash(HashBuilderImpl &HBuilder,
+  const VersionTuple &VT) {
+HBuilder.add(VT.Major, VT.Minor, VT.Subminor, VT.Build);
+  }
+
   /// Retrieve a string representation of the version number.
   std::string getAsString() const;
 
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -860,9 +860,7 @@
 return {};
   };
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override {
-return {};
-  }
+  void hashExtension(ExtensionHashBuilder &HBuilder) const override {}
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override {
Index: clang/lib/Serialization/ModuleFileExtension.cpp
===
--- clang/lib/Serialization/ModuleFileExtension.cpp
+++ clang/lib/Serialization/ModuleFileExtension.cpp
@@ -11,12 +11,10 @@
 
 char ModuleFileExtension::ID = 0;
 
-ModuleFileExtension::~ModuleFileExtension() { }
+ModuleFileExtension::~ModuleFileExtension() {}
 
-llvm::hash_code ModuleFileExtension::hashExtension(llvm::hash_code Code) const {
-  return Code;
-}
+void ModuleFileExtension::hashExtension(ExtensionHashBuilder &HBuilder) const {}
 
-ModuleFileExtensionWriter::~ModuleFileExtensionWriter() { }
+ModuleFileExtensionWriter::~ModuleFileExtensionWriter() {}
 
-ModuleFileExtensionReader::~ModuleFileExtensionReader() { }
+ModuleFileExtensionReader::~ModuleFileExtensionReader() {}
Index: clang/lib/Frontend/TestModuleFileExtension.h
===
--- clang/lib/Frontend/TestModuleFileExtension.h
+++ clang/lib/Frontend/TestModuleFileExtension.h
@@ -55,7 +55,7 @@
 
   ModuleFileExtensionMetadata getExtensionMetadata() const override;
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override;
+  void hashExtension(ExtensionHashBuilder &HBuilder) const override;
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override;
Index: clang/lib/Frontend/TestModuleFileExtension.cpp
===
--- clang/lib/Frontend/TestModuleFileExtension.cpp
+++ clang/lib/Frontend/TestModuleFileExtension.cpp
@@ -93,16 +93,14 @@
   return { BlockName, MajorVersion, MinorVersion, UserInfo };
 }
 
-llvm::hash_code TestModuleFileExtension::hashExtension(
-  llvm::hash_code Code) const {
+void TestModuleFileExtension::hashExtension(
+ExtensionHashBuilder &HBuilder) const {
   if (Hashed) {
-Code = llvm::hash_combine(Code, BlockName);
-Code = llvm::hash_combine(Code, MajorVersion);
-Code = llvm::hash_combine(Code, MinorVersion);
-Code = llvm::hash_combine(Code, UserInfo);
+HBuilder.add(BlockName);
+HBuilder.add(MajorVersion);
+HBuilder.add(MinorVersion);
+HBuilder.add(UserInfo);
   }
-
-  return Code;
 }
 
 std::unique_ptr
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -78,6 +78,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/HashBuilder.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -4476,116 +4477,99 @@
 }
 
 std::string CompilerInvocation::getModuleHash() const {
+  // FIXME: Consider using SHA1 instead of MD5.
+  llvm::HashBuilder HBuild

[PATCH] D102943: [modules] Use `HashBuilder` and `MD5` for the module hash.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 369794.
arames marked an inline comment as done.
arames added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

Files:
  clang/include/clang/Basic/ObjCRuntime.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/include/clang/Serialization/ModuleFileExtension.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/TestModuleFileExtension.cpp
  clang/lib/Frontend/TestModuleFileExtension.h
  clang/lib/Serialization/ModuleFileExtension.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Support/VersionTuple.h

Index: llvm/include/llvm/Support/VersionTuple.h
===
--- llvm/include/llvm/Support/VersionTuple.h
+++ llvm/include/llvm/Support/VersionTuple.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/Support/HashBuilder.h"
 #include 
 #include 
 
@@ -164,6 +165,12 @@
 return llvm::hash_combine(VT.Major, VT.Minor, VT.Subminor, VT.Build);
   }
 
+  template 
+  friend void addHash(HashBuilderImpl &HBuilder,
+  const VersionTuple &VT) {
+HBuilder.add(VT.Major, VT.Minor, VT.Subminor, VT.Build);
+  }
+
   /// Retrieve a string representation of the version number.
   std::string getAsString() const;
 
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -860,9 +860,7 @@
 return {};
   };
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override {
-return {};
-  }
+  void hashExtension(ExtensionHashBuilder &HBuilder) const override {}
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override {
Index: clang/lib/Serialization/ModuleFileExtension.cpp
===
--- clang/lib/Serialization/ModuleFileExtension.cpp
+++ clang/lib/Serialization/ModuleFileExtension.cpp
@@ -11,12 +11,10 @@
 
 char ModuleFileExtension::ID = 0;
 
-ModuleFileExtension::~ModuleFileExtension() { }
+ModuleFileExtension::~ModuleFileExtension() {}
 
-llvm::hash_code ModuleFileExtension::hashExtension(llvm::hash_code Code) const {
-  return Code;
-}
+void ModuleFileExtension::hashExtension(ExtensionHashBuilder &HBuilder) const {}
 
-ModuleFileExtensionWriter::~ModuleFileExtensionWriter() { }
+ModuleFileExtensionWriter::~ModuleFileExtensionWriter() {}
 
-ModuleFileExtensionReader::~ModuleFileExtensionReader() { }
+ModuleFileExtensionReader::~ModuleFileExtensionReader() {}
Index: clang/lib/Frontend/TestModuleFileExtension.h
===
--- clang/lib/Frontend/TestModuleFileExtension.h
+++ clang/lib/Frontend/TestModuleFileExtension.h
@@ -55,7 +55,7 @@
 
   ModuleFileExtensionMetadata getExtensionMetadata() const override;
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override;
+  void hashExtension(ExtensionHashBuilder &HBuilder) const override;
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override;
Index: clang/lib/Frontend/TestModuleFileExtension.cpp
===
--- clang/lib/Frontend/TestModuleFileExtension.cpp
+++ clang/lib/Frontend/TestModuleFileExtension.cpp
@@ -93,16 +93,14 @@
   return { BlockName, MajorVersion, MinorVersion, UserInfo };
 }
 
-llvm::hash_code TestModuleFileExtension::hashExtension(
-  llvm::hash_code Code) const {
+void TestModuleFileExtension::hashExtension(
+ExtensionHashBuilder &HBuilder) const {
   if (Hashed) {
-Code = llvm::hash_combine(Code, BlockName);
-Code = llvm::hash_combine(Code, MajorVersion);
-Code = llvm::hash_combine(Code, MinorVersion);
-Code = llvm::hash_combine(Code, UserInfo);
+HBuilder.add(BlockName);
+HBuilder.add(MajorVersion);
+HBuilder.add(MinorVersion);
+HBuilder.add(UserInfo);
   }
-
-  return Code;
 }
 
 std::unique_ptr
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -78,6 +78,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/HashBuilder.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -4469,116 +4470,99 @@
 }
 
 std::string CompilerInvocation::getModuleHash() const {
+  // FIXME: Consider using SHA1 instead of MD5.
+  llvm::HashBuilder HBuilder;
+
   // Note: For QoI reasons, the things we use

[PATCH] D102943: [modules] Use `HashBuilder` and `MD5` for the module hash.

[Alexandre Rames via Phabricator via cfe-commits]

arames marked 2 inline comments as done.
arames added inline comments.



Comment at: clang/include/clang/Basic/ObjCRuntime.h:486
+  template 
+  friend void addHash(llvm::HashBuilderImpl &HBuilder,
+  const ObjCRuntime &OCR) {

dexonsmith wrote:
> arames wrote:
> > I have added these in the same line as existing `hash_value` functions. The 
> > idea being others may also need to hash those objects.
> > Let me know if you would rather move some/all of these locally in 
> > `CompilerInvocation.cpp`.
> Seems reasonable to me.
> 
> An option to consider (maybe in a separate commit?) would be to add:
> ```
> lang=c++
> class HashCodeHasher {
> public:
>   void update(ArrayRef Bytes) {
> llvm::hash_code BytesCode = llvm::hash_value(Bytes);
> Code = Code ? BytesCode : llvm::hash_combine(*Code, BytesCode);
>   }
>   Optional Code;
> };
> templatestd::enable_if_t::value, bool> = true>
> llvm::hash_code hash_value(const T &Value) {
>   HashBuilder Builder;
>   Builder.add(Value);
>   return *Builder.Code;
> }
> ```
> Then objects that support HashBuilder also support `llvm::hash_value` without 
> extra code...
I created https://reviews.llvm.org/D109024 for this.
If we decide to go for that mechanism, whichever PR lands first, I can update 
the other to use the new mechanism.




Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4473-4475
+  // FIXME: We want to use something cryptographically sound. This was
+  // initially using CityHash (via `llvm::hash_code`). We moved to `llvm::MD5`.
+  // Is thie appropriate ?

dexonsmith wrote:
> I'd simplify:
> ```
> lang=c++
> // FIXME: Consider using SHA1 instead of MD5.
> ```
Do you think we should switch to `SHA1` now ?
I picked MD5 at random amongst the available hashes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: [modules] Use `HashBuilder` and `MD5` for the module hash.

[Alexandre Rames via Phabricator via cfe-commits]

arames added inline comments.



Comment at: clang/include/clang/Basic/ObjCRuntime.h:486
+  template 
+  friend void addHash(llvm::HashBuilderImpl &HBuilder,
+  const ObjCRuntime &OCR) {

I have added these in the same line as existing `hash_value` functions. The 
idea being others may also need to hash those objects.
Let me know if you would rather move some/all of these locally in 
`CompilerInvocation.cpp`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: [modules] Use `HashBuilder` and `MD5` for the module hash.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 369489.
arames added a comment.

Fix to native endianness.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

Files:
  clang/include/clang/Basic/ObjCRuntime.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/include/clang/Serialization/ModuleFileExtension.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/TestModuleFileExtension.cpp
  clang/lib/Frontend/TestModuleFileExtension.h
  clang/lib/Serialization/ModuleFileExtension.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Support/HashBuilder.h
  llvm/include/llvm/Support/VersionTuple.h

Index: llvm/include/llvm/Support/VersionTuple.h
===
--- llvm/include/llvm/Support/VersionTuple.h
+++ llvm/include/llvm/Support/VersionTuple.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/Support/HashBuilder.h"
 #include 
 #include 
 
@@ -164,6 +165,12 @@
 return llvm::hash_combine(VT.Major, VT.Minor, VT.Subminor, VT.Build);
   }
 
+  template 
+  friend void addHash(HashBuilderImpl &HBuilder,
+  const VersionTuple &VT) {
+HBuilder.add(VT.Major, VT.Minor, VT.Subminor, VT.Build);
+  }
+
   /// Retrieve a string representation of the version number.
   std::string getAsString() const;
 
Index: llvm/include/llvm/Support/HashBuilder.h
===
--- llvm/include/llvm/Support/HashBuilder.h
+++ llvm/include/llvm/Support/HashBuilder.h
@@ -164,7 +164,7 @@
   ///
   /// ```
   /// template 
-  /// void addHash(HashBuilder &HBuilder,
+  /// void addHash(HashBuilderImpl &HBuilder,
   ///  const UserDefinedStruct &Value);
   /// ```
   ///
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -860,9 +860,7 @@
 return {};
   };
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override {
-return {};
-  }
+  void hashExtension(ExtensionHashBuilder &HBuilder) const override {}
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override {
Index: clang/lib/Serialization/ModuleFileExtension.cpp
===
--- clang/lib/Serialization/ModuleFileExtension.cpp
+++ clang/lib/Serialization/ModuleFileExtension.cpp
@@ -11,12 +11,10 @@
 
 char ModuleFileExtension::ID = 0;
 
-ModuleFileExtension::~ModuleFileExtension() { }
+ModuleFileExtension::~ModuleFileExtension() {}
 
-llvm::hash_code ModuleFileExtension::hashExtension(llvm::hash_code Code) const {
-  return Code;
-}
+void ModuleFileExtension::hashExtension(ExtensionHashBuilder &HBuilder) const {}
 
-ModuleFileExtensionWriter::~ModuleFileExtensionWriter() { }
+ModuleFileExtensionWriter::~ModuleFileExtensionWriter() {}
 
-ModuleFileExtensionReader::~ModuleFileExtensionReader() { }
+ModuleFileExtensionReader::~ModuleFileExtensionReader() {}
Index: clang/lib/Frontend/TestModuleFileExtension.h
===
--- clang/lib/Frontend/TestModuleFileExtension.h
+++ clang/lib/Frontend/TestModuleFileExtension.h
@@ -55,7 +55,7 @@
 
   ModuleFileExtensionMetadata getExtensionMetadata() const override;
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override;
+  void hashExtension(ExtensionHashBuilder &HBuilder) const override;
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override;
Index: clang/lib/Frontend/TestModuleFileExtension.cpp
===
--- clang/lib/Frontend/TestModuleFileExtension.cpp
+++ clang/lib/Frontend/TestModuleFileExtension.cpp
@@ -93,16 +93,14 @@
   return { BlockName, MajorVersion, MinorVersion, UserInfo };
 }
 
-llvm::hash_code TestModuleFileExtension::hashExtension(
-  llvm::hash_code Code) const {
+void TestModuleFileExtension::hashExtension(
+ExtensionHashBuilder &HBuilder) const {
   if (Hashed) {
-Code = llvm::hash_combine(Code, BlockName);
-Code = llvm::hash_combine(Code, MajorVersion);
-Code = llvm::hash_combine(Code, MinorVersion);
-Code = llvm::hash_combine(Code, UserInfo);
+HBuilder.add(BlockName);
+HBuilder.add(MajorVersion);
+HBuilder.add(MinorVersion);
+HBuilder.add(UserInfo);
   }
-
-  return Code;
 }
 
 std::unique_ptr
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -78,6 +78,7 @@
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/

[PATCH] D102943: [modules] Use `HashBuilder` and `MD5` for the module hash.

[Alexandre Rames via Phabricator via cfe-commits]

arames added inline comments.



Comment at: clang/include/clang/Serialization/ModuleFileExtension.h:89
+  using ExtensionHashBuilder =
+  llvm::HashBuilderImpl;
+  virtual void hashExtension(ExtensionHashBuilder &HBuilder) const;

This obviously needs to be fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: [modules] Use `HashBuilder` and `MD5` for the module hash.

[Alexandre Rames via Phabricator via cfe-commits]

arames marked 3 inline comments as done.
arames added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:4580
 
-  return llvm::APInt(64, code).toString(36, /*Signed=*/false);
+  return llvm::APInt(64, Hash.getValue()).toString(36, /*Signed=*/false);
 }

dexonsmith wrote:
> Do we just want 64 bits, or do we want the full hash?
My guess is either would be fine. I thought combining the two parts was simple 
enough.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: [modules] Use `HashBuilder` and `MD5` for the module hash.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

I still need to go through earlier comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 369203.
arames marked an inline comment as not done.
arames added a comment.

Now using the `HashBuilder` interface.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

Files:
  clang/include/clang/Basic/ObjCRuntime.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/include/clang/Serialization/ModuleFileExtension.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/TestModuleFileExtension.cpp
  clang/lib/Frontend/TestModuleFileExtension.h
  clang/lib/Serialization/ModuleFileExtension.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Support/HashBuilder.h
  llvm/include/llvm/Support/VersionTuple.h

Index: llvm/include/llvm/Support/VersionTuple.h
===
--- llvm/include/llvm/Support/VersionTuple.h
+++ llvm/include/llvm/Support/VersionTuple.h
@@ -17,6 +17,7 @@
 #include "llvm/ADT/DenseMapInfo.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/Optional.h"
+#include "llvm/Support/HashBuilder.h"
 #include 
 #include 
 
@@ -164,6 +165,12 @@
 return llvm::hash_combine(VT.Major, VT.Minor, VT.Subminor, VT.Build);
   }
 
+  template 
+  friend void addHash(HashBuilderImpl &HBuilder,
+  const VersionTuple &VT) {
+HBuilder.add(VT.Major, VT.Minor, VT.Subminor, VT.Build);
+  }
+
   /// Retrieve a string representation of the version number.
   std::string getAsString() const;
 
Index: llvm/include/llvm/Support/HashBuilder.h
===
--- llvm/include/llvm/Support/HashBuilder.h
+++ llvm/include/llvm/Support/HashBuilder.h
@@ -164,7 +164,7 @@
   ///
   /// ```
   /// template 
-  /// void addHash(HashBuilder &HBuilder,
+  /// void addHash(HashBuilderImpl &HBuilder,
   ///  const UserDefinedStruct &Value);
   /// ```
   ///
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -860,9 +860,7 @@
 return {};
   };
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override {
-return {};
-  }
+  void hashExtension(ExtensionHashBuilder &HBuilder) const override {}
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override {
Index: clang/lib/Serialization/ModuleFileExtension.cpp
===
--- clang/lib/Serialization/ModuleFileExtension.cpp
+++ clang/lib/Serialization/ModuleFileExtension.cpp
@@ -11,12 +11,10 @@
 
 char ModuleFileExtension::ID = 0;
 
-ModuleFileExtension::~ModuleFileExtension() { }
+ModuleFileExtension::~ModuleFileExtension() {}
 
-llvm::hash_code ModuleFileExtension::hashExtension(llvm::hash_code Code) const {
-  return Code;
-}
+void ModuleFileExtension::hashExtension(ExtensionHashBuilder &HBuilder) const {}
 
-ModuleFileExtensionWriter::~ModuleFileExtensionWriter() { }
+ModuleFileExtensionWriter::~ModuleFileExtensionWriter() {}
 
-ModuleFileExtensionReader::~ModuleFileExtensionReader() { }
+ModuleFileExtensionReader::~ModuleFileExtensionReader() {}
Index: clang/lib/Frontend/TestModuleFileExtension.h
===
--- clang/lib/Frontend/TestModuleFileExtension.h
+++ clang/lib/Frontend/TestModuleFileExtension.h
@@ -55,7 +55,7 @@
 
   ModuleFileExtensionMetadata getExtensionMetadata() const override;
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override;
+  void hashExtension(ExtensionHashBuilder &HBuilder) const override;
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override;
Index: clang/lib/Frontend/TestModuleFileExtension.cpp
===
--- clang/lib/Frontend/TestModuleFileExtension.cpp
+++ clang/lib/Frontend/TestModuleFileExtension.cpp
@@ -93,16 +93,14 @@
   return { BlockName, MajorVersion, MinorVersion, UserInfo };
 }
 
-llvm::hash_code TestModuleFileExtension::hashExtension(
-  llvm::hash_code Code) const {
+void TestModuleFileExtension::hashExtension(
+ExtensionHashBuilder &HBuilder) const {
   if (Hashed) {
-Code = llvm::hash_combine(Code, BlockName);
-Code = llvm::hash_combine(Code, MajorVersion);
-Code = llvm::hash_combine(Code, MinorVersion);
-Code = llvm::hash_combine(Code, UserInfo);
+HBuilder.add(BlockName);
+HBuilder.add(MajorVersion);
+HBuilder.add(MinorVersion);
+HBuilder.add(UserInfo);
   }
-
-  return Code;
 }
 
 std::unique_ptr
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -78,6 +78,7 @@

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 349141.
arames added a comment.

Fix detail namespace.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

Files:
  clang/include/clang/Basic/ObjCRuntime.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/include/clang/Serialization/ModuleFileExtension.h
  clang/include/clang/Serialization/ModuleHash.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/TestModuleFileExtension.cpp
  clang/lib/Frontend/TestModuleFileExtension.h
  clang/lib/Serialization/ModuleFileExtension.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Support/VersionTuple.h

Index: llvm/include/llvm/Support/VersionTuple.h
===
--- llvm/include/llvm/Support/VersionTuple.h
+++ llvm/include/llvm/Support/VersionTuple.h
@@ -161,5 +161,10 @@
 /// Print a version number.
 raw_ostream &operator<<(raw_ostream &Out, const VersionTuple &V);
 
+template  void updateHash(HashT &Hash, const VersionTuple &V) {
+  Hash.update({V.getMajor(), V.getMinor().getValueOr(0),
+   V.getSubminor().getValueOr(0), V.getBuild().getValueOr(0)});
+}
+
 } // end namespace llvm
 #endif // LLVM_SUPPORT_VERSIONTUPLE_H
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -860,9 +860,7 @@
 return {};
   };
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override {
-return {};
-  }
+  void hashExtension(ModuleHash &Hash) const override {}
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override {
Index: clang/lib/Serialization/ModuleFileExtension.cpp
===
--- clang/lib/Serialization/ModuleFileExtension.cpp
+++ clang/lib/Serialization/ModuleFileExtension.cpp
@@ -13,10 +13,6 @@
 
 ModuleFileExtension::~ModuleFileExtension() { }
 
-llvm::hash_code ModuleFileExtension::hashExtension(llvm::hash_code Code) const {
-  return Code;
-}
-
 ModuleFileExtensionWriter::~ModuleFileExtensionWriter() { }
 
 ModuleFileExtensionReader::~ModuleFileExtensionReader() { }
Index: clang/lib/Frontend/TestModuleFileExtension.h
===
--- clang/lib/Frontend/TestModuleFileExtension.h
+++ clang/lib/Frontend/TestModuleFileExtension.h
@@ -55,7 +55,7 @@
 
   ModuleFileExtensionMetadata getExtensionMetadata() const override;
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override;
+  void hashExtension(ModuleHash &Hash) const override;
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override;
Index: clang/lib/Frontend/TestModuleFileExtension.cpp
===
--- clang/lib/Frontend/TestModuleFileExtension.cpp
+++ clang/lib/Frontend/TestModuleFileExtension.cpp
@@ -93,16 +93,13 @@
   return { BlockName, MajorVersion, MinorVersion, UserInfo };
 }
 
-llvm::hash_code TestModuleFileExtension::hashExtension(
-  llvm::hash_code Code) const {
+void TestModuleFileExtension::hashExtension(ModuleHash &Hash) const {
   if (Hashed) {
-Code = llvm::hash_combine(Code, BlockName);
-Code = llvm::hash_combine(Code, MajorVersion);
-Code = llvm::hash_combine(Code, MinorVersion);
-Code = llvm::hash_combine(Code, UserInfo);
+Hash.update(BlockName);
+Hash.update(MajorVersion);
+Hash.update(MinorVersion);
+Hash.update(UserInfo);
   }
-
-  return Code;
 }
 
 std::unique_ptr
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -45,13 +45,13 @@
 #include "clang/Sema/CodeCompleteOptions.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ModuleFileExtension.h"
+#include "clang/Serialization/ModuleHash.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/CachedHashString.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/FloatingPointMode.h"
-#include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -4474,47 +4474,43 @@
 std::string CompilerInvocation::getModuleHash() const {
   // Note: For QoI reasons, the things we use as a hash here should all be
   // dumped via the -module-info flag.
-  using llvm::hash_code;
-  using llvm::hash_value;
-  using llvm::hash_combine;
-  using llvm::hash_combine_range;
+  ModuleHash Hash;
 
   // Start the signature with the compiler version.
-  // FIXME: We'd rather use something more cryptograph

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames added inline comments.



Comment at: clang/include/clang/Basic/Sanitizers.h:81
 
+  template  void updateHash(HashT &Hash) const {
+Hash.updateRange(&maskLoToHigh[0], &maskLoToHigh[0] + kNumElem);

An alternative to having those in class would be to have helpers directly in 
`CompilerInvocation.cpp`. I decided to keep them here for two reasons:
1. Keep them next to `hash_value`, to make it clear they need to be updated as 
well.
2. At least this class does not have all elements going into the hash `public`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 349139.
arames added a comment.

Use `llvm::MD5`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

Files:
  clang/include/clang/Basic/ObjCRuntime.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/include/clang/Serialization/ModuleFileExtension.h
  clang/include/clang/Serialization/ModuleHash.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/TestModuleFileExtension.cpp
  clang/lib/Frontend/TestModuleFileExtension.h
  clang/lib/Serialization/ModuleFileExtension.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/Support/VersionTuple.h

Index: llvm/include/llvm/Support/VersionTuple.h
===
--- llvm/include/llvm/Support/VersionTuple.h
+++ llvm/include/llvm/Support/VersionTuple.h
@@ -161,5 +161,10 @@
 /// Print a version number.
 raw_ostream &operator<<(raw_ostream &Out, const VersionTuple &V);
 
+template  void updateHash(HashT &Hash, const VersionTuple &V) {
+  Hash.update({V.getMajor(), V.getMinor().getValueOr(0),
+   V.getSubminor().getValueOr(0), V.getBuild().getValueOr(0)});
+}
+
 } // end namespace llvm
 #endif // LLVM_SUPPORT_VERSIONTUPLE_H
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -860,9 +860,7 @@
 return {};
   };
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override {
-return {};
-  }
+  void hashExtension(ModuleHash &Hash) const override {}
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override {
Index: clang/lib/Serialization/ModuleFileExtension.cpp
===
--- clang/lib/Serialization/ModuleFileExtension.cpp
+++ clang/lib/Serialization/ModuleFileExtension.cpp
@@ -13,10 +13,6 @@
 
 ModuleFileExtension::~ModuleFileExtension() { }
 
-llvm::hash_code ModuleFileExtension::hashExtension(llvm::hash_code Code) const {
-  return Code;
-}
-
 ModuleFileExtensionWriter::~ModuleFileExtensionWriter() { }
 
 ModuleFileExtensionReader::~ModuleFileExtensionReader() { }
Index: clang/lib/Frontend/TestModuleFileExtension.h
===
--- clang/lib/Frontend/TestModuleFileExtension.h
+++ clang/lib/Frontend/TestModuleFileExtension.h
@@ -55,7 +55,7 @@
 
   ModuleFileExtensionMetadata getExtensionMetadata() const override;
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override;
+  void hashExtension(ModuleHash &Hash) const override;
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override;
Index: clang/lib/Frontend/TestModuleFileExtension.cpp
===
--- clang/lib/Frontend/TestModuleFileExtension.cpp
+++ clang/lib/Frontend/TestModuleFileExtension.cpp
@@ -93,16 +93,13 @@
   return { BlockName, MajorVersion, MinorVersion, UserInfo };
 }
 
-llvm::hash_code TestModuleFileExtension::hashExtension(
-  llvm::hash_code Code) const {
+void TestModuleFileExtension::hashExtension(ModuleHash &Hash) const {
   if (Hashed) {
-Code = llvm::hash_combine(Code, BlockName);
-Code = llvm::hash_combine(Code, MajorVersion);
-Code = llvm::hash_combine(Code, MinorVersion);
-Code = llvm::hash_combine(Code, UserInfo);
+Hash.update(BlockName);
+Hash.update(MajorVersion);
+Hash.update(MinorVersion);
+Hash.update(UserInfo);
   }
-
-  return Code;
 }
 
 std::unique_ptr
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -45,13 +45,13 @@
 #include "clang/Sema/CodeCompleteOptions.h"
 #include "clang/Serialization/ASTBitCodes.h"
 #include "clang/Serialization/ModuleFileExtension.h"
+#include "clang/Serialization/ModuleHash.h"
 #include "clang/StaticAnalyzer/Core/AnalyzerOptions.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/CachedHashString.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/FloatingPointMode.h"
-#include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
@@ -4474,47 +4474,43 @@
 std::string CompilerInvocation::getModuleHash() const {
   // Note: For QoI reasons, the things we use as a hash here should all be
   // dumped via the -module-info flag.
-  using llvm::hash_code;
-  using llvm::hash_value;
-  using llvm::hash_combine;
-  using llvm::hash_combine_range;
+  ModuleHash Hash;
 
   // Start the signature with the compiler version.
-  // FIXME: We'd rather use something more cryptographicall

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

This new version is an attempt to have modules not rely on `llvm::hash_code`, 
but on a new `llvm::stable_hash_code`.
I understand modifying `ADT/Hashing.h` is sensitive, so maybe we need to 
discuss the high-level approach first.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 348406.
arames added a comment.
Herald added a subscriber: mgorny.

Diff against the parent commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

Files:
  clang/include/clang/Basic/ObjCRuntime.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/include/clang/Serialization/ModuleFileExtension.h
  clang/lib/Basic/Sanitizers.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/TestModuleFileExtension.cpp
  clang/lib/Frontend/TestModuleFileExtension.h
  clang/lib/Serialization/ModuleFileExtension.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/ADT/Hashing.h
  llvm/include/llvm/ADT/StringRef.h
  llvm/include/llvm/CodeGen/MachineStableHash.h
  llvm/include/llvm/CodeGen/StableHashing.h
  llvm/include/llvm/Support/VersionTuple.h
  llvm/lib/CodeGen/MachineStableHash.cpp
  llvm/lib/IR/LLVMContextImpl.cpp
  llvm/lib/Support/StringRef.cpp
  llvm/unittests/ADT/CMakeLists.txt
  llvm/unittests/ADT/StableHashingTest.cpp

Index: llvm/unittests/ADT/StableHashingTest.cpp
===
--- /dev/null
+++ llvm/unittests/ADT/StableHashingTest.cpp
@@ -0,0 +1,433 @@
+//===- llvm/unittest/ADT/HashingTest.cpp --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// Hashing.h unit tests for `stable_hash_code`.
+//
+// This file is an adaptation of `HashingTest.cpp`.
+//
+//===--===//
+
+#include "llvm/ADT/Hashing.h"
+#include "llvm/Support/DataTypes.h"
+#include "gtest/gtest.h"
+#include 
+#include 
+#include 
+#include 
+
+namespace llvm {
+
+// Helper for test code to print hash codes.
+void PrintTo(const stable_hash_code &code, std::ostream *os) {
+  *os << static_cast(code);
+}
+
+// Fake an object that is recognized as hashable data to test super large
+// objects.
+struct LargeTestInteger {
+  uint64_t arr[8];
+};
+
+struct NonPOD {
+  uint64_t x, y;
+  NonPOD(uint64_t x, uint64_t y) : x(x), y(y) {}
+  friend stable_hash_code stable_hash_value(const NonPOD &obj) {
+return stable_hash_combine(obj.x, obj.y);
+  }
+};
+
+namespace hashing {
+namespace detail {
+template <> struct is_hashable_data : std::true_type {};
+} // namespace detail
+} // namespace hashing
+
+} // namespace llvm
+
+using namespace llvm;
+
+namespace {
+
+enum TestEnumeration { TE_Foo = 42, TE_Bar = 43 };
+
+TEST(StableHashingTest, HashValueBasicTest) {
+  int x = 42, y = 43, c = 'x';
+  void *p = nullptr;
+  uint64_t i = 71;
+  const unsigned ci = 71;
+  volatile int vi = 71;
+  const volatile int cvi = 71;
+  uintptr_t addr = reinterpret_cast(&y);
+  EXPECT_EQ(stable_hash_value(42), stable_hash_value(x));
+  EXPECT_EQ(stable_hash_value(42), stable_hash_value(TE_Foo));
+  EXPECT_NE(stable_hash_value(42), stable_hash_value(y));
+  EXPECT_NE(stable_hash_value(42), stable_hash_value(TE_Bar));
+  EXPECT_NE(stable_hash_value(42), stable_hash_value(p));
+  EXPECT_EQ(stable_hash_value(71), stable_hash_value(i));
+  EXPECT_EQ(stable_hash_value(71), stable_hash_value(ci));
+  EXPECT_EQ(stable_hash_value(71), stable_hash_value(vi));
+  EXPECT_EQ(stable_hash_value(71), stable_hash_value(cvi));
+  EXPECT_EQ(stable_hash_value(c), stable_hash_value('x'));
+  EXPECT_EQ(stable_hash_value('4'), stable_hash_value('0' + 4));
+  EXPECT_EQ(stable_hash_value(addr), stable_hash_value(&y));
+}
+
+TEST(StableHashingTest, HashValueStdPair) {
+  EXPECT_EQ(stable_hash_combine(42, 43),
+stable_hash_value(std::make_pair(42, 43)));
+  EXPECT_NE(stable_hash_combine(43, 42),
+stable_hash_value(std::make_pair(42, 43)));
+  EXPECT_NE(stable_hash_combine(42, 43),
+stable_hash_value(std::make_pair(42ull, 43ull)));
+  EXPECT_NE(stable_hash_combine(42, 43),
+stable_hash_value(std::make_pair(42, 43ull)));
+  EXPECT_NE(stable_hash_combine(42, 43),
+stable_hash_value(std::make_pair(42ull, 43)));
+
+  // Note that pairs are implicitly flattened to a direct sequence of data and
+  // hashed efficiently as a consequence.
+  EXPECT_EQ(stable_hash_combine(42, 43, 44),
+stable_hash_value(std::make_pair(42, std::make_pair(43, 44;
+  EXPECT_EQ(stable_hash_value(std::make_pair(42, std::make_pair(43, 44))),
+stable_hash_value(std::make_pair(std::make_pair(42, 43), 44)));
+
+  // Ensure that pairs which have padding bytes *inside* them don't get treated
+  // this way.
+  EXPECT_EQ(stable_hash_combine('0', stable_hash_combine(1ull, '2')),
+stable_hash_value(std::make_pair('0', std::make

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

The early commits are missing from the PR. Looking out to do this with `arc`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 348403.
arames added a comment.

Introduce and use `stable_hash_code` instead of modifying `hash_code`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

Files:
  clang/include/clang/Basic/ObjCRuntime.h
  clang/include/clang/Basic/Sanitizers.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/include/clang/Serialization/ModuleFileExtension.h
  clang/lib/Basic/Sanitizers.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/TestModuleFileExtension.cpp
  clang/lib/Frontend/TestModuleFileExtension.h
  clang/lib/Serialization/ModuleFileExtension.cpp
  clang/unittests/Frontend/CompilerInvocationTest.cpp
  llvm/include/llvm/ADT/StringRef.h
  llvm/include/llvm/Support/VersionTuple.h
  llvm/lib/Support/StringRef.cpp

Index: llvm/lib/Support/StringRef.cpp
===
--- llvm/lib/Support/StringRef.cpp
+++ llvm/lib/Support/StringRef.cpp
@@ -600,3 +600,7 @@
 hash_code llvm::hash_value(StringRef S) {
   return hash_combine_range(S.begin(), S.end());
 }
+
+stable_hash_code llvm::stable_hash_value(StringRef S) {
+  return stable_hash_combine_range(S.begin(), S.end());
+}
Index: llvm/include/llvm/Support/VersionTuple.h
===
--- llvm/include/llvm/Support/VersionTuple.h
+++ llvm/include/llvm/Support/VersionTuple.h
@@ -149,6 +149,10 @@
 return llvm::hash_combine(VT.Major, VT.Minor, VT.Subminor, VT.Build);
   }
 
+  friend llvm::stable_hash_code stable_hash_value(const VersionTuple &VT) {
+return llvm::stable_hash_combine(VT.Major, VT.Minor, VT.Subminor, VT.Build);
+  }
+
   /// Retrieve a string representation of the version number.
   std::string getAsString() const;
 
Index: llvm/include/llvm/ADT/StringRef.h
===
--- llvm/include/llvm/ADT/StringRef.h
+++ llvm/include/llvm/ADT/StringRef.h
@@ -925,6 +925,10 @@
   LLVM_NODISCARD
   hash_code hash_value(StringRef S);
 
+  /// Compute a hash_code for a StringRef.
+  LLVM_NODISCARD
+  stable_hash_code stable_hash_value(StringRef S);
+
 } // end namespace llvm
 
 #endif // LLVM_ADT_STRINGREF_H
Index: clang/unittests/Frontend/CompilerInvocationTest.cpp
===
--- clang/unittests/Frontend/CompilerInvocationTest.cpp
+++ clang/unittests/Frontend/CompilerInvocationTest.cpp
@@ -860,7 +860,8 @@
 return {};
   };
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override {
+  llvm::stable_hash_code
+  hashExtension(llvm::stable_hash_code Code) const override {
 return {};
   }
 
Index: clang/lib/Serialization/ModuleFileExtension.cpp
===
--- clang/lib/Serialization/ModuleFileExtension.cpp
+++ clang/lib/Serialization/ModuleFileExtension.cpp
@@ -13,7 +13,8 @@
 
 ModuleFileExtension::~ModuleFileExtension() { }
 
-llvm::hash_code ModuleFileExtension::hashExtension(llvm::hash_code Code) const {
+llvm::stable_hash_code
+ModuleFileExtension::hashExtension(llvm::stable_hash_code Code) const {
   return Code;
 }
 
Index: clang/lib/Frontend/TestModuleFileExtension.h
===
--- clang/lib/Frontend/TestModuleFileExtension.h
+++ clang/lib/Frontend/TestModuleFileExtension.h
@@ -55,7 +55,8 @@
 
   ModuleFileExtensionMetadata getExtensionMetadata() const override;
 
-  llvm::hash_code hashExtension(llvm::hash_code Code) const override;
+  llvm::stable_hash_code
+  hashExtension(llvm::stable_hash_code Code) const override;
 
   std::unique_ptr
   createExtensionWriter(ASTWriter &Writer) override;
Index: clang/lib/Frontend/TestModuleFileExtension.cpp
===
--- clang/lib/Frontend/TestModuleFileExtension.cpp
+++ clang/lib/Frontend/TestModuleFileExtension.cpp
@@ -93,13 +93,13 @@
   return { BlockName, MajorVersion, MinorVersion, UserInfo };
 }
 
-llvm::hash_code TestModuleFileExtension::hashExtension(
-  llvm::hash_code Code) const {
+llvm::stable_hash_code
+TestModuleFileExtension::hashExtension(llvm::stable_hash_code Code) const {
   if (Hashed) {
-Code = llvm::hash_combine(Code, BlockName);
-Code = llvm::hash_combine(Code, MajorVersion);
-Code = llvm::hash_combine(Code, MinorVersion);
-Code = llvm::hash_combine(Code, UserInfo);
+Code = llvm::stable_hash_combine(Code, BlockName);
+Code = llvm::stable_hash_combine(Code, MajorVersion);
+Code = llvm::stable_hash_combine(Code, MinorVersion);
+Code = llvm::stable_hash_combine(Code, UserInfo);
   }
 
   return Code;
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.c

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

In D102943#2775099 , @dexonsmith 
wrote:

> In D102943#2774732 , @jroelofs 
> wrote:
>
>> why do module hashes need to be stable when cross-compiling?
>
> IIUC, the use case is cross-compiling when building the modules, which are 
> then sent to the target to use when compiling other things.

Yes, that's right.
In particular, we are talking about the hash used for for (prebuilt) implicit 
modules paths.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

In D102943#2775131 , @dexonsmith 
wrote:

> In D102943#2775115 , @pcc wrote:
>
>> Isn't the bug here that module hashing is using `hash_code`? So shouldn't 
>> the correct fix be to use a specific hashing algorithm for module hashes?
>
> Yeah, I tend to agree. I thought modules already used MD5, but maybe just for 
> the AST signature.

For reference, the issue I am trying to fix stems from the hash computed as 
part of CompilerInvocation::getModuleHash() 
.

Reading the doc around `llvm::hash_code`, my first reaction was indeed that 
modules may have to use a different hash.
I thought the current proposal was something to consider, because:

1. Practically, everything is done on `uint64_t`, so I would expect switching 
from `size_t` to `uint64_t` to have little to no impact on non-64bit targets.
2. The current modules code kind of already assumes `hash_code` is stable over 
different executions. Plain simple use-case with implicit modules would be 
severely deficient if the hash was not stable across executions. To be fair, 
the assumption is different from the one I am trying to fix of 32bit vs 64bit.

Alternatively, we could introduce something along the lines of 
`llvm::stable_hash_code` (other name suggestions welcome).
We can probably restructure the code with very few changes. (Really, it looks 
like we only need a configurable storage type, and allow skipping the execution 
seed part.)

Would you think this is a better approach ? Or do you have something else to 
suggest ?

Cheers


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102943/new/

https://reviews.llvm.org/D102943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102943: Hashing: use a 64-bit storage type on all platforms.

[Alexandre Rames via Phabricator via cfe-commits]

arames created this revision.
Herald added subscribers: dexonsmith, usaxena95, kadircet, arphaman, hiraditya.
arames requested review of this revision.
Herald added projects: clang, LLVM, clang-tools-extra.
Herald added subscribers: cfe-commits, llvm-commits.

`size_t` varying across platforms can cause issues, for example mismatching
hashes when cross-compiling modules from a 64bit target to a 32bit target.

Although the comments note that `hash_code` values "are not stable to save or
persist", it is effectively used in such cases today, for example for implicit
module hashes.

Since hashing mechanisms in practice already use `uint64_t` for computations,
use `uint64_t` instead of `size_t` to store the value of `hash_code`.
This is similar to earlier changes in c0445098519defc4bd13624095fac92977c9cfe4.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102943

Files:
  clang-tools-extra/clangd/index/SymbolID.cpp
  clang/include/clang/AST/DataCollection.h
  clang/lib/Lex/HeaderSearch.cpp
  llvm/include/llvm/ADT/Hashing.h
  llvm/lib/CodeGen/GlobalISel/RegisterBankInfo.cpp
  llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp
  llvm/unittests/ADT/HashingTest.cpp

Index: llvm/unittests/ADT/HashingTest.cpp
===
--- llvm/unittests/ADT/HashingTest.cpp
+++ llvm/unittests/ADT/HashingTest.cpp
@@ -22,7 +22,7 @@
 
 // Helper for test code to print hash codes.
 void PrintTo(const hash_code &code, std::ostream *os) {
-  *os << static_cast(code);
+  *os << static_cast(code);
 }
 
 // Fake an object that is recognized as hashable data to test super large
@@ -134,7 +134,7 @@
 
 // Provide a dummy, hashable type designed for easy verification: its hash is
 // the same as its value.
-struct HashableDummy { size_t value; };
+struct HashableDummy { uint64_t value; };
 hash_code hash_value(HashableDummy dummy) { return dummy.value; }
 
 TEST(HashingTest, HashCombineRangeBasicTest) {
@@ -172,7 +172,7 @@
   EXPECT_NE(dummy_hash, arr4_hash);
   EXPECT_NE(arr1_hash, arr4_hash);
 
-  const size_t arr5[] = { 1, 2, 3 };
+  const uint64_t arr5[] = { 1, 2, 3 };
   const HashableDummy d_arr5[] = { {1}, {2}, {3} };
   hash_code arr5_hash = hash_combine_range(begin(arr5), end(arr5));
   hash_code d_arr5_hash = hash_combine_range(begin(d_arr5), end(d_arr5));
@@ -182,11 +182,11 @@
 TEST(HashingTest, HashCombineRangeLengthDiff) {
   // Test that as only the length varies, we compute different hash codes for
   // sequences.
-  std::map code_to_size;
+  std::map code_to_size;
   std::vector all_one_c(256, '\xff');
   for (unsigned Idx = 1, Size = all_one_c.size(); Idx < Size; ++Idx) {
 hash_code code = hash_combine_range(&all_one_c[0], &all_one_c[0] + Idx);
-std::map::iterator
+std::map::iterator
   I = code_to_size.insert(std::make_pair(code, Idx)).first;
 EXPECT_EQ(Idx, I->second);
   }
@@ -194,7 +194,7 @@
   std::vector all_zero_c(256, '\0');
   for (unsigned Idx = 1, Size = all_zero_c.size(); Idx < Size; ++Idx) {
 hash_code code = hash_combine_range(&all_zero_c[0], &all_zero_c[0] + Idx);
-std::map::iterator
+std::map::iterator
   I = code_to_size.insert(std::make_pair(code, Idx)).first;
 EXPECT_EQ(Idx, I->second);
   }
@@ -202,7 +202,7 @@
   std::vector all_one_int(512, -1);
   for (unsigned Idx = 1, Size = all_one_int.size(); Idx < Size; ++Idx) {
 hash_code code = hash_combine_range(&all_one_int[0], &all_one_int[0] + Idx);
-std::map::iterator
+std::map::iterator
   I = code_to_size.insert(std::make_pair(code, Idx)).first;
 EXPECT_EQ(Idx, I->second);
   }
@@ -210,7 +210,7 @@
   std::vector all_zero_int(512, 0);
   for (unsigned Idx = 1, Size = all_zero_int.size(); Idx < Size; ++Idx) {
 hash_code code = hash_combine_range(&all_zero_int[0], &all_zero_int[0] + Idx);
-std::map::iterator
+std::map::iterator
   I = code_to_size.insert(std::make_pair(code, Idx)).first;
 EXPECT_EQ(Idx, I->second);
   }
@@ -283,8 +283,7 @@
 fprintf(stderr, " { %-35s 0x%016llxULL },\n",
 member_str.c_str(), static_cast(hash));
 #endif
-EXPECT_EQ(static_cast(golden_data[i].hash),
-  static_cast(hash));
+EXPECT_EQ(golden_data[i].hash, static_cast(hash));
   }
 }
 
@@ -304,9 +303,9 @@
   // Hashing a sequence of heterogeneous types which *happen* to all produce the
   // same data for hashing produces the same as a range-based hash of the
   // fundamental values.
-  const size_t s1 = 1024, s2 = , s3 = 900;
+  const uint64_t s1 = 1024, s2 = , s3 = 900;
   const HashableDummy d1 = { 1024 }, d2 = {  }, d3 = { 900 };
-  const size_t arr2[] = { s1, s2, s3 };
+  const uint64_t arr2[] = { s1, s2, s3 };
   EXPECT_EQ(hash_combine_range(begin(arr2), end(arr2)),
 hash_combine(s1, s2, s3));
   EXPECT_EQ(hash_combine(s1, s2, s3), hash_combine(s1, s2, d3));
Index: llvm/lib/ExecutionEngine/Orc/CompileOnDemandLayer.cpp

[PATCH] D90963: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames created this revision.
arames added a reviewer: stella.stamenova.
Herald added subscribers: cfe-commits, dang.
Herald added a project: clang.
arames requested review of this revision.

This reverts commit c67656b994c87224e0b33e2c4b09093986a5cfa6 
, and 
addresses the
build issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D90963

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,35 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_a
+
+// Check a module cache path is not required when all modules resolve to
+// prebuilt implicit modules.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t
+
+// Check that we correctly fall back to implicit modules if the prebuilt implicit module is not found.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1 -fno-signed-char
+// RUN: find %t1 -name "module_a*.pcm" | grep module_a
+
+// Check that non-implicit prebuilt modules are always preferred to prebuilt implicit modules.
+// RUN: rm -rf %t2
+// RUN: mkdir -p %t2
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
+// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
+
+// expected-no-diagnostics
+@import module_a;
+int test() {
+  return a;
+}
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
@@ -0,0 +1 @@
+module module_a { header "a.h" }
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
@@ -0,0 +1 @@
+const int a = 1;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1320,6 +1320,7 @@
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
   Record.push_back(HSOpts.ModuleMapFileHomeIsCwd);
+  Record.push_back(HSOpts.EnablePrebuiltImplicitModules);
   Record.push_back(HSOpts.UseBuiltinIncludes);
   Record.push_back(HSOpts.UseStandardSystemIncludes);
   Record.push_back(HSOpts.UseStandardCXXIncludes);
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5844,6 +5844,7 @@
   HSOpts.DisableModuleHash = Record[Idx++];
   HSOpts.ImplicitModuleMaps = Record[Idx++];
   HSOpts.ModuleMapFil

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

In D68997#2377641 , @stella.stamenova 
wrote:

> This change broke the windows lldb bot:
>
> http://lab.llvm.org:8011/#/builders/83/builds/570
>
> Can you please fix this or revert?

Looking at this now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG71e108cd86e7: Allow searching for prebuilt implicit modules. 
(authored by arames).

Changed prior to commit:
  https://reviews.llvm.org/D68997?vs=300780&id=303243#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,35 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_a
+
+// Check a module cache path is not required when all modules resolve to
+// prebuilt implicit modules.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t
+
+// Check that we correctly fall back to implicit modules if the prebuilt implicit module is not found.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1 -fno-signed-char
+// RUN: find %t1 -name "module_a*.pcm" | grep module_a
+
+// Check that non-implicit prebuilt modules are always preferred to prebuilt implicit modules.
+// RUN: rm -rf %t2
+// RUN: mkdir -p %t2
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
+// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
+
+// expected-no-diagnostics
+@import module_a;
+int test() {
+  return a;
+}
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
@@ -0,0 +1 @@
+module module_a { header "a.h" }
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
@@ -0,0 +1 @@
+const int a = 1;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1320,6 +1320,7 @@
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
   Record.push_back(HSOpts.ModuleMapFileHomeIsCwd);
+  Record.push_back(HSOpts.EnablePrebuiltImplicitModules);
   Record.push_back(HSOpts.UseBuiltinIncludes);
   Record.push_back(HSOpts.UseStandardSystemIncludes);
   Record.push_back(HSOpts.UseStandardCXXIncludes);
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5844,6 +5844,7 @@
   HSOpts.DisableModuleHash = Record[Idx++];
   HSOpts.ImplicitModuleMaps = Record[Idx++

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames marked an inline comment as done.
arames added a comment.

Fixed the trailing whitespace.

I also just got commit rights, so I will commit it myself.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 300780.
arames added a comment.

Rebased on ToT.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,35 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_a
+
+// Check a module cache path is not required when all modules resolve to
+// prebuilt implicit modules.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t
+
+// Check that we correctly fall back to implicit modules if the prebuilt implicit module is not found.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1 -fno-signed-char
+// RUN: find %t1 -name "module_a*.pcm" | grep module_a
+
+// Check that non-implicit prebuilt modules are always preferred to prebuilt implicit modules.
+// RUN: rm -rf %t2
+// RUN: mkdir -p %t2
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
+// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
+
+// expected-no-diagnostics
+@import module_a;
+int test() {
+  return a;
+}
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
@@ -0,0 +1 @@
+module module_a { header "a.h" }
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
@@ -0,0 +1 @@
+const int a = 1;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1320,6 +1320,7 @@
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
   Record.push_back(HSOpts.ModuleMapFileHomeIsCwd);
+  Record.push_back(HSOpts.EnablePrebuiltImplicitModules);
   Record.push_back(HSOpts.UseBuiltinIncludes);
   Record.push_back(HSOpts.UseStandardSystemIncludes);
   Record.push_back(HSOpts.UseStandardCXXIncludes);
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5844,6 +5844,7 @@
   HSOpts.DisableModuleHash = Record[Idx++];
   HSOpts.ImplicitModuleMaps = Record[Idx++];
   HSOpts.ModuleMapFileHomeIsCwd = Record[Idx++];
+  HSOpts.EnablePrebuiltImplicitModules = Record[Idx++];
   HSOpts.UseBuiltinIncludes = Record[Idx++];
   HSOpts.UseStandardSystemIncludes = Record[Idx++];
   HSOpts.UseStandard

[PATCH] D88265: [Sema] Support Comma operator for fp16 vectors.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.



> Oh I think you'd need to edit the revision on Phabricator (top right on this 
> page, `edit revision`). But if you are fine with it I can commit the patch 
> for you with the adjusted commit title.

Done !
Thanks for your help !


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88265/new/

https://reviews.llvm.org/D88265

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88265: Fix comma with half vectors.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

In D88265#2302653 , @fhahn wrote:

> Could you adjust the commit message to be a bit more descriptive, e.g 
> something like `[Sema] Support Comma operator for fp16 vectors.`

Done!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88265/new/

https://reviews.llvm.org/D88265

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88265: Fix comma with half vectors.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 295316.
arames added a comment.

Update commit message.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88265/new/

https://reviews.llvm.org/D88265

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/fp16vec-sema.c


Index: clang/test/Sema/fp16vec-sema.c
===
--- clang/test/Sema/fp16vec-sema.c
+++ clang/test/Sema/fp16vec-sema.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify %s
 
 typedef __fp16 half4 __attribute__ ((vector_size (8)));
 typedef float float4 __attribute__ ((vector_size (16)));
@@ -28,6 +28,8 @@
   sv0 = hv0 >= hv1;
   sv0 = hv0 || hv1; // expected-error{{logical expression with vector types 
'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
   sv0 = hv0 && hv1; // expected-error{{logical expression with vector types 
'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
+  hv0, 1;
+  1, hv0;
 
   // Implicit conversion between half vectors and float vectors are not 
allowed.
   hv0 = fv0; // expected-error{{assigning to}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13933,9 +13933,10 @@
   // float vectors and truncating the result back to half vector. For now, we 
do
   // this only when HalfArgsAndReturn is set (that is, when the target is arm 
or
   // arm64).
-  assert(isVector(RHS.get()->getType(), Context.HalfTy) ==
- isVector(LHS.get()->getType(), Context.HalfTy) &&
- "both sides are half vectors or neither sides are");
+  assert(
+  (Opc == BO_Comma || isVector(RHS.get()->getType(), Context.HalfTy) ==
+  isVector(LHS.get()->getType(), Context.HalfTy)) 
&&
+  "both sides are half vectors or neither sides are");
   ConvertHalfVec =
   needsConversionOfHalfVec(ConvertHalfVec, Context, LHS.get(), RHS.get());
 


Index: clang/test/Sema/fp16vec-sema.c
===
--- clang/test/Sema/fp16vec-sema.c
+++ clang/test/Sema/fp16vec-sema.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify %s
 
 typedef __fp16 half4 __attribute__ ((vector_size (8)));
 typedef float float4 __attribute__ ((vector_size (16)));
@@ -28,6 +28,8 @@
   sv0 = hv0 >= hv1;
   sv0 = hv0 || hv1; // expected-error{{logical expression with vector types 'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
   sv0 = hv0 && hv1; // expected-error{{logical expression with vector types 'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
+  hv0, 1;
+  1, hv0;
 
   // Implicit conversion between half vectors and float vectors are not allowed.
   hv0 = fv0; // expected-error{{assigning to}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13933,9 +13933,10 @@
   // float vectors and truncating the result back to half vector. For now, we do
   // this only when HalfArgsAndReturn is set (that is, when the target is arm or
   // arm64).
-  assert(isVector(RHS.get()->getType(), Context.HalfTy) ==
- isVector(LHS.get()->getType(), Context.HalfTy) &&
- "both sides are half vectors or neither sides are");
+  assert(
+  (Opc == BO_Comma || isVector(RHS.get()->getType(), Context.HalfTy) ==
+  isVector(LHS.get()->getType(), Context.HalfTy)) &&
+  "both sides are half vectors or neither sides are");
   ConvertHalfVec =
   needsConversionOfHalfVec(ConvertHalfVec, Context, LHS.get(), RHS.get());
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88265: Fix comma with half vectors.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

I do not have commit rights, so it would be great if you can land it.

Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88265/new/

https://reviews.llvm.org/D88265

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88265: Fix comma with half vectors.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 295116.
arames added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88265/new/

https://reviews.llvm.org/D88265

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/fp16vec-sema.c


Index: clang/test/Sema/fp16vec-sema.c
===
--- clang/test/Sema/fp16vec-sema.c
+++ clang/test/Sema/fp16vec-sema.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify %s
 
 typedef __fp16 half4 __attribute__ ((vector_size (8)));
 typedef float float4 __attribute__ ((vector_size (16)));
@@ -28,6 +28,8 @@
   sv0 = hv0 >= hv1;
   sv0 = hv0 || hv1; // expected-error{{logical expression with vector types 
'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
   sv0 = hv0 && hv1; // expected-error{{logical expression with vector types 
'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
+  hv0, 1;
+  1, hv0;
 
   // Implicit conversion between half vectors and float vectors are not 
allowed.
   hv0 = fv0; // expected-error{{assigning to}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13933,9 +13933,10 @@
   // float vectors and truncating the result back to half vector. For now, we 
do
   // this only when HalfArgsAndReturn is set (that is, when the target is arm 
or
   // arm64).
-  assert(isVector(RHS.get()->getType(), Context.HalfTy) ==
- isVector(LHS.get()->getType(), Context.HalfTy) &&
- "both sides are half vectors or neither sides are");
+  assert(
+  (Opc == BO_Comma || isVector(RHS.get()->getType(), Context.HalfTy) ==
+  isVector(LHS.get()->getType(), Context.HalfTy)) 
&&
+  "both sides are half vectors or neither sides are");
   ConvertHalfVec =
   needsConversionOfHalfVec(ConvertHalfVec, Context, LHS.get(), RHS.get());
 


Index: clang/test/Sema/fp16vec-sema.c
===
--- clang/test/Sema/fp16vec-sema.c
+++ clang/test/Sema/fp16vec-sema.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-unused-value -verify %s
 
 typedef __fp16 half4 __attribute__ ((vector_size (8)));
 typedef float float4 __attribute__ ((vector_size (16)));
@@ -28,6 +28,8 @@
   sv0 = hv0 >= hv1;
   sv0 = hv0 || hv1; // expected-error{{logical expression with vector types 'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
   sv0 = hv0 && hv1; // expected-error{{logical expression with vector types 'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
+  hv0, 1;
+  1, hv0;
 
   // Implicit conversion between half vectors and float vectors are not allowed.
   hv0 = fv0; // expected-error{{assigning to}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13933,9 +13933,10 @@
   // float vectors and truncating the result back to half vector. For now, we do
   // this only when HalfArgsAndReturn is set (that is, when the target is arm or
   // arm64).
-  assert(isVector(RHS.get()->getType(), Context.HalfTy) ==
- isVector(LHS.get()->getType(), Context.HalfTy) &&
- "both sides are half vectors or neither sides are");
+  assert(
+  (Opc == BO_Comma || isVector(RHS.get()->getType(), Context.HalfTy) ==
+  isVector(LHS.get()->getType(), Context.HalfTy)) &&
+  "both sides are half vectors or neither sides are");
   ConvertHalfVec =
   needsConversionOfHalfVec(ConvertHalfVec, Context, LHS.get(), RHS.get());
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88265: Fix comma with half vectors.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 295025.
arames marked an inline comment as done.
arames added a comment.

Addressed review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88265/new/

https://reviews.llvm.org/D88265

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/fp16vec-sema.c


Index: clang/test/Sema/fp16vec-sema.c
===
--- clang/test/Sema/fp16vec-sema.c
+++ clang/test/Sema/fp16vec-sema.c
@@ -28,6 +28,8 @@
   sv0 = hv0 >= hv1;
   sv0 = hv0 || hv1; // expected-error{{logical expression with vector types 
'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
   sv0 = hv0 && hv1; // expected-error{{logical expression with vector types 
'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
+  hv0, 1; // expected-warning 2 {{expression result unused}}
+  1, hv0; // expected-warning 2 {{expression result unused}}
 
   // Implicit conversion between half vectors and float vectors are not 
allowed.
   hv0 = fv0; // expected-error{{assigning to}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13933,9 +13933,10 @@
   // float vectors and truncating the result back to half vector. For now, we 
do
   // this only when HalfArgsAndReturn is set (that is, when the target is arm 
or
   // arm64).
-  assert(isVector(RHS.get()->getType(), Context.HalfTy) ==
- isVector(LHS.get()->getType(), Context.HalfTy) &&
- "both sides are half vectors or neither sides are");
+  assert(
+  (Opc == BO_Comma || isVector(RHS.get()->getType(), Context.HalfTy) ==
+  isVector(LHS.get()->getType(), Context.HalfTy)) 
&&
+  "both sides are half vectors or neither sides are");
   ConvertHalfVec =
   needsConversionOfHalfVec(ConvertHalfVec, Context, LHS.get(), RHS.get());
 


Index: clang/test/Sema/fp16vec-sema.c
===
--- clang/test/Sema/fp16vec-sema.c
+++ clang/test/Sema/fp16vec-sema.c
@@ -28,6 +28,8 @@
   sv0 = hv0 >= hv1;
   sv0 = hv0 || hv1; // expected-error{{logical expression with vector types 'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
   sv0 = hv0 && hv1; // expected-error{{logical expression with vector types 'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
+  hv0, 1; // expected-warning 2 {{expression result unused}}
+  1, hv0; // expected-warning 2 {{expression result unused}}
 
   // Implicit conversion between half vectors and float vectors are not allowed.
   hv0 = fv0; // expected-error{{assigning to}}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13933,9 +13933,10 @@
   // float vectors and truncating the result back to half vector. For now, we do
   // this only when HalfArgsAndReturn is set (that is, when the target is arm or
   // arm64).
-  assert(isVector(RHS.get()->getType(), Context.HalfTy) ==
- isVector(LHS.get()->getType(), Context.HalfTy) &&
- "both sides are half vectors or neither sides are");
+  assert(
+  (Opc == BO_Comma || isVector(RHS.get()->getType(), Context.HalfTy) ==
+  isVector(LHS.get()->getType(), Context.HalfTy)) &&
+  "both sides are half vectors or neither sides are");
   ConvertHalfVec =
   needsConversionOfHalfVec(ConvertHalfVec, Context, LHS.get(), RHS.get());
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88265: Fix comma with half vectors.

[Alexandre Rames via Phabricator via cfe-commits]

arames marked 2 inline comments as done.
arames added inline comments.



Comment at: clang/test/Sema/fp16vec-sema.c:29
   sv0 = hv0 >= hv1;
+  hv0, 1; // expected-warning 2 {{expression result unused}}
   sv0 = hv0 || hv1; // expected-error{{logical expression with vector types 
'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}

fhahn wrote:
> nit: it might be slightly better to move it 2 lines further down, so all 
> operators that assign the result are grouped together. Also, should we add 
> `1, hv0` as well?
Moved.
I added `1, hv0` for completeness. It would hit the same path as the reverse.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88265/new/

https://reviews.llvm.org/D88265

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88265: Fix comma with half vectors.

[Alexandre Rames via Phabricator via cfe-commits]

arames created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
arames requested review of this revision.

The current half vector was enforcing an assert expecting
 "(LHS is half vector) == (RHS is half vector)"
for comma.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88265

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/Sema/fp16vec-sema.c


Index: clang/test/Sema/fp16vec-sema.c
===
--- clang/test/Sema/fp16vec-sema.c
+++ clang/test/Sema/fp16vec-sema.c
@@ -26,6 +26,7 @@
   sv0 = hv0 > hv1;
   sv0 = hv0 <= hv1;
   sv0 = hv0 >= hv1;
+  hv0, 1; // expected-warning 2 {{expression result unused}}
   sv0 = hv0 || hv1; // expected-error{{logical expression with vector types 
'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
   sv0 = hv0 && hv1; // expected-error{{logical expression with vector types 
'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13933,9 +13933,10 @@
   // float vectors and truncating the result back to half vector. For now, we 
do
   // this only when HalfArgsAndReturn is set (that is, when the target is arm 
or
   // arm64).
-  assert(isVector(RHS.get()->getType(), Context.HalfTy) ==
- isVector(LHS.get()->getType(), Context.HalfTy) &&
- "both sides are half vectors or neither sides are");
+  assert(Opc == BO_Comma ||
+ isVector(RHS.get()->getType(), Context.HalfTy) ==
+ isVector(LHS.get()->getType(), Context.HalfTy) &&
+ "both sides are half vectors or neither sides are");
   ConvertHalfVec =
   needsConversionOfHalfVec(ConvertHalfVec, Context, LHS.get(), RHS.get());
 


Index: clang/test/Sema/fp16vec-sema.c
===
--- clang/test/Sema/fp16vec-sema.c
+++ clang/test/Sema/fp16vec-sema.c
@@ -26,6 +26,7 @@
   sv0 = hv0 > hv1;
   sv0 = hv0 <= hv1;
   sv0 = hv0 >= hv1;
+  hv0, 1; // expected-warning 2 {{expression result unused}}
   sv0 = hv0 || hv1; // expected-error{{logical expression with vector types 'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
   sv0 = hv0 && hv1; // expected-error{{logical expression with vector types 'half4' (vector of 4 '__fp16' values) and 'half4' is only supported in C++}}
 
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -13933,9 +13933,10 @@
   // float vectors and truncating the result back to half vector. For now, we do
   // this only when HalfArgsAndReturn is set (that is, when the target is arm or
   // arm64).
-  assert(isVector(RHS.get()->getType(), Context.HalfTy) ==
- isVector(LHS.get()->getType(), Context.HalfTy) &&
- "both sides are half vectors or neither sides are");
+  assert(Opc == BO_Comma ||
+ isVector(RHS.get()->getType(), Context.HalfTy) ==
+ isVector(LHS.get()->getType(), Context.HalfTy) &&
+ "both sides are half vectors or neither sides are");
   ConvertHalfVec =
   needsConversionOfHalfVec(ConvertHalfVec, Context, LHS.get(), RHS.get());
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames added inline comments.



Comment at: clang/include/clang/Lex/HeaderSearch.h:187
+  /// The hash used for module cache paths.
+  std::string ModuleHash;
+

In the previous version of this patch, this value was derived from the stem of 
`ModuleCachePath`.
However we do not want to depend on `ModuleCachePath` being set to be able to 
retrieve the module hash. See the update in the test.



Comment at: clang/test/Modules/prebuilt-implicit-modules.m:11
+
+// Check a module cache path is not required when all modules resolve to a
+// prebuilt implicit modules.

For this to be possible, the computation of the paths to (potential) implicit 
prebuilt modules cannot depend on the cache path being set.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 282336.
arames added a comment.

Fix a typo.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,35 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_a
+
+// Check a module cache path is not required when all modules resolve to
+// prebuilt implicit modules.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t
+
+// Check that we correctly fall back to implicit modules if the prebuilt implicit module is not found.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1 -fno-signed-char
+// RUN: find %t1 -name "module_a*.pcm" | grep module_a
+
+// Check that non-implicit prebuilt modules are always preferred to prebuilt implicit modules.
+// RUN: rm -rf %t2
+// RUN: mkdir -p %t2
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
+// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
+
+// expected-no-diagnostics
+@import module_a;
+int test() {
+  return a;
+}
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
@@ -0,0 +1 @@
+module module_a { header "a.h" }
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
@@ -0,0 +1 @@
+const int a = 1;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1319,6 +1319,7 @@
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
   Record.push_back(HSOpts.ModuleMapFileHomeIsCwd);
+  Record.push_back(HSOpts.EnablePrebuiltImplicitModules);
   Record.push_back(HSOpts.UseBuiltinIncludes);
   Record.push_back(HSOpts.UseStandardSystemIncludes);
   Record.push_back(HSOpts.UseStandardCXXIncludes);
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5847,6 +5847,7 @@
   HSOpts.DisableModuleHash = Record[Idx++];
   HSOpts.ImplicitModuleMaps = Record[Idx++];
   HSOpts.ModuleMapFileHomeIsCwd = Record[Idx++];
+  HSOpts.EnablePrebuiltImplicitModules = Record[Idx++];
   HSOpts.UseBuiltinIncludes = Record[Idx++];
   HSOpts.UseStandardSystemIncludes = Record[Idx++];
   HSOpts.UseStandardCXXI

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 282335.
arames marked an inline comment as done.
arames added a comment.

Addressed review comments.

- Fixed typos in the doc.
- Added doc about module compatibility.
- Cleaned and tested commands in the doc.
- Reworked module hash code to not require `-fmodules-cache-path` when using 
`-fprebuilt-implicit-modules`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInstance.h
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,35 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_a
+
+// Check a module cache path is not required when all modules resolve to a
+// prebuilt implicit modules.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t
+
+// Check that we correctly fall back to implicit modules if the prebuilt implicit module is not found.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1 -fno-signed-char
+// RUN: find %t1 -name "module_a*.pcm" | grep module_a
+
+// Check that non-implicit prebuilt modules are always preferred to prebuilt implicit modules.
+// RUN: rm -rf %t2
+// RUN: mkdir -p %t2
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
+// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
+
+// expected-no-diagnostics
+@import module_a;
+int test() {
+  return a;
+}
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
@@ -0,0 +1 @@
+module module_a { header "a.h" }
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
@@ -0,0 +1 @@
+const int a = 1;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1319,6 +1319,7 @@
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
   Record.push_back(HSOpts.ModuleMapFileHomeIsCwd);
+  Record.push_back(HSOpts.EnablePrebuiltImplicitModules);
   Record.push_back(HSOpts.UseBuiltinIncludes);
   Record.push_back(HSOpts.UseStandardSystemIncludes);
   Record.push_back(HSOpts.UseStandardCXXIncludes);
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5847,6 +5847,7 @@
   HSOpts.DisableModuleHash = Record[Idx++];
 

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames marked 2 inline comments as done.
arames added inline comments.



Comment at: clang/docs/Modules.rst:295
+
+A trick to prebuilt required modules in one command is to generate implicit 
modules using the ``-fdisable-module-hash`` option.
+

Bigcheese wrote:
> I also think it's important to point out here that disabling the module hash 
> means you are responsible for making sure the modules are compatible. How to 
> do this is mentioned below, but it's never stated why you would want to do 
> this.
Fixed the typo.

Good point.
I am refactoring the examples to make everything clearer.



Comment at: clang/docs/Modules.rst:303
+  # prebuilt/A.pcm  prebuilt/B.pcm
+  clang -cc1 -x c use.c -fmodules -fprebuilt-module-path=prebuilt -o /use.o
+

Fixed a few other typos in the examples as well.



Comment at: clang/test/Modules/prebuilt-implicit-modules.m:9
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module 
-fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap 
-fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_e
+//

Bigcheese wrote:
> Is this actually supposed to be `module_e`? I would expect this to be 
> verifying that it didn't add `module_a` to `%t1`.
That was a typo. Fixed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 281602.
arames added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+//
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_e
+//
+// Check that we correctly fall back to implicit modules if the prebuilt implicit module is not found.
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1 -fno-signed-char
+// RUN: find %t1 -name "module_a*.pcm" | grep module_a
+
+// Check that non-implicit prebuilt modules are always preferred to prebuilt implicit modules.
+// RUN: rm -rf %t2
+// RUN: mkdir -p %t2
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
+// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
+
+// expected-no-diagnostics
+@import module_a;
+int test() {
+  return a;
+}
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
@@ -0,0 +1 @@
+module module_a { header "a.h" }
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
@@ -0,0 +1 @@
+const int a = 1;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1319,6 +1319,7 @@
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
   Record.push_back(HSOpts.ModuleMapFileHomeIsCwd);
+  Record.push_back(HSOpts.EnablePrebuiltImplicitModules);
   Record.push_back(HSOpts.UseBuiltinIncludes);
   Record.push_back(HSOpts.UseStandardSystemIncludes);
   Record.push_back(HSOpts.UseStandardCXXIncludes);
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5847,6 +5847,7 @@
   HSOpts.DisableModuleHash = Record[Idx++];
   HSOpts.ImplicitModuleMaps = Record[Idx++];
   HSOpts.ModuleMapFileHomeIsCwd = Record[Idx++];
+  HSOpts.EnablePrebuiltImplicitModules = Record[Idx++];
   HSOpts.UseBuiltinIncludes = Record[Idx++];
   HSOpts.UseStandardSystemIncludes = Record[Idx++];
   HSOpts.UseStandardCXXIncludes = Record[Idx++];
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -164,14 +164,41 @@
   return {};
 }
 
+std::string HeaderSearch::getPrebuiltImplicitModuleFileName(Module *Module) {
+  const FileEntry *ModuleMap =
+  getModuleMap().getModuleMapFileForUniquing(Module);
+  StringRef ModuleName = Module->Name

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 279263.
arames added a comment.
Herald added a subscriber: dang.

Rebase on top-of-tree.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+//
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_e
+//
+// Check that we correctly fall back to implicit modules if the prebuilt implicit module is not found.
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1 -fno-signed-char
+// RUN: find %t1 -name "module_a*.pcm" | grep module_a
+
+// Check that non-implicit prebuilt modules are always preferred to prebuilt implicit modules.
+// RUN: rm -rf %t2
+// RUN: mkdir -p %t2
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
+// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
+
+// expected-no-diagnostics
+@import module_a;
+int test() {
+  return a;
+}
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
@@ -0,0 +1 @@
+module module_a { header "a.h" }
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
@@ -0,0 +1 @@
+const int a = 1;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1321,6 +1321,7 @@
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
   Record.push_back(HSOpts.ModuleMapFileHomeIsCwd);
+  Record.push_back(HSOpts.EnablePrebuiltImplicitModules);
   Record.push_back(HSOpts.UseBuiltinIncludes);
   Record.push_back(HSOpts.UseStandardSystemIncludes);
   Record.push_back(HSOpts.UseStandardCXXIncludes);
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5849,6 +5849,7 @@
   HSOpts.DisableModuleHash = Record[Idx++];
   HSOpts.ImplicitModuleMaps = Record[Idx++];
   HSOpts.ModuleMapFileHomeIsCwd = Record[Idx++];
+  HSOpts.EnablePrebuiltImplicitModules = Record[Idx++];
   HSOpts.UseBuiltinIncludes = Record[Idx++];
   HSOpts.UseStandardSystemIncludes = Record[Idx++];
   HSOpts.UseStandardCXXIncludes = Record[Idx++];
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -164,14 +164,41 @@
   return {};
 }
 
+std::string HeaderSearch::getPrebuiltImplicitModuleFileName(Module *Module) {
+  const FileEntry *ModuleMap =
+  getModuleMap().getModuleMapFileForUniquing

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 264096.
arames added a comment.

Rebase on top of tree.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+//
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_e
+//
+// Check that we correctly fall back to implicit modules if the prebuilt implicit module is not found.
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1 -fno-signed-char
+// RUN: find %t1 -name "module_a*.pcm" | grep module_a
+
+// Check that non-implicit prebuilt modules are always preferred to prebuilt implicit modules.
+// RUN: rm -rf %t2
+// RUN: mkdir -p %t2
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
+// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
+
+// expected-no-diagnostics
+@import module_a;
+int test() {
+  return a;
+}
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
@@ -0,0 +1 @@
+module module_a { header "a.h" }
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
@@ -0,0 +1 @@
+const int a = 1;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1308,6 +1308,7 @@
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
   Record.push_back(HSOpts.ModuleMapFileHomeIsCwd);
+  Record.push_back(HSOpts.EnablePrebuiltImplicitModules);
   Record.push_back(HSOpts.UseBuiltinIncludes);
   Record.push_back(HSOpts.UseStandardSystemIncludes);
   Record.push_back(HSOpts.UseStandardCXXIncludes);
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5833,6 +5833,7 @@
   HSOpts.DisableModuleHash = Record[Idx++];
   HSOpts.ImplicitModuleMaps = Record[Idx++];
   HSOpts.ModuleMapFileHomeIsCwd = Record[Idx++];
+  HSOpts.EnablePrebuiltImplicitModules = Record[Idx++];
   HSOpts.UseBuiltinIncludes = Record[Idx++];
   HSOpts.UseStandardSystemIncludes = Record[Idx++];
   HSOpts.UseStandardCXXIncludes = Record[Idx++];
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -164,14 +164,41 @@
   return {};
 }
 
+std::string HeaderSearch::getPrebuiltImplicitModuleFileName(Module *Module) {
+  const FileEntry *ModuleMap =
+  getModuleMap().getModuleMapFileForUniquing(Module);
+  StringRef ModuleName

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 264048.
arames edited the summary of this revision.
arames added a comment.

Rebase on top of tree.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72100/new/

https://reviews.llvm.org/D72100

Files:
  clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/test/Frontend/verify-any-file.c
  clang/test/Frontend/verify-any-file.h

Index: clang/test/Frontend/verify-any-file.h
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.h
@@ -0,0 +1 @@
+unexpected var;
Index: clang/test/Frontend/verify-any-file.c
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.c
@@ -0,0 +1,14 @@
+// RUN: not %clang_cc1 -verify %s 2>&1 | FileCheck %s
+
+#include "verify-any-file.h"
+// expected-error@*:* {{unknown type name 'unexpected'}}
+
+// expected-error@*:* {{missing error}}
+
+// expected-error@*:123 {{invalid line : "*" required}}
+//
+//  CHECK: error: 'error' diagnostics expected but not seen:
+// CHECK-NEXT:   File * Line * (directive at {{.*}}verify-any-file.c:6): missing error
+// CHECK-NEXT: error: 'error' diagnostics seen but not expected:
+// CHECK-NEXT:   File {{.*}}verify-any-file.c Line 8: missing or invalid line number following '@' in expected '*'
+// CHECK-NEXT: 2 errors generated.
Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -89,9 +89,10 @@
 class StandardDirective : public Directive {
 public:
   StandardDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-bool MatchAnyLine, StringRef Text, unsigned Min,
-unsigned Max)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max) {}
+bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+unsigned Min, unsigned Max)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+  MatchAnyLine, Text, Min, Max) {}
 
   bool isValid(std::string &Error) override {
 // all strings are considered valid; even empty ones
@@ -107,9 +108,10 @@
 class RegexDirective : public Directive {
 public:
   RegexDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
- bool MatchAnyLine, StringRef Text, unsigned Min, unsigned Max,
- StringRef RegexStr)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max),
+ bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+ unsigned Min, unsigned Max, StringRef RegexStr)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+  MatchAnyLine, Text, Min, Max),
 Regex(RegexStr) {}
 
   bool isValid(std::string &Error) override {
@@ -294,11 +296,13 @@
 // Attach the specified directive to the line of code indicated by
 // \p ExpectedLoc.
 void attachDirective(DiagnosticsEngine &Diags, const UnattachedDirective &UD,
- SourceLocation ExpectedLoc, bool MatchAnyLine = false) {
+ SourceLocation ExpectedLoc,
+ bool MatchAnyFileAndLine = false,
+ bool MatchAnyLine = false) {
   // Construct new directive.
-  std::unique_ptr D =
-  Directive::create(UD.RegexKind, UD.DirectivePos, ExpectedLoc,
-MatchAnyLine, UD.Text, UD.Min, UD.Max);
+  std::unique_ptr D = Directive::create(
+  UD.RegexKind, UD.DirectivePos, ExpectedLoc, MatchAnyFileAndLine,
+  MatchAnyLine, UD.Text, UD.Min, UD.Max);
 
   std::string Error;
   if (!D->isValid(Error)) {
@@ -498,6 +502,7 @@
 // Next optional token: @
 SourceLocation ExpectedLoc;
 StringRef Marker;
+bool MatchAnyFileAndLine = false;
 bool MatchAnyLine = false;
 if (!PH.Next("@")) {
   ExpectedLoc = Pos;
@@ -526,26 +531,39 @@
 StringRef Filename(PH.C, PH.P-PH.C);
 PH.Advance();
 
-// Lookup file via Preprocessor, like a #include.
-const DirectoryLookup *CurDir;
-Optional File =
-PP->LookupFile(Pos, Filename, false, nullptr, nullptr, CurDir,
-   nullptr, nullptr, nullptr, nullptr, nullptr);
-if (!File) {
-  Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin),
-   diag::err_verify_missing_file) << Filename << KindStr;
-  continue;
-}
-
-const FileEntry *FE = &File->getFileEntry();
-if (SM.translateFile(FE).isInvalid())
-  SM.createFileID(FE, Pos, SrcMgr::C_User);
-
-if (PH.Next(Line) && Line > 0)
-  ExpectedLoc = SM.translateFileLineCol(FE, Line, 1);
-else if (PH.Ne

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 250247.
arames added a comment.

Apply `clang-format`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72100/new/

https://reviews.llvm.org/D72100

Files:
  clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/test/Frontend/verify-any-file.c
  clang/test/Frontend/verify-any-file.h

Index: clang/test/Frontend/verify-any-file.h
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.h
@@ -0,0 +1 @@
+unexpected var;
Index: clang/test/Frontend/verify-any-file.c
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.c
@@ -0,0 +1,14 @@
+// RUN: not %clang_cc1 -verify %s 2>&1 | FileCheck %s
+
+#include "verify-any-file.h"
+// expected-error@*:* {{unknown type name 'unexpected'}}
+
+// expected-error@*:* {{missing error}}
+
+// expected-error@*:123 {{invalid line : "*" required}}
+//
+//  CHECK: error: 'error' diagnostics expected but not seen:
+// CHECK-NEXT:   File * Line * (directive at {{.*}}verify-any-file.c:6): missing error
+// CHECK-NEXT: error: 'error' diagnostics seen but not expected:
+// CHECK-NEXT:   File {{.*}}verify-any-file.c Line 8: missing or invalid line number following '@' in expected '*'
+// CHECK-NEXT: 2 errors generated.
Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -89,9 +89,10 @@
 class StandardDirective : public Directive {
 public:
   StandardDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-bool MatchAnyLine, StringRef Text, unsigned Min,
-unsigned Max)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max) {}
+bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+unsigned Min, unsigned Max)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+  MatchAnyLine, Text, Min, Max) {}
 
   bool isValid(std::string &Error) override {
 // all strings are considered valid; even empty ones
@@ -107,9 +108,10 @@
 class RegexDirective : public Directive {
 public:
   RegexDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
- bool MatchAnyLine, StringRef Text, unsigned Min, unsigned Max,
- StringRef RegexStr)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max),
+ bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+ unsigned Min, unsigned Max, StringRef RegexStr)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine,
+  MatchAnyLine, Text, Min, Max),
 Regex(RegexStr) {}
 
   bool isValid(std::string &Error) override {
@@ -294,11 +296,13 @@
 // Attach the specified directive to the line of code indicated by
 // \p ExpectedLoc.
 void attachDirective(DiagnosticsEngine &Diags, const UnattachedDirective &UD,
- SourceLocation ExpectedLoc, bool MatchAnyLine = false) {
+ SourceLocation ExpectedLoc,
+ bool MatchAnyFileAndLine = false,
+ bool MatchAnyLine = false) {
   // Construct new directive.
-  std::unique_ptr D =
-  Directive::create(UD.RegexKind, UD.DirectivePos, ExpectedLoc,
-MatchAnyLine, UD.Text, UD.Min, UD.Max);
+  std::unique_ptr D = Directive::create(
+  UD.RegexKind, UD.DirectivePos, ExpectedLoc, MatchAnyFileAndLine,
+  MatchAnyLine, UD.Text, UD.Min, UD.Max);
 
   std::string Error;
   if (!D->isValid(Error)) {
@@ -498,6 +502,7 @@
 // Next optional token: @
 SourceLocation ExpectedLoc;
 StringRef Marker;
+bool MatchAnyFileAndLine = false;
 bool MatchAnyLine = false;
 if (!PH.Next("@")) {
   ExpectedLoc = Pos;
@@ -526,26 +531,39 @@
 StringRef Filename(PH.C, PH.P-PH.C);
 PH.Advance();
 
-// Lookup file via Preprocessor, like a #include.
-const DirectoryLookup *CurDir;
-Optional File =
-PP->LookupFile(Pos, Filename, false, nullptr, nullptr, CurDir,
-   nullptr, nullptr, nullptr, nullptr, nullptr);
-if (!File) {
-  Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin),
-   diag::err_verify_missing_file) << Filename << KindStr;
-  continue;
-}
-
-const FileEntry *FE = &File->getFileEntry();
-if (SM.translateFile(FE).isInvalid())
-  SM.createFileID(FE, Pos, SrcMgr::C_User);
-
-if (PH.Next(Line) && Line > 0)
-  ExpectedLoc = SM.translateFileLineCol(FE, Line, 1);
-else if (PH.Next("*")) {
+if (Filename == "*") {
+ 

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 247087.
arames added a comment.

Rename and clarify.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72100/new/

https://reviews.llvm.org/D72100

Files:
  clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/test/Frontend/verify-any-file.c
  clang/test/Frontend/verify-any-file.h

Index: clang/test/Frontend/verify-any-file.h
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.h
@@ -0,0 +1 @@
+unexpected var;
Index: clang/test/Frontend/verify-any-file.c
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.c
@@ -0,0 +1,14 @@
+// RUN: not %clang_cc1 -verify %s 2>&1 | FileCheck %s
+
+#include "verify-any-file.h"
+// expected-error@*:* {{unknown type name 'unexpected'}}
+
+// expected-error@*:* {{missing error}}
+
+// expected-error@*:123 {{invalid line : "*" required}}
+//
+//  CHECK: error: 'error' diagnostics expected but not seen:
+// CHECK-NEXT:   File * Line * (directive at {{.*}}verify-any-file.c:6): missing error
+// CHECK-NEXT: error: 'error' diagnostics seen but not expected:
+// CHECK-NEXT:   File {{.*}}verify-any-file.c Line 8: missing or invalid line number following '@' in expected '*'
+// CHECK-NEXT: 2 errors generated.
Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -89,9 +89,10 @@
 class StandardDirective : public Directive {
 public:
   StandardDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-bool MatchAnyLine, StringRef Text, unsigned Min,
-unsigned Max)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max) {}
+bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+unsigned Min, unsigned Max)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine, MatchAnyLine, Text,
+  Min, Max) {}
 
   bool isValid(std::string &Error) override {
 // all strings are considered valid; even empty ones
@@ -107,9 +108,10 @@
 class RegexDirective : public Directive {
 public:
   RegexDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
- bool MatchAnyLine, StringRef Text, unsigned Min, unsigned Max,
- StringRef RegexStr)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max),
+ bool MatchAnyFileAndLine, bool MatchAnyLine, StringRef Text,
+ unsigned Min, unsigned Max, StringRef RegexStr)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFileAndLine, MatchAnyLine, Text,
+  Min, Max),
 Regex(RegexStr) {}
 
   bool isValid(std::string &Error) override {
@@ -294,11 +296,12 @@
 // Attach the specified directive to the line of code indicated by
 // \p ExpectedLoc.
 void attachDirective(DiagnosticsEngine &Diags, const UnattachedDirective &UD,
- SourceLocation ExpectedLoc, bool MatchAnyLine = false) {
+ SourceLocation ExpectedLoc, bool MatchAnyFileAndLine = false,
+ bool MatchAnyLine = false) {
   // Construct new directive.
   std::unique_ptr D =
   Directive::create(UD.RegexKind, UD.DirectivePos, ExpectedLoc,
-MatchAnyLine, UD.Text, UD.Min, UD.Max);
+MatchAnyFileAndLine, MatchAnyLine, UD.Text, UD.Min, UD.Max);
 
   std::string Error;
   if (!D->isValid(Error)) {
@@ -498,6 +501,7 @@
 // Next optional token: @
 SourceLocation ExpectedLoc;
 StringRef Marker;
+bool MatchAnyFileAndLine = false;
 bool MatchAnyLine = false;
 if (!PH.Next("@")) {
   ExpectedLoc = Pos;
@@ -526,26 +530,39 @@
 StringRef Filename(PH.C, PH.P-PH.C);
 PH.Advance();
 
-// Lookup file via Preprocessor, like a #include.
-const DirectoryLookup *CurDir;
-Optional File =
-PP->LookupFile(Pos, Filename, false, nullptr, nullptr, CurDir,
-   nullptr, nullptr, nullptr, nullptr, nullptr);
-if (!File) {
-  Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin),
-   diag::err_verify_missing_file) << Filename << KindStr;
-  continue;
-}
-
-const FileEntry *FE = &File->getFileEntry();
-if (SM.translateFile(FE).isInvalid())
-  SM.createFileID(FE, Pos, SrcMgr::C_User);
-
-if (PH.Next(Line) && Line > 0)
-  ExpectedLoc = SM.translateFileLineCol(FE, Line, 1);
-else if (PH.Next("*")) {
+if (Filename == "*") {
+  MatchAnyFileAndLine = true;
+  if (!PH.Next("*")) {
+Diags.Report(Pos.

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Alexandre Rames via Phabricator via cfe-commits]

arames marked 2 inline comments as done.
arames added a comment.

In D72100#1855483 , @jkorous wrote:

> We should either simplify the implementation to reflect that we don't support 
> e. g. `*:42` (seems preferable to me) or have the codepaths that are 
> currently not accessible through `-fverify` tested by other means.


That makes sense. I have updated the naming and added a comment to reflect 
that. PTAL.
As a note and future reference, the issue was not on the side of the 
verification; I tested combinations of `MatchAnyFile` and `MatchAnyLine`, but 
the fact that we cannot always create a source location with the appropriate 
line number when we do not know what file we are dealing with.




Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:300
+ SourceLocation ExpectedLoc, bool MatchAnyFile = false,
+ bool MatchAnyLine = false) {
   // Construct new directive.

jkorous wrote:
> Should we make it clear from the interface that `MatchAnyFile` => 
> `MatchAnyLine`?
The naming should now make it clear. Additionally, I have added a comment for 
the implication in `Directive`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72100/new/

https://reviews.llvm.org/D72100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 239972.
arames added a comment.

Address review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72100/new/

https://reviews.llvm.org/D72100

Files:
  clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/test/Frontend/verify-any-file.c
  clang/test/Frontend/verify-any-file.h

Index: clang/test/Frontend/verify-any-file.h
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.h
@@ -0,0 +1 @@
+unexpected var;
Index: clang/test/Frontend/verify-any-file.c
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.c
@@ -0,0 +1,14 @@
+// RUN: not %clang_cc1 -verify %s 2>&1 | FileCheck %s
+
+#include "verify-any-file.h"
+// expected-error@*:* {{unknown type name 'unexpected'}}
+
+// expected-error@*:* {{missing error}}
+
+// expected-error@*:123 {{invalid line : "*" required}}
+//
+//  CHECK: error: 'error' diagnostics expected but not seen:
+// CHECK-NEXT:   File * Line * (directive at {{.*}}verify-any-file.c:6): missing error
+// CHECK-NEXT: error: 'error' diagnostics seen but not expected:
+// CHECK-NEXT:   File {{.*}}verify-any-file.c Line 8: missing or invalid line number following '@' in expected '*'
+// CHECK-NEXT: 2 errors generated.
Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -89,9 +89,10 @@
 class StandardDirective : public Directive {
 public:
   StandardDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-bool MatchAnyLine, StringRef Text, unsigned Min,
-unsigned Max)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max) {}
+bool MatchAnyFile, bool MatchAnyLine, StringRef Text,
+unsigned Min, unsigned Max)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFile, MatchAnyLine, Text,
+  Min, Max) {}
 
   bool isValid(std::string &Error) override {
 // all strings are considered valid; even empty ones
@@ -107,9 +108,10 @@
 class RegexDirective : public Directive {
 public:
   RegexDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
- bool MatchAnyLine, StringRef Text, unsigned Min, unsigned Max,
- StringRef RegexStr)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max),
+ bool MatchAnyFile, bool MatchAnyLine, StringRef Text,
+ unsigned Min, unsigned Max, StringRef RegexStr)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFile, MatchAnyLine, Text,
+  Min, Max),
 Regex(RegexStr) {}
 
   bool isValid(std::string &Error) override {
@@ -294,11 +296,12 @@
 // Attach the specified directive to the line of code indicated by
 // \p ExpectedLoc.
 void attachDirective(DiagnosticsEngine &Diags, const UnattachedDirective &UD,
- SourceLocation ExpectedLoc, bool MatchAnyLine = false) {
+ SourceLocation ExpectedLoc, bool MatchAnyFile = false,
+ bool MatchAnyLine = false) {
   // Construct new directive.
   std::unique_ptr D =
   Directive::create(UD.RegexKind, UD.DirectivePos, ExpectedLoc,
-MatchAnyLine, UD.Text, UD.Min, UD.Max);
+MatchAnyFile, MatchAnyLine, UD.Text, UD.Min, UD.Max);
 
   std::string Error;
   if (!D->isValid(Error)) {
@@ -498,6 +501,7 @@
 // Next optional token: @
 SourceLocation ExpectedLoc;
 StringRef Marker;
+bool MatchAnyFile = false;
 bool MatchAnyLine = false;
 if (!PH.Next("@")) {
   ExpectedLoc = Pos;
@@ -526,26 +530,39 @@
 StringRef Filename(PH.C, PH.P-PH.C);
 PH.Advance();
 
-// Lookup file via Preprocessor, like a #include.
-const DirectoryLookup *CurDir;
-Optional File =
-PP->LookupFile(Pos, Filename, false, nullptr, nullptr, CurDir,
-   nullptr, nullptr, nullptr, nullptr, nullptr);
-if (!File) {
-  Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin),
-   diag::err_verify_missing_file) << Filename << KindStr;
-  continue;
-}
-
-const FileEntry *FE = &File->getFileEntry();
-if (SM.translateFile(FE).isInvalid())
-  SM.createFileID(FE, Pos, SrcMgr::C_User);
-
-if (PH.Next(Line) && Line > 0)
-  ExpectedLoc = SM.translateFileLineCol(FE, Line, 1);
-else if (PH.Next("*")) {
+if (Filename == "*") {
+  MatchAnyFile = true;
+  if (!PH.Next("*")) {
+Diags.Report(Pos.getLocWithOffset(PH.C - PH.Begin),
+   

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Alexandre Rames via Phabricator via cfe-commits]

arames marked 3 inline comments as done.
arames added inline comments.



Comment at: clang/test/Frontend/verify-any-file.c:4
+#include "verify-any-file.h"
+// expected-error@*:1 {{unknown type name 'unexpected'}}
+#include "verify-any-file.h"

While testing, I realized this cannot be handled reliably, so will require the 
line number to be `*` when the filename is `*`.

I was using the current file and specified line for the `SourceLocation` in 
this case. But this fails if the specified line number is greater than the 
number of lines in the current file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72100/new/

https://reviews.llvm.org/D72100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

This is for example useful to add a catch-all clause like `// 
expected-note-re@*:* 1+ {{candidate function {{.+`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72100/new/

https://reviews.llvm.org/D72100



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72100: Allow matching "any file" in `VerifyDiagnosticConsumer`.

[Alexandre Rames via Phabricator via cfe-commits]

arames created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
arames added a reviewer: rsmith.

This allows specifying `*` for the filename to match any file. For example:

  // expected-note@*:* {{Match this note in any file at any line.}}
  // expected-note@*:123 {{Match this note in any file at line 123.}}


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72100

Files:
  clang/include/clang/Frontend/VerifyDiagnosticConsumer.h
  clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
  clang/test/Frontend/verify-any-file.c
  clang/test/Frontend/verify-any-file.h

Index: clang/test/Frontend/verify-any-file.h
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.h
@@ -0,0 +1 @@
+unexpected var;
Index: clang/test/Frontend/verify-any-file.c
===
--- /dev/null
+++ clang/test/Frontend/verify-any-file.c
@@ -0,0 +1,6 @@
+// RUN: %clang_cc1 -verify %s 2>&1
+
+#include "verify-any-file.h"
+// expected-error@*:1 {{unknown type name 'unexpected'}}
+#include "verify-any-file.h"
+// expected-error@*:* {{unknown type name 'unexpected'}}
Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -89,9 +89,10 @@
 class StandardDirective : public Directive {
 public:
   StandardDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
-bool MatchAnyLine, StringRef Text, unsigned Min,
-unsigned Max)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max) {}
+bool MatchAnyFile, bool MatchAnyLine, StringRef Text,
+unsigned Min, unsigned Max)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFile, MatchAnyLine, Text,
+  Min, Max) {}
 
   bool isValid(std::string &Error) override {
 // all strings are considered valid; even empty ones
@@ -107,9 +108,10 @@
 class RegexDirective : public Directive {
 public:
   RegexDirective(SourceLocation DirectiveLoc, SourceLocation DiagnosticLoc,
- bool MatchAnyLine, StringRef Text, unsigned Min, unsigned Max,
- StringRef RegexStr)
-  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyLine, Text, Min, Max),
+ bool MatchAnyFile, bool MatchAnyLine, StringRef Text,
+ unsigned Min, unsigned Max, StringRef RegexStr)
+  : Directive(DirectiveLoc, DiagnosticLoc, MatchAnyFile, MatchAnyLine, Text,
+  Min, Max),
 Regex(RegexStr) {}
 
   bool isValid(std::string &Error) override {
@@ -294,11 +296,12 @@
 // Attach the specified directive to the line of code indicated by
 // \p ExpectedLoc.
 void attachDirective(DiagnosticsEngine &Diags, const UnattachedDirective &UD,
- SourceLocation ExpectedLoc, bool MatchAnyLine = false) {
+ SourceLocation ExpectedLoc, bool MatchAnyFile = false,
+ bool MatchAnyLine = false) {
   // Construct new directive.
   std::unique_ptr D =
   Directive::create(UD.RegexKind, UD.DirectivePos, ExpectedLoc,
-MatchAnyLine, UD.Text, UD.Min, UD.Max);
+MatchAnyFile, MatchAnyLine, UD.Text, UD.Min, UD.Max);
 
   std::string Error;
   if (!D->isValid(Error)) {
@@ -498,6 +501,7 @@
 // Next optional token: @
 SourceLocation ExpectedLoc;
 StringRef Marker;
+bool MatchAnyFile = false;
 bool MatchAnyLine = false;
 if (!PH.Next("@")) {
   ExpectedLoc = Pos;
@@ -526,26 +530,36 @@
 StringRef Filename(PH.C, PH.P-PH.C);
 PH.Advance();
 
-// Lookup file via Preprocessor, like a #include.
-const DirectoryLookup *CurDir;
-Optional File =
-PP->LookupFile(Pos, Filename, false, nullptr, nullptr, CurDir,
-   nullptr, nullptr, nullptr, nullptr, nullptr);
-if (!File) {
-  Diags.Report(Pos.getLocWithOffset(PH.C-PH.Begin),
-   diag::err_verify_missing_file) << Filename << KindStr;
-  continue;
-}
+FileID FID;
+
+if (Filename == "*") {
+  MatchAnyFile = true;
+  FID = SM.getFileID(Pos);
+} else {
+  // Lookup file via Preprocessor, like a #include.
+  const DirectoryLookup *CurDir;
+  Optional File =
+  PP->LookupFile(Pos, Filename, false, nullptr, nullptr, CurDir,
+ nullptr, nullptr, nullptr, nullptr, nullptr);
+  if (!File) {
+Diags.Report(Pos.getLocWithOffset(PH.C - PH.Begin),
+ diag::err_verify_missing_file)
+<< Filename << KindStr;
+continue;
+  }
+
+  const FileEntry *FE = &File->get

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

Ping.
Still looking for someone to take a look. Happy to answer any questions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames added a comment.

Ping.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames updated this revision to Diff 228267.
arames added a comment.

Rebase.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68997/new/

https://reviews.llvm.org/D68997

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+//
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_e
+//
+// Check that we correctly fall back to implicit modules if the prebuilt implicit module is not found.
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1 -fno-signed-char
+// RUN: find %t1 -name "module_a*.pcm" | grep module_a
+
+// Check that non-implicit prebuilt modules are always preferred to prebuilt implicit modules.
+// RUN: rm -rf %t2
+// RUN: mkdir -p %t2
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -o %t/module_a.pcm -fno-signed-char
+// RUN: not %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t2
+// RUN: find %t2 -name "module_a*.pcm" | not grep module_a
+
+// expected-no-diagnostics
+@import module_a;
+int test() {
+  return a;
+}
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
@@ -0,0 +1 @@
+module module_a { header "a.h" }
Index: clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
===
--- /dev/null
+++ clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
@@ -0,0 +1 @@
+const int a = 1;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -1677,6 +1677,7 @@
   Record.push_back(HSOpts.DisableModuleHash);
   Record.push_back(HSOpts.ImplicitModuleMaps);
   Record.push_back(HSOpts.ModuleMapFileHomeIsCwd);
+  Record.push_back(HSOpts.EnablePrebuiltImplicitModules);
   Record.push_back(HSOpts.UseBuiltinIncludes);
   Record.push_back(HSOpts.UseStandardSystemIncludes);
   Record.push_back(HSOpts.UseStandardCXXIncludes);
Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -5789,6 +5789,7 @@
   HSOpts.DisableModuleHash = Record[Idx++];
   HSOpts.ImplicitModuleMaps = Record[Idx++];
   HSOpts.ModuleMapFileHomeIsCwd = Record[Idx++];
+  HSOpts.EnablePrebuiltImplicitModules = Record[Idx++];
   HSOpts.UseBuiltinIncludes = Record[Idx++];
   HSOpts.UseStandardSystemIncludes = Record[Idx++];
   HSOpts.UseStandardCXXIncludes = Record[Idx++];
Index: clang/lib/Lex/HeaderSearch.cpp
===
--- clang/lib/Lex/HeaderSearch.cpp
+++ clang/lib/Lex/HeaderSearch.cpp
@@ -164,14 +164,41 @@
   return {};
 }
 
+std::string HeaderSearch::getPrebuiltImplicitModuleFileName(Module *Module) {
+  const FileEntry *ModuleMap =
+  getModuleMap().getModuleMapFileForUniquing(Module);
+  StringRef ModuleName = Module->Name

[PATCH] D68997: Allow searching for prebuilt implicit modules.

[Alexandre Rames via Phabricator via cfe-commits]

arames created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
arames edited the summary of this revision.
arames added reviewers: bruno, rsmith.
Herald added a subscriber: dexonsmith.

The behavior is controlled by the `-fprebuilt-implicit-modules` option, and
allows searching for implicit modules in the prebuilt module cache paths.

The current command-line options for prebuilt modules do not allow to easily
maintain and use multiple versions of modules. Both the producer and users of
prebuilt modules are required to know the relationships between compilation
options and module file paths. Using a particular version of a prebuilt module
requires passing a particular option on the command line (e.g.
`-fmodule-file=[=]` or `-fprebuilt-module-path=`).

However the compiler already knows how to distinguish and automatically locate
implicit modules. Hence this proposal to introduce the
`-fprebuilt-implicit-modules` option. When set, it enables searching for
implicit modules in the prebuilt module paths (specified via
`-fprebuilt-module-path`). To not modify existing behavior, this search takes
place after the standard search for prebuilt modules. If not

Here is a workflow illustrating how both the producer and consumer of prebuilt
modules would need to know what versions of prebuilt modules are available and
where they are located.

  clang -cc1 -x c modulemap -fmodules -emit-module -fmodule-name=foo 
-fmodules-cache-path=prebuilt_modules_v1 
  clang -cc1 -x c modulemap -fmodules -emit-module -fmodule-name=foo 
-fmodules-cache-path=prebuilt_modules_v2 
  clang -cc1 -x c modulemap -fmodules -emit-module -fmodule-name=foo 
-fmodules-cache-path=prebuilt_modules_v3 
  
  clang -cc1 -x c use.c -fmodules fmodule-map-file=modulemap 
-fprebuilt-module-path=prebuilt_modules_v1 
  clang -cc1 -x c use.c -fmodules fmodule-map-file=modulemap 

With prebuilt implicit modules, the producer can generate prebuilt modules as
usual, all in the same output directory. The same mechanisms as for implicit
modules take care of incorporating hashes in the path to distinguish between
module versions.

Note that we do not specify the output module filename, so `-o` implicit 
modules are generated in the cache path `prebuilt_modules`.

  clang -cc1 -x c modulemap -fmodules -emit-module -fmodule-name=foo 
-fmodules-cache-path=prebuilt_modules 
  clang -cc1 -x c modulemap -fmodules -emit-module -fmodule-name=foo 
-fmodules-cache-path=prebuilt_modules 
  clang -cc1 -x c modulemap -fmodules -emit-module -fmodule-name=foo 
-fmodules-cache-path=prebuilt_modules 

The user can now simply enable prebuilt implicit modules and point to the
prebuilt modules cache. No need to "parse" command-line options to decide
what prebuilt modules (paths) to use.

  clang -cc1 -x c use.c -fmodules fmodule-map-file=modulemap 
-fprebuilt-module-path=prebuilt_modules -fprebuilt-implicit-modules 
  clang -cc1 -x c use.c -fmodules fmodule-map-file=modulemap 
-fprebuilt-module-path=prebuilt_modules -fprebuilt-implicit-modules 


This is for example particularly useful in a use-case where compilation is
expensive, and the configurations expected to be used are predictable, but not
controlled by the producer of prebuilt modules. Modules for the set of
predictable configurations can be prebuilt, and using them does not require
"parsing" the configuration (command-line options).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68997

Files:
  clang/docs/Modules.rst
  clang/include/clang/Driver/Options.td
  clang/include/clang/Lex/HeaderSearch.h
  clang/include/clang/Lex/HeaderSearchOptions.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
  clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
  clang/test/Modules/prebuilt-implicit-modules.m

Index: clang/test/Modules/prebuilt-implicit-modules.m
===
--- /dev/null
+++ clang/test/Modules/prebuilt-implicit-modules.m
@@ -0,0 +1,27 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -x objective-c -fmodules %S/Inputs/prebuilt-implicit-module/module.modulemap -emit-module -fmodule-name=module_a -fmodules-cache-path=%t
+// RUN: find %t -name "module_a*.pcm" | grep module_a
+//
+// Check we use a prebuilt module when available, and do not build an implicit module.
+// RUN: rm -rf %t1
+// RUN: mkdir -p %t1
+// RUN: %clang_cc1 -x objective-c %s -I%S/Inputs/prebuilt-implicit-module -fmodules -fmodule-map-file=%S/Inputs/prebuilt-implicit-module/module.modulemap -fprebuilt-implicit-modules -fprebuilt-module-path=%t -fmodules-cache-path=%t1
+// RUN: find %t1 -name "module_a*.pcm" | not grep module_e
+//
+// Check that we correctly fall back to implicit modules if the