[clang] Implement support for -defsym assembler option

[mgrang]

Summary

Adds support for -Wa,-defsym,abc=1 option.

Related llvm patch: https://reviews.llvm.org/D26214

[mgrang]

Ping!

[mgrang]

Changed setSymbolValue to use Streamer directly instead of getting it from Parser.

[rengolin]

I'm slightly wary of that test, and it would be better to split between driver and defsym features. See mips-ias-Wa.s for the driver test.

Adding Saleem, as he also had some good fiddling on the driver.

I'm not sure how we'll test the asm changes, though. Your test is valid, but we've moved from testing asm in Clang. Are those symbols exported to IR in any form? Global values?

Adding Eric, as he had a go on removing the code tests in Clang.

lib/Driver/Tools.cpp

3118 ↗

(On Diff #77089)

This is a bit of a mess, but the -mcpu above is "validated later" because it handles both integrated and non-integrated as, which is still a big deal on ARM world.

Can you try to move it up and see if you can make it work with no-integrated-as, too? To test this, just make sure with clang -v -### that the --defsym is passed to the assembler.

test/Driver/defsym.s

1 ↗

(On Diff #77089)

You're going to need -target options here.

25 ↗

(On Diff #77089)

Don't put code here, just checking for the values is enough in the driver test.

You may need a new CC1 test with the values.

[compnerd]

I think that we do need some testing for the definition itself propagating correctly. I think that the best way to handle this is to add command line handling for --defsym to llvm-mc and then using that to assemble bits of assembly on the LLVM side rather than the clang side. We could do that first, and then wire that up through to clang in a subsequent change (which this change does).

lib/Driver/Tools.cpp

3118 ↗

(On Diff #77089)

I wonder if its better to use A->getOption()->getID() rather than relying on the spelling of the argument. This is definitely not your fault, but we could stem the behavior here.

test/Driver/defsym.s

1 ↗

(On Diff #77089)

I dont think that a -target is necessary. The flag should go through on any target, so the host is as good as a specific target.

25 ↗

(On Diff #77089)

As @rengolin mentioned, please don't add assembly in the driver tests. They are meant to test just the argument handling in the driver.

[rengolin]

test/Driver/defsym.s

1 ↗

(On Diff #77089)

right, I made that comment before the one below. I agree, no --target and no asm code. :)

[mgrang]

@rengolin @compnerd Thanks for your comments/suggestions. I will address them.

Thanks,
Mandeep

[mgrang]

lib/Driver/Tools.cpp

3118 ↗

(On Diff #77089)

@compnerd Since this is a -Wa option here's what we have:

A->getOption().getID() gives options::OPT_Wa_COMMA
A->getOption().getValue gives StringRef("--defsym")
A->getOption().getName() gives Wa,

So I guess there is no easy way to get the option for a -Wa flag since the value is of type StringRef.

[mgrang]

lib/Driver/Tools.cpp

3118 ↗

(On Diff #77089)

@rengolin I see that the --defsym option is already being passed to the non-integrated-as.

clang -### a.s -no-integrated-as -c -Wa,--defsym,abc=1 -v
"/usr/bin/as" "--defsym" "abc=1" "-o" "a.o" "a.s"

[mgrang]

test/Driver/defsym.s

25 ↗

(On Diff #77089)

@rengolin Where should I add the cc1as test with the values?

[mgrang]

Ping.

[compnerd]

include/clang/Driver/CC1Options.td

748 ↗

(On Diff #78565)

I think that using a single dash would be nicer as it would fit in well with the rest of the options.

[mgrang]

Changed --defsym to -defsym as per @compnerd 's suggestion.

[rengolin]

Sorry, fell out of my radar. LGTM.

[tra]

cfe/trunk/test/Driver/defsym.s

14–22

These need -o /dev/null, otherwise compiler will attempt to create a file which is not something we want here.

Fixed in r288406.

[mgrang]

cfe/trunk/test/Driver/defsym.s

14–22

Thanks @tra

cfe/trunk/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]

cfe/trunk/lib/Driver/Tools.cpp

for (StringRef Value : A->getValues()) {
         } else {
           RenderDebugEnablingArgs(Args, CmdArgs,
                                   codegenoptions::LimitedDebugInfo,
                                   DwarfVersion, llvm::DebuggerKind::Default);
         }
       } else if (Value.startswith("-mcpu") || Value.startswith("-mfpu") ||
                  Value.startswith("-mhwdiv") || Value.startswith("-march")) {
         // Do nothing, we'll validate it later.
+      } else if (Value == "-defsym") {
+          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())

cfe/trunk/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 -no-integrated-as %s \
+// RUN: -Wa,-defsym,abc=5 -Wa,-defsym,xyz=0xa \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM1
+
+// CHECK-DEFSYM1: "-defsym"
+// CHECK-DEFSYM1: "abc=5"
+// CHECK-DEFSYM1: "-defsym"
+// CHECK-DEFSYM1: "xyz=0xa"
+
+// RUN: not %clang -c -integrated-as %s \
+// RUN: -Wa,-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 %s \
+// RUN: -Wa,-defsym,abc=1a2b3c \
+// RUN: 2>&1 | FileCheck %s --check-prefix=CHECK-DEFSYM-ERR2
+// CHECK-DEFSYM-ERR2: error: Value is not an integer: 1a2b3c

cfe/trunk/tools/driver/cc1as_main.cpp

struct AssemblerInvocation {
   /// If given, the name of the target CPU to determine which instructions
   /// are legal.
   std::string CPU;
 
   /// The list of target specific features to enable or disable -- this should
   /// be a list of strings starting with '+' or '-'.
   std::vector<std::string> Features;
 
+  /// The list of symbol definitions.
+  std::vector<std::string> SymbolDefs;
+
   /// @}
   /// @name Language Options
   /// @{
 
   std::vector<std::string> IncludePaths;
   unsigned NoInitialTextSection : 1;
   unsigned SaveTemporaryLabels : 1;
   unsigned GenDwarfForAssembly : 1;
bool AssemblerInvocation::CreateFromArgs(AssemblerInvocation &Opts,
 
   // Assemble Options
   Opts.RelaxAll = Args.hasArg(OPT_mrelax_all);
   Opts.NoExecStack = Args.hasArg(OPT_mno_exec_stack);
   Opts.FatalWarnings = Args.hasArg(OPT_massembler_fatal_warnings);
   Opts.RelocationModel = Args.getLastArgValue(OPT_mrelocation_model, "pic");
   Opts.IncrementalLinkerCompatible =
       Args.hasArg(OPT_mincremental_linker_compatible);
+  Opts.SymbolDefs = Args.getAllArgValues(OPT_defsym);
 
   return Success;
 }
 
 static std::unique_ptr<raw_fd_ostream>
 getOutputStream(AssemblerInvocation &Opts, DiagnosticsEngine &Diags,
                 bool Binary) {
   if (Opts.OutputPath.empty())
static bool ExecuteAssembler(AssemblerInvocation &Opts,
 
   // FIXME: init MCTargetOptions from sanitizer flags here.
   MCTargetOptions Options;
   std::unique_ptr<MCTargetAsmParser> TAP(
       TheTarget->createMCAsmParser(*STI, *Parser, *MCII, Options));
   if (!TAP)
     Failed = Diags.Report(diag::err_target_unknown_triple) << Opts.Triple;
 
+  // Set values for symbols, if any.
+  for (auto &S : Opts.SymbolDefs) {
+    if (Ctx.setSymbolValue(Parser->getStreamer(), S)) {
+      Failed = true;
+      break;
+    }
+  }
+
   if (!Failed) {
     Parser->setTargetParser(*TAP.get());
     Failed = Parser->Run(Opts.NoInitialTextSection);
   }
 
   // Close Streamer first.
   // It might have a reference to the output stream.
   Str.reset();