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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu 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;
+
----------------
dblaikie wrote:
> 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?
> 
> 
Oh, thanks for finding this. It is pretty bad that I didn't image we can specify multiple input module units in one command line.

> 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.

Now the name of the module file will be the same with the input file no matter if we specified `-o` or not. With `-o` specified, the module files will be generated into the same directory with the output file. Without `-o` specified, the module files will be generated in  the working directory. It'll still be problematic if the user specify two inputs with the same name in two different directory, the module file of the latter will overwrite the former one. But I guess we don't need to handle such cases?

> 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());

In the new implementation,  I image we'll generate  test-i386.pcm and test-x86_64.pcm even if we specified `-o` with `-fmodule-output`. But the weird thing is that when I tried to reproduce your example, the compiler told me the other archs are ignored. I'm not sure if I set something wrong or I must do it in Apple machine.

BTW, I am not sure if I misunderstand you, but the else block that starts with `SmallString<128> Output(getDefaultImageName());` handles about IMAGE types, which looks irreverent to me.


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

https://reviews.llvm.org/D137058