[LLD][COFF] Fix /linkrepro with options that take a filename or path

[aganea]

Summary

Some options like /pdb: or /out: or /implib: take a filename or path on the right side. When using partial or full paths with these options, previously this resulted in the paths not being translated relative-to-root by /linkrepro:

$ lld-link /pdb:"F:\some\path\mypdb.pdb" /out:"relative\..\..\real\output\file.exe" /implib:a.lib ... /linkrepro:.

The generated response file might have now non-existent paths on the reproducer's machine:

/pdb:"F:\some\path\mypdb.pdb"
/out:"relative\..\..\real\output\file.exe"
/out:a.lib
...

With this patch, the response is fully reproducible/usable as expected:

/pdb:mypdb.pdb
/out:file.exe
/implib:a.lib
...

[aganea]

Would you like to keep the filenames without a path as they are? ie. /implib:a.lib, keep a.lib in the response, instead of F/other/path/a.lib

[mstorsjo]

Aren't all of /implib, /pdband /out output files? In that sense it doesn't matter where they are written in the repro case (and writing them locally instead of somewhere else with an absolute path probably is more convenient?).

The one that would matter though, which iirc isn't handled right now, is when def files are passed as input. IIRC they aren't copied into the repro bundle.

[aganea]

In D59530#1434688, @mstorsjo wrote:

Aren't all of /implib, /pdband /out output files? In that sense it doesn't matter where they are written in the repro case (and writing them locally instead of somewhere else with an absolute path probably is more convenient?).

Yes, however our build system generates cmd-lines with full or relative paths for these options, which makes the response unusable, unless you edit it (because the paths aren't there on the reproducer's machine)

Are you suggesting that we strip those options from their path, and only kept the filename? That would be a good alternative.

[mstorsjo]

In D59530#1434691, @aganea wrote:

In D59530#1434688, @mstorsjo wrote:

Aren't all of /implib, /pdband /out output files? In that sense it doesn't matter where they are written in the repro case (and writing them locally instead of somewhere else with an absolute path probably is more convenient?).

Yes, however our build system generates cmd-lines with full or relative paths for these options, which makes the response unusable, unless you edit it (because the paths aren't there on the reproducer's machine)

Are you suggesting that we strip those options from their path, and only kept the filename? That would be a good alternative.

That sounds like a good alternative to me, yes.

[ruiu]

In D59530#1434700, @mstorsjo wrote:

In D59530#1434691, @aganea wrote:

In D59530#1434688, @mstorsjo wrote:

Aren't all of /implib, /pdband /out output files? In that sense it doesn't matter where they are written in the repro case (and writing them locally instead of somewhere else with an absolute path probably is more convenient?).

Yes, however our build system generates cmd-lines with full or relative paths for these options, which makes the response unusable, unless you edit it (because the paths aren't there on the reproducer's machine)

Are you suggesting that we strip those options from their path, and only kept the filename? That would be a good alternative.

That sounds like a good alternative to me, yes.

I agree, stripping directory names from a path seems a better approach. In particular, making it relative path as you proposed originally has an issue that you need to add an empty directory

F/other/path

to a generated tar file for

/out:F/other/path/file.exe

or otherwise the linker would fail when trying to create the output file. And adding an empty directory to an archive file is a little tricky -- I'm not 100% sure if all tar implementations restores an empty directory (a common idiom is to add an empty file .keep to an empty directory, so that the directory is not empty, but that's yet another hack.) All in all, keeping the only filename seems like the best option here.

[aganea]

Only output filenames, as requested.

[ruiu]

COFF/Driver.cpp

557

Let's just write out what you want here, instead of adding a variable and a new helper function.

OS << Arg->getSpelling() << ":" << sys::path::filename(Arg->getValue()) << "\n";

[aganea]

Simplified as requested.

[ruiu]

LGTM

COFF/Driver.cpp

for (auto *Arg : Args) {
     case OPT_libpath:
     case OPT_manifest:
     case OPT_manifest_colon:
     case OPT_manifestdependency:
     case OPT_manifestfile:
     case OPT_manifestinput:
     case OPT_manifestuac:
       break;
+    case OPT_implib:
+    case OPT_pdb:
+    case OPT_out:
+      OS << Arg->getSpelling() << sys::path::filename(Arg->getValue()) << "\n";
+      break;
     default:
       OS << toString(*Arg) << "\n";
     }
   }
 
   for (StringRef Path : SearchPaths) {
     std::string RelPath = relativeToRoot(Path);
     OS << "/libpath:" << quote(RelPath) << "\n";

test/COFF/linkrepro-pdb.test

 REQUIRES: x86, gnutar
 
 RUN: rm -rf %t && mkdir -p %t && cd %t
 RUN: yaml2obj %S/Inputs/pdb-type-server-simple-a.yaml -o a.obj
 RUN: yaml2obj %S/Inputs/pdb-type-server-simple-b.yaml -o b.obj
 RUN: llvm-pdbutil yaml2pdb %S/Inputs/pdb-type-server-simple-ts.yaml -pdb ts.pdb
-RUN: lld-link a.obj b.obj -entry:main -debug -out:t.exe -pdb:t.pdb -nodefaultlib -linkrepro:.
+RUN: lld-link a.obj b.obj -entry:main -debug -out:%t.exe -pdb:%t.pdb -nodefaultlib -linkrepro:.
 RUN: tar xOf repro.tar repro/%:t/ts.pdb > repro-ts.pdb
 RUN: diff ts.pdb repro-ts.pdb
+
+RUN: tar xf repro.tar
+RUN: cat repro/response.txt | FileCheck -check-prefix=PDB %s
+
+PDB: -out:linkrepro-pdb.test.tmp.exe
+PDB-NEXT: -pdb:linkrepro-pdb.test.tmp.pdb
+
+RUN: yaml2obj < %p/Inputs/export.yaml > %t1.obj
+RUN: lld-link /out:%t1.dll /dll %t1.obj /implib:%t1.lib /export:exportfn1 /export:exportfn2 /linkrepro:.
+RUN: tar xf repro.tar
+RUN: cat repro/response.txt | FileCheck -check-prefix=IMP %s
+
+IMP: /implib:linkrepro-pdb.test.tmp1.lib