[mips] [IAS] Add missing constraints and improve testing for the .module directive.

[tomatabacu]

Summary

None of the .set directives can be used before the .module directives. The .set mips0/pop/push were not triggering this constraint.
Also added testing for all the other implemented directives which are supposed to trigger this constraint.

[tomatabacu]

Moved tests to mips-abi-bad.s.
Added test for .set pop.

[tomatabacu]

Add more comprehensive test.

[tomatabacu]

Ping.

[dsanders]

The code change looks correct to me but I think there's a problem with the test.

test/MC/Mips/module-set-directives-bad.s

1–134 ↗

(On Diff #19469)

I don't think this test covers what you think it does. It looks like it's only testing that '.set mips0' triggers the errors. All the subsequent errors will appear as a consequence of '.set mips0' regardless of whether those directives do the right thing.

I think we need a special directive for testing purposes to reset the flag in between each test (.llvm_internal_reallow_module_directive?). That way we can reset the flag between each subtest. The alternative is to have a separate file (and associated process overhead) for each subtest which seems unreasonable.

[tomatabacu]

Added internal directive for resetting .module, as suggested in the review.
Added missing call to forbidModuleDirective in emitDirectiveSetFp.
Removed CHECKs for .set macro/nomacro because they don't get emitted at the moment.
Changed the name of the test file to a more appropriate one.

[dsanders]

LGTM with the CHECK-NOT's added

test/MC/Mips/module-directive-bad.s

10

You ought to have a CHECK-NOT on each of these to ensure the error doesn't trigger.

[tomatabacu]

Added CHECK-NOTs.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

#include "MipsGenAsmMatcher.inc"
   bool parseDataDirective(unsigned Size, SMLoc L);
   bool parseDirectiveGpWord();
   bool parseDirectiveGpDWord();
   bool parseDirectiveModule();
   bool parseDirectiveModuleFP();
   bool parseFpABIValue(MipsABIFlagsSection::FpABIKind &FpABI,
                        StringRef Directive);
 
+  bool parseInternalDirectiveReallowModule();
+
   MCSymbolRefExpr::VariantKind getVariantKind(StringRef Symbol);
 
   bool eatComma(StringRef ErrorStr);
 
   int matchCPURegisterName(StringRef Symbol);
 
   int matchHWRegsRegisterName(StringRef Symbol);
 
bool MipsAsmParser::ParseDirective(AsmToken DirectiveID) {
   }
 
   if (IDVal == ".cpsetup")
     return parseDirectiveCPSetup();
 
   if (IDVal == ".module")
     return parseDirectiveModule();
 
+  if (IDVal == ".llvm_internal_mips_reallow_module_directive")
+    return parseInternalDirectiveReallowModule();
+
   return true;
 }
 
+bool MipsAsmParser::parseInternalDirectiveReallowModule() {
+  // If this is not the end of the statement, report an error.
+  if (getLexer().isNot(AsmToken::EndOfStatement)) {
+    reportParseError("unexpected token, expected end of statement");
+    return false;
+  }
+
+  getTargetStreamer().reallowModuleDirective();
+
+  getParser().Lex(); // Eat EndOfStatement token.
+  return false;
+}
+
 extern "C" void LLVMInitializeMipsAsmParser() {
   RegisterMCAsmParser<MipsAsmParser> X(TheMipsTarget);
   RegisterMCAsmParser<MipsAsmParser> Y(TheMipselTarget);
   RegisterMCAsmParser<MipsAsmParser> A(TheMips64Target);
   RegisterMCAsmParser<MipsAsmParser> B(TheMips64elTarget);
 }
 
 #define GET_REGISTER_MATCHER
 #define GET_MATCHER_IMPLEMENTATION
 #include "MipsGenAsmMatcher.inc"

lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp

 void MipsTargetStreamer::emitFrame(unsigned StackReg, unsigned StackSize,
                                    unsigned ReturnReg) {}
 void MipsTargetStreamer::emitMask(unsigned CPUBitmask, int CPUTopSavedRegOff) {}
 void MipsTargetStreamer::emitFMask(unsigned FPUBitmask, int FPUTopSavedRegOff) {
 }
 void MipsTargetStreamer::emitDirectiveSetArch(StringRef Arch) {
   forbidModuleDirective();
 }
-void MipsTargetStreamer::emitDirectiveSetMips0() {}
+void MipsTargetStreamer::emitDirectiveSetMips0() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips1() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips2() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips3() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips4() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips5() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips32() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips32R2() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips32R3() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips32R5() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips32R6() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips64() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips64R2() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips64R3() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips64R5() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetMips64R6() { forbidModuleDirective(); }
-void MipsTargetStreamer::emitDirectiveSetPop() {}
-void MipsTargetStreamer::emitDirectiveSetPush() {}
+void MipsTargetStreamer::emitDirectiveSetPop() { forbidModuleDirective(); }
+void MipsTargetStreamer::emitDirectiveSetPush() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetDsp() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveSetNoDsp() { forbidModuleDirective(); }
 void MipsTargetStreamer::emitDirectiveCpLoad(unsigned RegNo) {}
 void MipsTargetStreamer::emitDirectiveCpsetup(unsigned RegNo, int RegOrOffset,
                                               const MCSymbol &Sym, bool IsReg) {
 }
 void MipsTargetStreamer::emitDirectiveModuleOddSPReg(bool Enabled,
                                                      bool IsO32ABI) {
   if (!Enabled && !IsO32ABI)
     report_fatal_error("+nooddspreg is only valid for O32");
 }
+void MipsTargetStreamer::emitDirectiveSetFp(
+    MipsABIFlagsSection::FpABIKind Value) {
+  forbidModuleDirective();
+}
 
 MipsTargetAsmStreamer::MipsTargetAsmStreamer(MCStreamer &S,
                                              formatted_raw_ostream &OS)
     : MipsTargetStreamer(S), OS(OS) {}
 
 void MipsTargetAsmStreamer::emitDirectiveSetMicroMips() {
   OS << "\t.set\tmicromips\n";
   forbidModuleDirective();
OS << "\t.frame\t$"
      << StringRef(MipsInstPrinter::getRegisterName(ReturnReg)).lower() << '\n';
 }
 
 void MipsTargetAsmStreamer::emitDirectiveSetArch(StringRef Arch) {
   OS << "\t.set arch=" << Arch << "\n";
   MipsTargetStreamer::emitDirectiveSetArch(Arch);
 }
 
-void MipsTargetAsmStreamer::emitDirectiveSetMips0() { OS << "\t.set\tmips0\n"; }
+void MipsTargetAsmStreamer::emitDirectiveSetMips0() {
+  OS << "\t.set\tmips0\n";
+  MipsTargetStreamer::emitDirectiveSetMips0();
+}
 
 void MipsTargetAsmStreamer::emitDirectiveSetMips1() {
   OS << "\t.set\tmips1\n";
   MipsTargetStreamer::emitDirectiveSetMips1();
 }
 
 void MipsTargetAsmStreamer::emitDirectiveSetMips2() {
   OS << "\t.set\tmips2\n";
void MipsTargetAsmStreamer::emitDirectiveSetDsp() {
   MipsTargetStreamer::emitDirectiveSetDsp();
 }
 
 void MipsTargetAsmStreamer::emitDirectiveSetNoDsp() {
   OS << "\t.set\tnodsp\n";
   MipsTargetStreamer::emitDirectiveSetNoDsp();
 }
 
-void MipsTargetAsmStreamer::emitDirectiveSetPop() { OS << "\t.set\tpop\n"; }
+void MipsTargetAsmStreamer::emitDirectiveSetPop() {
+  OS << "\t.set\tpop\n";
+  MipsTargetStreamer::emitDirectiveSetPop();
+}
 
-void MipsTargetAsmStreamer::emitDirectiveSetPush() { OS << "\t.set\tpush\n"; }
+void MipsTargetAsmStreamer::emitDirectiveSetPush() {
+ OS << "\t.set\tpush\n";
+ MipsTargetStreamer::emitDirectiveSetPush();
+}
 
 // Print a 32 bit hex number with all numbers.
 static void printHex32(unsigned Value, raw_ostream &OS) {
   OS << "0x";
   for (int i = 7; i >= 0; i--)
     OS.write_hex((Value & (0xF << (i * 4))) >> (i * 4));
 }
 
void MipsTargetAsmStreamer::emitDirectiveModuleFP(
 
   StringRef ModuleValue;
   OS << "\t.module\tfp=";
   OS << ABIFlagsSection.getFpABIString(Value) << "\n";
 }
 
 void MipsTargetAsmStreamer::emitDirectiveSetFp(
     MipsABIFlagsSection::FpABIKind Value) {
+  MipsTargetStreamer::emitDirectiveSetFp(Value);
+
   StringRef ModuleValue;
   OS << "\t.set\tfp=";
   OS << ABIFlagsSection.getFpABIString(Value) << "\n";
 }
 
 void MipsTargetAsmStreamer::emitMipsAbiFlags() {
   // No action required for text output.
 }

lib/Target/Mips/MipsTargetStreamer.h

public:
   /// Emit a '.module fp=value' directive using the current values of the
   /// .MIPS.abiflags section.
   void emitDirectiveModuleFP() {
     emitDirectiveModuleFP(ABIFlagsSection.getFpABI(),
                           ABIFlagsSection.Is32BitABI);
   }
 
   virtual void emitDirectiveModuleOddSPReg(bool Enabled, bool IsO32ABI);
-  virtual void emitDirectiveSetFp(MipsABIFlagsSection::FpABIKind Value){};
+  virtual void emitDirectiveSetFp(MipsABIFlagsSection::FpABIKind Value);
   virtual void emitMipsAbiFlags(){};
   void forbidModuleDirective() { ModuleDirectiveAllowed = false; }
+  void reallowModuleDirective() { ModuleDirectiveAllowed = true; }
   bool isModuleDirectiveAllowed() { return ModuleDirectiveAllowed; }
 
   // This method enables template classes to set internal abi flags
   // structure values.
   template <class PredicateLibrary>
   void updateABIInfo(const PredicateLibrary &P) {
     ABI = &P.getABI();
     ABIFlagsSection.setAllFromPredicates(P);

test/MC/Mips/mips-abi-bad.s

 # CHECK: :[[@LINE-1]]:12: error: expected .module option identifier
 # CHECK-NEXT: .module
 # CHECK-NEXT:        ^
 
     .module 34
 # CHECK: :[[@LINE-1]]:13: error: expected .module option identifier
 # CHECK-NEXT: .module 34
 # CHECK-NEXT:         ^
-
-    .set mips16
-    .module fp=32
-# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
-# CHECK-NEXT: .module fp=32
-# CHECK-NEXT:         ^

test/MC/Mips/module-directive-bad.s

+# RUN: not llvm-mc -triple mips-unknown-unknown %s 2>%t1
+# RUN: FileCheck %s < %t1
+
+    .set mips0
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips1
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips2
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips3
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips4
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips5
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips32
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips32r2
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips32r6
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips64
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips64r2
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips64r6
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set arch=mips32
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set mips16
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set nomips16
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set micromips
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set nomicromips
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set msa
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set nomsa
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set dsp
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set nodsp
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set push
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set pop
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set reorder
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set noreorder
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set macro
+    .module fp=64
+# FIXME: emitDirectiveSetMacro should call forbidModuleDirective().
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set nomacro
+    .module fp=64
+# FIXME: emitDirectiveSetNoMacro should call forbidModuleDirective().
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set at
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set at=$3
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set noat
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .set fp=32
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .cpload $25
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .llvm_internal_mips_reallow_module_directive
+    .module fp=32
+# CHECK-NOT: :[[@LINE-1]]:13: error: .module directive must appear before any code
+
+    .cpsetup $25, 8, __cerror
+    .module fp=64
+# CHECK: :[[@LINE-1]]:13: error: .module directive must appear before any code