[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

500

Why does the name contani "Generation"?

864

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

500

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.

864

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

[lantictac]

Ping

[ruiu]

COFF/Driver.cpp

510

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.

547

nit: no else after return.

548

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

864

DebugArg -> A for consistency.

869

Let's do this inside parseDebugKind.

COFF/Options.td

93

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

539

Please avoid using auto unless its actual type is obvious.

541

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

544

You probably should warn on unknown string.

549

Return early.

552

Just return DebugKind::Full.

555

Ditto

863–864

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

863–864

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.

865

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

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

[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

863–864

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

557

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

557

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

557

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();
 }
 
+enum class DebugGeneration {
+  Unknown,
+  None,
+  Full,
+  FastLink,
+  GHash,
+  Dwarf,
+  Symtab
+};
+
+static DebugGeneration parseDebugGeneration(StringRef Arg) {
+  return StringSwitch<DebugGeneration>(Arg.lower())
+      .Case("none", DebugGeneration::None)
+      .Case("full", DebugGeneration::Full)
+      .Case("fastlink", DebugGeneration::FastLink)
+      .Case("ghash", DebugGeneration::GHash)
+      .Case("dwarf", DebugGeneration::Dwarf)
+      .Case("symtab", DebugGeneration::Symtab)
+      .Default(DebugGeneration::Unknown);
+}
+
 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;
 }
 
 static unsigned parseDebugType(StringRef Arg) {
   SmallVector<StringRef, 3> Types;
   Arg.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);
   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:
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   if (Args.hasArg(OPT_verbose))
     Config->Verbose = true;
   errorHandler().Verbose = Config->Verbose;
 
   // Handle /force or /force:unresolved
   if (Args.hasArg(OPT_force, OPT_force_unresolved))
     Config->Force = true;
 
   // Handle /debug
-  if (Args.hasArg(OPT_debug, OPT_debug_dwarf, OPT_debug_ghash)) {
+  DebugGeneration DebugGen = DebugGeneration::None;
+  bool ShouldCreatePDB = false;
+  if (auto *DebugArg = Args.getLastArg(OPT_debug, OPT_debug_opt)) {
+    DebugGen = DebugGeneration::Full;
+    if (DebugArg->getNumValues() != 0) {
+      DebugGen = parseDebugGeneration(DebugArg->getValue());
+      if (DebugGen == DebugGeneration::FastLink) {
+        warn("/debug:fastlink unsupported; using /debug:full");
+        DebugGen = DebugGeneration::Full;
+      } else if (DebugGen == DebugGeneration::Unknown)
+        error("/debug: unknown option: " + Twine(DebugArg->getValue()));
+    }
+
+    if (DebugGen == DebugGeneration::Full ||
+        DebugGen == DebugGeneration::Dwarf ||
+        DebugGen == DebugGeneration::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 /pdb
-  bool ShouldCreatePDB = Args.hasArg(OPT_debug, OPT_debug_ghash);
+      ShouldCreatePDB = DebugGen == DebugGeneration::Full ||
+                        DebugGen == DebugGeneration::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))
       Config->NoEntry = true;
     else
       error("/noentry must be specified with /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 = DebugGen == DebugGeneration::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 = DebugGen == DebugGeneration::Dwarf;
+  Config->DebugGHashes = DebugGen == DebugGeneration::GHash;
+  Config->DebugSymtab = DebugGen == DebugGeneration::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

 include "llvm/Option/OptParser.td"
 
 // link.exe accepts options starting with either a dash or a slash.
 
 // Flag that takes no arguments.
 class F<string name> : Flag<["/", "-", "-?"], name>;
 
 // Flag that takes one argument after ":".
 class P<string name, string help> :
       Joined<["/", "-", "-?"], name#":">, HelpText<help>;
 
+// Flag that may take optional argument after ":".
+multiclass POpt<string name, string help> {
+  def NAME: F<name>;
+  def NAME # _opt: P<name, help>;
+}
+
 // Boolean flag which can be suffixed by ":no". Using it unsuffixed turns the
 // flag on and using it suffixed by ":no" turns it off.
 multiclass B<string name, string help_on, string help_off> {
   def "" : F<name>, HelpText<help_on>;
   def _no : F<name#":no">, HelpText<help_off>;
 }
 
 def align   : P<"align", "Section alignment">;

 // a reserved keyword in tablegen.
 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>;
+defm debug : POpt<"debug", "Embed a symbol table in the image">;
 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/..