r178105 - [modules] Re-enable the "ambiguous expansion of macro" warning.
Argyrios Kyrtzidis
akyrtzi at gmail.com
Sat Oct 26 10:14:46 PDT 2013
On Oct 25, 2013, at 6:49 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Fri, Oct 25, 2013 at 6:20 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>
> On Oct 25, 2013, at 5:33 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
>> On Fri, Oct 25, 2013 at 4:54 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>
>> On Oct 25, 2013, at 4:35 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>>> On Fri, Oct 25, 2013 at 4:25 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>> On Oct 25, 2013, at 2:44 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>
>>>> On Tue, Mar 26, 2013 at 6:25 PM, Argyrios Kyrtzidis <akyrtzi at gmail.com> wrote:
>>>> Author: akirtzidis
>>>> Date: Tue Mar 26 20:25:19 2013
>>>> New Revision: 178105
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=178105&view=rev
>>>> Log:
>>>> [modules] Re-enable the "ambiguous expansion of macro" warning.
>>>>
>>>> Also update "test/Modules/macros.c" to test modified semantics:
>>>> -When there is an ambiguous macro, expand using the latest introduced version, not the first one.
>>>> -#undefs in submodules cause the macro to not be exported by that submodule, it doesn't cause
>>>> undefining of macros in the translation unit that imported that submodule.
>>>> This reduces macro namespace interference across modules.
>>>>
>>>> Modified:
>>>> cfe/trunk/lib/Lex/PPMacroExpansion.cpp
>>>> cfe/trunk/test/Modules/macros.c
>>>>
>>>> Modified: cfe/trunk/lib/Lex/PPMacroExpansion.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPMacroExpansion.cpp?rev=178105&r1=178104&r2=178105&view=diff
>>>> ==============================================================================
>>>> --- cfe/trunk/lib/Lex/PPMacroExpansion.cpp (original)
>>>> +++ cfe/trunk/lib/Lex/PPMacroExpansion.cpp Tue Mar 26 20:25:19 2013
>>>> @@ -211,7 +211,9 @@ bool Preprocessor::isNextPPTokenLParen()
>>>> /// expanded as a macro, handle it and return the next token as 'Identifier'.
>>>> bool Preprocessor::HandleMacroExpandedIdentifier(Token &Identifier,
>>>> MacroDirective *MD) {
>>>> - MacroInfo *MI = MD->getMacroInfo();
>>>> + MacroDirective::DefInfo Def = MD->getDefinition();
>>>> + assert(Def.isValid());
>>>> + MacroInfo *MI = Def.getMacroInfo();
>>>>
>>>> // If this is a macro expansion in the "#if !defined(x)" line for the file,
>>>> // then the macro could expand to different things in other contexts, we need
>>>> @@ -286,25 +288,22 @@ bool Preprocessor::HandleMacroExpandedId
>>>> }
>>>> }
>>>>
>>>> - // FIXME: Temporarily disable this warning that is currently bogus with a PCH
>>>> - // that redefined a macro without undef'ing it first (test/PCH/macro-redef.c).
>>>> -#if 0
>>>> // If the macro definition is ambiguous, complain.
>>>> - if (MI->isAmbiguous()) {
>>>> + if (Def.getDirective()->isAmbiguous()) {
>>>> Diag(Identifier, diag::warn_pp_ambiguous_macro)
>>>> << Identifier.getIdentifierInfo();
>>>> Diag(MI->getDefinitionLoc(), diag::note_pp_ambiguous_macro_chosen)
>>>> << Identifier.getIdentifierInfo();
>>>> - for (MacroInfo *PrevMI = MI->getPreviousDefinition();
>>>> - PrevMI && PrevMI->isDefined();
>>>> - PrevMI = PrevMI->getPreviousDefinition()) {
>>>> - if (PrevMI->isAmbiguous()) {
>>>> - Diag(PrevMI->getDefinitionLoc(), diag::note_pp_ambiguous_macro_other)
>>>> + for (MacroDirective::DefInfo PrevDef = Def.getPreviousDefinition();
>>>> + PrevDef && !PrevDef.isUndefined();
>>>> + PrevDef = PrevDef.getPreviousDefinition()) {
>>>> + if (PrevDef.getDirective()->isAmbiguous()) {
>>>> + Diag(PrevDef.getMacroInfo()->getDefinitionLoc(),
>>>> + diag::note_pp_ambiguous_macro_other)
>>>> << Identifier.getIdentifierInfo();
>>>> }
>>>> }
>>>> }
>>>> -#endif
>>>>
>>>> // If we started lexing a macro, enter the macro expansion body.
>>>>
>>>>
>>>> Modified: cfe/trunk/test/Modules/macros.c
>>>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/macros.c?rev=178105&r1=178104&r2=178105&view=diff
>>>> ==============================================================================
>>>> --- cfe/trunk/test/Modules/macros.c (original)
>>>> +++ cfe/trunk/test/Modules/macros.c Tue Mar 26 20:25:19 2013
>>>> @@ -1,4 +1,3 @@
>>>> -// XFAIL: *
>>>> // RUN: rm -rf %t
>>>> // RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros_top %S/Inputs/module.map
>>>> // RUN: %clang_cc1 -fmodules -x objective-c -emit-module -fmodules-cache-path=%t -fmodule-name=macros_left %S/Inputs/module.map
>>>> @@ -10,12 +9,12 @@
>>>> // These notes come from headers in modules, and are bogus.
>>>>
>>>> // FIXME: expected-note{{previous definition is here}}
>>>> +// FIXME: expected-note{{previous definition is here}} expected-note{{expanding this definition of 'LEFT_RIGHT_DIFFERENT'}}
>>>> +// expected-note{{other definition of 'TOP_RIGHT_REDEF'}} expected-note{{expanding this definition of 'LEFT_RIGHT_DIFFERENT2'}}
>>>> // expected-note{{other definition of 'LEFT_RIGHT_DIFFERENT'}}
>>>> -// expected-note{{expanding this definition of 'TOP_RIGHT_REDEF'}}
>>>> -// FIXME: expected-note{{previous definition is here}} \
>>>> -// expected-note{{expanding this definition of 'LEFT_RIGHT_DIFFERENT'}}
>>>>
>>>> -// expected-note{{other definition of 'TOP_RIGHT_REDEF'}}
>>>> +
>>>> +// expected-note{{expanding this definition of 'TOP_RIGHT_REDEF'}}
>>>>
>>>> @import macros;
>>>>
>>>> @@ -80,8 +79,8 @@ void f() {
>>>> # error TOP should be visible
>>>> #endif
>>>>
>>>> -#ifdef TOP_LEFT_UNDEF
>>>> -# error TOP_LEFT_UNDEF should not be visible
>>>> +#ifndef TOP_LEFT_UNDEF
>>>> +# error TOP_LEFT_UNDEF should still be defined
>>>>
>>>> This seems completely broken to me, and it breaks libc++.
>>>>
>>>> glibc has something like:
>>>>
>>>> // stdio.h
>>>> int getc(FILE*);
>>>> #define foo_getc(x) /* ... */
>>>> #define getc(x) foo_getc(x)
>>>>
>>>> libc++ has:
>>>>
>>>> // cstdio
>>>> #include <stdio.h>
>>>>
>>>> #ifdef getc
>>>> #undef getc
>>>> #endif
>>>>
>>>> namespace std { using ::getc; }
>>>>
>>>> This results in any program which includes <cstdio> being unable to use std::getc:
>>>>
>>>> // main.cc
>>>> #include <cstdio>
>>>> int k = std::getc(stdin); // error, no foo_getc in namespace std
>>>>
>>>> Can you explain a bit about the effect you were trying to achieve here? Here's another case:
>>>>
>>>> #include <stdio.h>
>>>> #include <something>
>>>> int k = getc(stdin); // still uses the macro from <stdio.h>
>>>>
>>>> I think we might want to ignore an #undef in <something> if <something> comes from a module that doesn't depend on the module containing <stdio.h>. (If it's just using the macro for its own internal purposes, or more generally if rearranging the order in which we chose to load the modules would result in a consistent macro definition chain.)
>>>>
>>>> I think the behavior we want here is:
>>>>
>>>> * If a module M1 depends on a module M2, macro definitions/undefs in M1 and M2 are never ambiguous (M1 is always ordered after M2).
>>>> * If there are multiple ambiguous 'most recent' definitions of a macro (excluding chains that end in an undef), and the macro is used, warn.
>>>>
>>>> Put another way: consider the subgraph of the imported portion of the module dependency graph in which the macro is or was defined. All roots of that subgraph for which the macro is defined must define it to the same value, or we warn on ambiguity. If there are any such roots, the value of the macro is the value at such a root. Otherwise, the macro is not defined.
>>>>
>>>> Does that sound right to you?
>>>
>>> I agree that for
>>>
>>> @import cstdio
>>>
>>> 'cstdio' should be able to "not export" the 'getc' macro.
>>> Without some language support to explicitly state whether you are undef'ing to avoid exporting vs undef'ing for internal use, I think it's reasonable to choose the former as default (undef'ing to avoid exporting).
>>>
>>> I'm not so sure about M1 just disabling macros from M2, even when I have explicitly imported M2. I don't like that a module may affect another module import in a surprising way; for example:
>>>
>>> In my suggestion, this would only happen if both:
>>> 1) M1 depends on M2, so it's undefining the macro defined by M2 not some other macro that happens to have the same name, and
>>> 2) The undef of the macro is visible (you imported it into this TU).
>>
>> I imported M2 to use its macro; I also imported M1 to use some other symbol from it. It should not be my concern that M1 wants to undef the macro that I want to import, if I don't want to see the macro from M2 I will remove its import declaration.
>>
>> Who says you imported M2?
>
> I'm not sure what writing "@import M2" explicitly means, if not that I imported M2 ?
>
> In your case 1:
>
> @import M1 // this re-exports M2 and re-purposes BAZOO; I can use symbols from both as M1 intended
>
> The comment is false. You cannot use the symbol BAZOO from M1, because the BAZOO macro from M2 tramples on it.
Yes, I agreed this is wrong and needs fixing; I was probably unclear here.
>
>> If you didn't import M2, you don't want its macros leaking out of M1, especially since M1 explicitly tried to remove them.
>>
>> If you imported both of them, then the effect of importing M1 and M2 is that you don't get the macro. M1 gets to decide this, because it is higher up in the layering. This is exactly what would happen with header files.
>
> You can do some horrible things with header files, I don't find "this is exactly what would happen with header files" a strong argument; modules are supposed to be more resilient and self-contained when combined together.
>
> Modules are also supposed to provide a smooth upgrade path from headers.
That is true; but modules is also a chance to reconsider the horrible tricks that headers use and at least try first if there's an alternative way other than "crippling" the module design in order to accommodate some horrible headers.
Basically, making the module semantics worse in order to accommodate a horrible header should be a conscious/informed choice.
>
>>
>>> I don't think this applies to the situations you want to block; I think my proposal gives you want you want there.
>>
>> How am I going to be able to use the macro from M2 while still importing M1 ?
>>
>> Why would you want to? M1 is removing that macro for a reason. I've given you a concrete example of a case where what I'm proposing is the right thing;
>
> I agreed that "@import cstdio" should not export the 'getc' macro
>
>> I'd like to see an example of a case where what you're proposing is the right thing. No-one I've spoken to thinks it makes sense, but perhaps we're missing something here.
>>
>> If you want this effect, use the macro from M2 before you import M1, or in a different TU. Same as with header files.
>
> With your suggestion my issue is that I cannot access the macros that M2 exports, irrespective of whether or not I imported another module; your workaround is that I should restructure my code.
>
> We have a single global namespace for macro names. If there are macro definition conflicts between modules, we're going to have problems. In the case I care about, there *is* no conflict, but we break the program anyway. That doesn't seem good.
>
> With my suggestion your issue is that the macro from M2 affects a symbol with the same name from M1; my workaround is that you simply remove the import of M1 and import only M2 (as the author of M2 intended).
>
> I don't understand what you're saying here. I want to use the interface provided by M1. If I've not imported M2, I may not *care* about the interface of M2, except that M1 happens to re-export it with the macros changed to functions (as a user of M1, I may not even know that M2 *exists*). I don't see why that shouldn't work naturally.
As I said above, I think there's a misunderstanding here; to reiterate: when you say "I want to use the interface provided by M1. If I've not imported M2" this is case 1) I mentioned and I agree with what you said should happen. If this does not happen currently then it should be fixed.
Should we focus on what should happen when importing both M1 and M2 ?
>
> I should add: I don't care what happens with @import, just with a #include that is promoted to a module import (that is, the backwards-compatibility path).
>
> We can imagine #undef X as a pseudo-definition of X as "not a macro", and that absence-of-a-macro might be part of a module's interface (as it is for the C++ standard library). From that perspective:
> * if I import two *unrelated* modules, and one of them #undef's X and the other #defines it to something, I have a conflict that I should diagnose on a use of the macro
> * if I import two *related* modules, and one of them *replaces* the other one's definition, the later (higher in dependency graph) module's definition should win
How do I know that two modules are *related* ? One module may have imported the other but if it doesn't re-export it it might as well be an *unrelated* one from my perspective; why do I need to care what modules it imported internally ?
>
> You've not yet given any concrete examples of why our current behavior is right. I'd like to see such an example to understand why the approaches I've suggested are not applicable.
>
> In particular, you seem to be suggesting that if I have two related modules, and one of them deliberately replaces the other one's definition, then we have an ambiguity *unless* one of them happened to undefine the macro, in which case we silently ignore that undefine. I can't see how that's the right answer.
I'm suggesting that If I explicitly import 2 modules, and the modules' interfaces disagree on what an identifier means, then there is ambiguity.
One module may or may not have imported the other module internally, but that doesn't change the fact that the modules' interfaces disagree on the meaning of an identifier.
There is a question of how to resolve this but at least a warning is warranted.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131026/4f0d11b2/attachment.html>
More information about the cfe-commits
mailing list