<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><blockquote type="cite"><div>On Jun 18, 2014, at 9:11 AM, Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br class="Apple-interchange-newline">On Jun 18, 2014, at 1:08 AM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> wrote:<br><br>Thanks for the feedback!<br><br>Argyrios Kyrtzidis <<a href="mailto:kyrtzidis@apple.com">kyrtzidis@apple.com</a>> writes:<br><blockquote type="cite">Thank you for your work on this.<br><br>Some high level comments first; the way ModuleDependencyCollector is<br>getting hooked up, results in:<br><br>- If no module needs building (because module cache has it) no module<br>dependency is getting written out<br></blockquote><br>I realize this, but as far as I can tell there isn't really a practical<br>way to get all of the module dependencies except for when we actually<br>create a module. Unless there's a way to extract this information from<br>an imported ModuleFile that I've missed, I think we need to collect this<br>information as we parse the AST to build a module in the first place.<br><br>If there is a more reliable way to do this, I'm all ears. I should<br>mention though, for the purposes of getting more useful crash reports<br>with modules this limitation isn't very important. Notably, it's useful<br>to collect the .pcm files anyway, and using a fresh cache directory is a<br>very simple way to do that.<br><br><blockquote type="cite">- It doesn’t hook in the top level module, e.g. if I have "@import<br>Darwin;” then no module dependency is written out even if the module<br>cache is empty.<br></blockquote><br>This seems like a bigger problem, and I don't really understand it.<br>Where is the module built? I'm pretty convinced that the place I've<br>hooked into is the only one that creates a CompilerInstance with<br>BuildingModule set to true…<br></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Ben, could you help out on hooking up ModuleDependencyCollector in a way that will address the above ?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"></div></blockquote><div><br></div><div>Sure! The problem is that the first time we come to attempt loading a top-level module (before it is compiled), we will be here and not attach anything since ModuleDepCollector hasn’t been filled in yet.</div><div><blockquote type="cite"><div>+ if (ModuleDepCollector)</div><div>+ ModuleDepCollector->attachToASTReader(*ModuleManager);</div></blockquote><br></div><div>If you call ModuleDepCollector = getModuleDepCollector() before that if, it works for me.</div><div><br></div><div>Ben</div><br><blockquote type="cite"><div><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite">Some nitpicks:<br><br>@@ -105,6 +105,9 @@ class CompilerInstance : public ModuleLoader {<br> /// \brief The ASTReader, if one exists.<br> IntrusiveRefCntPtr<ASTReader> ModuleManager;<br><br>+ /// \brief The module dependency collector for crashdumps<br>+ std::shared_ptr<ModuleDependencyCollector> ModuleDepCollector;<br>+<br> /// \brief The dependency file generator.<br> std::unique_ptr<DependencyFileGenerator> TheDependencyFileGenerator;<br><br>Is there a policy to avoid IntrusiveRefCntPtr ?<br></blockquote><br>Since we're using C++11 now, I just assumed we'd be migrating to<br>shared_ptr where appropriate in modern code. I can change it to<br>IntrusiveRefCntPtr if you like, but I do think shared_ptr is appropriate<br>here.<br></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">That’s fine; In general IntrusiveRefCntPtr still has the advantage that it is pointer sized.</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite">+/// Collects the dependencies for imported modules into a directory. Users<br>+/// should attach to the AST reader whenever a module is loaded.<br>+class ModuleDependencyCollector {<br>+ StringRef DestDir;<br><br>Use std::string here.<br></blockquote><br>Sure.<br><br><blockquote type="cite">+ void reportError() { HasErrors = true; }<br><br>Name is a bit misleading, how about "setHasErrors()” ?<br></blockquote><br>Good idea.<br><br><blockquote type="cite">+llvm::error_code ModuleDependencyListener::copyToRoot(StringRef Src) {<br>+ using namespace llvm::sys;<br>+ SmallString<256> Dest = Collector.getDest();<br>+ path::append(Dest, path::relative_path(Src));<br><br>Do we need to turn Src into an absolute path first, in case it’s<br>relative to the current directory ?<br></blockquote><br>Yes, I think so.<br><br><blockquote type="cite">+ if (llvm::error_code EC = fs::copy_file(Src, Dest.str()))<br>+ return EC;<br><br>Do you have a separate patch that introduces copy_file ?<br></blockquote><br>Oops, forgot to attach that!<br><br>Updated patches attached.<br><br><clang-0001-Frontend-Add-a-CC1-flag-to-dump-module-dependencies.2.patch><clang-0002-Frontend-Include-a-VFS-map-when-dumping-module-depen.2.patch><llvm-0001-Support-Add-llvm-sys-fs-copy_file.patch>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></blockquote></div></blockquote></div><br></body></html>