Add module dependencies with -MD, -MMD

[Argyrios Kyrtzidis]

On Mar 6, 2014, at 5:31 PM, Ben Langmuir <blangmuir at apple.com> wrote:

> 
> 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…().

I'd probably change the one function to return an enum like  "InputFileVisitationKind { None, UserOnly, All }" or something, but WFM!

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140306/38e4cea4/attachment.html>