This is an archive of the discontinued LLVM Phabricator instance.

Don't diagnose non-modular includes when we are not compiling a module
ClosedPublic

Authored by manmanren on Aug 24 2016, 4:33 PM.

Details

Summary

We used to have -fmodule-implementation-of and it was merged with -fmodule-name. But this causes some regression on our internal projects. We are now seeing non-modular include warnings.

This is triggered when we have relative includes to a VFS-mapped module, -I that points us to the real header and -F that points us to the vfs. The real header that is part of the umbrella directory will be classified as non-modular.

It seems like we can use CompilingModule to tell if we are compiling the interface of a module or the implementation file of a module. When compiling the implementation file, we don't diagnose non-modular includes. The non-modular includes will be diagnosed when building the interface and writing the pcm file.

Related changes:
http://llvm.org/viewvc/llvm-project?rev=213767&view=rev
http://llvm.org/viewvc/llvm-project?rev=261372&view=rev
http://llvm.org/viewvc/llvm-project?rev=263449&view=rev

Diff Detail

Event Timeline

manmanren updated this revision to Diff 69183.Aug 24 2016, 4:33 PM
manmanren retitled this revision from to Don't diagnose non-modular includes when we are not compiling a module.
manmanren updated this object.
manmanren added reviewers: rsmith, benlangmuir.
manmanren added a subscriber: cfe-commits.
benlangmuir edited edge metadata.Aug 25 2016, 8:50 AM

LGTM, but @rsmith should glance at this too.

rsmith edited edge metadata.Aug 25 2016, 2:47 PM

It seems to me like this is papering over an issue rather than fixing it.

diagnoseHeaderInclusion calls isHeaderInUmbrellaDirs here, which is presumably failing to determine that Nonmodular/A.h is in the umbrella directory for the Nonmodular module. My first guess would be that this code from ModuleMap::findHeaderInUmbrellaDirs may be responsible:

// Note: as an egregious but useful hack we use the real path here, because
// frameworks moving from top-level frameworks to embedded frameworks tend
// to be symlinked from the top-level location to the embedded location,
// and we need to resolve lookups as if we had found the embedded location.
StringRef DirName = SourceMgr.getFileManager().getCanonicalName(Dir);
lib/Lex/PPDirectives.cpp
749–750

I don't see why this should depend on whether we're compiling a module. If we're asked to diagnose non-modular #includes in modular headers, why should it matter whether we entered that header textually or by triggering precompilation of the corresponding module?

It seems to me like this is papering over an issue rather than fixing it.

I guess that is why we introduced fmodule-implementation-of, to work around issues like "relative includes to a VFS-mapped module". Ben should know more about the background.

diagnoseHeaderInclusion calls isHeaderInUmbrellaDirs here, which is presumably failing to determine that Nonmodular/A.h is in the umbrella directory for the Nonmodular module.

Bruno and I looked at this together yesterday. With vfsoverlay, the header files in the module map are using the real path, while the umbrella directories in the module map are using the virtual path. We have issue figuring out the header file from the real path is in the umbrella directory. We have longer term goals on fixing up this. But this is a regression on our side and we are hoping to get back to our previous behavior.

Cheers,
Manman

lib/Lex/PPDirectives.cpp
749–750

I agree that here, we are actually including a non modular header from a modular header. But is it okay to not diagnose when we have specified fmodule-name and we are building the implementation of it? We should have diagnosed this when building the unit.

rsmith accepted this revision.Aug 25 2016, 4:07 PM
rsmith edited edge metadata.

OK, if this unblocks you and you're working towards properly handling umbrella headers in the VFS, I'm happy to go ahead with this.

Please either rename the variable or move that part of the check into diagnoseHeaderInclusion, though -- with this change, the variable means something quite different from its name.

lib/Lex/PPDirectives.cpp
749–750

Sure, if we're running in a mode where we actually build the modules, rather than just including them textually. Under -fmodules-local-submodule-visibility, we support providing modules semantics without doing separate compilation for module headers.

This revision is now accepted and ready to land.Aug 25 2016, 4:07 PM
This revision was automatically updated to reflect the committed changes.