[PATCH] D50404: [lld-link] Generalize handling of /debug and /debug:{none,full,fastlink,ghash,symtab}

Fri Jul 5 04:28:59 PDT 2019

thakis added inline comments.

================
Comment at: COFF/Driver.cpp:546
+    SmallVector<StringRef, 3> Types;
+    A->getSpelling().split(Types, ',', /*KeepEmpty=*/false);
+
----------------
thakis wrote:
> thakis wrote:
> > `getSpelling()` returns the spelling of the name of the option, but not its values. For `/debugtype:pdata`, it returns "/debugtype:" for example. You want `getValue()`, not `getSpelling()`, here.
> > 
> > The tests for `/debugtype:` aren't very good. `invalid-debug-type.test` checks that `/debugtype:invalid` produces a warning. It does: `Types` contains the string `/debugtype:` after the split, which counts as invalid and produces the warning.`pdb-none.test` passes `/debugtype:pdata` but that again just has `/debugtype:` in `Types` and produces the same warning, but the test doesn't use `/WX` or has a `CHECK-NOT` for it, and it doesn't check for the effect of `pdata`.
> > 
> > I.e. the `/debugtype:` switch hasn't been working since this commit, and apparently nobody noticed.
> Also, what's advertised as `/*KeepEmpty=*/` here is actually the `MaxSplit` parameter; `KeepEmpty` comes after it. I suppose we don't build lld with -Wdocumentation anywhere? This means this flag is never split on `,`s. (This bit isn't a new bug in this change.)
I tried to fix both of these problems in r365182.

Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D50404