r232721 - Add option to switch off putting header modules into the dependency file.

Tue Aug 11 12:03:44 PDT 2015

On Tue, Aug 11, 2015 at 11:49 AM, Manuel Klimek <klimek at google.com> wrote:

> Back in the day I'm pretty sure you agreed with the general concept :)
>

Yeah, I don't think I fully understood the reason for having a cc1
-module-file-deps flag at the time.

http://reviews.llvm.org/D8378
> I remember that we had problems with absolute paths ending up in the .d
> file (which should not happen if none of the paths provided are absolute),
> but I don't remember why we went for not writing them to the .d file
> instead of fixing the underlying problem.
>

Here's the situation from before your change:

 * Names of (implicit) module files were not written to the .pcm file;
they're an implementation detail of the compilation process and not an
actual input, so this is correct behavior (the input files used to *build*
the implicit modules do get written to the .pcm file, which again seems
right)
 * As a special case, when creating a .d file for a .pch file, implicit
module files *were* written to the .d file. This is unfortunate but
necessary, because rebuilding a .pcm file in the module cache does not give
a bit-for-bit identical result (sometimes just the signature changes,
sometimes the .pcm is built with a different configuration in a way that
the module cache hash doesn't detect, and very occasionally there's a hash
collision).

The -module-file-deps flag was an implementation detail of the .pch
building process, as a way of the driver telling the frontend to include
.pcm files in the produced .d file. The only effect of the flag added here
was to turn off this bugfix for PCH files; I don't think this is a useful
flag.

On Tue, Aug 11, 2015 at 8:41 PM Richard Smith <richard at metafoo.co.uk> wrote:
>
>> On Thu, Mar 19, 2015 at 5:00 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>>> Author: klimek
>>> Date: Thu Mar 19 07:00:22 2015
>>> New Revision: 232721
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=232721&view=rev
>>> Log:
>>> Add option to switch off putting header modules into the dependency file.
>>>
>>
>> Why? It is not correct to use a .pch file after the corresponding .pcm
>> file has been rebuilt. And why do you want a switch to turn on adding
>> implicit module files to the .d file? (I think the right behavior here is
>> that explicit modules are always listed, and implicit modules are never
>> listed except when building a .pch file.)
>>
>>
>>> Modified:
>>>     cfe/trunk/include/clang/Driver/Options.td
>>>     cfe/trunk/lib/Driver/Tools.cpp
>>>     cfe/trunk/test/Driver/pch-deps.c
>>>
>>> Modified: cfe/trunk/include/clang/Driver/Options.td
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=232721&r1=232720&r2=232721&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/include/clang/Driver/Options.td (original)
>>> +++ cfe/trunk/include/clang/Driver/Options.td Thu Mar 19 07:00:22 2015
>>> @@ -772,6 +772,10 @@ def fno_modules_strict_decluse : Flag <[
>>>    Flags<[DriverOption]>;
>>>  def fimplicit_modules : Flag <["-"], "fimplicit-modules">,
>>> Group<f_Group>,
>>>    Flags<[DriverOption]>;
>>> +def fmodule_file_deps : Flag <["-"], "fmodule-file-deps">,
>>> Group<f_Group>,
>>> +  Flags<[DriverOption]>;
>>> +def fno_module_file_deps : Flag <["-"], "fno-module-file-deps">,
>>> Group<f_Group>,
>>> +  Flags<[DriverOption]>;
>>>  def fno_ms_extensions : Flag<["-"], "fno-ms-extensions">,
>>> Group<f_Group>;
>>>  def fno_ms_compatibility : Flag<["-"], "fno-ms-compatibility">,
>>> Group<f_Group>;
>>>  def fno_delayed_template_parsing : Flag<["-"],
>>> "fno-delayed-template-parsing">, Group<f_Group>;
>>>
>>> Modified: cfe/trunk/lib/Driver/Tools.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Tools.cpp?rev=232721&r1=232720&r2=232721&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/lib/Driver/Tools.cpp (original)
>>> +++ cfe/trunk/lib/Driver/Tools.cpp Thu Mar 19 07:00:22 2015
>>> @@ -324,8 +324,9 @@ void Clang::AddPreprocessingOptions(Comp
>>>      if (A->getOption().matches(options::OPT_M) ||
>>>          A->getOption().matches(options::OPT_MD))
>>>        CmdArgs.push_back("-sys-header-deps");
>>> -
>>> -    if (isa<PrecompileJobAction>(JA))
>>> +    if ((isa<PrecompileJobAction>(JA) &&
>>> +         !Args.hasArg(options::OPT_fno_module_file_deps)) ||
>>> +        Args.hasArg(options::OPT_fmodule_file_deps))
>>>        CmdArgs.push_back("-module-file-deps");
>>>    }
>>>
>>>
>>> Modified: cfe/trunk/test/Driver/pch-deps.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/pch-deps.c?rev=232721&r1=232720&r2=232721&view=diff
>>>
>>> ==============================================================================
>>> --- cfe/trunk/test/Driver/pch-deps.c (original)
>>> +++ cfe/trunk/test/Driver/pch-deps.c Thu Mar 19 07:00:22 2015
>>> @@ -8,3 +8,14 @@
>>>  // RUN: FileCheck %s -check-prefix=CHECK-NOPCH -input-file=%t
>>>  // CHECK-NOPCH: -dependency-file
>>>  // CHECK-NOPCH-NOT: -module-file-deps
>>> +
>>> +// RUN: %clang -x c-header %s -o %t.pch -MMD -MT dependencies -MF %t.d \
>>> +// RUN:     -fno-module-file-deps -### 2> %t
>>> +// RUN: FileCheck %s -check-prefix=CHECK-EXPLICIT -input-file=%t
>>> +// CHECK-EXPLICIT: -dependency-file
>>> +// CHECK-EXPLICIT-NOT: -module-file-deps
>>> +
>>> +// RUN: %clang -x c++ %s -o %t.o -MMD -MT dependencies -MF %t.d
>>> -fmodule-file-deps -### 2> %t
>>> +// RUN: FileCheck %s -check-prefix=CHECK-EXPLICIT-NOPCH -input-file=%t
>>> +// CHECK-EXPLICIT-NOPCH: -dependency-file
>>> +// CHECK-EXPLICIT-NOPCH: -module-file-deps
>>>
>>>
>>> _______________________________________________
>>> 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/20150811/eba5abf9/attachment.html>