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

[Richard Smith via cfe-commits]

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.

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

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170811/81c543f8/attachment.html>