Thanks, I figured you were still traveling. Committed as r206201.
On Apr 13, 2014, at 5:10 PM, Richard Smith <[email protected]> wrote: > On Tue, Apr 8, 2014 at 4:38 PM, Ben Langmuir <[email protected]> wrote: > Gentle ping. Richard, did you want to continue to review this patch in > detail, or can I commit now that the high-level issues are resolved (at least > for this patch)? I’m happy to evolve this patch post-commit or here in this > review thread. > > Really sorry for the delay (EuroLLVM + catching up afterwards), LGTM. > > I'm rather surprised we don't already store the MODULE_NAME information into > the .pcm file... > > Ben > > On Apr 4, 2014, at 4:13 PM, Ben Langmuir <[email protected]> wrote: > >> >> On Apr 4, 2014, at 2:35 PM, Richard Smith <[email protected]> wrote: >> >>> On Fri, Apr 4, 2014 at 10:44 AM, Ben Langmuir <[email protected]> wrote: >>> >>> 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. >>> >>> That'll resolve symlinks in $PWD -- the problem is with build farms that >>> don't (or can't) make the file system look the same on each target, but try >>> to fake it up with symlinks; if you resolve the symlinks, you defeat this >>> attempt. >> >> I don’t know how that could break anything with the pcm filenames, but sure. >> >>> How about folding the path to the module map to lowercase before hashing >>> it, then when we come to load the module map, check that the FileEntry >>> found by header search for the module matches the FileEntry for the actual >>> module map path as you were planning? We'd get (and detect and recover >>> from) cache slot collisions if two module map files differ only by case, >>> but that sounds pretty unlikely to me. >> >> I can live with that. The attached patch uses StringRef::lower() and drops >> the modifications to FileManager. >> >> <modulemappath-v4.patch> >> >>> >>> 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
