[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:5541-5545
+    if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+      TempPath = FinalOutput->getValue();
+    else
+      TempPath = BaseInput;
+
----------------
ChuanqiXu wrote:
> dblaikie wrote:
> > dblaikie wrote:
> > > ChuanqiXu wrote:
> > > > dblaikie wrote:
> > > > > It'd be nice if we didn't have to recompute this/lookup `OPT_o` here - any way we can use the object file output path here (that's already handled `OPT_o` or using the base input name, etc)?
> > > > I didn't understand this a lot. We don't compute anything here and we just use the object file output path here if `-o` is provided and we replace the suffix then. I feel this is simple enough.
> > > Computing the path to write to is what I'm referring to - and the fact that this had a bug (was relative to the source path instead of the CWD)/divergence from the `-o` path logic is the sort of thing I want to avoid.
> > > 
> > > I'm not sure there's an easy way to do this - but looks like the logic to name the .o file output is in `Driver::GetNamedOutputPath` and gets stored in the `clang::driver::Compilation`.... which I guess is where we are, but we're going through here with a `PrecompileJobAction` insntead of the compile job action, I suppose.
> > > 
> > > Could we keep these two codepaths closer together?
> > > 
> > > It'd be really nice if we could reuse that in some way.
> > > 
> > > Hmm, actually, why doesn't this fall out of the existing algorithm without modification?
> > > 
> > > Ah, I see, since the precompile action isn't "at top level" it gets a temporary file name - so if we change only that, it seems to fall out naturally:
> > > ```
> > > diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
> > > index c7efe60b2335..db878cbfff46 100644
> > > --- a/clang/lib/Driver/Driver.cpp
> > > +++ b/clang/lib/Driver/Driver.cpp
> > > @@ -5556,9 +5556,9 @@ const char *Driver::GetNamedOutputPath(Compilation &C, const JobAction &JA,
> > >    }
> > >  
> > >    // Output to a temporary file?
> > > -  if ((!AtTopLevel && !isSaveTempsEnabled() &&
> > > +  if (((!AtTopLevel && !isSaveTempsEnabled() &&
> > >         !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
> > > -      CCGenDiagnostics) {
> > > +      CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
> > >      StringRef Name = llvm::sys::path::filename(BaseInput);
> > >      std::pair<StringRef, StringRef> Split = Name.split('.');
> > >      const char *Suffix = types::getTypeTempSuffix(JA.getType(), IsCLMode());
> > > ```
> > > 
> > > Without the need to reimplement/duplicate the `-o` handling logic?
> > Oh, I should say, this patch didn't actually have the flag support, but it'd be something like this ^ but with the command line argument test as well (so "other stuff that's already there && !(TY_ModuleFile && hasArg fmodule-output)")
> To be honest, I prefer the previous patch. I feel it has higher readability. But this is a problem about taste and it doesn't have standard answer. Someone's readability is redundancy for others : )
I think there's real functionality we're at risk of missing by having separate implementations.

For instance - how does this interact with Apple's multiarch support (eg: `clang++ test.cppm -fmodule-output -arch i386 -arch x86_64 -target x86_64_apple_darwin`) - from my testing, without specifying an output file, you get something semi-usable/not simply incorrect: `test-i386.pcm` and `test-x86_64.pcm`. But if you try to name the output file you get `foo.pcm` and then another `foo.pcm` that overwrites the previous one. I think this could be taken care of if the suffix handling code was delegated down towards the else block that starts with `SmallString<128> Output(getDefaultImageName());`

But maybe solving that ^ problem could come out of a more general solution to the next problem:

What if you specify multiple source files on the command line without `-c`? Without `-o` you get `test1.pcm` and `test2.pcm`, but with `-o foo` you get `foo.pcm` overwriting `foo.pcm`. Perhaps if the output file specified isn't a .o file, we should ignore the `-o` and use the input-filename based naming? I guess we could reject this situation outright, and require the user to run multiple separate compilations. Though keeping features composable is nice.

Perhaps this needs a bit more consideration of some of these cases?




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

https://reviews.llvm.org/D137058