Page MenuHomePhabricator

[modules] Attempt to fix PR31905 - #include "stddef.h" breaks module map search paths; causes redefinitions.
Needs ReviewPublic

Authored by EricWF on Apr 15 2017, 1:01 PM.

Details

Summary

This patch attempts to fix llvm.org/PR31905.

The fix is to only allow the compiler to search the top-level include directories when looking a module.modulemap definition in ASTReader.cpp. This prevent loading arbitrary module maps from directories that would never normally be searched for a module map for _Builtin_stddef_max_align_t.

Perhaps it would be better to figure out what top-level search directory the module was originally built under, and then remove the prefix of the top-level directory. Then we could allow the search to look under matching subdirectories of other top-level search paths. However this seems like it could be incorrect as well.

Diff Detail

Event Timeline

EricWF created this revision.Apr 15 2017, 1:01 PM
EricWF planned changes to this revision.Apr 20 2017, 9:54 PM

I've got a better, actually correct, solution incoming.

EricWF updated this revision to Diff 101307.Jun 2 2017, 8:56 PM
EricWF edited the summary of this revision. (Show Details)
  • Only allow the top level header search directories to be search for module.modulemap files when loading modules in ReadAST.cpp.

Perhaps it would be better to figure out what top-level search directory the module was originally built under, and then remove the prefix of the top-level directory. Then we could allow the search to look under matching subdirectories of other top-level search paths. However this seems like it could be incorrect as well.

@zygoloid: Any thoughts?

chapuni added a subscriber: chapuni.Jun 2 2017, 9:08 PM
rsmith added inline comments.Feb 23 2018, 2:18 PM
lib/Serialization/ASTReader.cpp
2554–2556

This is only trying to catch the case where we've already loaded a module map for the module and the two conflict. We should just set AllowSearch to false here. If we want to also perform validation on the case where we load the module map after importing another definition, that should be handled if/when we load the module map.