tahonermann 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:
> > 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.
> how does this interact with Apple's multiarch support
Good question. What does split dwarf do in this case? Are differently named
outputs generated or is a multi-arch dwarf file produced (assuming they exist)?
Rejecting the command line if it specifies multiple `-arch` options with
`-fmodule-output=` seems fair to me (unless/until support for multi-arch .pcm
files is added). How is this handled with `-split_dwarf_output`?
> I'm not sure if I set something wrong or I must do it in Apple machine.
I don't recall for sure, but I think Apple Clang is needed. We noticed
differences between community Clang and Apple Clang while I was at Coverity,
but I don't recall the details.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137058/new/
https://reviews.llvm.org/D137058
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
