[Mips] Error if a non-immediate operand is used while an immediate is expected

[hev]

Summary

The 32-bit type relocation (R_MIPS_32) cannot be used for instructions below:

ori $4, $4, start
ori $4, $4, (start - .)

We should print an error instead.

[MaskRay]

Can you please add a test under llvm/test/MC/Mips/? You can reuse the following template

## File-level comment about the purpose of the test
# RUN: llvm-mc -triple=... %s | FileCheck %s

# CHECK: ...

See llvm/test/MC/X86/reloc-directive-elf-32.s for an example. Use ninja check-llvm-mc-mips to test llvm/test/MC/Mips.

[hev]

Add a unit test case.

[atanasyan]

Does this patch depend on D81919?

llvm/test/MC/Mips/reloc-implicit-constraints-error.s

9 ↗

(On Diff #270999)

Do you need this line in the test?

10 ↗

(On Diff #270999)

You do not need to use two separate check prefixes O32 and N64:

ori  $4, $4, start            # CHECK: [[@LINE]]:{{[0-9]+}}: error: should use explicit constraints

16 ↗

(On Diff #270999)

Do you need to check both ori and addiu instructions?

[xiangzhai]

In D81908#2095335, @atanasyan wrote:

Does this patch depend on D81919?

No!

[hev]

Thanks for review.

llvm/test/MC/Mips/reloc-implicit-constraints-error.s

9 ↗

(On Diff #270999)

No.

10 ↗

(On Diff #270999)

I will remove it.

16 ↗

(On Diff #270999)

I think yes, It is used to check the changes of some instructions in the future. e.g. The behavior of the li instruction is different from that of addiu.

[atanasyan]

In D81908#2095355, @xiangzhai wrote:

In D81908#2095335, @atanasyan wrote:

Does this patch depend on D81919?

No!

But both patches change content of the same if (Kind == MCExpr::SymbolRef) statement.

[hev]

In D81908#2095524, @atanasyan wrote:

In D81908#2095355, @xiangzhai wrote:

In D81908#2095335, @atanasyan wrote:

Does this patch depend on D81919?

No!

But both patches change content of the same if (Kind == MCExpr::SymbolRef) statement.

I guess @xiangzhai wants more ways to help resolve the problem. :)

[MaskRay]

Rigid:

% llvm-mc -triple=ppc64 <<< 'or 3, 3, start; start:'
...
<stdin>:1:10: error: invalid operand for instruction
# The diagnostic is from `PPCGenAsmMatcher.inc:MatchAndEmitInstruction`, which is not great. llvm-mc improperly rejects or 3, 3, start-.

% powerpc64le-linux-gnu-as <<< 'or 3, 3, start; start:'
{standard input}: Assembler messages:
{standard input}:1: Error: unsupported relocation against .text

Permissive:

% aarch64-linux-gnu-as <<< 'orr w3, w3, start; start:'
% mipsel-linux-gnu-as <<< 'ori $3, $3, start; start:' # succeeded
% llvm-objdump -dr
...
       0: 04 00 63 34   ori     $3, $3, 4 <start>
                        00000000:  R_MIPS_LO16  .text
% mips64el-linux-gnuabi64-as <<< 'ori $3, $3, start; start:'  # succeeded
% llvm-objdump -dr
...
       0: 00 00 63 34   ori     $3, $3, 0 <.text>
                0000000000000000:  R_MIPS_LO16/R_MIPS_NONE/R_MIPS_NONE  .text+0x4

powerpc's behavior is definitely correct. AArch64 and MIPS's permissive behaviors are not good. A symbolic value (which can be 32-bit/64-bit) requires more bits than the immediate bits. I think flagging the instruction in the case of a symbolic value is fine.

FWIW: I filed a feature request for GNU as: https://sourceware.org/bugzilla/show_bug.cgi?id=26126

llvm/test/MC/Mips/reloc-implicit-constraints-error.s

10 ↗

(On Diff #271062)

ori $4, $4, (start - .) should probably be accepted. start - . computes to a representable constant.

[xiangzhai]

powerpc's behavior is definitely correct. AArch64 and MIPS's permissive behaviors are not good. A symbolic value (which can be 32-bit/64-bit) requires more bits than the immediate bits. I think flagging the instruction in the case of a symbolic value is fine.

FWIW: I filed a feature request for GNU as: https://sourceware.org/bugzilla/show_bug.cgi?id=26126

To MIPS64, ORI and DADDIU only have the 16-bit signed immediate, but GNU toolchain treat it as %lo(MCBinaryExpr), so LLVM toolchain is just able to reject it as @hev implementation, or workaround ExprKind::Binary to ExprKind::Target for BAD relocation directive N64 testcase D81919

[hev]

Thanks for review.

llvm/test/MC/Mips/reloc-implicit-constraints-error.s

10 ↗

(On Diff #271062)

The constant results of sub-expression may be overflow too. for implement, There is also no basic infrastructure for calculating sub-expressions. so, I prefer the explicit way for all cases.

[MaskRay]

This version looks good to me. The diagnostic should probably be similar to error: expected 16-bit signed immediate

[atanasyan]

LGTM

Do you have commit access?

[hev]

In D81908#2104333, @atanasyan wrote:

LGTM

Thank you.

Do you have commit access?

No.

[MaskRay]

In D81908#2104042, @MaskRay wrote:

This version looks good to me. The diagnostic should probably be similar to error: expected 16-bit signed immediate

Please fix the comment first.

[hev]

@MaskRay Sorry, I missed the comment. :)

[xiangzhai]

Do you have commit access?

Please commit it on @hev behalf.

Thanks,
Leslie Zhai

[MaskRay]

Fix code style issue
Improve tests

[MaskRay]

LGTM.

[MaskRay]

Simplify -triple

llvm/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp

case MipsMCExpr::MEK_NEG:
       FixupKind =
           isMicroMips(STI) ? Mips::fixup_MICROMIPS_SUB : Mips::fixup_Mips_SUB;
       break;
     }
     Fixups.push_back(MCFixup::create(0, MipsExpr, MCFixupKind(FixupKind)));
     return 0;
   }
 
-  if (Kind == MCExpr::SymbolRef) {
-    Mips::Fixups FixupKind = Mips::Fixups(0);
-
-    switch(cast<MCSymbolRefExpr>(Expr)->getKind()) {
-    default: llvm_unreachable("Unknown fixup kind!");
-      break;
-    case MCSymbolRefExpr::VK_None:
-      // FIXME: This is ok for O32/N32 but not N64.
-      FixupKind = Mips::fixup_Mips_32;
-      break;
-    } // switch
-
-    Fixups.push_back(MCFixup::create(0, Expr, MCFixupKind(FixupKind)));
-    return 0;
-  }
+  if (Kind == MCExpr::SymbolRef)
+    Ctx.reportError(Expr->getLoc(), "expected an immediate");
   return 0;
 }
 
 /// getMachineOpValue - Return binary encoding of operand. If the machine
 /// operand requires relocation, record the relocation and return zero.
 unsigned MipsMCCodeEmitter::
 getMachineOpValue(const MCInst &MI, const MCOperand &MO,
                   SmallVectorImpl<MCFixup> &Fixups,

llvm/test/MC/Mips/imm-operand-err.s

+## Print an error if a non-immediate operand is used while an immediate is expected
+# RUN: not llvm-mc -filetype=obj -triple=mips -o /dev/null %s 2>&1 | FileCheck %s
+# RUN: not llvm-mc -filetype=obj -triple=mips64 -o /dev/null %s 2>&1 | FileCheck %s
+
+# CHECK: [[#@LINE+1]]:16: error: expected an immediate
+  ori  $4, $4, start
+# CHECK: [[#@LINE+1]]:17: error: expected an immediate
+  ori  $4, $4, (start - .)
+
+# CHECK: [[#@LINE+1]]:18: error: expected an immediate
+  addiu  $4, $4, start
+# CHECK: [[#@LINE+1]]:19: error: expected an immediate
+  addiu  $4, $4, (start - .)
+
+start: