r302932 - [modules] Simplify module macro handling in non-local-submodule-visibility mode.

[Juergen Ributzka via cfe-commits]

Hi Richard,

I think this broke modules.

If I try to compile a simple test program (@import Foundation;) I get the
following error:
./bin/clang test.m -fmodules -c -isysroot
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk
While building module 'Foundation' imported from test.m:1:
While building module 'CoreGraphics' imported from
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Foundation.framework/Headers/NSGeometry.h:12:
While building module 'IOKit' imported from
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/CoreGraphics.framework/Headers/CGDisplayConfiguration.h:12:
In file included from <module-includes>:40:
In file included from
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/IOKit.framework/Headers/hid/IOHIDDevicePlugIn.h:39:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/IOKit.framework/Headers/hid/IOHIDLibObsolete.h:41:8:
error: redefinition of
      'IOHIDEventStruct'
struct IOHIDEventStruct

Cheers,
Juergen


On Fri, May 12, 2017 at 11:56 AM, Richard Smith via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> Author: rsmith
> Date: Fri May 12 13:56:03 2017
> New Revision: 302932
>
> URL: http://llvm.org/viewvc/llvm-project?rev=302932&view=rev
> Log:
> [modules] Simplify module macro handling in non-local-submodule-visibility
> mode.
>
> When reaching the end of a module, we used to convert its macros to
> ModuleMacros but also leave them in the MacroDirective chain for the
> identifier. This meant that every lookup of such a macro would find two
> (identical) definitions. It also made it difficult to determine the correct
> owner for a macro when reaching the end of a module: the most recent
> MacroDirective in the chain could be from an #included submodule rather
> than
> the current module.
>
> Simplify this: whenever we convert a MacroDirective to a ModuleMacro when
> leaving a module, clear out the MacroDirective chain for that identifier,
> and
> just rely on the ModuleMacro to provide the macro definition information.
>
> (We don't want to do this for local submodule visibility mode, because in
> that
> mode we maintain a distinct MacroDirective chain for each submodule, and we
> need to keep around the prior MacroDirective in case we re-enter the
> submodule
> -- for instance, if its header is #included more than once in a module
> build,
> we need the include guard directive to stick around. But the problem
> doesn't
> arise in this case for the same reason: each submodule has its own
> MacroDirective chain, so the macros don't leak out of submodules in the
> first
> place.)
>
> Modified:
>     cfe/trunk/lib/Lex/PPLexerChange.cpp
>
> Modified: cfe/trunk/lib/Lex/PPLexerChange.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/
> PPLexerChange.cpp?rev=302932&r1=302931&r2=302932&view=diff
> ============================================================
> ==================
> --- cfe/trunk/lib/Lex/PPLexerChange.cpp (original)
> +++ cfe/trunk/lib/Lex/PPLexerChange.cpp Fri May 12 13:56:03 2017
> @@ -731,7 +731,7 @@ Module *Preprocessor::LeaveSubmodule(boo
>    Module *LeavingMod = Info.M;
>    SourceLocation ImportLoc = Info.ImportLoc;
>
> -  if (!needModuleMacros() ||
> +  if (!needModuleMacros() ||
>        (!getLangOpts().ModulesLocalVisibility &&
>         LeavingMod->getTopLevelModuleName() !=
> getLangOpts().CurrentModule)) {
>      // If we don't need module macros, or this is not a module for which
> we
> @@ -777,17 +777,6 @@ Module *Preprocessor::LeaveSubmodule(boo
>      for (auto *MD = Macro.getLatest(); MD != OldMD; MD =
> MD->getPrevious()) {
>        assert(MD && "broken macro directive chain");
>
> -      // Stop on macros defined in other submodules of this module that we
> -      // #included along the way. There's no point doing this if we're
> -      // tracking local submodule visibility, since there can be no such
> -      // directives in our list.
> -      if (!getLangOpts().ModulesLocalVisibility) {
> -        Module *Mod = getModuleContainingLocation(MD->getLocation());
> -        if (Mod != LeavingMod &&
> -            Mod->getTopLevelModule() == LeavingMod->getTopLevelModule())
> -          break;
> -      }
> -
>        if (auto *VisMD = dyn_cast<VisibilityMacroDirective>(MD)) {
>          // The latest visibility directive for a name in a submodule
> affects
>          // all the directives that come before it.
> @@ -809,6 +798,12 @@ Module *Preprocessor::LeaveSubmodule(boo
>          if (Def || !Macro.getOverriddenMacros().empty())
>            addModuleMacro(LeavingMod, II, Def,
>                           Macro.getOverriddenMacros(), IsNew);
> +
> +        if (!getLangOpts().ModulesLocalVisibility) {
> +          // This macro is exposed to the rest of this compilation as a
> +          // ModuleMacro; we don't need to track its MacroDirective any
> more.
> +          Macro.setLatest(nullptr);
> +        }
>          break;
>        }
>      }
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170512/8c49d358/attachment-0001.html>