[COFF] Clean up boolean flag handling

[smeenai]

Summary

LLD's handling of boolean flags is suboptimal:

• All boolean flags have a corresponding :no flag to turn the flag off, and the linker should scan for both the non-suffixed and suffixed flags (and the last one should win), but right now it only scans for either the suffixed or non-suffixed flag (depending on the default flag value).
• The B multiclass only allows specifying help text for the suffixed (:no) flag, but for some flags (e.g. /appcontainer) the help text should be associated with the non-suffixed flag instead.

Extend the B multiclass to have help text for both non-suffixed and
suffixed flag variants, and alter the existing help text accordingly in
some cases. Scan for both the non-suffixed and suffixed variants in the
driver and set config values accordingly.

This should mostly have no behavior change, apart from the added help
text and the modified argument scanning. Some flags are handled slightly
differently now, however; for example, LLD would previously always treat
64-bit images as large address aware, whereas /largeaddressaware:no is
now respected for 64-bit images (which is also how link.exe behaves).

[ruiu]

LGTM

lld/trunk/COFF/Driver.cpp

void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   }
 
   // Handle /dll
   if (Args.hasArg(OPT_dll)) {
     Config->DLL = true;
     Config->ManifestID = 2;
   }
 
-  // Handle /fixed
-  if (Args.hasArg(OPT_fixed)) {
-    if (Args.hasArg(OPT_dynamicbase)) {
+  // Handle /dynamicbase and /fixed. We can't use hasFlag for /dynamicbase
+  // because we need to explicitly check whether that option or its inverse was
+  // present in the argument list in order to handle /fixed.
+  auto *DynamicBaseArg = Args.getLastArg(OPT_dynamicbase, OPT_dynamicbase_no);
+  if (DynamicBaseArg &&
+      DynamicBaseArg->getOption().getID() == OPT_dynamicbase_no)
+    Config->DynamicBase = false;
+
+  bool Fixed = Args.hasFlag(OPT_fixed, OPT_fixed_no, false);
+  if (Fixed) {
+    if (DynamicBaseArg &&
+        DynamicBaseArg->getOption().getID() == OPT_dynamicbase) {
       error("/fixed must not be specified with /dynamicbase");
     } else {
       Config->Relocatable = false;
       Config->DynamicBase = false;
     }
   }
 
-  if (Args.hasArg(OPT_appcontainer))
-    Config->AppContainer = true;
+  // Handle /appcontainer
+  Config->AppContainer =
+      Args.hasFlag(OPT_appcontainer, OPT_appcontainer_no, false);
 
   // Handle /machine
   if (auto *Arg = Args.getLastArg(OPT_machine))
     Config->Machine = getMachineType(Arg->getValue());
 
   // Handle /nodefaultlib:<filename>
   for (auto *Arg : Args.filtered(OPT_nodefaultlib))
     Config->NoDefaultLibs.insert(doFindLib(Arg->getValue()));
for (auto *Arg : Args.filtered(OPT_manifestinput))
     Config->ManifestInput.push_back(Arg->getValue());
 
   if (!Config->ManifestInput.empty() &&
       Config->Manifest != Configuration::Embed) {
     fatal("/MANIFESTINPUT: requires /MANIFEST:EMBED");
   }
 
   // Handle miscellaneous boolean flags.
-  if (Args.hasArg(OPT_allowbind_no))
-    Config->AllowBind = false;
-  if (Args.hasArg(OPT_allowisolation_no))
-    Config->AllowIsolation = false;
-  if (Args.hasArg(OPT_dynamicbase_no))
-    Config->DynamicBase = false;
-  if (Args.hasArg(OPT_nxcompat_no))
-    Config->NxCompat = false;
-  if (Args.hasArg(OPT_tsaware_no))
-    Config->TerminalServerAware = false;
+  Config->AllowBind = Args.hasFlag(OPT_allowbind, OPT_allowbind_no, true);
+  Config->AllowIsolation =
+      Args.hasFlag(OPT_allowisolation, OPT_allowisolation_no, true);
+  Config->NxCompat = Args.hasFlag(OPT_nxcompat, OPT_nxcompat_no, true);
+  Config->TerminalServerAware = Args.hasFlag(OPT_tsaware, OPT_tsaware_no, true);
   if (Args.hasArg(OPT_nosymtab))
     Config->WriteSymtab = false;
 
   Config->MapFile = getMapFile(Args);
 
   if (ErrorCount)
     return;
 
if (!Resources.empty())
     Symtab->addFile(make<ObjFile>(convertResToCOFF(Resources)));
 
   if (Tar)
     Tar->append("response.txt",
                 createResponseFile(Args, FilePaths,
                                    ArrayRef<StringRef>(SearchPaths).slice(1)));
 
   // Handle /largeaddressaware
-  if (Config->is64() || Args.hasArg(OPT_largeaddressaware))
-    Config->LargeAddressAware = true;
+  Config->LargeAddressAware = Args.hasFlag(
+      OPT_largeaddressaware, OPT_largeaddressaware_no, Config->is64());
 
   // Handle /highentropyva
-  if (Config->is64() && !Args.hasArg(OPT_highentropyva_no))
-    Config->HighEntropyVA = true;
+  Config->HighEntropyVA =
+      Config->is64() &&
+      Args.hasFlag(OPT_highentropyva, OPT_highentropyva_no, true);
 
   // Handle /entry and /dll
   if (auto *Arg = Args.getLastArg(OPT_entry)) {
     Config->Entry = addUndefined(mangle(Arg->getValue()));
   } else if (Args.hasArg(OPT_dll) && !Config->NoEntry) {
     StringRef S = (Config->Machine == I386) ? "__DllMainCRTStartup@12"
                                             : "_DllMainCRTStartup";
     Config->Entry = addUndefined(S);
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   // that from entry point name.
   if (Config->Subsystem == IMAGE_SUBSYSTEM_UNKNOWN) {
     Config->Subsystem = inferSubsystem();
     if (Config->Subsystem == IMAGE_SUBSYSTEM_UNKNOWN)
       fatal("subsystem must be defined");
   }
 
   // Handle /safeseh.
-  if (Args.hasArg(OPT_safeseh)) {
+  if (Args.hasFlag(OPT_safeseh, OPT_safeseh_no, false)) {
     for (ObjFile *File : ObjFile::Instances)
       if (!File->SEHCompat)
         error("/safeseh: " + File->getName() + " is not compatible with SEH");
     if (ErrorCount)
       return;
   }
 
   // In MinGW, all symbols are automatically exported if no symbols

lld/trunk/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>;
 
-// Boolean flag suffixed by ":no".
-multiclass B<string name, string help> {
-  def "" : F<name>;
-  def _no : F<name#":no">, HelpText<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">;
 def aligncomm : P<"aligncomm", "Set common symbol alignment">;
 def alternatename : P<"alternatename", "Define weak alias">;
 def base    : P<"base", "Base address of the program">;
 def defaultlib : P<"defaultlib", "Add the library to the list of input files">;
 def delayload : P<"delayload", "Delay loaded DLL name">;

 def swaprun_net : F<"swaprun:net">;
 def verbose : F<"verbose">;
 def wholearchive_flag : F<"wholearchive">;
 
 def force : F<"force">,
     HelpText<"Allow undefined symbols when creating executables">;
 def force_unresolved : F<"force:unresolved">;
 
-defm allowbind: B<"allowbind", "Disable DLL binding">;
-defm allowisolation : B<"allowisolation", "Set NO_ISOLATION bit">;
+defm allowbind : B<"allowbind", "Enable DLL binding (default)",
+                   "Disable DLL binding">;
+defm allowisolation : B<"allowisolation", "Enable DLL isolation (default)",
+                        "Disable DLL isolation">;
 defm appcontainer : B<"appcontainer",
-                      "Image can only be run in an app container">;
-defm dynamicbase : B<"dynamicbase",
-                     "Disable address space layout randomization">;
-defm fixed    : B<"fixed", "Enable base relocations">;
-defm highentropyva : B<"highentropyva", "Set HIGH_ENTROPY_VA bit">;
-defm largeaddressaware : B<"largeaddressaware", "Disable large addresses">;
-defm nxcompat : B<"nxcompat", "Disable data execution provention">;
-defm safeseh : B<"safeseh", "Produce an image with Safe Exception Handler">;
-defm tsaware  : B<"tsaware", "Create non-Terminal Server aware executable">;
+                      "Image can only be run in an app container",
+                      "Image can run outside an app container (default)">;
+defm dynamicbase : B<"dynamicbase", "Enable ASLR (default unless /fixed)",
+                     "Disable ASLR (default when /fixed)">;
+defm fixed : B<"fixed", "Disable base relocations",
+               "Enable base relocations (default)">;
+defm highentropyva : B<"highentropyva",
+                       "Enable 64-bit ASLR (default on 64-bit)",
+                       "Disable 64-bit ASLR">;
+defm largeaddressaware : B<"largeaddressaware",
+                           "Enable large addresses (default on 64-bit)",
+                           "Disable large addresses (default on 32-bit)">;
+defm nxcompat : B<"nxcompat", "Enable data execution prevention (default)",
+                  "Disable data execution provention">;
+defm safeseh : B<"safeseh",
+                 "Produce an image with Safe Exception Handler (only for x86)",
+                 "Don't produce an image with Safe Exception Handler">;
+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 export_all_symbols : F<"export-all-symbols">;
 def nopdb : F<"nopdb">, HelpText<"Disable PDB generation for DWARF users">;
 def nosymtab : F<"nosymtab">;