<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><blockquote type="cite"><div>On Jul 18, 2014, at 3:22 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 18, 2014 at 11:48 AM, Ben Langmuir <span dir="ltr"><<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word">Thanks! Reverted in r213395. For your convenience, I’ve attached my test case as a patch on top of your test cases.</div>
</blockquote><div><br></div><div>Thanks for the revert and the testcase; re-committed with a fix in r213416. Please let me know if this is still a problem; I don't have a Darwin system to test with.</div></div></div></div></div></blockquote><div><br></div><div>Looks good on Darwin. Thanks, Richard!</div><div><br></div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><span class=""><font color="#888888">Ben<div><div><br></div><div></div></div></font></span></div><br><div style="word-wrap:break-word"><br><div><blockquote type="cite">
<div>On Jul 18, 2014, at 11:39 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div><br><div><p dir="ltr">Please revert if this is blocking you; I'll dig into the root cause today.</p>
<div class="gmail_quote">On 18 Jul 2014 11:22, "Ben Langmuir" <<a href="mailto:blangmuir@apple.com" target="_blank">blangmuir@apple.com</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hey Richard,<br>
<br>
After this commit, the system modules on Darwin don’t build.<br>
<br>
Using your test case as a starting point, if you add a header conddef.h:<br>
#include “a2.h”<br>
#ifndef assert<br>
#define assert(X)<br>
#endif<br>
<br>
which has an obvious module:<br>
module conddef { header “conddef.h” export * }<br>
<br>
And then include that and try to use assert(), it will not be there. It looks like assert is incorrectly visible in conddef.h, but now we’re not re-exporting it. So it might not really be this change that’s the problem, but maybe it’s interacting really poorly with another bug?<br>
<br>
This seems really bad to me, because this pattern of ifndef define is really common. Do you know what the fix is, or can we revert for the time being?<br>
<br>
Ben<br>
<br>
<br>
> On Jul 17, 2014, at 9:53 PM, Richard Smith <<a href="mailto:richard-llvm@metafoo.co.uk" target="_blank">richard-llvm@metafoo.co.uk</a>> wrote:<br>
><br>
> Author: rsmith<br>
> Date: Thu Jul 17 23:53:37 2014<br>
> New Revision: 213348<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=213348&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=213348&view=rev</a><br>
> Log:<br>
> [modules] Fix macro hiding bug exposed if:<br>
><br>
> * A submodule of module A is imported into module B<br>
> * Another submodule of module A that is not imported into B exports a macro<br>
> * Some submodule of module B also exports a definition of the macro, and<br>
> happens to be the first submodule of B that imports module A.<br>
><br>
> In this case, we would incorrectly determine that A's macro redefines B's<br>
> macro, and so we don't need to re-export B's macro at all.<br>
><br>
> This happens with the 'assert' macro in an LLVM self-host. =(<br>
><br>
> Added:<br>
> cfe/trunk/test/Modules/Inputs/macro-hiding/<br>
> cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h<br>
> cfe/trunk/test/Modules/Inputs/macro-hiding/a2.h<br>
> cfe/trunk/test/Modules/Inputs/macro-hiding/b1.h<br>
> cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h<br>
> cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h<br>
> cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap<br>
> cfe/trunk/test/Modules/macro-hiding.cpp<br>
> Modified:<br>
> cfe/trunk/include/clang/Serialization/ASTReader.h<br>
> cfe/trunk/lib/Serialization/ASTReader.cpp<br>
> cfe/trunk/lib/Serialization/ASTWriter.cpp<br>
><br>
> Modified: cfe/trunk/include/clang/Serialization/ASTReader.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=213348&r1=213347&r2=213348&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Serialization/ASTReader.h?rev=213348&r1=213347&r2=213348&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/include/clang/Serialization/ASTReader.h (original)<br>
> +++ cfe/trunk/include/clang/Serialization/ASTReader.h Thu Jul 17 23:53:37 2014<br>
> @@ -1366,7 +1366,8 @@ public:<br>
> bool Complain);<br>
><br>
> /// \brief Make the names within this set of hidden names visible.<br>
> - void makeNamesVisible(const HiddenNames &Names, Module *Owner);<br>
> + void makeNamesVisible(const HiddenNames &Names, Module *Owner,<br>
> + bool FromFinalization);<br>
><br>
> /// \brief Set the AST callbacks listener.<br>
> void setListener(ASTReaderListener *listener) {<br>
> @@ -1831,7 +1832,7 @@ public:<br>
> ModuleFile &M, uint64_t Offset);<br>
><br>
> void installImportedMacro(IdentifierInfo *II, ModuleMacroInfo *MMI,<br>
> - Module *Owner);<br>
> + Module *Owner, bool FromFinalization);<br>
><br>
> typedef llvm::TinyPtrVector<DefMacroDirective *> AmbiguousMacros;<br>
> llvm::DenseMap<IdentifierInfo*, AmbiguousMacros> AmbiguousMacroDefs;<br>
><br>
> Modified: cfe/trunk/lib/Serialization/ASTReader.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=213348&r1=213347&r2=213348&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=213348&r1=213347&r2=213348&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Serialization/ASTReader.cpp (original)<br>
> +++ cfe/trunk/lib/Serialization/ASTReader.cpp Thu Jul 17 23:53:37 2014<br>
> @@ -1790,7 +1790,7 @@ void ASTReader::resolvePendingMacro(Iden<br>
> // install if we make this module visible.<br>
> HiddenNamesMap[Owner].HiddenMacros.insert(std::make_pair(II, MMI));<br>
> } else {<br>
> - installImportedMacro(II, MMI, Owner);<br>
> + installImportedMacro(II, MMI, Owner, /*FromFinalization*/false);<br>
> }<br>
> }<br>
><br>
> @@ -1941,11 +1941,11 @@ ASTReader::removeOverriddenMacros(Identi<br>
> }<br>
><br>
> void ASTReader::installImportedMacro(IdentifierInfo *II, ModuleMacroInfo *MMI,<br>
> - Module *Owner) {<br>
> + Module *Owner, bool FromFinalization) {<br>
> assert(II && Owner);<br>
><br>
> SourceLocation ImportLoc = Owner->MacroVisibilityLoc;<br>
> - if (ImportLoc.isInvalid()) {<br>
> + if (ImportLoc.isInvalid() && !FromFinalization) {<br>
> // FIXME: If we made macros from this module visible but didn't provide a<br>
> // source location for the import, we don't have a location for the macro.<br>
> // Use the location at which the containing module file was first imported<br>
> @@ -3320,7 +3320,9 @@ static void moveMethodToBackOfGlobalList<br>
> }<br>
> }<br>
><br>
> -void ASTReader::makeNamesVisible(const HiddenNames &Names, Module *Owner) {<br>
> +void ASTReader::makeNamesVisible(const HiddenNames &Names, Module *Owner,<br>
> + bool FromFinalization) {<br>
> + // FIXME: Only do this if Owner->NameVisibility == AllVisible.<br>
> for (unsigned I = 0, N = Names.HiddenDecls.size(); I != N; ++I) {<br>
> Decl *D = Names.HiddenDecls[I];<br>
> bool wasHidden = D->Hidden;<br>
> @@ -3333,10 +3335,12 @@ void ASTReader::makeNamesVisible(const H<br>
> }<br>
> }<br>
><br>
> + assert((FromFinalization || Owner->NameVisibility >= Module::MacrosVisible) &&<br>
> + "nothing to make visible?");<br>
> for (HiddenMacrosMap::const_iterator I = Names.HiddenMacros.begin(),<br>
> E = Names.HiddenMacros.end();<br>
> I != E; ++I)<br>
> - installImportedMacro(I->first, I->second, Owner);<br>
> + installImportedMacro(I->first, I->second, Owner, FromFinalization);<br>
> }<br>
><br>
> void ASTReader::makeModuleVisible(Module *Mod,<br>
> @@ -3370,7 +3374,8 @@ void ASTReader::makeModuleVisible(Module<br>
> // mark them as visible.<br>
> HiddenNamesMapType::iterator Hidden = HiddenNamesMap.find(Mod);<br>
> if (Hidden != HiddenNamesMap.end()) {<br>
> - makeNamesVisible(Hidden->second, Hidden->first);<br>
> + makeNamesVisible(Hidden->second, Hidden->first,<br>
> + /*FromFinalization*/false);<br>
> HiddenNamesMap.erase(Hidden);<br>
> }<br>
><br>
> @@ -3897,7 +3902,7 @@ void ASTReader::finalizeForWriting() {<br>
> for (HiddenNamesMapType::iterator Hidden = HiddenNamesMap.begin(),<br>
> HiddenEnd = HiddenNamesMap.end();<br>
> Hidden != HiddenEnd; ++Hidden) {<br>
> - makeNamesVisible(Hidden->second, Hidden->first);<br>
> + makeNamesVisible(Hidden->second, Hidden->first, /*FromFinalization*/true);<br>
> }<br>
> HiddenNamesMap.clear();<br>
> }<br>
><br>
> Modified: cfe/trunk/lib/Serialization/ASTWriter.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=213348&r1=213347&r2=213348&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriter.cpp?rev=213348&r1=213347&r2=213348&view=diff</a><br>
> ==============================================================================<br>
> --- cfe/trunk/lib/Serialization/ASTWriter.cpp (original)<br>
> +++ cfe/trunk/lib/Serialization/ASTWriter.cpp Thu Jul 17 23:53:37 2014<br>
> @@ -3089,7 +3089,17 @@ class ASTIdentifierTableTrait {<br>
> }<br>
><br>
> SubmoduleID getSubmoduleID(MacroDirective *MD) {<br>
> - return Writer.inferSubmoduleIDFromLocation(MD->getLocation());<br>
> + if (MD->getLocation().isValid())<br>
> + return Writer.inferSubmoduleIDFromLocation(MD->getLocation());<br>
> +<br>
> + // If we have no directive location, this macro was installed when<br>
> + // finalizing the ASTReader.<br>
> + if (DefMacroDirective *DefMD = dyn_cast<DefMacroDirective>(MD))<br>
> + return DefMD->getInfo()->getOwningModuleID();<br>
> +<br>
> + // Skip imports that only produce #undefs for now.<br>
> + // FIXME: We should still re-export them!<br>
> + return 0;<br>
> }<br>
><br>
> public:<br>
><br>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h?rev=213348&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h?rev=213348&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h (added)<br>
> +++ cfe/trunk/test/Modules/Inputs/macro-hiding/a1.h Thu Jul 17 23:53:37 2014<br>
> @@ -0,0 +1 @@<br>
> +#define assert(x)<br>
><br>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/a2.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/a2.h?rev=213348&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/a2.h?rev=213348&view=auto</a><br>
> ==============================================================================<br>
> (empty)<br>
><br>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/b1.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/b1.h?rev=213348&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/b1.h?rev=213348&view=auto</a><br>
> ==============================================================================<br>
> (empty)<br>
><br>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h?rev=213348&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h?rev=213348&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h (added)<br>
> +++ cfe/trunk/test/Modules/Inputs/macro-hiding/b2.h Thu Jul 17 23:53:37 2014<br>
> @@ -0,0 +1,2 @@<br>
> +#include "a2.h"<br>
> +#define assert(x)<br>
><br>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h?rev=213348&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h?rev=213348&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h (added)<br>
> +++ cfe/trunk/test/Modules/Inputs/macro-hiding/c1.h Thu Jul 17 23:53:37 2014<br>
> @@ -0,0 +1,2 @@<br>
> +#include "b1.h"<br>
> +#define assert(x)<br>
><br>
> Added: cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap?rev=213348&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap?rev=213348&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap (added)<br>
> +++ cfe/trunk/test/Modules/Inputs/macro-hiding/module.modulemap Thu Jul 17 23:53:37 2014<br>
> @@ -0,0 +1,11 @@<br>
> +module a {<br>
> + module a1 { header "a1.h" export * }<br>
> + module a2 { header "a2.h" export * }<br>
> +}<br>
> +module b {<br>
> + module b1 { header "b1.h" export * }<br>
> + module b2 { header "b2.h" export * }<br>
> +}<br>
> +module c {<br>
> + module c1 { header "c1.h" export * }<br>
> +}<br>
><br>
> Added: cfe/trunk/test/Modules/macro-hiding.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/macro-hiding.cpp?rev=213348&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/macro-hiding.cpp?rev=213348&view=auto</a><br>
> ==============================================================================<br>
> --- cfe/trunk/test/Modules/macro-hiding.cpp (added)<br>
> +++ cfe/trunk/test/Modules/macro-hiding.cpp Thu Jul 17 23:53:37 2014<br>
> @@ -0,0 +1,6 @@<br>
> +// RUN: rm -rf %t<br>
> +// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t -I%S/Inputs/macro-hiding %s<br>
> +#include "c1.h"<br>
> +#include "b2.h"<br>
> +<br>
> +void h() { assert(true); }<br>
><br>
><br>
> _______________________________________________<br>
> cfe-commits mailing list<br>
> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br>
</blockquote></div>
</div></blockquote></div><br></div><br></blockquote></div><br></div></div>
</div></blockquote></div><br></body></html>