[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:5736
+      C.getArgs().hasArg(options::OPT_fmodule_output) &&
+      C.getArgs().hasArg(options::OPT_o)) {
+    SmallString<128> OutputPath;
----------------
ChuanqiXu wrote:
> dblaikie wrote:
> > tahonermann wrote:
> > > dblaikie wrote:
> > > > ChuanqiXu wrote:
> > > > > dblaikie wrote:
> > > > > > Is there some way we can avoid this (`-fmodule-output  -o ...`) being a special case? It'd be good if we could use a single common implementation regardless of whether `-o` is used (ie: Figure out the output file name (whether it's implicitly determined by clang, in the absence of `-o`, or explicitly provided by the user through `-o`) and then replace the suffix with `pcm`)?
> > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm file will always lives in the same directory with the input file.
> > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm file will always lives in the same directory with the input file.
> > > > 
> > > > I don't follow/understand. What I mean is, I'd like it, if possible, this was implemented by something like "find the path for the .o file output, then replace the extension with .pcm".
> > > > 
> > > > I worry atht code like this that recomputes something similar to the object output path risks getting out of sync with the actual object path.
> > > That is certainly a valid concern and I agree it would be better if the shared output path is computed exactly once. If that would require significant change, then tests to ensure consistent behavior would be the next best thing. I'm not sure what infrastructure is in place for validation of output file locations though.
> > Well, I guess the Split DWARF file naming isn't fundamentally better - it looks at the OPT_O argument directly too:
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1250
> > 
> > Perhaps we could at least reuse this same logic - make it a reusable function in some way? (for instance, it checks `-c` too, which seems relevant - we wouldn't want to name the .pcm after the linked executable name, right?
> > 
> > Perhaps we could at least reuse this same logic - make it a reusable function in some way? 
> 
> Done. It looks better now.
> 
> > (for instance, it checks -c too, which seems relevant - we wouldn't want to name the .pcm after the linked executable name, right?
> 
> Oh, right. Although the previous conclusion is that if `-o` is specified, the .pcm file should be in the same dir with the output. But it is indeed weird that the .pcm file are related to the linked executable if we thought it deeper. 
Ah, I was hoping it could reuse the same code, rather than duplicate it - any chance it could be refactored into a common implementation between Split DWARF and modules?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137058/new/

https://reviews.llvm.org/D137058