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; + ---------------- tahonermann wrote: > 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. > How is this handled with -split_dwarf_output? Although I am not familiar with `split-dwarf`, according to https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/BackendUtil.cpp#L1076-L1087, it looks like `-split_dwarf_output` wouldn't handle the multiple `-arch` options. But I am not sure if the reason is the `dwarf information` is arch-independent (or not). Maybe we need to ask @dblaikie to make sure. > 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). Yeah, it sounds good to me too. It is simpler and I **feel** like it wouldn't hurt so much if we don't support `-fmodule-output` with multiple arch. 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