<br><br><div class="gmail_quote">On Thu, Oct 11, 2012 at 11:07 PM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: dgregor<br>
Date: Thu Oct 11 16:07:39 2012<br>
New Revision: 165746<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=165746&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=165746&view=rev</a><br>
Log:<br>
Diagnose the expansion of ambiguous macro definitions. This can happen<br>
only with modules, when two disjoint modules #define the same<br>
identifier to different token sequences.<br>
<br></blockquote><div><br>Would it make sense to diagnose an ambiguity even if the macros both expand to the same sequence of tokens ?<br><br>I am concerned with the future actually. If I am linking in two modules and they "work for now" but later on one of them change its macro definition and not the other, I'll get the warning very late.<br>
<br>(I understand it's somewhat of a subjective opinion, but since there is a precedent with ODR for inline functions...)<br><br>-- Matthieu<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td<br>
cfe/trunk/include/clang/Lex/MacroInfo.h<br>
cfe/trunk/lib/Lex/MacroInfo.cpp<br>
cfe/trunk/lib/Lex/PPMacroExpansion.cpp<br>
cfe/trunk/lib/Serialization/ASTReader.cpp<br>
cfe/trunk/test/Modules/Inputs/macros_left.h<br>
cfe/trunk/test/Modules/Inputs/macros_right.h<br>
cfe/trunk/test/Modules/Inputs/macros_top.h<br>
cfe/trunk/test/Modules/macros.c<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu Oct 11 16:07:39 2012<br>
@@ -212,6 +212,7 @@<br>
<br>
// Preprocessor warnings.<br>
def : DiagGroup<"builtin-macro-redefined">;<br>
+def AmbiguousMacro : DiagGroup<"ambiguous-macro">;<br>
<br>
// Just silence warnings about -Wstrict-aliasing for now.<br>
def : DiagGroup<"strict-aliasing=0">;<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td?rev=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td Thu Oct 11 16:07:39 2012<br>
@@ -228,6 +228,12 @@<br>
def warn_pp_undef_identifier : Warning<<br>
"%0 is not defined, evaluates to 0">,<br>
InGroup<DiagGroup<"undef">>, DefaultIgnore;<br>
+def warn_pp_ambiguous_macro : Warning<<br>
+ "ambiguous expansion of macro %0">, InGroup<AmbiguousMacro>;<br>
+def note_pp_ambiguous_macro_chosen : Note<<br>
+ "expanding this definition of %0">;<br>
+def note_pp_ambiguous_macro_other : Note<<br>
+ "other definition of %0">;<br>
<br>
def pp_invalid_string_literal : Warning<<br>
"invalid string literal, ignoring final '\\'">;<br>
<br>
Modified: cfe/trunk/include/clang/Lex/MacroInfo.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/MacroInfo.h?rev=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Lex/MacroInfo.h?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Lex/MacroInfo.h (original)<br>
+++ cfe/trunk/include/clang/Lex/MacroInfo.h Thu Oct 11 16:07:39 2012<br>
@@ -111,6 +111,10 @@<br>
/// file.<br>
bool IsHidden : 1;<br>
<br>
+ /// \brief Whether the definition of this macro is ambiguous, due to<br>
+ /// multiple definitions coming in from multiple modules.<br>
+ bool IsAmbiguous : 1;<br>
+<br>
~MacroInfo() {<br>
assert(ArgumentList == 0 && "Didn't call destroy before dtor!");<br>
}<br>
@@ -340,6 +344,13 @@<br>
/// \brief Set whether this macro definition is hidden.<br>
void setHidden(bool Val) { IsHidden = Val; }<br>
<br>
+ /// \brief Determine whether this macro definition is ambiguous with<br>
+ /// other macro definitions.<br>
+ bool isAmbiguous() const { return IsAmbiguous; }<br>
+<br>
+ /// \brief Set whether this macro definition is ambiguous.<br>
+ void setAmbiguous(bool Val) { IsAmbiguous = Val; }<br>
+<br>
private:<br>
unsigned getDefinitionLengthSlow(SourceManager &SM) const;<br>
};<br>
<br>
Modified: cfe/trunk/lib/Lex/MacroInfo.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/MacroInfo.cpp?rev=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/MacroInfo.cpp?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Lex/MacroInfo.cpp (original)<br>
+++ cfe/trunk/lib/Lex/MacroInfo.cpp Thu Oct 11 16:07:39 2012<br>
@@ -32,7 +32,8 @@<br>
IsAllowRedefinitionsWithoutWarning(false),<br>
IsWarnIfUnused(false),<br>
IsPublic(true),<br>
- IsHidden(false) {<br>
+ IsHidden(false),<br>
+ IsAmbiguous(false) {<br>
}<br>
<br>
MacroInfo::MacroInfo(const MacroInfo &MI, llvm::BumpPtrAllocator &PPAllocator)<br>
@@ -56,7 +57,8 @@<br>
IsAllowRedefinitionsWithoutWarning(MI.IsAllowRedefinitionsWithoutWarning),<br>
IsWarnIfUnused(MI.IsWarnIfUnused),<br>
IsPublic(MI.IsPublic),<br>
- IsHidden(MI.IsHidden) {<br>
+ IsHidden(MI.IsHidden),<br>
+ IsAmbiguous(MI.IsAmbiguous) {<br>
setArgumentList(MI.ArgumentList, MI.NumArguments, PPAllocator);<br>
}<br>
<br>
<br>
Modified: cfe/trunk/lib/Lex/PPMacroExpansion.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPMacroExpansion.cpp?rev=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPMacroExpansion.cpp?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Lex/PPMacroExpansion.cpp (original)<br>
+++ cfe/trunk/lib/Lex/PPMacroExpansion.cpp Thu Oct 11 16:07:39 2012<br>
@@ -89,15 +89,25 @@<br>
// Find the end of the definition chain.<br>
MacroInfo *Prev = StoredMI;<br>
MacroInfo *PrevPrev;<br>
- bool Ambiguous = false;<br>
+ bool Ambiguous = StoredMI->isAmbiguous();<br>
+ bool MatchedOther = false;<br>
do {<br>
// If the macros are not identical, we have an ambiguity.<br>
- if (!Prev->isIdenticalTo(*MI, *this))<br>
- Ambiguous = true;<br>
+ if (!Prev->isIdenticalTo(*MI, *this)) {<br>
+ if (!Ambiguous) {<br>
+ Ambiguous = true;<br>
+ StoredMI->setAmbiguous(true);<br>
+ }<br>
+ } else {<br>
+ MatchedOther = true;<br>
+ }<br>
} while ((PrevPrev = Prev->getPreviousDefinition()) &&<br>
PrevPrev->isDefined());<br>
<br>
- // FIXME: Actually use the ambiguity information for something.<br>
+ // If there are ambiguous definitions, and we didn't match any other<br>
+ // definition, then mark us as ambiguous.<br>
+ if (Ambiguous && !MatchedOther)<br>
+ MI->setAmbiguous(true);<br>
<br>
// Wire this macro information into the chain.<br>
MI->setPreviousDefinition(Prev->getPreviousDefinition());<br>
@@ -360,7 +370,23 @@<br>
}<br>
}<br>
}<br>
-<br>
+<br>
+ // If the macro definition is ambiguous, complain.<br>
+ if (MI->isAmbiguous()) {<br>
+ Diag(Identifier, diag::warn_pp_ambiguous_macro)<br>
+ << Identifier.getIdentifierInfo();<br>
+ Diag(MI->getDefinitionLoc(), diag::note_pp_ambiguous_macro_chosen)<br>
+ << Identifier.getIdentifierInfo();<br>
+ for (MacroInfo *PrevMI = MI->getPreviousDefinition();<br>
+ PrevMI && PrevMI->isDefined();<br>
+ PrevMI = PrevMI->getPreviousDefinition()) {<br>
+ if (PrevMI->isAmbiguous()) {<br>
+ Diag(PrevMI->getDefinitionLoc(), diag::note_pp_ambiguous_macro_other)<br>
+ << Identifier.getIdentifierInfo();<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
// If we started lexing a macro, enter the macro expansion body.<br>
<br>
// If this macro expands to no tokens, don't bother to push it onto the<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=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTReader.cpp?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Serialization/ASTReader.cpp (original)<br>
+++ cfe/trunk/lib/Serialization/ASTReader.cpp Thu Oct 11 16:07:39 2012<br>
@@ -1254,6 +1254,24 @@<br>
SmallVector<IdentifierInfo*, 16> MacroArgs;<br>
MacroInfo *Macro = 0;<br>
<br>
+ // RAII object to add the loaded macro information once we're done<br>
+ // adding tokens.<br>
+ struct AddLoadedMacroInfoRAII {<br>
+ Preprocessor &PP;<br>
+ MacroInfo *Hint;<br>
+ MacroInfo *MI;<br>
+ IdentifierInfo *II;<br>
+<br>
+ AddLoadedMacroInfoRAII(Preprocessor &PP, MacroInfo *Hint)<br>
+ : PP(PP), Hint(Hint), MI(), II() { }<br>
+ ~AddLoadedMacroInfoRAII( ) {<br>
+ if (MI) {<br>
+ // Finally, install the macro.<br>
+ PP.addLoadedMacroInfo(II, MI, Hint);<br>
+ }<br>
+ }<br>
+ } AddLoadedMacroInfo(PP, Hint);<br>
+<br>
while (true) {<br>
unsigned Code = Stream.ReadCode();<br>
switch (Code) {<br>
@@ -1370,8 +1388,9 @@<br>
}<br>
MI->setHidden(Hidden);<br>
<br>
- // Finally, install the macro.<br>
- PP.addLoadedMacroInfo(II, MI, Hint);<br>
+ // Make sure we install the macro once we're done.<br>
+ AddLoadedMacroInfo.MI = MI;<br>
+ AddLoadedMacroInfo.II = II;<br>
<br>
// Remember that we saw this macro last so that we add the tokens that<br>
// form its body to it.<br>
<br>
Modified: cfe/trunk/test/Modules/Inputs/macros_left.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macros_left.h?rev=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macros_left.h?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Modules/Inputs/macros_left.h (original)<br>
+++ cfe/trunk/test/Modules/Inputs/macros_left.h Thu Oct 11 16:07:39 2012<br>
@@ -7,6 +7,8 @@<br>
<br>
<br>
#define LEFT_RIGHT_IDENTICAL int<br>
-#define LEFT_RIGHT_DIFFERENT float<br>
+<br>
#define LEFT_RIGHT_DIFFERENT2 float<br>
#define LEFT_RIGHT_DIFFERENT3 float<br>
+<br>
+#define LEFT_RIGHT_DIFFERENT float<br>
<br>
Modified: cfe/trunk/test/Modules/Inputs/macros_right.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macros_right.h?rev=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macros_right.h?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Modules/Inputs/macros_right.h (original)<br>
+++ cfe/trunk/test/Modules/Inputs/macros_right.h Thu Oct 11 16:07:39 2012<br>
@@ -1,8 +1,8 @@<br>
#include "macros_top.h"<br>
#define RIGHT unsigned short<br>
<br>
-#undef TOP_RIGHT_REDEF<br>
-#define TOP_RIGHT_REDEF float<br>
+<br>
+<br>
<br>
<br>
<br>
@@ -13,3 +13,5 @@<br>
#define LEFT_RIGHT_DIFFERENT2 int<br>
#define LEFT_RIGHT_DIFFERENT3 int<br>
<br>
+#undef TOP_RIGHT_REDEF<br>
+#define TOP_RIGHT_REDEF float<br>
<br>
Modified: cfe/trunk/test/Modules/Inputs/macros_top.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macros_top.h?rev=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/Inputs/macros_top.h?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Modules/Inputs/macros_top.h (original)<br>
+++ cfe/trunk/test/Modules/Inputs/macros_top.h Thu Oct 11 16:07:39 2012<br>
@@ -2,4 +2,12 @@<br>
<br>
#define TOP_LEFT_UNDEF 1<br>
<br>
+<br>
+<br>
+<br>
+<br>
+<br>
+<br>
+<br>
+<br>
#define TOP_RIGHT_REDEF int<br>
<br>
Modified: cfe/trunk/test/Modules/macros.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/macros.c?rev=165746&r1=165745&r2=165746&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/macros.c?rev=165746&r1=165745&r2=165746&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Modules/macros.c (original)<br>
+++ cfe/trunk/test/Modules/macros.c Thu Oct 11 16:07:39 2012<br>
@@ -7,8 +7,14 @@<br>
// RUN: %clang_cc1 -E -fmodules -x objective-c -fmodule-cache-path %t %s | FileCheck -check-prefix CHECK-PREPROCESSED %s<br>
// FIXME: When we have a syntax for modules in C, use that.<br>
// These notes come from headers in modules, and are bogus.<br>
+<br>
// FIXME: expected-note{{previous definition is here}}<br>
-// FIXME: expected-note{{previous definition is here}}<br>
+// expected-note{{other definition of 'LEFT_RIGHT_DIFFERENT'}}<br>
+// expected-note{{expanding this definition of 'TOP_RIGHT_REDEF'}}<br>
+// FIXME: expected-note{{previous definition is here}} \<br>
+// expected-note{{expanding this definition of 'LEFT_RIGHT_DIFFERENT'}}<br>
+<br>
+// expected-note{{other definition of 'TOP_RIGHT_REDEF'}}<br>
<br>
@__experimental_modules_import macros;<br>
<br>
@@ -109,11 +115,11 @@<br>
int i;<br>
float f;<br>
double d;<br>
- TOP_RIGHT_REDEF *ip = &i; // FIXME: warning<br>
+ TOP_RIGHT_REDEF *ip = &i; // expected-warning{{ambiguous expansion of macro 'TOP_RIGHT_REDEF'}}<br>
<br>
LEFT_RIGHT_IDENTICAL *ip2 = &i;<br>
- LEFT_RIGHT_DIFFERENT *fp = &f; // FIXME: warning<br>
- LEFT_RIGHT_DIFFERENT2 *dp = &d; // okay<br>
+ LEFT_RIGHT_DIFFERENT *fp = &f; // expected-warning{{ambiguous expansion of macro 'LEFT_RIGHT_DIFFERENT'}}<br>
+ LEFT_RIGHT_DIFFERENT2 *dp = &d;<br>
int LEFT_RIGHT_DIFFERENT3;<br>
}<br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">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>
</blockquote></div><br>