[PATCH] Allow multiple modules with the same name to coexist in the module cache

[Richard Smith]

On Fri, Mar 28, 2014 at 1:12 PM, Ben Langmuir <blangmuir at apple.com> wrote:

> This patch allows multiple modules that have the same name to coexist in
> the module cache.  To differentiate between two modules with the same name,
> we will consider the path the module map file that they are defined by*
> part of the 'key' for looking up the precompiled module (pcm file).
>  Specifically, this patch renames the precompiled module (pcm) files from
>
> cache-path/<module hash>/Foo.pcm
>
> to
>
> cache-path/<module hash>/Foo-<hash of module map path>.pcm
>

>From a high level, I don't really see why we need a second hash here.
Shouldn't the -I options be included in the <module hash>? If I build the
same module with different -I flags, that should resolve to different .pcm
files, regardless of whether it makes the module name resolve to a
different module.map file.

Are you trying to cope with the case where the -I path finds multiple
module.map files defining the same module (where it's basically chance
which one will get built and used)? I don't really feel like this is the
right solution to that problem either -- we should remove the 'luck' aspect
and use some sane mechanism to determine which module.map files are loaded,
and in what order.

Is this addressing some other case?



> In addition, I've taught the ASTReader to re-resolve the names of imported
> modules during module loading so that if the header search context changes
> between when a module was originally built and when it is loaded we can
> rebuild it if necessary.  For example, if module A imports module B
>
> first time:
> clang -I /path/to/A -I /path/to/B ...
>
> second time:
> clang -I /path/to/A -I /different/path/to/B ...
>
> will now rebuild A as expected.
>
>
> * in the case of inferred modules, we use the module map file that
> *allowed* the inference, not the __inferred_module.map file, since the
> inferred file path is the same for every inferred module.



Review comments on the patch itself:

 +  /// the inferrence (e.g. contained 'module *') rather than the virtual

Typo 'inference', 'Module *'.

+  /// For an explanaition of \p ModuleMap, see Module::ModuleMap.

Typo 'explanation'.

+  // safe becuase the FileManager is shared between the compiler instances.

Typo 'because'

+  // the inferred module. If this->ModuleMap is nullptr, then we are using
+  // -emit-module directly, and we cannot have an inferred module.

I don't understand what this comment is trying to say. If we're using
-emit-module, then we were given a module map on the command-line; should
that not be referred to by this->ModuleMap? (Also, why 'this->'?) How can a
top-level module be inferred? Is that a framework-specific thing?

+    StringRef ModuleMap = this->ModuleMap ? this->ModuleMap->getName() :
InFile;

Please pick a different variable name rather than shadowing a member of
'*this' here.

+    // Construct the name <ModuleName>-<hash of ModuleMapPath>.pcm which
should
+    // be globally unique to this particular module.
+    llvm::APInt Code(64, llvm::hash_value(ModuleMapPath));
+    SmallString<128> HashStr;
+    Code.toStringUnsigned(HashStr);

Use base 36, like the module hash.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140328/0c410e82/attachment.html>