[LLVM][inline-asm][Altmacor] Altmacro string delimiter '<..>'

[m_zuckerman]

Summary

In this patch, I introduce a new altmacro string delimiter.
This review is the second review in a series of four reviews.
(one for each altmacro feature: LOCAL, string delimiter, string '!' escape sign and absolute expression as a string '%' ).

In the alternate macro mode, you can delimit strings with matching angle brackets <..>
when using it as a part of calling macro arguments.

As described in the https://sourceware.org/binutils/docs-2.27/as/Altmacro.html
"<string>
You can delimit strings with matching angle brackets."

assumptions:

1. If an argument begins with '<' and ends with '>'. The argument is considered as a string.
2. Except adding new string mark '<..>', a regular macro behavior is expected.
3. The altmacro cannot affect the regular less/greater behavior.
4. If a comma is present inside an angle brackets it considered as a character and not as a separator.

[rengolin]

lib/MC/MCParser/AsmParser.cpp

1201 ↗

(On Diff #97400)

I'd add single quotes at the same time.

This seems like a simple addition, as you can pass the expected "ending char" as an argument, in addition to \r\n\0.

2501 ↗

(On Diff #97400)

Format else in the same line, like } else {

[rengolin]

lib/MC/MCParser/AsmParser.cpp

1201 ↗

(On Diff #97400)

The problem here, IIUC, is that you could start with '<' and end with single-quote (or vice-versa) and this code would accept. Also, if you start with '<', single-quote isn't actually a terminator.

To make sure this works, you have to test four overall patterns:

• <foo,bar>
• <foo'bar>
• 'foo,bar'
• 'foo>bar'

They all have to produce a single string "foo?bar".

What I meant was something like:

bool AsmParser::isAltmacroString(SMLoc &StrLoc, SMLoc &EndLoc, const char Term) {
  ...
  while ((*CharPtr != Term) && (*CharPtr != '\n') && (*CharPtr != '\r') && (*CharPtr != '\0')) {
    CharPtr++;
}

And then down there, something like:

} else if (Lexer.IsaAltMacroMode() && Lexer.is(AsmToken::Less) && Lexer.is(AsmToken::SingleQuote) &&
           isAltmacroString(StrLoc, EndLoc, Lex()) {

or something.

[m_zuckerman]

lib/MC/MCParser/AsmParser.cpp

1201 ↗

(On Diff #97400)

1. The code will not accept it because the last character that I check is '>'. So if the code ends with "\ '". The function returns false since it does not equal to '>' as required by the second if.

2. regard the <foo'bar>: I don't really sure what is the usage of it.

GAS doesn't really support the "\'" delimiter and the only full support that I found using https://godbolt.org/ was for the "<...>".

3. I prefer only to support '<..>' as in GCC. And I don't mind to define single quote as a string in altmacro, but it must end with a single quote.

[rengolin]

With that in mind, I think you should then go back to your original implementation (ignore quotes) and let the expression evaluation crash if there's any single quote in between (or whatever is the expected behaviour).

Maybe we should, then, add a comment on that function, saying the docs mention single-quotes but GCC doesn't support, so we won't either.

Sorry for the confusion.

cheers,
--renato

lib/MC/MCParser/AsmParser.cpp

1201 ↗

(On Diff #97400)

Oh, I was not aware the single quotes were unsupported in GCC. That's what I get for trusting their docs... :)

[m_zuckerman]

The behaviors are very strange when working with single quotes, so I prefer only to do partial support (strings) or not at all.

[m_zuckerman]

No worry :) we love to play with the code

[rengolin]

You seem to have lost the negative test.

lib/MC/MCParser/AsmLexer.cpp

464 ↗

(On Diff #97639)

Delimiter

test/MC/AsmParser/altmacro_string.s

28 ↗

(On Diff #97639)

Shouldn't this be $1?

[m_zuckerman]

As discussed, I have returned to the <...> implementation without the single quote and I added a negative test.

[rengolin]

LGTM. Thanks!

llvm/trunk/lib/MC/MCParser/AsmParser.cpp

public:
   bool parseIdentifier(StringRef &Res) override;
   void eatToEndOfStatement() override;
 
   bool checkForValidSection() override;
 
   /// }
 
 private:
+  bool isAltmacroString(SMLoc &StrLoc, SMLoc &EndLoc);
   bool parseStatement(ParseStatementInfo &Info,
                       MCAsmParserSemaCallback *SI);
   bool parseCurlyBlockScope(SmallVectorImpl<AsmRewrite>& AsmStrRewrites);
   bool parseCppHashLineFilenameComment(SMLoc L);
 
   void checkForBadMacro(SMLoc DirectiveLoc, StringRef Name, StringRef Body,
                         ArrayRef<MCAsmMacroParameter> Parameters);
   bool expandMacro(raw_svector_ostream &OS, StringRef Body,
case MCExpr::Binary: {
 
     return MCBinaryExpr::create(BE->getOpcode(), LHS, RHS, getContext());
   }
   }
 
   llvm_unreachable("Invalid expression kind!");
 }
 
+/// This function checks if the next token is <string> type or arithmetic.
+/// string that begin with character '<' must end with character '>'.
+/// otherwise it is arithmetics.
+/// If the function returns a 'true' value,
+/// the End argument will be filled with the last location pointed to the '>'
+/// character.
+
+/// There is a gap between the AltMacro's documentation and the single quote implementation. 
+/// GCC does not fully support this feature and so we will not support it.
+/// TODO: Adding single quote as a string.
+bool AsmParser::isAltmacroString(SMLoc &StrLoc, SMLoc &EndLoc) {
+  assert((StrLoc.getPointer() != NULL) &&
+         "Argument to the function cannot be a NULL value");
+  const char *CharPtr = StrLoc.getPointer();
+  while ((*CharPtr != '>') && (*CharPtr != '\n') &&
+         (*CharPtr != '\r') && (*CharPtr != '\0')){
+    CharPtr++;
+  }
+  if (*CharPtr == '>') {
+    EndLoc = StrLoc.getFromPointer(CharPtr + 1);
+    return true;
+  }
+  return false;
+}
+
 /// \brief Parse an expression and return it.
 ///
 ///  expr ::= expr &&,|| expr               -> lowest.
 ///  expr ::= expr |,^,&,! expr
 ///  expr ::= expr ==,!=,<>,<,<=,>,>= expr
 ///  expr ::= expr <<,>> expr
 ///  expr ::= expr +,- expr
 ///  expr ::= expr *,/,% expr               -> highest.
if (Lexer.is(AsmToken::Identifier) && Lexer.peekTok().is(AsmToken::Equal)) {
 
       NamedParametersFound = true;
     }
     bool Vararg = HasVararg && Parameter == (NParameters - 1);
 
     if (NamedParametersFound && FA.Name.empty())
       return Error(IDLoc, "cannot mix positional and keyword arguments");
 
-    if (Lexer.IsaAltMacroMode() && Lexer.is(AsmToken::Percent)) {
-        SMLoc StrLoc = Lexer.getLoc();
-        SMLoc EndLoc;
+    if (Lexer.IsaAltMacroMode() && Lexer.is(AsmToken::Percent)) {
         const MCExpr *AbsoluteExp;
         int64_t Value;
         /// Eat '%'
         Lex();
         if (parseExpression(AbsoluteExp, EndLoc))
           return false;
         if (!AbsoluteExp->evaluateAsAbsolute(Value))
           return Error(StrLoc, "expected absolute expression");
         const char *StrChar = StrLoc.getPointer();
         const char *EndChar = EndLoc.getPointer();
         AsmToken newToken(AsmToken::Integer, StringRef(StrChar , EndChar - StrChar), Value);
         FA.Value.push_back(newToken);
-    }
-    else if(parseMacroArgument(FA.Value, Vararg))
+    } else if (Lexer.IsaAltMacroMode() && Lexer.is(AsmToken::Less) &&
+               isAltmacroString(StrLoc, EndLoc)) {
+        const char *StrChar = StrLoc.getPointer();
+        const char *EndChar = EndLoc.getPointer();
+        jumpToLoc(EndLoc, CurBuffer);
+        /// Eat from '<' to '>'
+        Lex();
+        AsmToken newToken(AsmToken::String, StringRef(StrChar, EndChar - StrChar));
+        FA.Value.push_back(newToken);
+    } else 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;

llvm/trunk/test/MC/AsmParser/altmacro_string.s

+# RUN: llvm-mc -triple i386-linux-gnu  %s| FileCheck %s
+
+# This test checks the altmacro string delimiter '<' and '>'.
+
+.altmacro
+
+# Test #1:
+# You can delimit strings with matching angle brackets '<' '>'.
+# If an argument begins with '<' and ends with '>'.
+# The argument is considered as a string.
+
+# CHECK: simpleCheck:
+.macro simple_check_0 name
+    \name:
+   addl $5,%eax
+.endm
+
+simple_check_0 <simpleCheck>
+
+# Test #2:
+# Except adding new string marks '<..>', a regular macro behavior is expected.
+
+# CHECK:  simpleCheck0:
+# CHECK: addl    $0, %eax
+.macro concat string1 string2 string3
+   \string1\string2\string3:
+        addl $\string3, %eax
+.endm
+
+concat <simple>,<Check>,<0>
+
+# Test #3:
+# The altmacro cannot affect the regular less/greater behavior.
+
+# CHECK: addl $1, %eax
+# CHECK: addl $0, %eax
+
+.macro fun3 arg1 arg2
+   addl $\arg1,%eax
+   addl $\arg2,%eax
+.endm
+
+fun3 5<6 , 5>8
+
+# Test #4:
+# If a comma is present inside an angle brackets,
+# the comma considered as a character and not as a separator.
+# This check checks the ability to split the string to different
+# arguments according to the use of the comma.
+# Fun2 sees the comma as a character.
+# Fun3 sees the comma as a separator.
+
+# CHECK: addl $5, %eax
+# CHECK: addl $6, %eax
+.macro fun2 arg
+   fun3 \arg
+.endm
+
+fun2 <5,6>
+
+# Test #5:
+# If argument begin with '<' and there is no '>' to close it.
+# A regular macro behavior is expected.
+
+# CHECK: addl $4, %eax
+.macro fun4 arg1 arg2
+   .if \arg2\arg1
+   addl $\arg2,%eax
+   .endif
+.endm
+
+fun4 <5,4
+.noaltmacro

llvm/trunk/test/MC/AsmParser/negative_altmacro_string.s

+# RUN: not llvm-mc -triple i386-linux-gnu %s 2>&1 | FileCheck %s
+
+# This test checks the altmacro string delimiter '<' and '>'.
+# In this test we check the '.noaltmacro' directive.
+# We expect that '.altmacro' and '.noaltmacro' will act as a switch on/off directives to the alternate macro mode.
+# .noaltmacro returns the format into a regular macro handling.
+# The default mode is ".noaltmacro". 
+
+# Test #1: default mode
+# CHECK:  error: unexpected token at start of statement
+# CHECK-NEXT: <simpleCheck>:
+.macro simple_check_0 name
+    \name:
+.endm
+
+simple_check_0 <simpleCheck>
+
+
+.altmacro
+.noaltmacro
+
+# Test #2: Switching from alternate mode to default mode
+# CHECK:  error: unexpected token at start of statement
+# CHECK-NEXT: <simpleCheck1>:
+.macro simple_check_1 name
+    \name:
+.endm
+
+simple_check_1 <simpleCheck1>