On Apr 4, 2014, at 10:35 AM, Richard Smith <[email protected]> wrote:
> > On 4 Apr 2014 09:09, "Ben Langmuir" <[email protected]> wrote: > > > > > > On Apr 3, 2014, at 7:49 PM, Richard Smith <[email protected]> wrote: > > > >> On Thu, Apr 3, 2014 at 3:40 PM, Ben Langmuir <[email protected]> wrote: > >>> > >>> > >>> On Mar 28, 2014, at 2:45 PM, Richard Smith <[email protected]> wrote: > >>> > >>>> On Fri, Mar 28, 2014 at 1:12 PM, Ben Langmuir <[email protected]> > >>>> wrote: > >>>>> > >>>>> This patch allows multiple modules that have the same name to coexist > >>>>> in the module cache. To differentiate between two modules with the > >>>>> same name, we will consider the path the module map file that they are > >>>>> defined by* part of the ‘key’ for looking up the precompiled module > >>>>> (pcm file). Specifically, this patch renames the precompiled module > >>>>> (pcm) files from > >>>>> > >>>>> cache-path/<module hash>/Foo.pcm > >>>>> > >>>>> to > >>>>> > >>>>> cache-path/<module hash>/Foo-<hash of module map path>.pcm > >>>> > >>>> > >>>> From a high level, I don't really see why we need a second hash here. > >>>> Shouldn't the -I options be included in the <module hash>? If I build > >>>> the same module with different -I flags, that should resolve to > >>>> different .pcm files, regardless of whether it makes the module name > >>>> resolve to a different module.map file. > >>>> > >>>> Are you trying to cope with the case where the -I path finds multiple > >>>> module.map files defining the same module (where it's basically chance > >>>> which one will get built and used)? I don't really feel like this is the > >>>> right solution to that problem either -- we should remove the 'luck' > >>>> aspect and use some sane mechanism to determine which module.map files > >>>> are loaded, and in what order. > >>>> > >>>> Is this addressing some other case? > >>>> > >>>> > >>>>> > >>>>> In addition, I’ve taught the ASTReader to re-resolve the names of > >>>>> imported modules during module loading so that if the header search > >>>>> context changes between when a module was originally built and when it > >>>>> is loaded we can rebuild it if necessary. For example, if module A > >>>>> imports module B > >>>>> > >>>>> first time: > >>>>> clang -I /path/to/A -I /path/to/B … > >>>>> > >>>>> second time: > >>>>> clang -I /path/to/A -I /different/path/to/B … > >>>>> > >>>>> will now rebuild A as expected. > >>>>> > >>>>> > >>>>> * in the case of inferred modules, we use the module map file that > >>>>> *allowed* the inference, not the __inferred_module.map file, since the > >>>>> inferred file path is the same for every inferred module. > >>>> > >>>> > >>>> > >>>> Review comments on the patch itself: > >>>> > >>>> + /// the inferrence (e.g. contained 'module *') rather than the > >>>> virtual > >>>> > >>>> Typo 'inference', 'Module *'. > >>>> > >>>> + /// For an explanaition of \p ModuleMap, see Module::ModuleMap. > >>>> > >>>> Typo 'explanation'. > >>>> > >>>> + // safe becuase the FileManager is shared between the compiler > >>>> instances. > >>>> > >>>> Typo 'because' > >>>> > >>>> + // the inferred module. If this->ModuleMap is nullptr, then we are > >>>> using > >>>> + // -emit-module directly, and we cannot have an inferred module. > >>>> > >>>> I don't understand what this comment is trying to say. If we're using > >>>> -emit-module, then we were given a module map on the command-line; > >>>> should that not be referred to by this->ModuleMap? (Also, why 'this->'?) > >>>> How can a top-level module be inferred? Is that a framework-specific > >>>> thing? > >>>> > >>>> + StringRef ModuleMap = this->ModuleMap ? this->ModuleMap->getName() > >>>> : InFile; > >>>> > >>>> Please pick a different variable name rather than shadowing a member of > >>>> '*this' here. > >>>> > >>>> + // Construct the name <ModuleName>-<hash of ModuleMapPath>.pcm > >>>> which should > >>>> + // be globally unique to this particular module. > >>>> + llvm::APInt Code(64, llvm::hash_value(ModuleMapPath)); > >>>> + SmallString<128> HashStr; > >>>> + Code.toStringUnsigned(HashStr); > >>>> > >>>> Use base 36, like the module hash. > >>> > >>> > >>> I’ve attached an updated patch. Changes since the previous one: > >>> 1. Fixed the typos and other issues Richard pointed out > >>> 2. I’ve added code to canonicalize the module map path (using realpath); > >>> I was getting spurious failures on case-intensitive filesystems. > >> > >> > >> This part is probably not OK, because it'll do the wrong thing on some > >> build farms (where the canonical path is not predictable, but the path > >> that make_absolute returns is, by careful control of $PWD). I'll look into > >> this more, but will be traveling for the next couple of days. > > > > > > Okay, I have a better idea: I already store the actual module map path in > > MODULE_MAP_FILE, so when that is read, we can just get a FileEntry for it > > and compare to what header search finds for the current module (since > > FileEntries are uniqued by inode). That also means I can give a better > > diagnostic with the module map paths rather than the pcm filenames. > > Sounds great, thanks! > > Nuts, it turns out I still need a canonical path - when we hash the path into the pcm name we don’t want to get a different value depending on the case-sensitivity of the the file system. Shall I just make_absolute and then realpath()? That should incorporate $PWD correctly, I think. Ben > >> > >>> 3. I’ve moved the initialization of the MainFileID (in source manager) > >>> from Execute() to BeginSourceFile(), since we are now potentially > >>> creating file ids for module map files during pch loading and need to be > >>> able to find the main file reliably to construct a correct include stack. > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
