On Fri, Aug 11, 2017 at 9:27 PM, Richard Smith <rich...@metafoo.co.uk> wrote: > On 11 August 2017 at 16:51, Bruno Cardoso Lopes via cfe-commits > <cfe-commits@lists.llvm.org> wrote: >> >> On Thu, Aug 10, 2017 at 5:36 PM, Richard Smith <rich...@metafoo.co.uk> >> wrote: >> > On 10 August 2017 at 10:42, Hans Wennborg via cfe-commits >> > <cfe-commits@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.card...@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@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits