Make -defsym a driver option

[salari01]

Summary

Extended the integrated assembler -Wa,-defsym option to be usable with the Clang driver. Both options are handled in the same way. Using -defsym when not assembling files shows an unused argument warning.

[rogfer01]

lib/Driver/ToolChains/Clang.cpp

1882

I wonder if you should break; here if validation fails like the original code did?

[salari01]

lib/Driver/ToolChains/Clang.cpp

1882

The reason for removing it is that this way, all erroneous -defsym options specified can be listed during the same invocation. This provides a better user experience than tackling them one at a time, as with the original code.

[rogfer01]

lib/Driver/ToolChains/Clang.cpp

1882

Ah, I see, thanks!

[sanwou01]

Hi Salman,

This essentially looks good to me. I don't think I'm the right person to LGTM this however, so I've added some reviewers who might be able to have a look.

Thanks,
Sanne

[rnk]

-Wl,--defsym is also a pretty common linker option. I'm hesitant to add this to the driver interface, since it seems like they could easily be confused.

include/clang/Driver/Options.td

546

I'm pretty sure this shouldn't be in "d_Group":

def d_Group : OptionGroup<"<d group>">, Group<Preprocessor_Group>,
              DocName<"Dumping preprocessor state">, DocBrief<[{
Flags allowing the state of the preprocessor to be dumped in various ways.}]>;

[salari01]

In D31496#721262, @rnk wrote:

-Wl,--defsym is also a pretty common linker option. I'm hesitant to add this to the driver interface, since it seems like they could easily be confused.

I see. Was not aware of that linker option. Having looked it up now, I agree that the risk of confusion with the linker option outweighs the convenience of having this as a Driver option. I'll adandon this revision.

include/clang/Driver/CC1Options.td

def show_encoding : Flag<["-"], "show-encoding">,
     HelpText<"Show instruction encoding information in transliterate mode">;
 def show_inst : Flag<["-"], "show-inst">,
     HelpText<"Show internal instruction representation in transliterate mode">;
 
 // Assemble Options
 def dwarf_debug_producer : Separate<["-"], "dwarf-debug-producer">,
   HelpText<"The string to embed in the Dwarf debug AT_producer record.">;
 
-def defsym : Separate<["-"], "defsym">,
-  HelpText<"Define a value for a symbol">;
 } // let Flags = [CC1AsOption]

include/clang/Driver/Options.td

 def dA : Flag<["-"], "dA">, Group<d_Group>;
 def dD : Flag<["-"], "dD">, Group<d_Group>, Flags<[CC1Option]>,
   HelpText<"Print macro definitions in -E mode in addition to normal output">;
 def dI : Flag<["-"], "dI">, Group<d_Group>, Flags<[CC1Option]>,
   HelpText<"Print include directives in -E mode in addition to normal output">;
 def dM : Flag<["-"], "dM">, Group<d_Group>, Flags<[CC1Option]>,
   HelpText<"Print macro definitions in -E mode instead of normal output">;
 def dead__strip : Flag<["-"], "dead_strip">;
+def defsym : Separate<["-"], "defsym">, Group<d_Group>, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Define an assembly-time variable with the given name and value">;
 def dependency_file : Separate<["-"], "dependency-file">, Flags<[CC1Option]>,
   HelpText<"Filename (or -) to write dependency output to">;
 def dependency_dot : Separate<["-"], "dependency-dot">, Flags<[CC1Option]>,
   HelpText<"Filename to write DOT-formatted header dependencies to">;
 def module_dependency_dir : Separate<["-"], "module-dependency-dir">,
   Flags<[CC1Option]>, HelpText<"Directory to dump module dependencies to">;
 def dumpmachine : Flag<["-"], "dumpmachine">;
 def dumpspecs : Flag<["-"], "dumpspecs">, Flags<[Unsupported]>;

lib/Driver/ToolChains/Clang.cpp

for (unsigned i = 0, e = Target.size(); i != e; ++i) {
     default:
       break;
     }
 
     Res.push_back(Target[i]);
   }
 }
 
+// Validate the value to a -defsym option
+static bool validateDefsym(const char *S, const Driver &D) {
+  auto Pair = StringRef(S).split('=');
+  auto Sym = Pair.first;
+  auto SVal = Pair.second;
+
+  if (Sym.empty() || SVal.empty()) {
+    D.Diag(diag::err_drv_defsym_invalid_format) << S;
+    return false;
+  }
+  int64_t IVal;
+  if (SVal.getAsInteger(0, IVal)) {
+    D.Diag(diag::err_drv_defsym_invalid_symval) << SVal;
+    return false;
+  }
+  return true;
+}
+
 /// Apply \a Work on the current tool chain \a RegularToolChain and any other
 /// offloading tool chain that is associated with the current action \a JA.
 static void
 forAllAssociatedToolChains(Compilation &C, const JobAction &JA,
                            const ToolChain &RegularToolChain,
                            llvm::function_ref<void(const ToolChain &)> Work) {
   // Apply Work on the current/regular tool chain.
   Work(RegularToolChain);
static void CollectArgsForIntegratedAssembler(Compilation &C,
 
   // When passing -I arguments to the assembler we sometimes need to
   // unconditionally take the next argument.  For example, when parsing
   // '-Wa,-I -Wa,foo' we need to accept the -Wa,foo arg after seeing the
   // -Wa,-I arg and when parsing '-Wa,-I,foo' we need to accept the 'foo'
   // arg after parsing the '-I' arg.
   bool TakeNextArg = false;
 
+  // Handle the top-level implementation of the integrated assembler
+  // -defsym option
+  for (const Arg *A : Args.filtered(options::OPT_defsym)) {
+    A->claim();
+    if (A->getNumValues() != 1) {
+      D.Diag(diag::err_drv_defsym_invalid_format) << A->getAsString(Args);
+      break;
+    }
+    const char *S = A->getValue(0);
+    if (validateDefsym(S, D)) {
+      CmdArgs.push_back("-defsym");
+      CmdArgs.push_back(S);
+    }
+  }
+
   // When using an integrated assembler, translate -Wa, and -Xassembler
   // options.
   bool CompressDebugSections = false;
 
   bool UseRelaxRelocations = ENABLE_X86_RELAX_RELOCATIONS;
   const char *MipsTargetFeature = nullptr;
   for (const Arg *A :
        Args.filtered(options::OPT_Wa_COMMA, options::OPT_Xassembler)) {
for (StringRef Value : A->getValues()) {
                  Value.startswith("-mhwdiv") || Value.startswith("-march")) {
         // Do nothing, we'll validate it later.
       } else if (Value == "-defsym") {
           if (A->getNumValues() != 2) {
             D.Diag(diag::err_drv_defsym_invalid_format) << Value;
             break;
           }
           const char *S = A->getValue(1);
-          auto Pair = StringRef(S).split('=');
-          auto Sym = Pair.first;
-          auto SVal = Pair.second;
-
-          if (Sym.empty() || SVal.empty()) {
-            D.Diag(diag::err_drv_defsym_invalid_format) << S;
-            break;
-          }
-          int64_t IVal;
-          if (SVal.getAsInteger(0, IVal)) {
-            D.Diag(diag::err_drv_defsym_invalid_symval) << SVal;
-            break;
-          }
+          if (validateDefsym(S, D)) {
             CmdArgs.push_back(Value.data());
             TakeNextArg = true;
+          }
       } else {
         D.Diag(diag::err_drv_unsupported_option_argument)
             << A->getOption().getName() << Value;
       }
     }
   }
   if (CompressDebugSections) {
     if (llvm::zlib::isAvailable())

lib/Frontend/CompilerInvocation.cpp

bool CompilerInvocation::CreateFromArgs(CompilerInvocation &Res,
 
   // Check for missing argument error.
   if (MissingArgCount) {
     Diags.Report(diag::err_drv_missing_argument)
         << Args.getArgString(MissingArgIndex) << MissingArgCount;
     Success = false;
   }
 
+  // Issue warning for integrated assembler -defsym option
+  for (const Arg *A : Args.filtered(options::OPT_defsym)) {
+    A->claim();
+    Diags.Report(diag::warn_drv_unused_argument) << A->getAsString(Args);
+  }
+
   // Issue errors on unknown arguments.
   for (const Arg *A : Args.filtered(OPT_UNKNOWN)) {
     Diags.Report(diag::err_drv_unknown_argument) << A->getAsString(Args);
     Success = false;
   }
 
   Success &= ParseAnalyzerArgs(*Res.getAnalyzerOpts(), Args, Diags);
   Success &= ParseMigratorArgs(Res.getMigratorOpts(), Args);

test/Driver/defsym.c

+// RUN: not %clang -c -o /dev/null %s \
+// RUN: -defsym \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR
+// CHECK-DEFSYM-ERR: error: argument to '-defsym' is missing (expected 1 value)
+
+// RUN: %clang -c -o /dev/null %s \
+// RUN: -defsym bar=1 \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-WARN
+// CHECK-DEFSYM-WARN: warning: argument unused during compilation: '-defsym bar=1'
+
+int foo(void) {
+  return 0;
+}

test/Driver/defsym.s

 // RUN: %clang -### -c -integrated-as %s \
 // RUN: -Wa,-defsym,abc=5 -Wa,-defsym,xyz=0xa \
 // RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM1
+// RUN: %clang -### -c -integrated-as %s \
+// RUN: -defsym abc=5 -defsym xyz=0xa \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM1
 
 // RUN: %clang -### -c -no-integrated-as -target x86_64-unknown-unknown %s \
 // RUN: -Wa,-defsym,abc=5 -Wa,-defsym,xyz=0xa \
 // RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM1
+// RUN: %clang -### -c -no-integrated-as -target x86_64-unknown-unknown %s \
+// RUN: -defsym abc=5 -defsym xyz=0xa \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-UNUSED
 
 // CHECK-DEFSYM1: "-defsym"
 // CHECK-DEFSYM1: "abc=5"
 // CHECK-DEFSYM1: "-defsym"
 // CHECK-DEFSYM1: "xyz=0xa"
+// CHECK-UNUSED:  warning: argument unused during compilation: '-defsym abc=5' [-Wunused-command-line-argument]
+// CHECK-UNUSED:  warning: argument unused during compilation: '-defsym xyz=0xa' [-Wunused-command-line-argument]
 
 // RUN: not %clang -c -integrated-as -o /dev/null %s \
 // RUN: -Wa,-defsym,abc= \
 // RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR1
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -defsym abc= \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR1
 // CHECK-DEFSYM-ERR1: error: defsym must be of the form: sym=value: abc=
 
 // RUN: not %clang -c -integrated-as -o /dev/null %s \
-// RUN: -Wa,-defsym,=123 \
+// RUN: -Wa,-defsym,abc \
 // RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR2
-// CHECK-DEFSYM-ERR2: error: defsym must be of the form: sym=value: =123
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -defsym abc \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR2
+// CHECK-DEFSYM-ERR2: error: defsym must be of the form: sym=value: abc
 
 // RUN: not %clang -c -integrated-as -o /dev/null %s \
-// RUN: -Wa,-defsym,abc=1a2b3c \
+// RUN: -Wa,-defsym,=123 \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR3
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -defsym =123 \
 // RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR3
-// CHECK-DEFSYM-ERR3: error: Value is not an integer: 1a2b3c
+// CHECK-DEFSYM-ERR3: error: defsym must be of the form: sym=value: =123
 
 // RUN: not %clang -c -integrated-as -o /dev/null %s \
-// RUN: -Wa,-defsym \
+// RUN: -Wa,-defsym,123 \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR4
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -defsym 123 \
 // RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR4
+// CHECK-DEFSYM-ERR4: error: defsym must be of the form: sym=value: 123
 
 // RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -Wa,-defsym,abc=1a2b3c \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR5
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -defsym abc=1a2b3c \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR5
+// CHECK-DEFSYM-ERR5: error: Value is not an integer: 1a2b3c
+
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -Wa,-defsym \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR6
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
 // RUN: -Wa,-defsym, \
-// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR4
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR6
+// CHECK-DEFSYM-ERR6: error: defsym must be of the form: sym=value: -defsym
 
-// CHECK-DEFSYM-ERR4: error: defsym must be of the form: sym=value: -defsym
+// RUN: not %clang -c -integrated-as -o /dev/null %s \
+// RUN: -defsym \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR7
+// CHECK-DEFSYM-ERR7: error: argument to '-defsym' is missing (expected 1 value)
 No newline at end of file