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

[Justin Bogner]

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, /).

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

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

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