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

[lantictac]

Summary

Implement final argument precedence if multiple /debug arguments are passed on the command-line to match expected link.exe behavior. Support /debug:none and emit warning for /debug:fastlink with automatic fallback to /debug:full. Emit error if last /debug:option is unknown.

[ruiu]

This code was written when we supported only /debug and /debug:dwarf. At that time handling them as two separate options was a reasonable design. Now, we have /debug and /debug:{full,ghash,dwarf,symtab}, so I believe it's time to handle this just like other options that take a value after colon.

[lantictac]

Generalized /debug handling based on suggestion from ruiu.

[lantictac]

Ping

[ruiu]

COFF/Driver.cpp

511

Why does the name contani "Generation"?

956

It feels more complicated than before. I believe this piece of code can be written in a more flat style with less nesting. Maybe you could just dispatch based on a /debug value like this:

StringRef S = Args.getLastArg(OPT_debug, OPT_debug_opt))
if (S == "full") {
  Config->Debug = true;
  Config->Incremental = true;
  ShouldCreatePDB = true;
  ...
} else if (S == "dwarf") {
  Config->Debug = true;
  Config->DebugTypes = DWARF;
  ...
} else if ...

[lantictac]

Renamed DebugGeneration enum (name was based on MSDN description of /debug argument) to DebugKind. Flattened the argument handling code to be a little clearer.

[lantictac]

COFF/Driver.cpp

511

This was based on the MSDN docs as DebugType was already taken. I've changed to DebugKind in the hope it makes a little more sense.

956

Agreed. I've attempted to flatten the code a little in the latest patch.

[lantictac]

Ping

[ruiu]

COFF/Driver.cpp

521

Let's report an error inside this function and return a dummy kind (e.g. DebugKind::None) for an unknown value, so that the caller doesn't have to care about the error condition.

576

nit: no else after return.

577

If getDefaultDebugType is called by this line, I'd inline that function here.

938

DebugArg -> A for consistency.

943

Let's do this inside parseDebugKind.

COFF/Options.td

88

Since this is the only place where you are using POpt, I wouldn't define that multiclass. Instead, I'd jsut define debug and debug_opt right here.

[lantictac]

Moved/inlined parsing and diagnostic code into the specialized parse functions to (hopefully) simplify the calls in LinkerDriver::link().

[ruiu]

COFF/Driver.cpp

573

You probably should warn on unknown string.

576

Please avoid using auto unless its actual type is obvious.

578

StringSwitch has CaseLower, so you can use that instead of calling lower beforehand.

578

Return early.

589

Just return DebugKind::Full.

592

Ditto

937–939

I'd name DK Debug. We don't usually use acronym for variable names.

[lantictac]

Updated patch based on suggestions from ruiu

[ruiu]

Looks much better!

COFF/Driver.cpp

939

It is better to initialize directly to a desired value here:

bool ShouldCreatePDB = (Debug == DebugKind::Full || Debug == DebugKind::GHash);

941–942

Do you really have to call this function only when a debug flag is given? It looks to me that it is safe to do this unconditionally.

[lantictac]

Updated patch with ruiu's suggestions. Also added a test for the warning emitted when an unknown /debugtype option is used.

[ruiu]

LGTM

COFF/Driver.cpp

950

If ShouldCreatePDB is used only once, remove the variable and directly write the boolean expression in this if.

[zturner]

It would be great if we could land this soon-ish. I have a lot of people complaining that lld-link rejects /DEBUG:FASTLINK when using the VS extension. Is there any work left to be done or can we land this?

[lantictac]

I committed this as r342894 but for some reason the review didn't complete. Apologies, the last LLVM commits I did were pre-phabricator so it seems I messed up somewhere. Any pointers appreciated.

[zturner]

Ahh I see. You need to say "Differential Revision:
https://reviews.llvm.org/D50404" in the commit message. just the URL is
not sufficient to trigger the magic auto-close.

[lantictac]

Thanks for the clarification. Is there any way to manually close the review?

[thakis]

COFF/Driver.cpp

546

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.

[thakis]

COFF/Driver.cpp

546

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.)

[thakis]

COFF/Driver.cpp

546

I tried to fix both of these problems in r365182.

COFF/Driver.cpp

static std::string createResponseFile(const opt::InputArgList &Args,
   }
 
   for (StringRef Path : FilePaths)
     OS << quote(relativeToRoot(Path)) << "\n";
 
   return Data.str();
 }
 
-static unsigned getDefaultDebugType(const opt::InputArgList &Args) {
-  unsigned DebugTypes = static_cast<unsigned>(DebugType::CV);
-  if (Args.hasArg(OPT_driver))
-    DebugTypes |= static_cast<unsigned>(DebugType::PData);
-  if (Args.hasArg(OPT_profile))
-    DebugTypes |= static_cast<unsigned>(DebugType::Fixup);
-  return DebugTypes;
+enum class DebugKind { Unknown, None, Full, FastLink, GHash, Dwarf, Symtab };
+
+static DebugKind parseDebugKind(const opt::InputArgList &Args) {
+  auto *A = Args.getLastArg(OPT_debug, OPT_debug_opt);
+  if (!A)
+    return DebugKind::None;
+  if (A->getNumValues() == 0)
+    return DebugKind::Full;
+
+  DebugKind Debug = StringSwitch<DebugKind>(A->getValue())
+                     .CaseLower("none", DebugKind::None)
+                     .CaseLower("full", DebugKind::Full)
+                     .CaseLower("fastlink", DebugKind::FastLink)
+                     // LLD extensions
+                     .CaseLower("ghash", DebugKind::GHash)
+                     .CaseLower("dwarf", DebugKind::Dwarf)
+                     .CaseLower("symtab", DebugKind::Symtab)
+                     .Default(DebugKind::Unknown);
+
+  if (Debug == DebugKind::FastLink) {
+    warn("/debug:fastlink unsupported; using /debug:full");
+    return DebugKind::Full;
+  }
+  if (Debug == DebugKind::Unknown) {
+    error("/debug: unknown option: " + Twine(A->getValue()));
+    return DebugKind::None;
   }
+  return Debug;
+}
+
+static unsigned parseDebugTypes(const opt::InputArgList &Args) {
+  unsigned DebugTypes = static_cast<unsigned>(DebugType::None);
 
-static unsigned parseDebugType(StringRef Arg) {
+  if (auto *A = Args.getLastArg(OPT_debugtype)) {
     SmallVector<StringRef, 3> Types;
-  Arg.split(Types, ',', /*KeepEmpty=*/false);
+    A->getSpelling().split(Types, ',', /*KeepEmpty=*/false);
 
-  unsigned DebugTypes = static_cast<unsigned>(DebugType::None);
-  for (StringRef Type : Types)
-    DebugTypes |= StringSwitch<unsigned>(Type.lower())
+    for (StringRef Type : Types) {
+      unsigned V = StringSwitch<unsigned>(Type.lower())
                        .Case("cv", static_cast<unsigned>(DebugType::CV))
                        .Case("pdata", static_cast<unsigned>(DebugType::PData))
                        .Case("fixup", static_cast<unsigned>(DebugType::Fixup))
                        .Default(0);
+      if (V == 0) {
+        warn("/debugtype: unknown option: " + Twine(A->getValue()));
+        continue;
+      }
+      DebugTypes |= V;
+    }
+    return DebugTypes;
+  }
+
+  // Default debug types
+  DebugTypes = static_cast<unsigned>(DebugType::CV);
+  if (Args.hasArg(OPT_driver))
+    DebugTypes |= static_cast<unsigned>(DebugType::PData);
+  if (Args.hasArg(OPT_profile))
+    DebugTypes |= static_cast<unsigned>(DebugType::Fixup);
+
   return DebugTypes;
 }
 
 static std::string getMapFile(const opt::InputArgList &Args) {
   auto *Arg = Args.getLastArg(OPT_lldmap, OPT_lldmap_file);
   if (!Arg)
     return "";
   if (Arg->getOption().getID() == OPT_lldmap_file)
     return Arg->getValue();
 
   assert(Arg->getOption().getID() == OPT_lldmap);
   StringRef OutFile = Config->OutputFile;
   return (OutFile.substr(0, OutFile.rfind('.')) + ".map").str();
 }
 
 static std::string getImplibPath() {
   if (!Config->Implib.empty())
     return Config->Implib;
   SmallString<128> Out = StringRef(Config->OutputFile);
   sys::path::replace_extension(Out, ".lib");
   return Out.str();
 }
 
 //
 // The import name is caculated as the following:
 //
 //        | LIBRARY w/ ext |   LIBRARY w/o ext   | no LIBRARY
 //   -----+----------------+---------------------+------------------
 //   LINK | {value}        | {value}.{.dll/.exe} | {output name}
 //    LIB | {value}        | {value}.dll         | {output name}.dll
 //
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   if (Args.hasArg(OPT_force, OPT_force_unresolved))
     Config->ForceUnresolved = true;
 
   // Handle /force or /force:multiple
   if (Args.hasArg(OPT_force, OPT_force_multiple))
     Config->ForceMultiple = true;
 
   // Handle /debug
-  if (Args.hasArg(OPT_debug, OPT_debug_dwarf, OPT_debug_ghash)) {
+  DebugKind Debug = parseDebugKind(Args);
+  if (Debug == DebugKind::Full || Debug == DebugKind::Dwarf ||
+      Debug == DebugKind::GHash) {
     Config->Debug = true;
     Config->Incremental = true;
-    if (auto *Arg = Args.getLastArg(OPT_debugtype))
-      Config->DebugTypes = parseDebugType(Arg->getValue());
-    else
-      Config->DebugTypes = getDefaultDebugType(Args);
   }
 
+  // Handle /debugtype
+  Config->DebugTypes = parseDebugTypes(Args);
+
   // Handle /pdb
-  bool ShouldCreatePDB = Args.hasArg(OPT_debug, OPT_debug_ghash);
+  bool ShouldCreatePDB =
+      (Debug == DebugKind::Full || Debug == DebugKind::GHash);
   if (ShouldCreatePDB) {
     if (auto *Arg = Args.getLastArg(OPT_pdb))
       Config->PDBPath = Arg->getValue();
     if (auto *Arg = Args.getLastArg(OPT_pdbaltpath))
       Config->PDBAltPath = Arg->getValue();
     if (Args.hasArg(OPT_natvis))
       Config->NatvisFiles = Args.getAllArgValues(OPT_natvis);
 
     if (auto *Arg = Args.getLastArg(OPT_pdb_source_path))
       Config->PDBSourcePath = Arg->getValue();
   }
 
   // Handle /noentry
   if (Args.hasArg(OPT_noentry)) {
     if (Args.hasArg(OPT_dll))
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   for (auto *Arg : Args.filtered(OPT_incl))
     addUndefined(Arg->getValue());
 
   // Handle /implib
   if (auto *Arg = Args.getLastArg(OPT_implib))
     Config->Implib = Arg->getValue();
 
   // Handle /opt.
-  bool DoGC = !Args.hasArg(OPT_debug) || Args.hasArg(OPT_profile);
+  bool DoGC = Debug == DebugKind::None || Args.hasArg(OPT_profile);
   unsigned ICFLevel =
       Args.hasArg(OPT_profile) ? 0 : 1; // 0: off, 1: limited, 2: on
   unsigned TailMerge = 1;
   for (auto *Arg : Args.filtered(OPT_opt)) {
     std::string Str = StringRef(Arg->getValue()).lower();
     SmallVector<StringRef, 1> Vec;
     StringRef(Str).split(Vec, ',');
     for (StringRef S : Vec) {
Config->Incremental =
       Args.hasFlag(OPT_incremental, OPT_incremental_no,
                    !Config->DoGC && !Config->DoICF && !Args.hasArg(OPT_order) &&
                        !Args.hasArg(OPT_profile));
   Config->IntegrityCheck =
       Args.hasFlag(OPT_integritycheck, OPT_integritycheck_no, false);
   Config->NxCompat = Args.hasFlag(OPT_nxcompat, OPT_nxcompat_no, true);
   Config->TerminalServerAware =
       !Config->DLL && Args.hasFlag(OPT_tsaware, OPT_tsaware_no, true);
-  Config->DebugDwarf = Args.hasArg(OPT_debug_dwarf);
-  Config->DebugGHashes = Args.hasArg(OPT_debug_ghash);
-  Config->DebugSymtab = Args.hasArg(OPT_debug_symtab);
+  Config->DebugDwarf = Debug == DebugKind::Dwarf;
+  Config->DebugGHashes = Debug == DebugKind::GHash;
+  Config->DebugSymtab = Debug == DebugKind::Symtab;
 
   Config->MapFile = getMapFile(Args);
 
   if (Config->Incremental && Args.hasArg(OPT_profile)) {
     warn("ignoring '/incremental' due to '/profile' specification");
     Config->Incremental = false;
   }
 

COFF/Options.td

 def incl : Joined<["/", "-"], "include:">,
     HelpText<"Force symbol to be added to symbol table as undefined one">;
 
 // "def" is also a keyword.
 def deffile : Joined<["/", "-"], "def:">,
     HelpText<"Use module-definition file">;
 
 def debug : F<"debug">, HelpText<"Embed a symbol table in the image">;
-def debug_full : F<"debug:full">, Alias<debug>;
+def debug_opt : P<"debug", "Embed a symbol table in the image with option">;
 def debugtype : P<"debugtype", "Debug Info Options">;
 def dll : F<"dll">, HelpText<"Create a DLL">;
 def driver : P<"driver", "Generate a Windows NT Kernel Mode Driver">;
 def nodefaultlib_all : F<"nodefaultlib">;
 def noentry : F<"noentry">;
 def profile : F<"profile">;
 def repro : F<"Brepro">, HelpText<"Use a hash of the executable as the PE header timestamp">;
 def swaprun_cd : F<"swaprun:cd">;

 defm tsaware  : B<"tsaware",
                   "Create Terminal Server aware executable (default)",
                   "Create non-Terminal Server aware executable">;
 
 def help : F<"help">;
 def help_q : Flag<["/?", "-?"], "">, Alias<help>;
 
 // LLD extensions
-def debug_ghash : F<"debug:ghash">;
-def debug_dwarf : F<"debug:dwarf">;
-def debug_symtab : F<"debug:symtab">;
 def export_all_symbols : F<"export-all-symbols">;
 def kill_at : F<"kill-at">;
 def lldmingw : F<"lldmingw">;
 def output_def : Joined<["/", "-"], "output-def:">;
 def pdb_source_path : P<"pdbsourcepath",
                         "Base path used to make relative source file path absolute in PDB">;
 def rsp_quoting : Joined<["--"], "rsp-quoting=">,
   HelpText<"Quoting style for response files, 'windows' (default) or 'posix'">;

test/COFF/debug-fastlink.test

+# RUN: yaml2obj < %p/Inputs/pdb1.yaml > %t1.obj
+# RUN: yaml2obj < %p/Inputs/pdb2.yaml > %t2.obj
+
+; If /DEBUG:FASTLINK is specified, /DEBUG:FULL is used instead
+# RUN: rm -f %t.pdb
+# RUN: lld-link /DEBUG /pdb:%t.pdb /DEBUG:FASTLINK /entry:main /nodefaultlib %t1.obj %t2.obj \
+# RUN: 2>&1 | FileCheck %s
+
+# CHECK: /debug:fastlink unsupported; using /debug:full
+
+# RUN: ls %t.pdb
+

test/COFF/invalid-debug-type.test

 # RUN: yaml2obj < %p/Inputs/pdb1.yaml > %t1.obj
 # RUN: yaml2obj < %p/Inputs/pdb2.yaml > %t2.obj
 # RUN: lld-link /debug /debugtype:invalid /pdb:%t.pdb /dll /out:%t.dll /entry:main /nodefaultlib \
-# RUN:   %t1.obj %t2.obj
+# RUN:   %t1.obj %t2.obj 2>&1 | FileCheck %s
 
+# CHECK: /debugtype: unknown option: invalid
 No newline at end of file

test/COFF/invalid-debug.test

+# RUN: yaml2obj < %p/Inputs/pdb1.yaml > %t1.obj
+# RUN: yaml2obj < %p/Inputs/pdb2.yaml > %t2.obj
+# RUN: not lld-link /debug /debug:invalid /pdb:%t.pdb /dll /out:%t.dll /entry:main /nodefaultlib \
+# RUN:   %t1.obj %t2.obj 2>&1 | FileCheck %s
+
+# CHECK: /debug: unknown option: invalid

test/COFF/pdb-options.test

 # RUN: yaml2obj < %p/Inputs/pdb1.yaml > %t1.obj
 # RUN: yaml2obj < %p/Inputs/pdb2.yaml > %t2.obj
 
 ; If /DEBUG is not specified, /pdb is ignored.
 # RUN: rm -f %t.pdb
 # RUN: lld-link /pdb:%t.pdb /entry:main /nodefaultlib %t1.obj %t2.obj
 # RUN: not ls %t.pdb
 
+; If /DEBUG:NONE is specified after /DEBUG, /pdb is ignored.
+# RUN: rm -f %t.pdb
+# RUN: lld-link /DEBUG /pdb:%t.pdb /DEBUG:NONE /entry:main /nodefaultlib %t1.obj %t2.obj
+# RUN: not ls %t.pdb
+
 ; If /DEBUG and /pdb are specified, it uses the specified name.
 # RUN: lld-link /DEBUG /pdb:%t.pdb /entry:main /nodefaultlib %t1.obj %t2.obj
 # RUN: ls %t.pdb
 # RUN: rm %t.pdb
 
 ; If /DEBUG is specified but not /pdb, it uses a default name in the current
 ; directory.  This is a bit hacky since but we need to be IN our test specific
 ; temporary directory when we run this command or we can't test this
 # RUN: cd %T
 # RUN: lld-link /DEBUG /entry:main /nodefaultlib %t1.obj %t2.obj
 # RUN: ls %t1.pdb
 # RUN: rm %t*
 # RUN: cd %T/..