[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)
David Blaikie via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 12 18:00:46 PST 2022
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;
+
----------------
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)?
================
Comment at: clang/lib/Driver/Driver.cpp:5541-5542
+ //
+ // FIXME: Do we need to handle the case that the calculated output is
+ // conflicting with the specified output file or the input file?
+ if (!AtTopLevel && isa<PrecompileJobAction>(JA) &&
----------------
tahonermann wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > Do we do that for `-o` today? (eg: if you try to `-o` and specify the input file name, such that the output would overwrite the input, what happens? I'm not sure - but I guess it's OK to do whatever that is for this case too)
> > > Do we do that for -o today? (eg: if you try to -o and specify the input file name, such that the output would overwrite the input, what happens? I'm not sure - but I guess it's OK to do whatever that is for this case too)
> >
> > In this case, the input file will be overwrite and no warning shows. So it looks like we didn't take special treatment here. So I remove the FIXME.
> Basing the location of the module output on the presence of `-o` sounds confusing to me. Likewise, defaulting to writing next to the input file as opposed to the current directory (where a `.o` file is written by default) sounds wrong. I think this option should be handled similarly to `-o`; it should accept a path and:
> - If an absolute path is provided, write to that location.
> - If a relative path is provided, write to that location relative to the current working directory.
> Leave it up to the user or build system to ensure that the `.o` and `.pcm` file locations coincide if that is what they want. In general, I don't expect colocation of `.o` and `.pcm` files to be what is desired.
>
>
@tahonermann there's precedent for this with Split DWARF (.dwo files go next to the .o file) & I'd argued for possibly only providing this behavior, letting consumers move files elsewhere if they needed to, but got voted down there.
I do think this is a reasonable default, though. Users and build systems have the option to pass a path to place the .pcm somewhere else (in the follow-up patch to this one).
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137058/new/
https://reviews.llvm.org/D137058
More information about the cfe-commits
mailing list