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

[Ben Langmuir]

LGTM.

> On Jun 19, 2014, at 11:53 AM, Justin Bogner <mail at justinbogner.com> wrote:
> 
> 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.
> 
> <clang-0001-Frontend-Add-a-CC1-flag-to-dump-module-dependencies.4.patch><llvm-0001-Support-Add-llvm-sys-fs-copy_file.3.patch>