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

[Justin Bogner]

Ben Langmuir <blangmuir at apple.com> writes:
> LGTM.

r211302 and r211303.

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