[PATCH] D52942: lld-link: Implement support for %_PDB% and %_EXT% for /pdbaltpath:.

[Nico Weber via Phabricator via llvm-commits]

thakis marked 2 inline comments as done.
thakis added a comment.

Thanks!



================
Comment at: COFF/Driver.cpp:843
+  // Invariant:
+  //   +--------- Cursor ('a...' might be the empty string).
+  //   |   +----- FirstMark
----------------
ruiu wrote:
> 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
>       ....
>   }
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 :-)


https://reviews.llvm.org/D52942