[Mips] Use addAliasForDirective rather than custom parsing logic for data directives

[asb]

Summary

The target independent AsmParser doesn't recognise .hword, .word, .dword which are required for Mips. Currently MipsAsmParser recognises these through dispatch to MipsAsmParser::parseDataDirective. This contains equivalent logic to AsmParser::parseDirectiveValue. This patch allows reuse of AsmParser::parseDirectiveValue by making use of addAliasForDirective to support .hword, .word and .dword.

[asb]

Apologies, I missed that there is a test failure/change with this patch. Symbol h in micromips-label-test.s now looks like:

Symbol {
  Name: h (40)
  Value: 0x8
  Size: 0
  Binding: Local (0x0)
  Type: None (0x0)
  Other [ (0x80)
    STO_MIPS_MICROMIPS (0x80)
  ]
  Section: .text (0x2)
}

Rather than:

Symbol {
  Name: h (40)
  Value: 0x8
  Size: 0
  Binding: Local (0x0)
  Type: None (0x0)
  Other: 0
  Section: .text (0x2)
}

I'm not familiar with STO_MIPS_MICROMIPS so I'm not sure if this is a breaking change or not.

[sdardis]

This change isn't quite correct, as microMIPS traditionally doesn't mark inline data (.word and friends) which have a symbol associated with them as being microMIPS. The linker needs to know what symbols are microMIPS / MIPS to support interlinking the two. Inline data isn't code, so marking the symbol associated with it as microMIPS could result in buggy linkers seeing a microMIPS symbol in an address calculation, setting the lowest bit (as required for jumps to microMIPS symbols) leading to an unaligned load.

I believe this patch also needs to override EmitIntValue() as well so that the pending set of microMIPS symbols can be cleared.

See D6039 / rL221355.

[asb]

Thanks for the explanation. In that case, the wrong marker is going to be produced for any of the target independent data directives that aren't handled specifically by the Mips asm parser (e.g. .4byte, .long etc) so this is definitely something that's worth fixing. That's probably a separate, parent patch to this one.

[atanasyan]

I suggest to abandon / close this review. Any objections?

[asb]

Sure, but it does sound like the handling of .4byte, .long may incorrectly set STO_MIPS_MICROMIPS in the current implementation so perhaps there's a bug to be filed.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

#include "MipsGenAsmMatcher.inc"
   bool parseSetHardFloatDirective();
   bool parseSetMtDirective();
   bool parseSetNoMtDirective();
   bool parseSetNoCRCDirective();
   bool parseSetNoVirtDirective();
 
   bool parseSetAssignment();
 
-  bool parseDataDirective(unsigned Size, SMLoc L);
   bool parseDirectiveGpWord();
   bool parseDirectiveGpDWord();
   bool parseDirectiveDtpRelWord();
   bool parseDirectiveDtpRelDWord();
   bool parseDirectiveTpRelWord();
   bool parseDirectiveTpRelDWord();
   bool parseDirectiveModule();
   bool parseDirectiveModuleFP();
#undef GET_OPERAND_DIAGNOSTIC_TYPES
   MipsAsmParser(const MCSubtargetInfo &sti, MCAsmParser &parser,
                 const MCInstrInfo &MII, const MCTargetOptions &Options)
     : MCTargetAsmParser(Options, sti, MII),
         ABI(MipsABIInfo::computeTargetABI(Triple(sti.getTargetTriple()),
                                           sti.getCPU(), Options)) {
     MCAsmParserExtension::Initialize(parser);
 
     parser.addAliasForDirective(".asciiz", ".asciz");
+    parser.addAliasForDirective(".hword", ".2byte");
+    parser.addAliasForDirective(".word", ".4byte");
+    parser.addAliasForDirective(".dword", ".8byte");
 
     // Initialize the set of available features.
     setAvailableFeatures(ComputeAvailableFeatures(getSTI().getFeatureBits()));
 
     // Remember the initial assembler options. The user can not modify these.
     AssemblerOptions.push_back(
         llvm::make_unique<MipsAssemblerOptions>(getSTI().getFeatureBits()));
 
if (Tok.getString() == "noat") {
     // It is just an identifier, look for an assignment.
     parseSetAssignment();
     return false;
   }
 
   return true;
 }
 
-/// parseDataDirective
-///  ::= .word [ expression (, expression)* ]
-bool MipsAsmParser::parseDataDirective(unsigned Size, SMLoc L) {
-  MCAsmParser &Parser = getParser();
-  if (getLexer().isNot(AsmToken::EndOfStatement)) {
-    while (true) {
-      const MCExpr *Value;
-      if (getParser().parseExpression(Value))
-        return true;
-
-      getParser().getStreamer().EmitValue(Value, Size);
-
-      if (getLexer().is(AsmToken::EndOfStatement))
-        break;
-
-      if (getLexer().isNot(AsmToken::Comma))
-        return Error(L, "unexpected token, expected comma");
-      Parser.Lex();
-    }
-  }
-
-  Parser.Lex();
-  return false;
-}
-
 /// parseDirectiveGpWord
 ///  ::= .gpword local_sym
 bool MipsAsmParser::parseDirectiveGpWord() {
   MCAsmParser &Parser = getParser();
   const MCExpr *Value;
   // EmitGPRel32Value requires an expression, so we are using base class
   // method to evaluate the expression.
   if (getParser().parseExpression(Value))
bool MipsAsmParser::ParseDirective(AsmToken DirectiveID) {
   if (IDVal == ".cpload") {
     parseDirectiveCpLoad(DirectiveID.getLoc());
     return false;
   }
   if (IDVal == ".cprestore") {
     parseDirectiveCpRestore(DirectiveID.getLoc());
     return false;
   }
-  if (IDVal == ".dword") {
-    parseDataDirective(8, DirectiveID.getLoc());
-    return false;
-  }
   if (IDVal == ".ent") {
     StringRef SymbolName;
 
     if (Parser.parseIdentifier(SymbolName)) {
       reportParseError("expected identifier after .ent");
       return false;
     }
 
if (IDVal == ".tprelword") {
     return false;
   }
 
   if (IDVal == ".tpreldword") {
     parseDirectiveTpRelDWord();
     return false;
   }
 
-  if (IDVal == ".word") {
-    parseDataDirective(4, DirectiveID.getLoc());
-    return false;
-  }
-
-  if (IDVal == ".hword") {
-    parseDataDirective(2, DirectiveID.getLoc());
-    return false;
-  }
-
   if (IDVal == ".option") {
     parseDirectiveOption();
     return false;
   }
 
   if (IDVal == ".abicalls") {
     getTargetStreamer().emitDirectiveAbiCalls();
     if (Parser.getTok().isNot(AsmToken::EndOfStatement)) {