[AsmParser] Add support to LOCAL directive.

[jcai19]

Summary

This addresses PR#45051 and addes support to LOCAL directive when
alternate macro mode is enabled.

Link:
https://sourceware.org/binutils/docs/as/Altmacro.html#Altmacro

[nickdesaulniers]

llvm/lib/MC/MCParser/AsmParser.cpp

2504

would it be preferable to use llvm::StringSet?

https://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringset-h

[jcai19]

Use StringSet instead of std::unordered_set <std::string>.

llvm/lib/MC/MCParser/AsmParser.cpp

2504

Sounds good! Thanks.

[MaskRay]

DenseSet<CachedHashStringRef> may be better than StringSet. The former doesn't need to duplicate the string.

Does LOCAL (https://bugs.llvm.org//show_bug.cgi?id=45051) have a use case?
As you mentioned, this can usually be replaced with local labels (1:)

---

If the feature turns out to be useful,

The implementation looks hackish to me. A proper implemention should make the LOCAL symbol similar to a macro argument.

I think the replacement should use a .LL symbol instead. For non-RISC-V the local symbols are not intended to be retained in the symbol table.

[jcai19]

In D105720#2872322, @MaskRay wrote:

DenseSet<CachedHashStringRef> may be better than StringSet. The former doesn't need to duplicate the string.

Thanks!

Does LOCAL (https://bugs.llvm.org//show_bug.cgi?id=45051) have a use case?
As you mentioned, this can usually be replaced with local labels (1:)

Yes, the reported case should be fixed by the proposed workaround but sometimes it may be more difficult to find a workaround such as in the the test cases. While this might not be common, PR#45051 shows there still could be real-world use cases of this directive, and I think we should try to support it especially when it does not introduce significant change to the existing code.

---

If the feature turns out to be useful,

The implementation looks hackish to me. A proper implemention should make the LOCAL symbol similar to a macro argument.

That would look cleaner but if I understand your comment correctly, it is not easy to integrate the code into the following loop that handles macro arguments. For macro arguments, IAS could easily identify their uses by checking \. This does not apply to uses of LOCAL arguments, and if we would integrate the code, we need to check against possible LOCAL arguments at every character of Body, which would be more expensive. The currently implementation goes through each use case of LOCAL arguments once (after it identifies the corresponding LOCAL directive call), while integrating it into the for loop would introduce (length of Body * possible LOCAL arguments) checks. WDYT?

I think the replacement should use a .LL symbol instead. For non-RISC-V the local symbols are not intended to be retained in the symbol table.

Agreed, will update my code.

[MaskRay]

In D105720#2875779, @jcai19 wrote:

Yes, the reported case should be fixed by the proposed workaround but sometimes it may be more difficult to find a workaround such as in the the test cases. While this might not be common, PR#45051 shows there still could be real-work use cases of this directive, and I think we should try to support it especially when it does not introduce significant change to the existing code.

The PR doesn't list a use case. This could be an obscure feature which probably no code uses.
(There are now ~80 lines so I am reluctant on accepting that this is insignificant amount of code.)

[jcai19]

In D105720#2875813, @MaskRay wrote:

In D105720#2875779, @jcai19 wrote:

Yes, the reported case should be fixed by the proposed workaround but sometimes it may be more difficult to find a workaround such as in the the test cases. While this might not be common, PR#45051 shows there still could be real-work use cases of this directive, and I think we should try to support it especially when it does not introduce significant change to the existing code.

The PR doesn't list a use case. This could be an obscure feature which probably no code uses.
(There are now ~80 lines so I am reluctant on accepting that this is insignificant amount of code.)

RP#4505 stated this issue happened while building risc-v kernel with IAS so I assume it was real-world usage. @nickdesaulniers is there an associated CBL link with more information? Thanks.

[jcai19]

On second thought, my earlier analysis regarding the time complexity was not correct. Integrating the implementation of LOCAL directive into the for loop that handles macro arguments does not make it more expensive. This implements LOCAL directive within the for loop and avoids copying the macro body, and hopefully is more consistent with the existing code. This also reduces the number of lines added.

[jcai19]

Friendly ping.

[MaskRay]

I still don't see a sufficient justification for making the change.

[jcai19]

In D105720#2950863, @MaskRay wrote:

I still don't see a sufficient justification for making the change.

Would supporting this directive to match GNU spec be a sufficient justification? I've reduced the implementation to 50 lines of code, so hopefully this is a less significant change compared to my initial implementation. I also found many tests in GAS test suite for this directive, so this should also help if anyone is interested in running GAS test suite with IAS to assess the incompatibility between IAS and GAS.

[ArthurHeymans]

Actually the coreboot project (open source firmware) uses the 'LOCAL' directive: https://review.coreboot.org/c/coreboot/+/63045/ .
I'm trying to get that project working and building with clang so this feature would certainly help.

llvm/lib/MC/MCParser/AsmParser.cpp

bool AsmParser::expandMacro(raw_svector_ostream &OS, StringRef Body,
                             ArrayRef<MCAsmMacroParameter> Parameters,
                             ArrayRef<MCAsmMacroArgument> A,
                             bool EnableAtPseudoVariable, 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");
 
+  enum TokenKind { ArgU, AltLocalArg, AltLocalArgU } K;
+  StringRef LocalArgument;
+  StringMap<std::string> LocalSyms;
   // 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;
+    K = ArgU;
     for (; Pos != End; ++Pos) {
       // Check for a substitution or escape.
       if (IsDarwin && !NParameters) {
         // This macro has no parameters, look for $0, $1, etc.
         if (Body[Pos] != '$' || Pos + 1 == End)
           continue;
 
         char Next = Body[Pos + 1];
         if (Next == '$' || Next == 'n' ||
             isdigit(static_cast<unsigned char>(Next)))
           break;
       } else {
         // This macro has parameters, look for \foo, \bar, etc.
         if (Body[Pos] == '\\' && Pos + 1 != End)
           break;
       }
+      // Handles LOCAL directives in alternative macro mode.
+      if (AltMacroMode) {
+        if ((Pos == 0 || !isIdentifierChar(Body[Pos - 1])) && Pos + 6 < End &&
+            Body.substr(Pos, 6) == "LOCAL ")
+          K = AltLocalArg;
+        else {
+          for (const auto &Sym : LocalSyms) {
+            size_t S = Sym.first().size();
+            std::size_t E = Pos + S;
+            if ((Sym.first() == Body.substr(Pos, S)) &&
+                (Pos == 0 || !isIdentifierChar(Body[Pos - 1])) &&
+                (E >= End || !isIdentifierChar(Body[E]))) {
+              K = AltLocalArgU;
+              LocalArgument = Sym.first();
+              break;
             }
-
+          }
+        }
+        if (K != ArgU)
+          break;
+      }
+    }
+    if (K == ArgU) {
       // Add the prefix.
       OS << Body.slice(0, Pos);
 
       // Check if we reached the end.
       if (Pos == End)
         break;
 
       if (IsDarwin && !NParameters) {
         switch (Body[Pos + 1]) {
         // $$ => $
         case '$':
           OS << '$';
           break;
 
           // $n => number of arguments
         case 'n':
           OS << A.size();
           break;
 
           // $[0-9] => argument
         default: {
           // Missing arguments are ignored.
           unsigned Index = Body[Pos + 1] - '0';
           if (Index >= A.size())
             break;
 
           // Otherwise substitute with the token values, with spaces eliminated.
           for (const AsmToken &Token : A[Index])
             OS << Token.getString();
           break;
         }
         }
         Pos += 2;
       } else {
         unsigned I = Pos + 1;
 
         // Check for the \@ pseudo-variable.
         if (EnableAtPseudoVariable && Body[I] == '@' && I + 1 != End)
           ++I;
         else
           while (isIdentifierChar(Body[I]) && I + 1 != End)
             ++I;
 
         const char *Begin = Body.data() + Pos + 1;
         StringRef Argument(Begin, I - (Pos + 1));
         unsigned Index = 0;
 
         if (Argument == "@") {
           OS << NumOfMacroInstantiations;
           Pos += 2;
         } else {
           for (; Index < NParameters; ++Index)
             if (Parameters[Index].Name == Argument)
               break;
 
           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 (const AsmToken &Token : A[Index])
               // For altmacro mode, you can write '%expr'.
               // The prefix '%' evaluates the expression 'expr'
               // and uses the result as a string (e.g. replace %(1+2) with the
               // string "3").
               // Here, we identify the integer token which is the result of the
               // absolute expression evaluation and replace it with its string
               // representation.
               if (AltMacroMode && Token.getString().front() == '%' &&
                   Token.is(AsmToken::Integer))
                 // Emit an integer value to the buffer.
                 OS << Token.getIntVal();
               // Only Token that was validated as a string and begins with '<'
               // is considered altMacroString!!!
               else if (AltMacroMode && Token.getString().front() == '<' &&
                        Token.is(AsmToken::String)) {
                 OS << angleBracketString(Token.getStringContents());
               }
               // We expect no quotes around the string's contents when
               // parsing for varargs.
               else if (Token.isNot(AsmToken::String) || VarargParameter)
                 OS << Token.getString();
               else
                 OS << Token.getStringContents();
 
             Pos += 1 + Argument.size();
           }
         }
       }
       // Update the scan point.
       Body = Body.substr(Pos);
+    } else if (K == AltLocalArg) {
+      // Get arguments to the current LOCAL directive
+      std::size_t I = Pos + 6;
+      std::size_t E = Body.find('\n', I);
+      while (I < E) {
+        while (I < E && !isIdentifierChar(Body[I]))
+          ++I;
+        std::size_t B = I;
+        while (I < E && isIdentifierChar(Body[I]))
+          ++I;
+        std::string Sym(Body.data() + B, I - B);
+        if (LocalSyms.find(Sym) != LocalSyms.end())
+          return Error(
+              L, Sym + " was already used as an argument to a LOCAL directive");
+        std::string LocalSym;
+        do {
+          static unsigned LocalCnt = 0;
+          LocalSym = ".LL" + std::to_string(LocalCnt++);
+        } while (getContext().lookupSymbol(LocalSym));
+        LocalSyms[Sym] = LocalSym;
+      }
+      OS << Body.slice(0, Pos);
+      // Update the scan point.
+      Body = Body.substr(E + 1);
+    } else {
+      OS << Body.slice(0, Pos) << LocalSyms[LocalArgument];
+      Body = Body.substr(Pos + LocalArgument.size());
+    }
   }
 
   return false;
 }
 
 static bool isOperator(AsmToken::TokenKind kind) {
   switch (kind) {
   default:

llvm/test/MC/AsmParser/directive_local_alt_macro_mode.s

+// RUN: llvm-mc -triple i386 -filetype=asm %s | FileCheck %s
+// RUN: not llvm-mc -triple i386 -filetype=asm --defsym ERR=1 %s -o /dev/null 2>&1  | FileCheck --check-prefix=ERR %s
+
+.altmacro
+
+.macro write arg
+LOCAL x,y
+.set x, \arg
+.set y, \arg
+.set z, \arg
+LOCAL z
+.endm
+
+.macro read
+LOCAL foo
+foo:
+mov x, %eax
+mov y, %ebx
+mov z, %ecx
+jmp foo
+.endm
+
+.macro err
+LOCAL x,y
+LOCAL x
+.endm
+
+write 16
+read
+.ifdef ERR
+err
+// ERR:[[#@LINE-1]]:4: error: x was already used as an argument to a LOCAL directive
+.endif
+
+
+// CHECK:      .set .LL0, 16
+// CHECK-NEXT: .set .LL1, 16
+// CHECK-NEXT: .set z, 16
+// CHECK-EMPTY:
+// CHECK-NEXT: .LL3:
+// CHECK-NEXT: movl x, %eax
+// CHECK-NEXT: movl y, %ebx
+// CHECK-NEXT: movl 16, %ecx
+// CHECK-NEXT: jmp .LL3