Page MenuHomePhabricator

[Modules] Add Darwin-specific compatibility module map parsing hacks
ClosedPublic

Authored by benlangmuir on Jul 21 2015, 8:21 PM.

Details

Reviewers
silvas
rsmith
Summary

I ended up restricting the "requires excluded" hack to the specifically named modules after all. It turned out to only affect the two modules I had found: Darwin.C.excluded and Tcl.Private. I tested this back to OS X 10.8 and iOS 7.

This preserves backwards compatibility for two hacks in the Darwin
 system module map files:
 
 1. The use of 'requires excluded' to make headers non-modular, which
 should really be mapped to 'textual' now that we have this feature.
 
 2. Silently removes a bogus cplusplus requirement from IOKit.avc.
 
 Once we start diagnosing missing requirements and headers on
 auto-imports these would have broken compatibility with existing Darwin
 SDKs.

Diff Detail

Repository
rL LLVM

Event Timeline

benlangmuir retitled this revision from to [Modules] Add Darwin-specific compatibility module map parsing hacks.
benlangmuir updated this object.
benlangmuir added reviewers: rsmith, silvas.
benlangmuir set the repository for this revision to rL LLVM.
benlangmuir added a subscriber: cfe-commits.
silvas edited edge metadata.Jul 22 2015, 8:52 PM

My gut is that this seems too complicated, but on the other hand I don't have a suggestion for simplifying it. I'll let Richard decide on what is an acceptable level of complexity.

Could you put a lengthy comment somewhere describing exactly which shipping module map files are affected by this, and the range of affected SDK versions (hopefully this will be fixed in the next release?)?

benlangmuir updated this revision to Diff 31354.Aug 5 2015, 8:43 AM
benlangmuir edited edge metadata.

Sorry for the slow response, I somehow missed your original comment.

Add a more cohesive comment. I don't think we can usefully specify the SDK range it affects until a fixed SDK is available (currently it's every SDK that has module maps at all). I'd love to update the comment then. Ideally at that point we could also check the SDK version before applying the hack, but I'm not sure it's worth reading the SDK version plist file from disk just for that. We already read this file when creating the module cache hash, but I'd like to remove that at some point.

rsmith added inline comments.Aug 5 2015, 2:20 PM
include/clang/Basic/Module.h
203–211

Do we really need to write this into the Module object? Could we instead just track this while we're parsing the module map?

lib/Lex/ModuleMap.cpp
1590–1603

Please handle the excluded and cplusplus cases separately, to keep this special case as narrow as possible.

1591

Seems like overkill to compute the full module name for every module that requires cplusplus. Instead, how about walking the module path a component at a time and checking you get the expected sequence of components?

1891–1908

Is there a better way of handling this? If the parent directory isn't itself an umbrella directory of some module, maybe you could just ignore the umbrella directory declaration for this module entirely?

benlangmuir added inline comments.Aug 5 2015, 2:36 PM
include/clang/Basic/Module.h
203–211

Will do.

lib/Lex/ModuleMap.cpp
1590–1603

Will do. Not sure why I ever combined them like that...

1591

Makes sense, will do.

1891–1908

This only affects Tcl.Private, and Tcl has an umbrella header so I don't think that will work. The only other way I can think of making this work is to have a notion of a *directory* that is exempt from its containing umbrella, but I'm not sure that's a generally good feature and it seems like a lot more work. Let me know if you have any suggestions though.

rsmith added inline comments.Aug 5 2015, 2:44 PM
lib/Lex/ModuleMap.cpp
1891–1908

Ugh, OK. In that case:

  • maybe take the file name from I->path() rather than FE->getName() (we want to imagine the files were named within the umbrella directory rather than some other way)
  • sort the paths before you add them so that the serialized pcm doesn't depend on file system traversal order

Also, you'll be paying this file system traversal cost whenever the relevant module map is parsed, not only when the Tcl module is used -- and if it's the /usr/include module map, you'll walk this umbrella directory on every build. Just wanted to confirm you're OK with that.

benlangmuir added inline comments.Aug 5 2015, 3:01 PM
lib/Lex/ModuleMap.cpp
1891–1908
  • maybe take the file name from I->path() rather than FE->getName()
  • sort the paths

Ah, good catches!

Also, you'll be paying this file system traversal cost whenever the relevant module map is parsed,

Tcl is a framework so not affected by looking at usr/include. But your comment made me double check where else we might parse this. I think the answer is:

  1. When Tcl is named in an import or header include. This seems fine.
  2. When we call collectAllModules()...

I'm not happy about (2) since it can happen during code completion of imports. I suppose we're already iterating directories and parsing a tonne of other module map files here. I'll benchmark this and see how awful it is...

Changes per review. I still need to check performance of code-completing import statements to make sure the extra directory iteration isn't a big regression.

rsmith accepted this revision.Aug 12 2015, 11:36 AM
rsmith edited edge metadata.

Looks fine, go ahead once you're satisfied that the performance is OK for the requires excluded / umbrella dir hack.

include/clang/Basic/Module.h
363

fullModuleNameEquals (extra 's' at the end) or fullModuleNameIs maybe?

lib/Basic/Module.cpp
142–147

Seems weird to do this by modifying nameParts rather than tracking an index, but OK...

lib/Lex/ModuleMap.cpp
1025–1026

Unnecessary newline?

1607–1608

Overindented.

This revision is now accepted and ready to land.Aug 12 2015, 11:36 AM
benlangmuir closed this revision.Aug 13 2015, 10:20 AM

Any regression to code completion was below the noise level in my testing.

r244912

include/clang/Basic/Module.h
363

I like fullModuleNameIs.