<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Mar 28, 2014 at 3:27 PM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Hi Richard,<div><br></div><div><br><div><div class=""><div>On Mar 28, 2014, at 2:45 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div>
<br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Mar 28, 2014 at 1:12 PM, Ben Langmuir<span> </span><span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span><span> </span>wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">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<br>
<br>cache-path/<module hash>/Foo.pcm<br><br>to<br><br>cache-path/<module hash>/Foo-<hash of module map path>.pcm<br></blockquote><div><br></div><div>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.</div>
</div></div></div></div></blockquote><div><br></div></div>If we include the -I options in the module hash, we will explode the number of module compilations needed.  The following should all be able to share a module ‘A’.</div>
</div></div></blockquote><div><br></div><div>I really don't think they should, if that second -I path is used in any way when building module 'A'.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div>clang -fmodules -I /path/to/A -I /path/to/B some_file.c</div><div>clang -fmodules -I /path/to/A -I /path/to/C some_file2.c</div><div>clang -fmodules -I /path/to/A -I /path/to/D some_file3.c</div>
</div></div></blockquote><div><br></div><div>I would think a better solution here would be to not have that second -I path in header search when building module A (and thus not include it in the hash for a principled reason).</div>
<div><br></div><div>Ultimately, each module should have its own header search path -- the model of one set of include paths for the entire TU (with that include path potentially causing some libraries to find the wrong files) is a broken, antiquated legacy of the non-modules world. In the short term, we should probably drop all header search paths that are before the path in which the module map was found.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div class=""><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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.</div>
</div></div></div></div></blockquote><div><br></div></div>Nope - that is not being addressed.</div><div><div class=""><br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Is this addressing some other case?</div><div><br></div></div></div></div></div></blockquote></div><div>See above.</div><div class=""><br>
<blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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<br>
<br>first time:<br>clang -I /path/to/A -I /path/to/B …<br><br>second time:<br>clang -I /path/to/A -I /different/path/to/B …<br><br>will now rebuild A as expected.<br><br><br>* 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.</blockquote>
<div><br></div><div><br></div><div>Review comments on the patch itself:</div><div><br></div><div> +  /// the inferrence (e.g. contained 'module *') rather than the virtual</div><div><br></div><div>Typo 'inference', 'Module *'.</div>
<div><br><div class="gmail_extra">+  /// For an explanaition of \p ModuleMap, see Module::ModuleMap.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Typo 'explanation'.</div><div class="gmail_extra">
<br></div><div class="gmail_extra">+  // safe becuase the FileManager is shared between the compiler instances.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Typo ‘because'<br></div></div></div></div>
</div></div></blockquote><div><br></div></div>Thanks for catching the embarrassing quantity of typos :)</div><div><div class=""><br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="gmail_extra"><br><div class="gmail_extra">+  // the inferred module. If this->ModuleMap is nullptr, then we are using</div><div class="gmail_extra">
+  // -emit-module directly, and we cannot have an inferred module.</div><div class="gmail_extra"><br></div><div class="gmail_extra">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?</div>
</div></div></div></div></div></div></blockquote><div><br></div></div><div>Hmm, I don’t recall why I didn’t just pass in the InputFile as the module map.  I’ll do that.</div><div><br></div><div>Yes, AFAIK only framework modules can be inferred at the top-level.</div>
<div class=""><br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="gmail_extra"><div class="gmail_extra"><br></div><div class="gmail_extra">+    StringRef ModuleMap = this->ModuleMap ? this->ModuleMap->getName() : InFile;<br>
</div><div class="gmail_extra"><br></div><div class="gmail_extra">Please pick a different variable name rather than shadowing a member of '*this' here.</div></div></div></div></div></div></div></blockquote><div><br>
</div></div>Will do.</div><div class=""><div><br><blockquote type="cite"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="gmail_extra"><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra">+    // Construct the name <ModuleName>-<hash of ModuleMapPath>.pcm which should</div>
<div class="gmail_extra">+    // be globally unique to this particular module.</div><div class="gmail_extra">+    llvm::APInt Code(64, llvm::hash_value(ModuleMapPath));</div><div class="gmail_extra">+    SmallString<128> HashStr;</div>
<div class="gmail_extra">+    Code.toStringUnsigned(HashStr);</div><div><br></div><div>Use base 36, like the module hash.</div></div></div></div></div></div></div></div></blockquote><br></div></div><div>I could have sworn I did… must have got lost along the way.  Will do.</div>
<br></div></div></blockquote></div><br></div></div>