[COFF] Clean up debug option handling

[smeenai]

Summary

/debug and /debug:dwarf are orthogonal. An object file can contain both
CodeView and DWARF debug info, so the combination of /debug:dwarf and
/debug should generate both DWARF and a PDB, rather than /debug:dwarf
always suppressing PDB creation.

/nopdb is now redundant and can be removed. /debug /nopdb was previously
used to support DWARF, but specifying /debug:dwarf is entirely
equivalent to that combination now.

[ruiu]

I think this change makes sense. LGTM

[zturner]

Is there any reason to have more than one option that begins with /DEBUG? Maybe they should all be mutually exclusive.

[ruiu]

Conceptually, an executable that has both DAWRF and PDB debug info is valid, no?

[smeenai]

@zturner I do agree that /debug and /debug:dwarf is a bit confusing. We have to keep /debug for link.exe compatibility, so perhaps /debug:dwarf could be renamed to something else? Any suggestions?

[smeenai]

In D41310#957239, @ruiu wrote:

Conceptually, an executable that has both DAWRF and PDB debug info is valid, no?

That's true in theory, but the current implementation assumes they're mutually exclusive (e.g. PDBs aren't generated for /debug:dwarf).

I don't know how much utility there is for DWARF + PDB. I also though clang could only generate one or the other (i.e. if you specify -gcodeview you'll only get CodeView and not DWARF), though I may be mistaken there.

[zturner]

In D41310#957241, @smeenai wrote:

@zturner I do agree that /debug and /debug:dwarf is a bit confusing. We have to keep /debug for link.exe compatibility, so perhaps /debug:dwarf could be renamed to something else? Any suggestions?

Sorry, I didn't mean the linker shouldn't support more than one option, I just meant it shouldn't supporting using them at the same time. This agrees with what you were saying about /debug and /debug:dwarf not making sense when used together, but actually goes one step further. Do any two /DEBUG option variants make sense together?

[ruiu]

I don't find /debug:dwarf that odd. link.exe accepts /debug:{full,fastlink,none}, and some of them collide (e.g. /debug vs. /debug:none), some are not (e.g. /debug vs. /debug:full).

[smeenai]

So it looks like clang produces both CodeView and DWARF in the same object file if you pass both -gcodeview and -gdwarf. I haven't verified the debug info is sensible, but the object file at least contains both CodeView and DWARF debug sections.

With that in mind, I think the following makes sense:

1. /debug by itself produces a PDB (this is unchanged).
2. /debug:dwarf by itself produces DWARF (this is unchanged).
3. /debug and /debug:dwarf produce both a PDB and DWARF (this is different from today, where /debug:dwarf always prevents a PDB from being created).
4. /nopdb can still be deleted; just don't specify /debug if you don't want a PDB (or perhaps we can add /debug:none to cancel out /debug, but then it's confusing as to whether it should also cancel out /debug:dwarf ...)

Does this sound good to everyone?

[smeenai]

Update after discussion

[smeenai]

This has changed significantly since the last iteration.

[ruiu]

test/COFF/debug-dwarf.test

4 ↗

(On Diff #127201)

Is it guaranteed that ls exits with a non-zero value if a given file does not exist?

[smeenai]

test/COFF/debug-dwarf.test

4 ↗

(On Diff #127201)

I'm basing this on the existing nopdb.test. The same pattern is also used in pdb-options.test.

[ruiu]

LGTM

test/COFF/debug-dwarf.test

4 ↗

(On Diff #127201)

Thanks. Then it should be fine as it's already tested.

lld/trunk/COFF/Driver.cpp

if (Args.hasArg(OPT_debug, OPT_debug_dwarf, OPT_debug_ghash)) {
     Config->Debug = 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);
+  if (ShouldCreatePDB)
     if (auto *Arg = Args.getLastArg(OPT_pdb))
       Config->PDBPath = 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) {
 
   // Set default image name if neither /out or /def set it.
   if (Config->OutputFile.empty()) {
     Config->OutputFile =
         getOutputPath((*Args.filtered(OPT_INPUT).begin())->getValue());
   }
 
   // Put the PDB next to the image if no /pdb flag was passed.
-  if (Config->Debug && Config->PDBPath.empty()) {
+  if (ShouldCreatePDB && Config->PDBPath.empty()) {
     Config->PDBPath = Config->OutputFile;
     sys::path::replace_extension(Config->PDBPath, ".pdb");
   }
 
-  // Disable PDB generation if the user requested it.
-  if (Args.hasArg(OPT_nopdb, OPT_debug_dwarf))
-    Config->PDBPath = "";
-
   // Set default image base if /base is not given.
   if (Config->ImageBase == uint64_t(-1))
     Config->ImageBase = getDefaultImageBase();
 
   Symtab->addSynthetic(mangle("__ImageBase"), nullptr);
   if (Config->Machine == I386) {
     Symtab->addAbsolute("___safe_se_handler_table", 0);
     Symtab->addAbsolute("___safe_se_handler_count", 0);

lld/trunk/COFF/Options.td

 
 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 export_all_symbols : F<"export-all-symbols">;
-def nopdb : F<"nopdb">, HelpText<"Disable PDB generation for DWARF users">;
 def lldmingw : F<"lldmingw">;
 def msvclto : F<"msvclto">;
 def output_def : Joined<["/", "-"], "output-def:">;
 def rsp_quoting : Joined<["--"], "rsp-quoting=">,
   HelpText<"Quoting style for response files, 'windows' (default) or 'posix'">;
 def dash_dash_version : Flag<["--"], "version">,
   HelpText<"Print version information">;
 

lld/trunk/test/COFF/debug-dwarf.test

+# Check that /debug creates %t.pdb.
+# RUN: rm -f %t.pdb
+# RUN: lld-link /debug /entry:main /out:%t.exe %p/Inputs/ret42.obj
+# RUN: ls %t.pdb
+
+# Check that /debug:dwarf does not create %t.pdb.
+# RUN: rm -f %t.pdb
+# RUN: lld-link /debug:dwarf /entry:main /out:%t.exe %p/Inputs/ret42.obj
+# RUN: not ls %t.pdb
+
+# Check that /debug:dwarf /debug creates %t.pdb.
+# RUN: rm -f %t.pdb
+# RUN: lld-link /debug:dwarf /debug /entry:main /out:%t.exe %p/Inputs/ret42.obj
+# RUN: ls %t.pdb
+
+# Check that /debug:dwarf /pdb:%t.pdb does not create %t.pdb.
+# RUN: rm -f %t.pdb
+# RUN: lld-link /debug:dwarf /pdb:%t.pdb /entry:main /out:%t.exe %p/Inputs/ret42.obj
+# RUN: not ls %t.pdb

lld/trunk/test/COFF/nopdb.test

-# Check that /debug creates %t.pdb.
-# RUN: rm -f %t.pdb
-# RUN: lld-link /debug /entry:main /out:%t.exe %p/Inputs/ret42.obj
-# RUN: ls %t.pdb
-
-# Check that /debug /nopdb does not create %t.pdb.
-# RUN: rm -f %t.pdb
-# RUN: lld-link /debug /nopdb /entry:main /out:%t.exe %p/Inputs/ret42.obj
-# RUN: not ls %t.pdb
-
-# Check that /debug /nopdb /pdb:%t.pdb does not create %t.pdb.
-# RUN: rm -f %t.pdb
-# RUN: lld-link /debug /nopdb /pdb:%t.pdb /entry:main /out:%t.exe %p/Inputs/ret42.obj
-# RUN: not ls %t.pdb