Integrated assembler. :vararg macro.

[dyatkovskiy]

Summary

Please review fix for PR18919, that adds support for :vararg macro in integrated assembler.

[dyatkovskiy]

Ping.

[dyatkovskiy]

Ping

[rnk]

IIRC David was hacking on some gnu assembly parsing stuff.

[majnemer]

I'd like to see more tests here. .macro statements with normal arguments preceding the :vararg one. Something like:

.macro foo bar, baz:vararg
  ...
.endmacro

A test without a comma between bar and baz would be good as well.

lib/MC/MCParser/AsmParser.cpp

1828–1829

This comment is a bit hard to read, perhaps something more like:

// We want the quotes around the string's contents when parsing for varargs.

3295

Please use /*Varag=*/false to make it clear what is going on.

[dyatkovskiy]

Thank you guys!
Everything has been fixed according to your remarks:

• Fixed comments
• Added new tests
• I have also found small bug (thank you for test proposals again) and fixed it too.

[dyatkovskiy]

test/MC/AsmParser/vararg.s

20

Sorry for extra lines. Of course they will be removed!

[• rafael]

You should probably have a test with a darwin triple too. I cced Jim Grosbach for him to comment if this is a desirable feature for darwin or not. I know the macro system is a bit different, but I don't know the details.

[dyatkovskiy]

Hi Rafael,
I have implementation for darwin, though in draft state.
There are few reasons/questions:

1. I didn't find anything about vararg in darwin assembler reference. Or even some .s examples.
2. If I remember properly, you can substitude parameters by their number: $0, $1, etc. So, may he then, you can refer to some of vararg params as well? Am I right here?
3. Can I use whole varargs set substitution? Like in Linux tests?
4. If answer to #2 and #3 is "yes", then I suspect we have to keep vararg values in two forms: as parsed and separated values, and as single string. We need second form, because we need to remember separators as well, in another words we have to expand it as-is. At least we have to keep separators for second form.

Anyway I'm ready to work on it too. But darwin support could contain a bit more changes. Perhaps we could commit it separately?

[dyatkovskiy]

Please review updated patch.

• Added test, that checks :vararg state for darwin.

Thanks!

[• rafael]

LGTM with nits.

test/MC/AsmParser/macros-darwin-vararg.s

7

cc is not needed in this test.

test/MC/AsmParser/vararg-default-value.s

17

cc is never 0 in this test, do you need it?

[dyatkovskiy]

r206951
Thanks for reviews!

lib/MC/MCParser/AsmParser.cpp

 /// \brief Helper types for tracking macro definitions.
 typedef std::vector<AsmToken> MCAsmMacroArgument;
 typedef std::vector<MCAsmMacroArgument> MCAsmMacroArguments;
 
 struct MCAsmMacroParameter {
   StringRef Name;
   MCAsmMacroArgument Value;
   bool Required;
+  bool Vararg;
 
-  MCAsmMacroParameter() : Required(false) { }
+  MCAsmMacroParameter() : Required(false), Vararg(false) {}
 };
 
 typedef std::vector<MCAsmMacroParameter> MCAsmMacroParameters;
 
 struct MCAsmMacro {
   StringRef Name;
   StringRef Body;
   MCAsmMacroParameters Parameters;
private:
   /// \param M The macro.
   /// \param NameLoc Instantiation location.
   bool handleMacroEntry(const MCAsmMacro *M, SMLoc NameLoc);
 
   /// \brief Handle exit from macro instantiation.
   void handleMacroExit();
 
   /// \brief Extract AsmTokens for a macro argument.
-  bool parseMacroArgument(MCAsmMacroArgument &MA);
+  bool parseMacroArgument(MCAsmMacroArgument &MA, bool Vararg);
 
   /// \brief Parse all macro arguments for a given macro.
   bool parseMacroArguments(const MCAsmMacro *M, MCAsmMacroArguments &A);
 
   void printMacroInstantiations();
   void printMessage(SMLoc Loc, SourceMgr::DiagKind Kind, const Twine &Msg,
                     ArrayRef<SMRange> Ranges = None) const {
     SrcMgr.PrintMessage(Loc, Kind, Msg, Ranges);
static bool isIdentifierChar(char c) {
   return isalnum(static_cast<unsigned char>(c)) || c == '_' || c == '$' ||
          c == '.';
 }
 
 bool AsmParser::expandMacro(raw_svector_ostream &OS, StringRef Body,
                             ArrayRef<MCAsmMacroParameter> Parameters,
                             ArrayRef<MCAsmMacroArgument> A, const SMLoc &L) {
   unsigned NParameters = Parameters.size();
+  bool HasVararg = NParameters ? Parameters.back().Vararg : false;
   if ((!IsDarwin || NParameters != 0) && NParameters != A.size())
     return Error(L, "Wrong number of arguments");
 
   // A macro without parameters is handled differently on Darwin:
   // gas accepts no arguments and does no substitutions
   while (!Body.empty()) {
     // Scan for the next substitution.
     std::size_t End = Body.size(), Pos = 0;
if (IsDarwin && !NParameters) {
       if (Index == NParameters) {
         if (Body[Pos + 1] == '(' && Body[Pos + 2] == ')')
           Pos += 3;
         else {
           OS << '\\' << Argument;
           Pos = I;
         }
       } else {
+        bool VarargParameter = HasVararg && Index == (NParameters - 1);
         for (MCAsmMacroArgument::const_iterator it = A[Index].begin(),
                                                 ie = A[Index].end();
              it != ie; ++it)
-          if (it->getKind() == AsmToken::String)
-            OS << it->getStringContents();
-          else
+          // We expect no quotes around the string's contents when
+          // parsing for varargs.
+          if (it->getKind() != AsmToken::String || VarargParameter)
             OS << it->getString();
+          else
+            OS << it->getStringContents();
 
         Pos += 1 + Argument.size();
       }
     }
     // Update the scan point.
     Body = Body.substr(Pos);
   }
 
~AsmLexerSkipSpaceRAII() {
     Lexer.setSkipSpace(true);
   }
 
 private:
   AsmLexer &Lexer;
 };
 }
 
-bool AsmParser::parseMacroArgument(MCAsmMacroArgument &MA) {
+bool AsmParser::parseMacroArgument(MCAsmMacroArgument &MA, bool Vararg) {
+
+  if (Vararg) {
+    if (Lexer.isNot(AsmToken::EndOfStatement)) {
+      StringRef Str = parseStringToEndOfStatement();
+      MA.push_back(AsmToken(AsmToken::String, Str));
+    }
+    return false;
+  }
+
   unsigned ParenLevel = 0;
   unsigned AddTokens = 0;
 
   // Darwin doesn't use spaces to delmit arguments.
   AsmLexerSkipSpaceRAII ScopedSkipSpace(Lexer, IsDarwin);
 
   for (;;) {
     if (Lexer.is(AsmToken::Eof) || Lexer.is(AsmToken::Equal))
bool AsmParser::parseMacroArguments(const MCAsmMacro *M,
   SmallVector<SMLoc, 4> FALocs;
 
   A.resize(NParameters);
   FALocs.resize(NParameters);
 
   // Parse two kinds of macro invocations:
   // - macros defined without any parameters accept an arbitrary number of them
   // - macros defined with parameters accept at most that many of them
+  bool HasVararg = NParameters ? M->Parameters.back().Vararg : false;
   for (unsigned Parameter = 0; !NParameters || Parameter < NParameters;
        ++Parameter) {
     SMLoc IDLoc = Lexer.getLoc();
     MCAsmMacroParameter FA;
 
     if (Lexer.is(AsmToken::Identifier) && Lexer.peekTok().is(AsmToken::Equal)) {
       if (parseIdentifier(FA.Name)) {
         Error(IDLoc, "invalid argument identifier for formal argument");
for (unsigned Parameter = 0; !NParameters || Parameter < NParameters;
     }
 
     if (NamedParametersFound && FA.Name.empty()) {
       Error(IDLoc, "cannot mix positional and keyword arguments");
       eatToEndOfStatement();
       return true;
     }
 
-    if (parseMacroArgument(FA.Value))
+    bool Vararg = HasVararg && Parameter == (NParameters - 1);
+    if (parseMacroArgument(FA.Value, Vararg))
       return true;
 
     unsigned PI = Parameter;
     if (!FA.Name.empty()) {
       unsigned FAI = 0;
       for (FAI = 0; FAI < NParameters; ++FAI)
         if (M->Parameters[FAI].Name == FA.Name)
           break;
bool AsmParser::parseDirectiveMacro(SMLoc DirectiveLoc) {
   if (parseIdentifier(Name))
     return TokError("expected identifier in '.macro' directive");
 
   if (getLexer().is(AsmToken::Comma))
     Lex();
 
   MCAsmMacroParameters Parameters;
   while (getLexer().isNot(AsmToken::EndOfStatement)) {
+
+    if (Parameters.size() && Parameters.back().Vararg)
+      return Error(Lexer.getLoc(),
+                   "Vararg parameter '" + Parameters.back().Name +
+                   "' should be last one in the list of parameters.");
+
     MCAsmMacroParameter Parameter;
     if (parseIdentifier(Parameter.Name))
       return TokError("expected identifier in '.macro' directive");
 
     if (Lexer.is(AsmToken::Colon)) {
       Lex();  // consume ':'
 
       SMLoc QualLoc;
       StringRef Qualifier;
 
       QualLoc = Lexer.getLoc();
       if (parseIdentifier(Qualifier))
         return Error(QualLoc, "missing parameter qualifier for "
                      "'" + Parameter.Name + "' in macro '" + Name + "'");
 
       if (Qualifier == "req")
         Parameter.Required = true;
+      else if (Qualifier == "vararg" && !IsDarwin)
+        Parameter.Vararg = true;
       else
         return Error(QualLoc, Qualifier + " is not a valid parameter qualifier "
                      "for '" + Parameter.Name + "' in macro '" + Name + "'");
     }
 
     if (getLexer().is(AsmToken::Equal)) {
       Lex();
 
       SMLoc ParamLoc;
 
       ParamLoc = Lexer.getLoc();
-      if (parseMacroArgument(Parameter.Value))
+      if (parseMacroArgument(Parameter.Value, /*Vararg=*/false ))
         return true;
 
       if (Parameter.Required)
         Warning(ParamLoc, "pointless default value for required parameter "
                 "'" + Parameter.Name + "' in macro '" + Name + "'");
     }
 
     Parameters.push_back(Parameter);

test/MC/AsmParser/macros-darwin-vararg.s

+// RUN: not llvm-mc -triple i386-apple-darwin10 %s 2>&1 | FileCheck %s
+
+// CHECK: error: vararg is not a valid parameter qualifier for 'arg' in macro 'ifcc'
+// CHECK: .macro ifcc arg:vararg
+
+.macro ifcc arg:vararg
+.if cc
+    \arg
+.endif
+.endm

test/MC/AsmParser/vararg-default-value.s

+// RUN: llvm-mc -triple x86_64-linux-gnu %s | FileCheck %s
+.macro ifcc arg:vararg=nop
+.if cc
+            \arg
+.endif
+.endm
+
+.macro ifcc2 arg0=%eax arg1:vararg=%ebx
+.if cc
+            movl \arg0, \arg1
+.endif
+.endm
+
+.text
+
+// CHECK: nop
+.set cc,1
+  ifcc
+// CHECK: movl %eax, %ebx
+  ifcc2 ,

test/MC/AsmParser/vararg.s

+// RUN: llvm-mc -triple x86_64-linux-gnu %s | FileCheck %s
+.macro ifcc arg:vararg
+.if cc
+            \arg
+.endif
+.endm
+
+.macro ifcc2 arg0 arg1:vararg
+.if cc
+            movl \arg0, \arg1
+.endif
+.endm
+
+.macro ifcc3 arg0, arg1:vararg
+.if cc
+            movl \arg0, \arg1
+.endif
+.endm
+
+.text
+
+// CHECK: movl %esp, %ebp
+// CHECK: subl $0, %esp
+// CHECK: movl %eax, %ebx
+// CHECK: movl %ecx, %ebx
+// CHECK: movl %ecx, %eax
+// CHECK: movl %eax, %ecx
+.set cc,1
+  ifcc  movl    %esp, %ebp
+        subl $0, %esp
+
+  ifcc2 %eax %ebx
+  ifcc2 %ecx, %ebx
+  ifcc3 %ecx %eax
+  ifcc3 %eax, %ecx
+
+// CHECK-NOT movl
+// CHECK: subl $1, %esp
+.set cc,0
+  ifcc  movl    %esp, %ebp
+        subl $1, %esp