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

[Justin Bogner]

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

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

> +/// 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-0001-Frontend-Add-a-CC1-flag-to-dump-module-dependencies.2.patch
Type: text/x-patch
Size: 10740 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140618/8a291cb8/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-0002-Frontend-Include-a-VFS-map-when-dumping-module-depen.2.patch
Type: text/x-patch
Size: 3255 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140618/8a291cb8/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-0001-Support-Add-llvm-sys-fs-copy_file.patch
Type: text/x-patch
Size: 2242 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140618/8a291cb8/attachment-0002.bin>