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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

> (I'd probably still reduce the test down to one example, using -o and skipping the extra OUTPUT_PATH details, only checking the last part of the path is as specified (or perhaps checking that it matches the .o file?))

Done.

> Also, could you consider the questions @urnathan asked in the cross-project/flag naming thread about the semantics of this flag - I imagine the right behavior is "whatever we do for .o files", but it'd be good to check/consider/test them (they may not need automated testing - if the behaviour is implemented with the same utilities for .o files as for .pcm files then I don't mind just a statement that that's the case and some manual testing has been done to verify that all works)?

Yeah, I've replied in that thread. And it looks like we can't test the behaviors if we can only check the output of `-###` in this patch. I've made some manual testing locally and let's document the behaviors later.



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


================
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) &&
----------------
dblaikie wrote:
> 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).
@tahonermann here is another patch which implements the behavior you described: https://reviews.llvm.org/D137059


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

https://reviews.llvm.org/D137058