[MC] Parse directives with arguments as macro arguments

[jcai19]

Summary

Currently when expanding an macro that takes directives as arguments,
the integrated assembler treats the name of a directive and its
arguments separated by space as separate arguments to the macro instead
of one argument. For example, the following sample code will fail to
assemble since .section and .foo are treated as two arguments to the
alternative_inst macro.

.macro alternative_insn insn
\insn
+.endm

+alternative_insn .section .foo

This change fixes such issue by identifying directives used as macro
arguments and include their arguments as part of the macro argument,
such as https://github.com/ClangBuiltLinux/linux/issues/939.

[jcai19]

Update summary to include the bug reported.

[jcai19]

Remove a blank line.

[nickdesaulniers]

Thanks for the patch!

llvm/lib/MC/MCParser/AsmParser.cpp

2635

Is this the most precise way to check for directives? I see DirectiveKind, and DirectiveKindMap, maybe there's a way to verify that the Tok is a valid directive?

[jcai19]

llvm/lib/MC/MCParser/AsmParser.cpp

2635

Right that's a good point I'll investigate further.

[MaskRay]

This change looks more like a hack rather than a proper solution. It does not even improve GNU as compatibility. With this patch, MC will accept alternative_insn .section .foo, .section .bar, however, this is rejected by GNU as.

GNU as preprocesses a line with do_scrub_chars. For the input .inst (0), do_scrub_chars will delete the space before ( and make it .inst(0).
macro_expand will read .inst(0) as an actual parameter (the process is rather hacky: .init is tokenized first but then get_any_string reads .inst(0))

This change just makes the code even harder to understand, I think. I don't know how to fix the problem in general. We probably should work around it in the Linux kernel. One approach is to take ihalip's suggestion on https://github.com/ClangBuiltLinux/linux/issues/939#issuecomment-601776123. Another is to quote actual parameters with double quotes, i.e. alternative_insn ".section .foo", ".section .bar"

[jcai19]

Change how to identify directives.

[jcai19]

This patch is not for GNU-compatibility. GNU's handling of spaces between directives and their arguments when expanding macros are confusing IMO -- it only supports selective directives. For example, it supports .inst (XXX) for aarch64, but not .section .foo or .space 0xf on x86 as you mentioned. For .section, if works if you do not have any space between .section and the argument, and surround it with parenthesis, but them will also be included as part of the section name, e.g. alternative_insn .section(.foo), .section(.bar) will work on gcc, but the object file will have (.foo) and (.bar) sections. I think we should either unconditionally support all of such directives, or not at all. I am fine with either option, and would happily close this if one of the workarounds is accepted into Linux.

llvm/lib/MC/MCParser/AsmParser.cpp

2635

DirectiveKindMap does not include all the directives, such .section used in this example or .inst for aarch64.

[MaskRay]

In D76962#1948922, @jcai19 wrote:

This patch is not for GNU-compatibility. GNU's handling of spaces between directives and their arguments when expanding macros are confusing IMO -- it only supports selective directives. For example, it supports .inst (XXX) for aarch64, but not .section .foo or .space 0xf on x86 as you mentioned. For .section, if works if you do not have any space between .section and the argument, and surround it with parenthesis, but them will also be included as part of the section name, e.g. alternative_insn .section(.foo), .section(.bar) will work on gcc, but the object file will have (.foo) and (.bar) sections. I think we should either unconditionally support all of such directives, or not at all. I am fine with either option, and would happily close this if one of the workarounds is accepted into Linux.

You may not follow my comment. It is not that GNU as special cases certain directives, but rather it has a subtle do_scrub_chars rule.

.macro alternative_insn insn1, insn2
\insn1
\insn2
.endm

alternative_insn .section .foo, .section .bar       # Error: too many positional arguments
alternative_insn ".section .foo", ".section .bar"  # accepted

arch/arm64/include/asm/sysreg.h __emit_inst(x) really is likely a use of an unintentional GNU as "feature". The right solution is simply to quote the macro arguments. The rule as implemented by this patch can just make the clang vs GNU as differences more confusing.

[jcai19]

This patch is motived by the issue exposed by https://github.com/ClangBuiltLinux/linux/issues/939. It is however believed to better fix the issue on Linux instead, so close this revision.

llvm/lib/MC/MCParser/AsmParser.cpp

while (true) {
     if (Lexer.is(AsmToken::LParen))
       ++ParenLevel;
     else if (Lexer.is(AsmToken::RParen) && ParenLevel)
       --ParenLevel;
 
     // Append the token to the current argument list.
     MA.push_back(getTok());
     Lexer.Lex();
+
+    // If the token is a directive, append all the following arguments if there is any
+    const StringRef &IDVal = MA.back().getString();
+    // Directives start with "."
+    if (IDVal.startswith(".") && IDVal != ".") {
+      while (!Lexer.is(AsmToken::Comma) && !Lexer.is(AsmToken::EndOfStatement)) {
+        MA.push_back(getTok());
+        Lexer.Lex();
+      }
+    }
   }
 
   if (ParenLevel != 0)
     return TokError("unbalanced parentheses in macro argument");
   return false;
 }
 
 // Parse the macro instantiation arguments.

llvm/test/MC/X86/expand-macro-directive-arguments.s

+# RUN: llvm-mc -filetype=obj -triple=i386 %s -o - | llvm-readelf --sections | FileCheck %s
+
+.macro alternative_insn insn1, insn2
+# CHECK: .foo              PROGBITS        00000000 000034 000000 00      0   0  1
+\insn1
+# CHECK: .bar PROGBITS        00000000 000034 000000 00      0   0  1
+\insn2
+.endm
+
+alternative_insn .section .foo, .section .bar