On Apr 4, 2014, at 9:09 AM, 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. In case it wasn’t clear that means we don’t need the canonical path. > >> >> 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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
