[cfe-dev] Conflicting vs. redefined ModuleMacros

[Jordan Rose via cfe-dev]

> On Oct 6, 2017, at 15:43, Richard Smith <richard at metafoo.co.uk> wrote:
> 
> On 6 October 2017 at 15:23, Jordan Rose via cfe-dev <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>> On Oct 6, 2017, at 15:06, Richard Smith <richard at metafoo.co.uk <mailto:richard at metafoo.co.uk>> wrote:
>> 
>> Hey Jordan,
>> 
>> I wrote up an answer, then realised what you're probably seeing is what happens when -fmodules-local-submodule-visibility is turned off. In that mode, macros from, say, ConflictingA.h will leak into ConflictingB.h (as if ConflictingB.h started by performing a private import of ConflictingA.h), so ConflictingB.h's macro *does* override ConflictingA.h's macro, and from clang's perspective the Conflicting and Redefined modules would then have the same behavior -- if you import A and B, you get the macro from B.
>> 
>> So then I think the question is, what is the difference between the two cases that you want to detect? (The #undef? The import of A into B prior to the redefinition? Something else?) I think we retain sufficient information to do this, but it's not necessarily convenient, and some of it isn't ever read from the PCM file currently.
> 
> Hm, interesting. Yes, we're not using -fmodules-local-submodule-visibility; apart from Apple's SDK not yet being vetted with this option, it seems to break simple things like
> 
> module X {
>   header "A.h"
>   header "B.h"
>   header "C.h"
>   export *
> }
> 
> with declarations from B.h not being visible to Swift at all. I suspect we'll have to rework our lookup logic in some way to do whatever's needed for this mode, but it's not an option for us at the moment.
> 
> I think the import of A into B would be a fair thing to look for. Another alternative that's not 100% correct but is likely to work in practice is to see if the macros are in different explicit submodules.
> 
> Any suggestions?
> 
> You can query whether B is imported into A with Module::isModuleVisible. That doesn't give you the import location, but might be good enough if you don't care whether the import was before the redefinition of the macro.
> 
> You could extend the ASTReader to (conditionally) load the macro definition chains for identifies even when the AST file is for a module. Once you have the chain, you could traverse it to see if the prior macro was #undef'd before the new one was #defined, if you want to base the outcome on that.
> 
> I have some other ideas below, but I don't think I have a good enough idea of what behavior you want in the various cases to give you particularly useful advice.
>  
> Or is no-local-submodule-visibility just on its way out, and not something you're interested in spending time on?
> 
> I don't think that mode has a long-term future, and I'd encourage you to find a way off it, but I expect it to stick around and keep working until you do :)
> 
> Thanks, this has already been helpful.
> Jordan
> 
> 
>> 
>> 
>> If you are using -fmodules-local-submodule-visibility, then:
>> 
>> On 6 October 2017 at 13:28, Jordan Rose via cfe-dev <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>> Hi, Richard. Jordan's back with another annoying ModuleMacro question for Swift. Let's say I have these two modules:
>> 
>> module Conflicting {
>>   explicit module A { header "ConflictingA.h" }
>>   explicit module B { header "ConflictingB.h" }
>> }
>> 
>> module Redefined {
>>   module A { header "RedefinedA.h" }
>>   module B { header "RedefinedB.h" }
>> }
>> 
>> Both headers in 'Conflicting' define a macro CONFLICTING, but with different values; a client is only supposed to import one of them. 'Redefined' is a little different: RedefinedB.h includes RedefinedA.h before defining the new value, and the client is probably going to import the entire top-level module. Let's say RedefinedB.h is even polite enough to use #undef.*
>> 
>> * If these examples sound bad, well, I agree, but the former is what the OpenGL framework on macOS has been doing for years, and the latter is how Clang's limits.h deals with a system limits.h.
>> 
>> The question: using ModuleMacro, how can I distinguish these two cases while building a PCM?
>> 
>> When you say "while building a PCM", do you mean before you get to the end of one of the headers / submodules, or after? Depending on when you ask, you'd need to look at either the set of overridden module macros in the preprocessor (while we're still lexing within the overriding module, before we create a ModuleMacro) or the list of overridden module macros on the ModuleMacro (after it's created).
>> 
>> We build a chain of MacroDirectives while we parse, and then convert them to ModuleMacros at the end of each submodule. So, taking the example of Redefined.B:
>> 
>> // start of RedefinedB.h, preprocessor's MacroState for CONFLICTING will be that the ModuleMacro from Redefined.A is active and there is no MacroDirective
>> #undef CONFLICTING
>> // now the MacroState will have Redefined.A overridden, with the UndefMacroDirective being the current MacroDirective
>> #define CONFLICTING something_else
>> // now the MacroState will have Redefined.A overridden, with the DefMacroDirective being the current MacroDirective
>> 
>> At the end of Redefined.A, we convert the DefMacroDirective to a ModuleMacro for Redefined.B that overrides Redefined.A's macro.
>> 
>> The obvious correct Clang answer is "don't bother, visibility will handle everything when the modules get imported", but unfortunately that doesn't fly for Swift, because of this third example:
>> 
>> module CrossModuleRedefinedCore {
>>   header "CrossModuleRedefinedCore.h"
>> }
>> module CrossModuleRedefined {
>>   // imports CrossModuleRedefinedCore
>>   header "CrossModuleRedefined.h"
>> }
>> 
>> In Swift, I'm allowed to say `CrossModuleRedefinedCore.REDEFINED` to get the old value, and `CrossModuleRedefined.REDEFINED` to get the new value. I'm not hugely concerned about this use case if I can get the other two to work well, but I haven't given up on it yet.
> 
> 
> What should happen if CrossModuleRedefined didn't redefine the macro, assuming CrossModuleRedefined includes an "export *"? Would CrossModuleRedefined.REDEFINED be an error or would it refer to the macro from CrossModuleRedefinedCore?
> 
> (In the former case, you could search the ModuleMacro graph for REDEFINED, looking for a ModuleMacro definition in the nominated module. The latter case seems trickier, but you could perhaps figure out the right module visibility state for having CrossModuleRedefined and all of its transitively exported modules visible, and then query the preprocessor for the visible ModuleMacro in that state.)

The latter, but we handle that at the Swift level rather than the Clang level, for better or worse. (Probably worse.)

I ended up sticking with the "wrong but practical" choice of just looking to see if the macros were in different explicit submodules. At some point that might bite us, but not today, and I suspect we'll need to redo this / get to rip out a bunch of hacks if/when we switch over to local submodule visibility mode.

Thanks for the help. (https://github.com/apple/swift/pull/12413 <https://github.com/apple/swift/pull/12413> up in Swift-land, if you're curious.)

Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20171012/776fb335/attachment.html>