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

[Argyrios Kyrtzidis]

> 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 ?

> 
>> 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