Add module dependencies with -MD, -MMD

[Ben Langmuir]

On Mar 6, 2014, at 4:59 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:

> 
> On Mar 6, 2014, at 4:48 PM, Ben Langmuir <blangmuir at apple.com> wrote:
> 
>> 
>> On Mar 5, 2014, at 7:54 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>> 
>>> @@ -2014,13 +2074,17 @@ ASTReader::ReadControlBlock(ModuleFile &F,
>>>       // files.
>>>       unsigned NumInputs = Record[0];
>>>       unsigned NumUserInputs = Record[1];
>>> -        unsigned N = ValidateSystemInputs ||
>>> -                             (HSOpts.ModulesValidateOncePerBuildSession &&
>>> -                              F.Kind == MK_Module)
>>> -                         ? NumInputs
>>> -                         : NumUserInputs;
>>> +        unsigned N = NumUserInputs;
>>> +        if (ValidateSystemInputs ||
>>> +            (Listener && Listener->needsInputFileVisitation()) ||
>>> +            (HSOpts.ModulesValidateOncePerBuildSession && F.Kind == MK_Module))
>>> +          N = NumInputs;
>>> +
>>>       for (unsigned I = 0; I < N; ++I) {
>>>         InputFile IF = getInputFile(F, I+1, Complain);
>>> +          if (const FileEntry *F = IF.getFile())
>>> +            Listener->visitInputFile(F->getName(), I >= NumUserInputs);
>>>         if (!IF.getFile() || IF.isOutOfDate())
>>>           return OutOfDate;
>>>       }
>>> 
>>> The code here combines validation with reporting the dependencies and things start getting problematic..
>>> 
>>> One issue is that earlier there is this check:
>>> 
>>>    if (!DisableValidation &&
>>>        (!HSOpts.ModulesValidateOncePerBuildSession ||
>>>         F.InputFilesValidationTimestamp <= HSOpts.BuildSessionTimestamp)) {
>>> 
>>> so no dependencies are going to get reported if we are skipping validation.
>>> 
>>> Another issue is that even if we don't need to report the system dependencies we still go through all the system headers and stat them to get an InputFile to pass to the Listener.
>>> 
>>> I'd suggest separating the validation block from the "reporting dependencies".
>>> Also, inside ASTReader::getInputFile() there is the code to get the file path string from the record ID; factor this out into a separate function and use that to go through and report the dependencies (to avoid stat'ing).
>>> I believe we always store absolute paths in the module files so this should work for reporting the dependencies.
>> 
>> I’ve gone with this approach.  We will end up reading the filename, size, etc. information twice for any file that we are both validating *and* outputting dependency info for.  I would like to avoid that, but I didn’t see a nice way to factor the code for that while still keeping the validation and dependency stuff separate.  I’m not sure if this overhead matters in practice, and it will only affect clients that validate input files and print dependency info.
> 
> As far as building, dependency info will be always requested, but we may skip validation during a build session.
> 
> But it seems unfortunate that we go through all the system headers even though we don't need them, how about changing needsInputFileVisitation() to also return whether it needs all files or just the user ones ?
> 

Okay, I’ve done that in the attached patch.  It was cleaner to just add a new function needsSystem…().


>> 
>>> 
>>> 
>>> +++ b/test/Modules/dependency-gen.m
>>> @@ -0,0 +1,20 @@
>>> +// RUN: rm -rf %t-mcp
>>> +// RUN: mkdir -p %t-mcp
>>> +
>>> +// RUN: %clang_cc1 -x objective-c -dependency-file %t.d.1 -MT %s.o -I %S/Inputs -fsyntax-only -fmodules -fmodules-cache-path=%t-mcp %s
>>> +// RUN: FileCheck %s < %t.d.1
>>> +// CHECK: dependency-gen.m
>>> +// CHECK: Inputs/diamond_top.h
>>> +// CHECK: Inputs/module.map
>>> +// CHECK-NOT: string.h
>>> +
>>> +
>>> +// RUN: %clang_cc1 -x objective-c -dependency-file %t.d.2 -MT %s.o -I %S/Inputs -sys-header-deps -fsyntax-only -fmodules -fmodules-cache-path=%t-mcp %s
>>> +// RUN: FileCheck %s -check-prefix=CHECK-SYS < %t.d.2
>>> +// CHECK-SYS: dependency-gen.m
>>> +// CHECK-SYS: Inputs/diamond_top.h
>>> +// CHECK-SYS: Inputs/module.map
>>> +// CHECK-SYS: string.h
>>> +
>>> +#import "diamond_top.h"
>>> +#import "string.h"
>>> 
>>> Is this importing 'string.h' from the system ?
>> 
>> Ha, yes.  Luckily there was another test that already had a sysroot with a module.map.
>> 
>> The updated patch is attached.  Let me know if you’d like me to send a patch with whitespace changes omitted, since there are a lot of indentation changes.
>> 
>> Ben
>> 
>> <print-module-deps.patch>
>> 
>>> 
>>> 
>>> On Mar 4, 2014, at 9:37 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>>> 
>>>> Annnnnd I finally got around to updating this patch!
>>>> 
>>>> The new patch is simplified to rely on the ASTWriter’s concept of what a ‘system’ dependency is.  To that end, I’ve changed module.map files to no longer be unconditionally ‘user’ files.  Previously we forced module.map to be ‘user’ input files as a cheap way to detect system header changes without ‘stat’ing all the headers.  With Dmitri’s work on a timestamp-file-based approach, we shouldn’t need that trick anymore.
>>>> 
>>>> Ben
>>>> 
>>>> <print-module-deps.patch>
>>>> On Jan 22, 2014, at 4:40 PM, Ben Langmuir <blangmuir at apple.com> wrote:
>>>> 
>>>>> This patch adds module dependencies to the dependency files created by -MD/-MMD/etc.  It does so by attaching an ASTReaderListener that will call into the dependency file generator when a module input file is seen in the serialized AST.  In order to add/not add system headers appropriately, the IsSystem flag from the clang::Module is passed through the ASTReader and ModuleFile. There was also an addition to allow chaining ASTReaderListeners (let me know if you’d prefer that to be a separate patch).
>>>>> 
>>>>> Ben
>>>>> 
>>>>> <module-deps.patch>_______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140306/b42fe3c5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: print-module-deps.patch
Type: application/octet-stream
Size: 34619 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140306/b42fe3c5/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140306/b42fe3c5/attachment-0001.html>