r310605 - [Modules] Prevent #import to reenter header if not building a module.

[Bruno Cardoso Lopes via cfe-commits]

On Fri, Aug 11, 2017 at 9:27 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On 11 August 2017 at 16:51, Bruno Cardoso Lopes via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>>
>> On Thu, Aug 10, 2017 at 5:36 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > On 10 August 2017 at 10:42, Hans Wennborg via cfe-commits
>> > <cfe-commits at lists.llvm.org> wrote:
>> >>
>> >> Sounds good to me.
>> >>
>> >> Richard, what do you think?
>> >>
>> >> On Thu, Aug 10, 2017 at 9:38 AM, Bruno Cardoso Lopes
>> >> <bruno.cardoso at gmail.com> wrote:
>> >> > Hi Hans, can we please get this merged into 5.0?
>> >> >
>> >> > Thanks,
>> >> >
>> >> > On Thu, Aug 10, 2017 at 12:16 PM, Bruno Cardoso Lopes via cfe-commits
>> >> > <cfe-commits at lists.llvm.org> wrote:
>> >> >> Author: bruno
>> >> >> Date: Thu Aug 10 08:16:24 2017
>> >> >> New Revision: 310605
>> >> >>
>> >> >> URL: http://llvm.org/viewvc/llvm-project?rev=310605&view=rev
>> >> >> Log:
>> >> >> [Modules] Prevent #import to reenter header if not building a
>> >> >> module.
>> >> >>
>> >> >> When non-modular headers are imported while not building a module
>> >> >> but
>> >> >> in -fmodules mode, be conservative and preserve the default #import
>> >> >> semantic: do not reenter headers.
>> >
>> >
>> > This comment doesn't appear to describe what this patch does: even when
>> > building a module it does not re-enter headers except for ones declared
>> > 'textual header' within the module being built. (isCompilingModuleHeader
>> > doesn't mean "are we compiling a module header right now?", it means "is
>> > this file declared as some kind of header within the current
>> > CompilingModule?".)
>>
>> What I'm trying to achieve here is: do not try to enter headers that
>> are not described as part of any module (textual) unless while
>> building a module. AFAIU, FileInfo.isCompilingModuleHeader is
>> basically LangOpts.isCompilingModule() && Mod->getTopLevelModule() ==
>> SourceModule, shouldn't it account for a more restrictive way to say
>> "are we compiling a textual header while building a module"?
>
>
> The "more restrictive" part is the problem: this also changes the behavior
> when we *are* building a module. Consider:
>
> // once.h
> #ifndef ONCE_H
> #define ONCE_H
> #pragma once
> extern int once;
> #endif
>
> // a.h
> #include "once.h"
>
> // b.h
> #include "once.h"
>
> // module.map
> module X { module A { header "a.h" } module B { header "b.h" } }
>
> This change appears to break the above module (when built with local
> submodule visibility enabled): module X.B no longer includes "once.h" and so
> no longer exports the "once" variable.

Nice testcase. I'll add it soon.

>> Checking for FileInfo.isCompilingModuleHeader has the additional
>> advantage that it would track previous attempts to import that header
>> while building a module, in which case it's going to try to re-enter.
>> It won't try to re-enter only in cases where that header was never
>> imported while building a header -> the same behavior #import has
>> without modules.
>>
>> Prior to r291644, clang would always avoid to re-enter a header. After
>> r291644 clang has a relaxed version of that, which I now proposing to
>> be less relaxed for #imports.
>>
>> > I'm nervous about taking this change: it will presumably break some
>> > cases
>> > (particularly with local submodule visibility enabled) where a #pragma
>> > once
>> > header is #included into multiple headers in the same module, by
>> > refusing to
>> > re-enter such headers -- and it will fix other such cases, depending on
>> > what
>> > exactly the header does and the structure of the module.
>>
>> I'd be interested in a such testcase to make sure we don't regress here!
>>
>> > If this were restricted to the case where we're not building a module
>> > (!LangOpts.isCompilingModule()), then I'd be OK putting it on the
>> > branch.
>>
>> Ok. It might not be a good idea to have this in 5.0 anyway, better
>> bake this for a while. But thinking forward, am I missing something
>> here? What about:
>>
>> if ((LangOpts.isCompilingModule() || !isImport) &&
>> !FileInfo.isModuleHeader &&
>>     FileInfo.getControllingMacro(ExternalLookup))
>>   TryEnterHdr = true;
>
>
> I think perhaps what we want is this:
>
> if (FileInfo.getControllingMacro(ExternalLookup) &&
> FileInfo.DefinesControllingMacro)
>   TryEnterHdr = true;
>
> ... where DefinesControllingMacro is set to false if we ever included the
> header and found that it did not actually define its controlling macro (as
> happens in the testcase for your patch).
>
> Justification: we have no idea if the file has actually already been
> included in our current preprocessing context if we're compiling a module.
> If the file *also* has an include guard that it defines, we should just use
> the include guard logic to figure out whether it has been or needs to be
> included, since it handles module visibility issues correctly. (I think the
> only case where this is going to go wrong is if someone #imports the header,
> then #undef's the include guard macro and #imports the header again, but
> that seems like a fringe case.)

I see, that would solve the issue indeed, while we still avoid
reentering when no controlling macros are available at all. Thanks for
the explanation. I reverted it for now in r310775.

> Note that this justification applies any time modules is enabled, not just
> when compiling a module. If you merely *load* a .pcm file whose compilation
> #imported some header file (without even importing any modules from it),
> then we will skip entering it with the current model.
>
> In the longer term we should track "visibility" of a #import / #pragma once
> event, like we do for macros and declarations, and only suppress the
> inclusion if there is a prior *visible* import / pragma once event.

Right, I'm looking forward to go for this approach in the long term.

-- 
Bruno Cardoso Lopes
http://www.brunocardoso.cc