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

[Justin Bogner]

Ben Langmuir <blangmuir at apple.com> writes:
>     On Jun 19, 2014, at 10:31 AM, Justin Bogner <mail at justinbogner.com> wrote:
>    
>     Ben Langmuir <blangmuir at apple.com> writes:
>    
>         Hi Justin,
>        
>         Thanks for working on this! It’s looking pretty close.
>
>             +/// Append the absolute path in Nested to the path given by Root.
>             This will
>             +/// remove directory traversal from the resulting nested path.
>             +static void appendNestedPath(SmallVectorImpl<char> &Root,
>             +                             SmallVectorImpl<char> &Nested) {
>             +  using namespace llvm::sys;
>             +  SmallVector<StringRef, 16> ComponentStack;
>             +
>             + StringRef Rel = path::relative_path(StringRef(Nested.begin(),
>             Nested.size()));
>
>         You seem to have manually inlined Nested.str() ;-) Maybe just make
>         your Nested parameter a StringRef?
>
>     Right, not sure what I was thinking there :). I'll pass in a StringRef
>     instead.
>
>             +  // We need an absolute path to append to the root.
>             +  SmallString<256> AbsoluteSrc = Src;
>             +  fs::make_absolute(AbsoluteSrc);
>             +  // Build the destination path.
>             +  SmallString<256> Dest = Collector.getDest();
>             +  size_t RootLen = Dest.size();
>             +  appendNestedPath(Dest, AbsoluteSrc);
>
>         Do we need to escape this somehow on Windows, since you might get C:
>         in the middle of your path?
>        
>         And in general, will this work if Dest ends with a path separator?
>         Then you would end up with // in the middle, which could potentially
>         be eaten at some point (not sure).
>
>     The call to path::relative_path in appendNestedPath takes care of both
>     of these issues. It's strips off root_name (ie, C:) and root_directory
>     (ie, /).
>
> It sure does, silly me.
>
>             +bool ModuleDependencyListener::visitInputFile(StringRef Filename,
>             bool IsSystem,
>             +                                              bool IsOverridden)
>             {
>             +  if (!Collector.insertSeen(Filename))
>             +    return true;
>             +  if (copyToRoot(Filename))
>             +    Collector.setHasErrors();
>             +  return true;
>             +}
>
>         This is clearer to me if you invert the first if, but you decide.
>         if (Collector.insertSeen(Filename))
>          if (copyToRoot(Filename))
>            Collector.setHasErrors();
>         return true;
>
>     Sure, I'm happy with either.
>
>             +// RUN: find %t/vfs -type f | FileCheck %s -check-prefix=DUMP
>             +// DUMP: AlsoDependsOnModule.framework/Headers/
>             AlsoDependsOnModule.h
>             +// DUMP:
>             Module.framework/Frameworks/SubFramework.framework/Headers/
>             SubFramework.h
>
>         REQUIRES: shell, since you need ‘find’.  This applies to both tests.
>         Also you won’t get the path separators you expect on Windows.
>
>     Hmm. Is there a good way to check if the files are created without find?
>     Assuming there is, I'll change it use regex for the path separators, as
>     I think the extra platform coverage here is worthwhile.
>
> I can’t think of a cleaner way to do this.
>
>         This isn’t really the place to discuss llvm patches, but...
>
>             +  char *Buf = new char[BufSize];
>        
>         If you don’t want to put 4 KB on the stack, how about std::vector with
>         its data() method?
>
>     Yeah, 4k seemed like a bit much for the stack (though, this is always a
>     leaf call, so maybe it's fine).
>
>     Is a vector really better here? Since I have to manually manage closing
>     the files anyway, the new/delete doesn't feel out of place, and using a
>     std::vector or a std::unique_ptr purely as an RAII container muddies up
>     what this is doing a bit.
>
> I don’t feel strongly about it, so go with what you have.
>
>             +  for (;;) {
>             +    Bytes = read(ReadFD, Buf, BufSize);
>             +    if (Bytes <= 0)
>             +      break;
>             +    Bytes = write(WriteFD, Buf, Bytes);
>             +    if (Bytes <= 0)
>             +      break;
>             +  }
>
>         This doesn’t seem sufficiently paranoid about the number of bytes
>         actually written by ‘write’.
>
>     Right. This should probably loop on the write as well. I'll update that.

New patches attached.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-0001-Frontend-Add-a-CC1-flag-to-dump-module-dependencies.4.patch
Type: text/x-patch
Size: 15595 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140619/83ce657d/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-0001-Support-Add-llvm-sys-fs-copy_file.3.patch
Type: text/x-patch
Size: 2472 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140619/83ce657d/attachment-0001.bin>