[LLD] [COFF] Rewrite handling of the /debug: option. NFC.

[mstorsjo]

Don't treat the options as unique enum items, but more as flags that can be composed, like the /opt: options.

This still only processes the last option on the command line though, so the behaviour should still remain exactly as it was, in all corner cases.

[llvmbot]

@llvm/pr-subscribers-lld
@llvm/pr-subscribers-lld-coff

@llvm/pr-subscribers-platform-windows

Author: Martin Storsjö (mstorsjo)

Changes

Don't treat the options as unique enum items, but more as flags that can be composed, like the /opt: options.

This still only processes the last option on the command line though, so the behaviour should still remain exactly as it was, in all corner cases.

---

Full diff: https://github.com/llvm/llvm-project/pull/75175.diff

4 Files Affected:

• (modified) lld/COFF/Config.h (+2-2)
• (modified) lld/COFF/Driver.cpp (+42-57)
• (modified) lld/COFF/InputFiles.cpp (+2-2)
• (modified) lld/COFF/Writer.cpp (+1-1)

diff --git a/lld/COFF/Config.h b/lld/COFF/Config.h
index 24126f635a06f..48c48cefe91d8 100644
--- a/lld/COFF/Config.h
+++ b/lld/COFF/Config.h
@@ -130,9 +130,9 @@ struct Configuration {
   bool forceMultipleRes = false;
   bool forceUnresolved = false;
   bool debug = false;
-  bool debugDwarf = false;
+  bool includeDwarfChunks = false;
   bool debugGHashes = false;
-  bool debugSymtab = false;
+  bool writeSymtab = false;
   bool driver = false;
   bool driverUponly = false;
   bool driverWdm = false;
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 99c1a60735adc..2b0959199e545 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -858,46 +858,6 @@ static std::string createResponseFile(const opt::InputArgList &args,
   return std::string(data.str());
 }
 
-enum class DebugKind {
-  Unknown,
-  None,
-  Full,
-  FastLink,
-  GHash,
-  NoGHash,
-  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("noghash", DebugKind::NoGHash)
-                        .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);
 
@@ -1647,12 +1607,47 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   if (args.hasArg(OPT_force, OPT_force_multipleres))
     config->forceMultipleRes = true;
 
+  // Don't warn about long section names, such as .debug_info, for mingw (or
+  // when -debug:dwarf is requested, handled below).
+  if (config->mingw)
+    config->warnLongSectionNames = false;
+
+  bool doGC = true;
+
   // Handle /debug
-  DebugKind debug = parseDebugKind(args);
-  if (debug == DebugKind::Full || debug == DebugKind::Dwarf ||
-      debug == DebugKind::GHash || debug == DebugKind::NoGHash) {
-    config->debug = true;
-    config->incremental = true;
+  bool shouldCreatePDB = false;
+  if (auto *arg = args.getLastArg(OPT_debug, OPT_debug_opt)) {
+    std::string s;
+    if (arg->getOption().getID() == OPT_debug)
+      s = "full";
+    else
+      s = StringRef(arg->getValue()).lower();
+    if (s == "fastlink") {
+      warn("/debug:fastlink unsupported; using /debug:full");
+      s = "full";
+    }
+    if (s == "none") {
+    } else if (s == "full" || s == "ghash" || s == "noghash") {
+      config->debug = true;
+      config->incremental = true;
+      config->includeDwarfChunks = true;
+      if (s == "full" || s == "ghash")
+        config->debugGHashes = true;
+      shouldCreatePDB = true;
+      doGC = false;
+    } else if (s == "dwarf") {
+      config->debug = true;
+      config->incremental = true;
+      config->includeDwarfChunks = true;
+      config->writeSymtab = true;
+      config->warnLongSectionNames = false;
+      doGC = false;
+    } else if (s == "symtab") {
+      config->writeSymtab = true;
+      doGC = false;
+    } else {
+      error("/debug: unknown option: " + Twine(arg->getValue()));
+    }
   }
 
   // Handle /demangle
@@ -1672,9 +1667,6 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
       config->driverUponly || config->driverWdm || args.hasArg(OPT_driver);
 
   // Handle /pdb
-  bool shouldCreatePDB =
-      (debug == DebugKind::Full || debug == DebugKind::GHash ||
-       debug == DebugKind::NoGHash);
   if (shouldCreatePDB) {
     if (auto *arg = args.getLastArg(OPT_pdb))
       config->pdbPath = arg->getValue();
@@ -1837,8 +1829,9 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
 
   config->noimplib = args.hasArg(OPT_noimplib);
 
+  if (args.hasArg(OPT_profile))
+    doGC = true;
   // Handle /opt.
-  bool doGC = debug == DebugKind::None || args.hasArg(OPT_profile);
   std::optional<ICFLevel> icfLevel;
   if (args.hasArg(OPT_profile))
     icfLevel = ICFLevel::None;
@@ -2028,9 +2021,6 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
     parseSwaprun(arg->getValue());
   config->terminalServerAware =
       !config->dll && args.hasFlag(OPT_tsaware, OPT_tsaware_no, true);
-  config->debugDwarf = debug == DebugKind::Dwarf;
-  config->debugGHashes = debug == DebugKind::GHash || debug == DebugKind::Full;
-  config->debugSymtab = debug == DebugKind::Symtab;
   config->autoImport =
       args.hasFlag(OPT_auto_import, OPT_auto_import_no, config->mingw);
   config->pseudoRelocs = args.hasFlag(
@@ -2047,11 +2037,6 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
   if (args.hasFlag(OPT_inferasanlibs, OPT_inferasanlibs_no, false))
     warn("ignoring '/inferasanlibs', this flag is not supported");
 
-  // Don't warn about long section names, such as .debug_info, for mingw or
-  // when -debug:dwarf is requested.
-  if (config->mingw || config->debugDwarf)
-    config->warnLongSectionNames = false;
-
   if (config->incremental && args.hasArg(OPT_profile)) {
     warn("ignoring '/incremental' due to '/profile' specification");
     config->incremental = false;
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index dd2e1419bb10a..cd744800cb0de 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -236,8 +236,8 @@ SectionChunk *ObjFile::readSection(uint32_t sectionNumber,
   // CodeView needs linker support. We need to interpret debug info,
   // and then write it to a separate .pdb file.
 
-  // Ignore DWARF debug info unless /debug is given.
-  if (!ctx.config.debug && name.starts_with(".debug_"))
+  // Ignore DWARF debug info unless requested to be included.
+  if (!ctx.config.includeDwarfChunks && name.starts_with(".debug_"))
     return nullptr;
 
   if (sec->Characteristics & llvm::COFF::IMAGE_SCN_LNK_REMOVE)
diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index 7b1ff8071e2e3..ec5d10ed99032 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -1376,7 +1376,7 @@ void Writer::createSymbolAndStringTable() {
     sec->setStringTableOff(addEntryToStringTable(sec->name));
   }
 
-  if (ctx.config.debugDwarf || ctx.config.debugSymtab) {
+  if (ctx.config.writeSymtab) {
     for (ObjFile *file : ctx.objFileInstances) {
       for (Symbol *b : file->getSymbols()) {
         auto *d = dyn_cast_or_null<Defined>(b);

[mstorsjo]

This goes on top of #75172; only the latter commit should be reviewed in this PR. The overall direction of the larger patchset that this belongs to, was presented/discussed in #74820.

[ZequanWu]

Maybe make doGC default to false and set to true here. So the following doGC = false can be removed.

[mstorsjo]

But if there's no /debug: option at all, then we'd go with the initial value of doGC - which should be true for that case.

[mstorsjo]

@ZequanWu Can you follow up here?

[ZequanWu]

Oh, I missed that. That makes sense.

[ZequanWu]

Oh, I missed that. That makes sense.