[Remarks][LTO] Infer remarks file path from -object_path_lto

[thegameg]

Summary

In the common flow, remarks are expected to be in the .dSYM bundle after dsymutil collects them from the object files and creates a standalone remark file. After that, the linked binary and the .dSYM bundle are sufficient to execute, debug, and process the remarks.

Even if users pass -object_path_lto to the linker, we generate the temporary remark files based on the output file requested through -o, which is very inconvenient when users try to separate their intermediate products from their final products. This will leave intermediate remarks in the final products, like Frameworks or other bundles.

This patch will change that behavior to try to infer the remarks file path from the path specified in -object_path_lto.

It is a little messy as the driver will try to read flags that are intended to be passed to the linker, but for now, it's the only option and hasExportSymbolDirective is already doing this.

The cleaner way to do this would be to add an interface for the linker to specify the remark output file when setting up LTO.

[steven_wu]

clang/docs/UsersManual.rst

363

The implementation doesn't check -flto is used. You don't need to document this part.

clang/lib/Driver/ToolChains/Darwin.cpp

433

You can use a filtered iterator here. Also I think the last argument of the same type wins, rather than the first one.

This is rather ugly. Can you let linker pass the file path to you through LTO interface, rather to infer that from -Xlinker or -Wl option?

[thegameg]

clang/lib/Driver/ToolChains/Darwin.cpp

433

I agree with you here. The reasons I went for this approach is that it avoids adding remark-specific logic to the LTO interface and the linker. The linker would need to know how to build a remark filename based on things like the format or whether it's ThinLTO or not.

Thinking more about this, maybe the best option is to never pass the path to the remark file unless it's explicitly specified by the user, and let the LTOCodeGenerator create the file if asked (through another option like -lto-pass-remarks).

[steven_wu]

clang/lib/Driver/ToolChains/Darwin.cpp

433

I would argue that if LTO is missing the interface to support Remarks which is supported for normal compilation, adding an interface to libLTO is totally fine.

Using LTOCodeGenerator to write output remark file while writing out each object file sounds like a good idea.

clang/docs/UsersManual.rst

**-foptimization-record-file**
    option controls the file to which those reports are written.
 
    If this option is not used, optimization records are output to a file named
    after the primary file being compiled. If that's "foo.c", for example,
    optimization records are output to "foo.opt.yaml". If a specific
    serialization format is specified, the file will be named
    "foo.opt.<format>".
 
+   On Darwin platforms, if ``-flto`` is used and the file path is not provided,
+   the output file will be chosen according to the linker argument
+   ``-object_path_lto`` if present, or according to the output file specified
+   through ``-o`` otherwise.
+
 .. _opt_foptimization-record-passes:
 
 **-foptimization-record-passes**
    Only include passes which match a specified regular expression.
 
    When optimization reports are being output (see
    :ref:`-fsave-optimization-record <opt_fsave-optimization-record>`), this
    option controls the passes that will be included in the final report.

clang/lib/Driver/ToolChains/Darwin.cpp

 static bool isObjCRuntimeLinked(const ArgList &Args) {
   if (isObjCAutoRefCount(Args)) {
     Args.ClaimAllArgs(options::OPT_fobjc_link_runtime);
     return true;
   }
   return Args.hasArg(options::OPT_fobjc_link_runtime);
 }
 
+/// Check if the link command contains a path for the lto'd object file.
+static Optional<StringRef> getObjectPathLTO(const ArgList &Args) {
+  bool ExpectPathNext = false;
+  for (Arg *A : Args) {
+    if (ExpectPathNext) {
+      // Expecting -Xlinker <path>, but it's not there.
+      if (!A->getOption().matches(options::OPT_Xlinker))
+        return None;
+      return StringRef(A->getValue(0));
+    }
+
+    bool isWl = A->getOption().matches(options::OPT_Wl_COMMA);
+    bool isXlinker = A->getOption().matches(options::OPT_Xlinker);
+    // We're looking for either:
+    // * -Wl,-object_path_lto,<path>
+    // or
+    // * -Xlinker -object_path_lto -Xlinker <path>
+    if (!isWl && !isXlinker)
+      continue;
+
+    if (!A->containsValue("-object_path_lto"))
+      continue;
+
+    // In case of -Wl, the path is in the same argument.
+    if (isWl)
+      return StringRef(A->getValue(1));
+
+    // In case of -Xlinker, we need to catch the next -Xlinker option and get
+    // the path from there.
+    ExpectPathNext = true;
+  }
+  return None;
+}
+
 void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                   const InputInfo &Output,
                                   const InputInfoList &Inputs,
                                   const ArgList &Args,
                                   const char *LinkingOutput) const {
   assert(Output.getType() == types::TY_Image && "Invalid linker output type.");
 
   // If the number of arguments surpasses the system limits, we will encode the
void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   if (Args.hasFlag(options::OPT_fsave_optimization_record,
                    options::OPT_fsave_optimization_record_EQ,
                    options::OPT_fno_save_optimization_record, false)) {
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back("-lto-pass-remarks-output");
     CmdArgs.push_back("-mllvm");
 
     SmallString<128> F;
+    if (Optional<StringRef> Filename = getObjectPathLTO(Args))
+      F = *Filename;
+    else
       F = Output.getFilename();
+
     F += ".opt.";
     if (const Arg *A =
             Args.getLastArg(options::OPT_fsave_optimization_record_EQ))
       F += A->getValue();
     else
       F += "yaml";
 
     CmdArgs.push_back(Args.MakeArgString(F));

clang/test/Driver/darwin-ld.c

 // LINK_VERSION_DIGITS: invalid version number in '-mlinker-version=133.3.0.1a'
 
 // Check that we're passing -lto-pass-remarks-output for LTO
 // RUN: %clang -target x86_64-apple-darwin12 %t.o -fsave-optimization-record -### -o foo/bar.out 2> %t.log
 // RUN: FileCheck -check-prefix=PASS_REMARKS_OUTPUT %s < %t.log
 // PASS_REMARKS_OUTPUT: "-mllvm" "-lto-pass-remarks-output" "-mllvm" "foo/bar.out.opt.yaml"
 // PASS_REMARKS_OUTPUT-NOT: -lto-pass-remarks-with-hotness
 
+// Check that when -object_path_lto is used, we're passing the right path to -lto-pass-remarks-output.
+// RUN: %clang -target x86_64-apple-darwin12 %t.o -fsave-optimization-record -### -o foo/bar.out -Wl,-object_path_lto,foo/bar_lto.o 2> %t.log
+// RUN: FileCheck -check-prefix=REMARKS_OBJECT_PATH_LTO_WL %s < %t.log
+// REMARKS_OBJECT_PATH_LTO_WL: "-mllvm" "-lto-pass-remarks-output" "-mllvm" "foo/bar_lto.o.opt.yaml"
+// Also check for -Xlinker.
+// RUN: %clang -target x86_64-apple-darwin12 %t.o -fsave-optimization-record -### -o foo/bar.out -Xlinker -object_path_lto -Xlinker foo/bar_lto.o 2> %t.log
+// RUN: FileCheck -check-prefix=REMARKS_OBJECT_PATH_LTO_XLINKER %s < %t.log
+// REMARKS_OBJECT_PATH_LTO_XLINKER: "-mllvm" "-lto-pass-remarks-output" "-mllvm" "foo/bar_lto.o.opt.yaml"
+// Make sure that we pick the path based on -o if there is no second -Xlinker
+// with the path after -object_path_lto.
+// RUN: %clang -target x86_64-apple-darwin12 %t.o -fsave-optimization-record -### -o foo/bar.out -Xlinker -object_path_lto 2> %t.log
+// RUN: FileCheck -check-prefix=REMARKS_OBJECT_PATH_LTO_XLINKER_MISSING %s < %t.log
+// REMARKS_OBJECT_PATH_LTO_XLINKER_MISSING: "-mllvm" "-lto-pass-remarks-output" "-mllvm" "foo/bar.out.opt.yaml"
+
 // RUN: %clang -target x86_64-apple-darwin12 %t.o -fsave-optimization-record -### 2> %t.log
 // RUN: FileCheck -check-prefix=PASS_REMARKS_OUTPUT_NO_O %s < %t.log
 // PASS_REMARKS_OUTPUT_NO_O: "-mllvm" "-lto-pass-remarks-output" "-mllvm" "a.out.opt.yaml"
 
 // RUN: %clang -target x86_64-apple-darwin12 %t.o -fsave-optimization-record -fprofile-instr-use=blah -### -o foo/bar.out 2> %t.log
 // RUN: FileCheck -check-prefix=PASS_REMARKS_WITH_HOTNESS %s < %t.log
 // PASS_REMARKS_WITH_HOTNESS: "-mllvm" "-lto-pass-remarks-output" "-mllvm" "foo/bar.out.opt.yaml" "-mllvm" "-lto-pass-remarks-with-hotness"