lld-link: Implement support for %_PDB% and %_EXT% for /pdbaltpath:.

[thakis]

Summary

Fixes PR38940.

[thakis]

forgot to regen diff after latest changes, sorry.

[ruiu]

COFF/Driver.cpp

827

Move this comment before this function definition to make it a function comment.

834

I think you can omit llvm::.

843

Instead of handling a... fragment all at once, you can read one character at a time, and I believe that's slightly simpler. E.g.

while (!AltPath.empty()) {
  if (!AltPath.startswith("%")
    Buf.append(AltPath[0]);
  else if (AltPath.startswith("%_pdb%")
    ....
  else if (AltPath.startswith("%_ext%")
    ....
  else
    ....
}

[thakis]

Thanks!

COFF/Driver.cpp

843

I'm not sure it's much simpler, because the else needs to skip until the next % (for printing the warning, and for correctness). Here's how it looks with the .... filled in (maybe I didn't fill in the .... as you pictured though):

static void parsePDBAltPath(StringRef AltPath) {
  SmallString<128> Buf;
  StringRef PDBBasename =
      sys::path::filename(Config->PDBPath, sys::path::Style::windows);
  StringRef BinaryExtension =
      sys::path::extension(Config->OutputFile, sys::path::Style::windows);
  if (!BinaryExtension.empty())
    BinaryExtension = BinaryExtension.substr(1); // %_EXT% does not include '.'.

  while (!AltPath.empty()) {
    if (!AltPath.startswith("%")) {
      Buf.append(1, AltPath[0]);
      AltPath = AltPath.drop_front(1);
    } else if (AltPath.startswith_lower("%_pdb%")) {
      Buf.append(PDBBasename);
      AltPath = AltPath.drop_front(strlen("%_pdb%"));
    } else if (AltPath.startswith_lower("%_ext%")) {
      Buf.append(BinaryExtension);
      AltPath = AltPath.drop_front(strlen("%_ext%"));
    } else {
      size_t P = AltPath.find('%', 1);
      if (P == StringRef::npos) {
        Buf.append(AltPath);
        break;
      }
      StringRef Var = AltPath.substr(0, P + 1);
      warn("only %_PDB% and %_EXT% supported in /pdbaltpath:, keeping " + Var +
           " as literal");
      Buf.append(Var);
      AltPath = AltPath.drop_front(Var.size());
    }
  }

  Config->PDBAltPath = Buf;
}

or, slightly different:

static void parsePDBAltPath(StringRef AltPath) {
  SmallString<128> Buf;
  StringRef PDBBasename =
      sys::path::filename(Config->PDBPath, sys::path::Style::windows);
  StringRef BinaryExtension =
      sys::path::extension(Config->OutputFile, sys::path::Style::windows);
  if (!BinaryExtension.empty())
    BinaryExtension = BinaryExtension.substr(1); // %_EXT% does not include '.'.

  while (!AltPath.empty()) {
    if (!AltPath.startswith("%")) {
      Buf.append(1, AltPath[0]);
      AltPath = AltPath.drop_front(1);
    } else {
      size_t P = AltPath.find('%', 1);
      if (P == StringRef::npos) {
        Buf.append(AltPath);
        break;
      }
      StringRef Var = AltPath.substr(0, P + 1);
      if (Var.equals_lower("%_pdb%"))
        Buf.append(PDBBasename);
      else if (Var.equals_lower("%_ext%")) 
        Buf.append(BinaryExtension);
      else {
        warn("only %_PDB% and %_EXT% supported in /pdbaltpath:, keeping " +
             Var + " as literal");
        Buf.append(Var);
      }
      AltPath = AltPath.drop_front(Var.size());
    }
  }

  Config->PDBAltPath = Buf;
}

Here's the algorithm in the patch with drop_front:

static void parsePDBAltPath(StringRef AltPath) {
  SmallString<128> Buf;
  StringRef PDBBasename =
      sys::path::filename(Config->PDBPath, sys::path::Style::windows);
  StringRef BinaryExtension =
      sys::path::extension(Config->OutputFile, sys::path::Style::windows);
  if (!BinaryExtension.empty())
    BinaryExtension = BinaryExtension.substr(1); // %_EXT% does not include '.'.

  size_t Cursor = 0;
  while (Cursor < AltPath.size()) {
    size_t FirstMark, SecondMark;
    if ((FirstMark = AltPath.find('%', Cursor)) == StringRef::npos ||
        (SecondMark = AltPath.find('%', FirstMark + 1)) == StringRef::npos) {
      Buf.append(AltPath.substr(Cursor));
      break;
    }

    Buf.append(AltPath.substr(Cursor, FirstMark - Cursor));
    StringRef Var = AltPath.substr(FirstMark, SecondMark - FirstMark + 1);
    if (Var.equals_lower("%_pdb%"))
      Buf.append(PDBBasename);
    else if (Var.equals_lower("%_ext%"))
      Buf.append(BinaryExtension);
    else {
      warn("only %_PDB% and %_EXT% supported in /pdbaltpath:, keeping " +
           Var + " as literal");
      Buf.append(Var);
    }

    Cursor = SecondMark + 1;
  }

  Config->PDBAltPath = Buf;
}

Take your pick :-)

[ruiu]

Hmm, okay, it seems all the choices are not that different, so I'm fine with your original one. Thank you for writing all the pieces of code by the way! :)

LGTM

COFF/Driver.cpp

if (Obj->AddrsigSec) {
       // If an object file does not have an address-significance table,
       // conservatively mark all of its symbols as address-significant.
       for (Symbol *S : Syms)
         markAddrsig(S);
     }
   }
 }
 
+static void parsePDBAltPath(StringRef AltPath) {
+  // link.exe replaces each %foo% in AltPath with the contents of environment
+  // variable foo, and adds the two magic env vars _PDB (expands to the basename
+  // of pdb's output path) and _EXT (expands to the extension of the output
+  // binary).
+  // lld only supports %_PDB% and %_EXT% and warns on references to all other
+  // env vars.
+
+  llvm::SmallString<128> Buf;
+  StringRef PDBBasename =
+      sys::path::filename(Config->PDBPath, sys::path::Style::windows);
+  StringRef BinaryExtension =
+      sys::path::extension(Config->OutputFile, sys::path::Style::windows);
+
+  // Invariant:
+  //   +--------- Cursor ('a...' might be the empty string).
+  //   |   +----- FirstMark
+  //   |   |   +- SecondMark
+  //   v   v   v
+  //   a...%...%...
+  size_t Cursor = 0;
+  while (Cursor < AltPath.size()) {
+    size_t FirstMark, SecondMark;
+    if ((FirstMark = AltPath.find('%', Cursor)) == StringRef::npos ||
+        (SecondMark = AltPath.find('%', FirstMark + 1)) == StringRef::npos) {
+      // Didn't find another full fragment, treat rest of string as literal.
+      Buf.append(AltPath.substr(Cursor));
+      break;
+    }
+
+    // Found a full fragment. Append text in front of first %, and interpret
+    // text between first and second % as variable name.
+    Buf.append(AltPath.substr(Cursor, FirstMark - Cursor));
+    StringRef Var = AltPath.substr(FirstMark, SecondMark - FirstMark + 1);
+    if (Var == "%_PDB%")
+      Buf.append(PDBBasename);
+    else if (Var == "%_EXT%")
+      Buf.append(BinaryExtension);
+    else {
+      warn("only %_PDB% and %_EXT% supported in /pdbaltpath:, keeping " +
+           Var + " as literal");
+      Buf.append(Var);
+    }
+
+    Cursor = SecondMark + 1;
+  }
+
+  Config->PDBAltPath = Buf;
+}
+
 void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   // If the first command line argument is "/lib", link.exe acts like lib.exe.
   // We call our own implementation of lib.exe that understands bitcode files.
   if (ArgsArr.size() > 1 && StringRef(ArgsArr[1]).equals_lower("/lib")) {
     if (llvm::libDriverMain(ArgsArr.slice(1)) != 0)
       fatal("lib failed");
     return;
   }
if (ShouldCreatePDB) {
     if (Config->PDBAltPath.empty()) {
       Config->PDBAltPath = Config->PDBPath;
 
       // It's important to make the path absolute and remove dots.  This path
       // will eventually be written into the PE header, and certain Microsoft
       // tools won't work correctly if these assumptions are not held.
       sys::fs::make_absolute(Config->PDBAltPath);
       sys::path::remove_dots(Config->PDBAltPath);
+    } else {
+      // Don't do this earlier, so that Config->OutputFile is ready.
+      parsePDBAltPath(Config->PDBAltPath);
     }
   }
 
   // Set default image base if /base is not given.
   if (Config->ImageBase == uint64_t(-1))
     Config->ImageBase = getDefaultImageBase();
 
   Symtab->addSynthetic(mangle("__ImageBase"), nullptr);

test/COFF/Inputs/empty.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_I386
+  Characteristics: [  ]
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       4
+    SectionData:     31C0C3
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     ''
+  - Name:            .bss
+    Characteristics: [ IMAGE_SCN_CNT_UNINITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     ''
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          3
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        3963538403
+      Number:          1
+  - Name:            .data
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          0
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          2
+  - Name:            .bss
+    Value:           0
+    SectionNumber:   3
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          0
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          3
+  - Name:            '@feat.00'
+    Value:           1
+    SectionNumber:   -1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+  - Name:            _main
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...

test/COFF/pdbaltpath.test

+# RUN: yaml2obj %p/Inputs/empty.yaml > %t.obj
+
+# RUN: lld-link /entry:main %t.obj /out:%t.exe /debug /pdbaltpath:hello.pdb
+# RUN: llvm-readobj -coff-debug-directory %t.exe | FileCheck --check-prefix HELLO %s
+# HELLO:       PDBFileName: hello.pdb
+
+# RUN: lld-link /entry:main %t.obj /out:%t.exe /debug /pdbaltpath:%_PDB%
+# RUN: llvm-readobj -coff-debug-directory %t.exe | FileCheck --check-prefix PDBVAR %s
+# PDBVAR:       PDBFileName: pdbaltpath.test.tmp.pdb
+
+# RUN: lld-link /entry:main %t.obj /out:%t.exe /debug /pdbaltpath:foo%_EXT%.pdb
+# RUN: llvm-readobj -coff-debug-directory %t.exe | FileCheck --check-prefix EXTVAR %s
+# EXTVAR:       PDBFileName: foo.exe.pdb
+
+# RUN: lld-link /entry:main %t.obj /out:%t.exe /debug /pdbaltpath:%_PDB
+# RUN: llvm-readobj -coff-debug-directory %t.exe | FileCheck --check-prefix NOCLOSE %s
+# NOCLOSE:       PDBFileName: %_PDB
+
+# RUN: lld-link /entry:main %t.obj /out:%t.exe /debug /pdbaltpath:foo%_PDB
+# RUN: llvm-readobj -coff-debug-directory %t.exe | FileCheck --check-prefix NOCLOSE2 %s
+# NOCLOSE2:       PDBFileName: foo%_PDB
+
+# RUN: lld-link /entry:main %t.obj /out:%t.exe /debug /pdbaltpath:foo%_PDB%bar%_EXT
+# RUN: llvm-readobj -coff-debug-directory %t.exe | FileCheck --check-prefix CLOSEONE %s
+# CLOSEONE:       PDBFileName: foopdbaltpath.test.tmp.pdbbar%_EXT
+
+# RUN: lld-link /entry:main %t.obj /out:%t.exe /debug /pdbaltpath:foo%_PDB%bar%_EXT%
+# RUN: llvm-readobj -coff-debug-directory %t.exe | FileCheck --check-prefix CLOSETWO %s
+# CLOSETWO:       PDBFileName: foopdbaltpath.test.tmp.pdbbar.exe
+
+# RUN: lld-link /entry:main %t.obj /out:%t.exe /debug /pdbaltpath:foo%_PDB%bar%_EXT%a
+# RUN: llvm-readobj -coff-debug-directory %t.exe | FileCheck --check-prefix CLOSETWO2 %s
+# CLOSETWO2:       PDBFileName: foopdbaltpath.test.tmp.pdbbar.exea
+
+# RUN: lld-link /entry:main %t.obj /out:%t.exe /debug /pdbaltpath:foo%FoO%bar%r%a 2>&1 | FileCheck --check-prefix UNKNOWN-WARN %s
+# RUN: llvm-readobj -coff-debug-directory %t.exe | FileCheck --check-prefix ENVVARS %s
+# UNKNOWN-WARN: only %_PDB% and %_EXT% supported in /pdbaltpath:, keeping %FoO% as literal
+# UNKNOWN-WARN: only %_PDB% and %_EXT% supported in /pdbaltpath:, keeping %r% as literal
+# ENVVARS:       PDBFileName: foo%FoO%bar%r%a