rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
Only typographical nits and post-commit suggestions, so please go ahead. Thanks!
================
Comment at: include/clang/Serialization/ModuleManager.h:63
+ /// \brief Preprocessor's HeaderSearchInfo containing the module map.
+ const HeaderSearch& HeaderSearchInfo;
+
----------------
` &`, not `& `, please.
================
Comment at: lib/Frontend/CompilerInvocation.cpp:986
+ if (Val.find('=') == StringRef::npos)
+ Opts.ExtraDeps.push_back(Val);
+ }
----------------
Does a module file specified via `-fmodule-file=foo=foo.pcm` get included in
the dep file if it is used? (If not, I'm happy for that to be fixed in a
separate change, but it does need to be fixed for depfile-based build systems.)
================
Comment at: lib/Frontend/CompilerInvocation.cpp:982
+ StringRef Val = A->getValue();
+ if (Val.find('=') == StringRef::npos)
+ Opts.ExtraDeps.push_back(Val);
----------------
boris wrote:
> rsmith wrote:
> > There should be some way to specify a module file that contains a `=` in
> > its file name. Ideally, that way would be compatible with our
> > currently-supported form of `-fmodule-name=filename`. I don't see a way to
> > support that other than `stat`ing every value we're given and checking to
> > see whether it exists before attempting to split on a `=`... thoughts?
> >
> > One somewhat hackish alternative would be to only recognize an `=` if there
> > is no preceding `/`. (So `-fmodule-file=foo=bar.pcm` specifies a mapping,
> > and `-fmodule-file=./foo=bar.pcm` specifies the file `./foo=bar.pcm`.)
> >
> > (FWIW, we also support module names containing `=` characters.)
> A couple of thoughts:
>
> 1. Using stat() is also not without issues: I may have an unrelated file with
> a name that matches the whole value -- this will be fun to debug.
>
> 2. GCC seems to have given up on trying to support paths with '=' since
> AFAIK, none of their key=value options (e.g., -fdebug-prefix-map) support any
> kind of escaping. I think the implicit assumption is that if you are using
> paths with '=' in them, then you've asked for it.
>
> 3. The '/' idea will work but will get a bit hairier if we also want to
> support Windows (will need to check for both slashes).
>
> 4. Another option is to reserve empty module name as a way to escape '=':
> -fmodule-file==foo=bar.pcm. Not backwards compatible (but neither is ./) and
> looks a bit weird but is the simplest to implement.
>
> My preference order is (2), (4), (3), (1). Let me know which way you want to
> go.
>
>
We've had a while to think about this, and haven't found a completely
satisfactory answer, so let's go with (2) for now, without prejudice to future
alternatives. It's fully GCC-compatible, and happens to be what this patch
already does :)
================
Comment at: lib/Serialization/ASTReader.cpp:10180
+ ModuleMgr(PP.getFileManager(), PP.getPCMCache(), PCHContainerRdr,
+ PP.getHeaderSearchInfo ()),
PCMCache(PP.getPCMCache()), DummyIdResolver(PP),
----------------
No space before `()`.
================
Comment at: lib/Serialization/ASTReader.cpp:4145-4146
+ // by-name lookup.
+ if (Type == MK_PrebuiltModule || Type == MK_ExplicitModule)
+ ModuleMgr.registerPrebuilt(F);
break;
----------------
boris wrote:
> rsmith wrote:
> > I'm worried by this: the `Type` can be different for different imports of
> > the same .pcm file, and you'll only get here for the *first* import edge.
> > So you can fail to add the module to the "prebuilt modules" map here, and
> > then later attempt to look it up there (when processing the module offset
> > map) and fail to find it.
> >
> > Instead of keeping a separate lookup structure on the module manager, could
> > you look up the `Module`, and then use its `ASTFile` to find the
> > corresponding `ModuleFile`?
> >
> > (If you go that way, the changes to module lookup when reading the module
> > offset map could be generalized to apply to all `MK_*Module` types.)
> Yes, I was not entirely happy with this register prebuilt business so thanks
> for the hint. I've implemented this and all tests pass (both Clang's and my
> own). Though I cannot say I fully understand all the possible scenarios (like
> when can ASTFile be NULL).
>
>
>
> > (If you go that way, the changes to module lookup when reading the module
> > offset map could be generalized to apply to all MK_*Module types.)
>
> Are you suggesting that we switch to storing module names instead of module
> files for all the module types in the offset map? If so, I think I've tried
> that at some point but it didn't go well (existing tests were failing if I
> remember correctly). I can try again if you think this should work.
>
>
Yes, that's what I was suggesting, but don't block this commit on it.
Ultimately, we should probably be using something like an index into the
`IMPORTS` list instead (but it's not quite as easy as that since we also have
module offsets for imports of imports and so on).
https://reviews.llvm.org/D35020
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
