[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

2505

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

2505

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

 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/DebugInfo/CodeView/SymbolRecord.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCCodeView.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDirectives.h"
 #include "llvm/MC/MCDwarf.h"
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");
 
+  // Handles LOCAL directives in alternate macro mode.
+  std::string Buf;
+  if (AltMacroMode && Body.find("LOCAL ") != std::string::npos) {
+    std::size_t Pos = 0;
+    StringSet<> Locals;
+
+    // Checks if there are duplicate arguments to LOCAL directives
+    while (true) {
+      Pos = Body.find("LOCAL ", Pos);
+      if (Pos == std::string::npos)
+        break;
+      if (Pos > 0 && isalpha(Body[Pos - 1]))
+        continue;
+
+      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 SymB = I;
+        while (I < E && isIdentifierChar(Body[I]))
+          ++I;
+        std::string Sym(Body.data() + SymB, I - SymB);
+        if (!Locals.insert(Sym).second)
+          return Error(
+              L, Sym + " was already used as an argument to a LOCAL directive");
+      }
+      Pos = E + 1;
+    }
+
+    Buf = std::string(Body);
+    Pos = 0;
+
+    // Replaces all the uses of the argument to a LOCAL directive with a unique
+    // name
+    while (true) {
+      Pos = Buf.find("LOCAL ", Pos);
+      if (Pos == std::string::npos)
+        break;
+      if (Pos > 0 && isalpha(Buf[Pos - 1]))
+        continue;
+
+      std::size_t I = Pos + 6;
+      std::size_t E = Buf.find('\n', I);
+      while (I < E) {
+        while (I < E && !isIdentifierChar(Buf[I]))
+          ++I;
+        std::size_t SymB = I;
+        while (I < E && isIdentifierChar(Buf[I]))
+          ++I;
+        std::string Sym(Buf.data() + SymB, I - SymB);
+        std::string LocalSym;
+        do {
+          static unsigned LocalCnt = 0;
+          LocalSym = Sym + std::to_string(LocalCnt++);
+        } while (getContext().lookupSymbol(LocalSym));
+
+        // Replace all the uses of the current argument
+        size_t SymI = E + 1;
+        while (true) {
+          SymI = Buf.find(Sym, SymI);
+          if (SymI == std::string::npos)
+            break;
+          std::size_t SymE = SymI + Sym.size();
+          if ((SymI > 0 && isIdentifierChar(Buf[SymI - 1])) ||
+              (SymE < Buf.size() && isIdentifierChar(Buf[SymE]))) {
+            SymI = SymE;
+            continue;
+          }
+          Buf = Buf.substr(0, SymI) + LocalSym + Buf.substr(SymE);
+          SymI += LocalSym.size();
+        }
+      }
+      Buf = Buf.substr(0, Pos) + Buf.substr(E + 1);
+    }
+    Body = Buf;
+  }
+
   // 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;
     for (; Pos != End; ++Pos) {
       // Check for a substitution or escape.
       if (IsDarwin && !NParameters) {

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 err
+LOCAL x,y
+LOCAL x
+.endm
+
+
+.macro read
+mov x, %eax
+mov y, %ebx
+mov z, %ecx
+.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 x0, 16
+// CHECK-NEXT: .set y1, 16
+// CHECK-NEXT: .set z, 16
+// CHECK-EMPTY:
+// CHECK-NEXT: movl x, %eax
+// CHECK-NEXT: movl y, %ebx
+// CHECK-NEXT: movl 16, %ecx
+