[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"?

946

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.

946

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.

567

nit: no else after return.

568

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

929

DebugArg -> A for consistency.

934

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

520

Please avoid using auto unless its actual type is obvious.

522

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

533

Just return DebugKind::Full.

536

Ditto

547

You probably should warn on unknown string.

552

Return early.

928

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

930

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

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

935–936

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

940

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

545

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

545

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

545

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;
+
+  auto V = StringRef(A->getValue()).lower();
+  DebugKind DK = StringSwitch<DebugKind>(V)
+                     .Case("none", DebugKind::None)
+                     .Case("full", DebugKind::Full)
+                     .Case("fastlink", DebugKind::FastLink)
+                     // LLD extensions
+                     .Case("ghash", DebugKind::GHash)
+                     .Case("dwarf", DebugKind::Dwarf)
+                     .Case("symtab", DebugKind::Symtab)
+                     .Default(DebugKind::Unknown);
+
+  if (DK == DebugKind::FastLink) {
+    warn("/debug:fastlink unsupported; using /debug:full");
+    DK = DebugKind::Full;
+  } else if (DK == DebugKind::Unknown) {
+    error("/debug: unknown option: " + Twine(A->getValue()));
+    DK = DebugKind::None;
+  }
+  return DK;
 }
 
-static unsigned parseDebugType(StringRef Arg) {
+static unsigned parseDebugTypes(const opt::InputArgList &Args) {
+  unsigned DebugTypes = static_cast<unsigned>(DebugType::None);
+  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())
                         .Case("cv", static_cast<unsigned>(DebugType::CV))
                         .Case("pdata", static_cast<unsigned>(DebugType::PData))
                         .Case("fixup", static_cast<unsigned>(DebugType::Fixup))
                         .Default(0);
+  } else {
+    // 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() {
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 DK = parseDebugKind(Args);
+
+  bool ShouldCreatePDB = false;
+  if (DK == DebugKind::Full || DK == DebugKind::Dwarf ||
+      DK == 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);
+    Config->DebugTypes = parseDebugTypes(Args);
+    ShouldCreatePDB = DK != DebugKind::Dwarf;
   }
 
   // Handle /pdb
-  bool ShouldCreatePDB = Args.hasArg(OPT_debug, OPT_debug_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 = DK == 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 = DK == DebugKind::Dwarf;
+  Config->DebugGHashes = DK == DebugKind::GHash;
+  Config->DebugSymtab = DK == 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.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/..