[PATCH] Teach CC1 to dump module dependencies to a directory

[Ben Langmuir]

> On Jun 18, 2014, at 9:11 AM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
> 
>> 
>> On Jun 18, 2014, at 1:08 AM, Justin Bogner <mail at justinbogner.com> wrote:
>> 
>> Thanks for the feedback!
>> 
>> Argyrios Kyrtzidis <kyrtzidis at apple.com> writes:
>>> Thank you for your work on this.
>>> 
>>> Some high level comments first; the way ModuleDependencyCollector is
>>> getting hooked up, results in:
>>> 
>>> - If no module needs building (because module cache has it) no module
>>> dependency is getting written out
>> 
>> I realize this, but as far as I can tell there isn't really a practical
>> way to get all of the module dependencies except for when we actually
>> create a module. Unless there's a way to extract this information from
>> an imported ModuleFile that I've missed, I think we need to collect this
>> information as we parse the AST to build a module in the first place.
>> 
>> If there is a more reliable way to do this, I'm all ears. I should
>> mention though, for the purposes of getting more useful crash reports
>> with modules this limitation isn't very important. Notably, it's useful
>> to collect the .pcm files anyway, and using a fresh cache directory is a
>> very simple way to do that.
>> 
>>> - It doesn’t hook in the top level module, e.g. if I have "@import
>>> Darwin;” then no module dependency is written out even if the module
>>> cache is empty.
>> 
>> This seems like a bigger problem, and I don't really understand it.
>> Where is the module built? I'm pretty convinced that the place I've
>> hooked into is the only one that creates a CompilerInstance with
>> BuildingModule set to true…
> 
> Ben, could you help out on hooking up ModuleDependencyCollector in a way that will address the above ?

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.
> +    if (ModuleDepCollector)
> +      ModuleDepCollector->attachToASTReader(*ModuleManager);

If you call ModuleDepCollector = getModuleDepCollector() before that if, it works for me.

Ben

> 
>> 
>>> Some nitpicks:
>>> 
>>> @@ -105,6 +105,9 @@ class CompilerInstance : public ModuleLoader {
>>>  /// \brief The ASTReader, if one exists.
>>>  IntrusiveRefCntPtr<ASTReader> ModuleManager;
>>> 
>>> +  /// \brief The module dependency collector for crashdumps
>>> +  std::shared_ptr<ModuleDependencyCollector> ModuleDepCollector;
>>> +
>>>  /// \brief The dependency file generator.
>>>  std::unique_ptr<DependencyFileGenerator> TheDependencyFileGenerator;
>>> 
>>> Is there a policy to avoid IntrusiveRefCntPtr ?
>> 
>> Since we're using C++11 now, I just assumed we'd be migrating to
>> shared_ptr where appropriate in modern code. I can change it to
>> IntrusiveRefCntPtr if you like, but I do think shared_ptr is appropriate
>> here.
> 
> That’s fine; In general IntrusiveRefCntPtr still has the advantage that it is pointer sized.
> 
>> 
>>> +/// Collects the dependencies for imported modules into a directory.  Users
>>> +/// should attach to the AST reader whenever a module is loaded.
>>> +class ModuleDependencyCollector {
>>> +  StringRef DestDir;
>>> 
>>> Use std::string here.
>> 
>> Sure.
>> 
>>> +  void reportError() { HasErrors = true; }
>>> 
>>> Name is a bit misleading, how about "setHasErrors()” ?
>> 
>> Good idea.
>> 
>>> +llvm::error_code ModuleDependencyListener::copyToRoot(StringRef Src) {
>>> +  using namespace llvm::sys;
>>> +  SmallString<256> Dest = Collector.getDest();
>>> +  path::append(Dest, path::relative_path(Src));
>>> 
>>> Do we need to turn Src into an absolute path first, in case it’s
>>> relative to the current directory ?
>> 
>> Yes, I think so.
>> 
>>> +  if (llvm::error_code EC = fs::copy_file(Src, Dest.str()))
>>> +    return EC;
>>> 
>>> Do you have a separate patch that introduces copy_file ?
>> 
>> Oops, forgot to attach that!
>> 
>> Updated patches attached.
>> 
>> <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>_______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140618/44a51eb5/attachment.html>