[MIPS64] Workaround fixup_Mips_32 for getExprOpValue

[xiangzhai]

Summary

Hi,

Testcase:

$ cat llvm/test/MC/Mips/reloc-directive-bad-n64.s 
# RUN: llvm-mc -triple mips64el-unknown-linux < %s -show-encoding \
# RUN:     -target-abi=n64 2>&1 | FileCheck %s
	.text
       .global main
main:
       nop
       daddiu $t9, $ra, main - .    # CHECK: :[[@LINE]]:25: warning: should use explicit constraints!
       nop
       .word main

GNU toolchain:

$ gcc -fPIC -c ./llvm/test/MC/Mips/reloc-directive-bad-n64.s -o t.gnu.o
$ objdump -r t.gnu.o

t.gnu.o:     file format elf64-tradlittlemips

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE 
000000000000000c R_MIPS_32         main
000000000000000c R_MIPS_NONE       *ABS*
000000000000000c R_MIPS_NONE       *ABS*

But LLVM toolchain:

$ clang -fPIC -c ./llvm/test/MC/Mips/reloc-directive-bad-n64.s -o t.llvm.o
$ objdump -r t.llvm.o 

t.llvm.o:     file format elf64-tradlittlemips

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE 
0000000000000004 R_MIPS_32         main
0000000000000004 R_MIPS_NONE       *ABS*
0000000000000004 R_MIPS_NONE       *ABS*
0000000000000004 R_MIPS_32         .text+0x0000000000000004
0000000000000004 R_MIPS_NONE       *ABS*+0x0000000000000004
0000000000000004 R_MIPS_NONE       *ABS*+0x0000000000000004
000000000000000c R_MIPS_32         main
000000000000000c R_MIPS_NONE       *ABS*
000000000000000c R_MIPS_NONE       *ABS*

Warning the user and behaviour same as GNU toolchain after applied the workaround patch:

$ ./build/bin/clang -fPIC -c ./llvm/test/MC/Mips/reloc-directive-bad-n64.s -o t.llvm.o

./llvm/test/MC/Mips/reloc-directive-bad-n64.s:7:25: warning: should use explicit constraints!
       daddiu $t9, $ra, main - .    # CHECK: :[[@LINE]]:25: warning: should use explicit constraints!
                        ^
<unknown>:0: warning: should use explicit constraints!

$ objdump -r t.llvm.o

t.llvm.o:     file format elf64-tradlittlemips

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE 
000000000000000c R_MIPS_32         main
000000000000000c R_MIPS_NONE       *ABS*
000000000000000c R_MIPS_NONE       *ABS*

Passed make check-llvm-mc-mips:

[100%] Running lit suite /home/loongson/zhaixiang/llvm-project-8.x/llvm/test/MC/Mips
Testing Time: 5.36s
  Expected Passes    : 415
  Expected Failures  : 19
[100%] Built target check-llvm-mc-mips

Please review the patch, and give me some advice!

Thanks,
Leslie Zhai

[hev]

It is not enough to just skip the generation of fixup, because we did not calculate the sub-expression of operand 3 of the daddiu instruction, so after linking, the immediate encoding of daddiu is also incorrect:

test.S:

    .text
    .global main
main:
    nop
    daddiu $4, $4, main - .

compile to object file:

llvm-mc -triple mips64el-unknown-linux-gnu -filetype obj -o test.o test.S

objdump test.o:

objdump -r -d test.o

test.o:     file format elf64-tradlittlemips


Disassembly of section .text:

0000000000000000 <main>:
   0:	00000000 	nop
   4:	64840000 	daddiu	a0,a0,0  # NOTE: zero imm and **no** relocation record

link:

gcc -o test test.o

objdump test:

objdump -d test

0000000120000a70 <main>:                   
   120000a70:   00000000    nop            
   120000a74:   64840000    daddiu  a0,a0,0 # NOTE: still zero :(
    ...

[xiangzhai]

0000000120000a70 <main>:                   
   120000a70:   00000000    nop            
   120000a74:   64840000    daddiu  a0,a0,0 # NOTE: still zero :(
    ...

I got it:

$ objdump -D t.llvm.o 

t.llvm.o:     file format elf64-tradlittlemips


Disassembly of section .text:

0000000000000000 <main>:
   0:	00000000 	nop
   4:	67f90000 	daddiu	t9,ra,0
                                                          ^--- GNU is -4

Perhaps I need to investigate bool MipsAsmParser::parseOperand(OperandVector &Operands, StringRef Mnemonic) in the llvm/lib/Target/Mips/AsmParser/MipsAsmParser.cpp:

bool MipsAsmParser::parseOperand(OperandVector &Operands, StringRef Mnemonic) {
...
  default: {                                                                    
    LLVM_DEBUG(dbgs() << ".. generic integer expression\n");                    
                                                                                
    const MCExpr *Expr;                                                         
    SMLoc S = Parser.getTok().getLoc(); // Start location of the operand.           
    if (getParser().parseExpression(Expr))                                      
      return true;                                                              
                                                                                
    SMLoc E = SMLoc::getFromPointer(Parser.getTok().getLoc().getPointer() - 1);  
                                                                                
    Operands.push_back(MipsOperand::CreateImm(Expr, S, E, *this));              
    return false;                                                               
  }                                                                             
  } // switch(getLexer().getKind())                                             
  return true;

[hev]

So do we need to maintain the same behavior as the GNU toolchain? The current behavior of including constraints is correct, and it seems clear to use explicit constraints, isn't it?

lui $0, %hi(label - offset)
ori $0, %lo(label - offset)

daddiu $0, $0, %lo(label - offset)

[xiangzhai]

This comment was removed by xiangzhai.

[atanasyan]

Please include context in all patches sent to review.
To get a full diff, use one of the following commands:

git show HEAD -U999999 > mypatch.patch
git diff -U999999 @{u} > mypatch.patch
git diff HEAD~1 -U999999 > mypatch.patch

https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[xiangzhai]

Behaviour the same as GNU toolchain:

$ objdump -r t.llvm.o 

t.llvm.o:     file format elf64-tradlittlemips

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE 
000000000000000c R_MIPS_32         main
000000000000000c R_MIPS_NONE       *ABS*
000000000000000c R_MIPS_NONE       *ABS*

The same ABS value -4:

$ objdump -D t.llvm.o 

t.llvm.o:     file format elf64-tradlittlemips


Disassembly of section .text:

0000000000000000 <main>:
   0:	00000000 	nop
   4:	67f9fffc 	daddiu	t9,ra,-4

[hev]

In D81919#2095516, @xiangzhai wrote:

Behaviour the same as GNU toolchain:

$ objdump -r t.llvm.o 

t.llvm.o:     file format elf64-tradlittlemips

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE 
000000000000000c R_MIPS_32         main
000000000000000c R_MIPS_NONE       *ABS*
000000000000000c R_MIPS_NONE       *ABS*

The same ABS value -4:

$ objdump -D t.llvm.o 

t.llvm.o:     file format elf64-tradlittlemips


Disassembly of section .text:

0000000000000000 <main>:
   0:	00000000 	nop
   4:	67f9fffc 	daddiu	t9,ra,-4

How about?

    .text
main:
    nop
    daddiu $t9, $ra, main

IIRC, The R_MIPS_PCLO16 means imm = (PC - label) & 65535, but here needs imm = label & 65535, that same as daddiu $t9, $ra, %lo(main). :)

[xiangzhai]

How about?

    .text
main:
    nop
    daddiu $t9, $ra, main

I got the difference:

GNU toolchain:

$ objdump -r t.gnu.o 

t.gnu.o:     file format elf64-tradlittlemips

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE 
0000000000000008 R_MIPS_LO16       main
0000000000000008 R_MIPS_NONE       *ABS*
0000000000000008 R_MIPS_NONE       *ABS*
0000000000000010 R_MIPS_32         main
0000000000000010 R_MIPS_NONE       *ABS*
0000000000000010 R_MIPS_NONE       *ABS*

But LLVM toolchain:

$ objdump -r t.llvm.o 

t.llvm.o:     file format elf64-tradlittlemips

RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE 
0000000000000010 R_MIPS_32         main
0000000000000010 R_MIPS_NONE       *ABS*
0000000000000010 R_MIPS_NONE       *ABS*

Perhaps I need to investigate AsmParser :)

[xiangzhai]

Just workaround as GNU toolchain:

$ objdump -dr t.llvm.o 

t.llvm.o:     file format elf64-tradlittlemips


Disassembly of section .text:

0000000000000000 <main>:
   0:	00000000 	nop
   4:	67f9fffc 	daddiu	t9,ra,-4
   8:	67f90000 	daddiu	t9,ra,0
	...
			10: R_MIPS_32	main
			10: R_MIPS_NONE	*ABS*
			10: R_MIPS_NONE	*ABS*

The ExprKind of main - . is ExprKind::Binary, but GNU toolchain treat it as %lo(main - .), because ORI and DADDIU only have the 16-bit signed immediate!

So just workaround ExprKind::Binary to ExprKind::Target for BAD relocation directive N64 testcase.

[hev]

In D81919#2097431, @xiangzhai wrote:

Just workaround as GNU toolchain:

$ objdump -dr t.llvm.o 

t.llvm.o:     file format elf64-tradlittlemips


Disassembly of section .text:

0000000000000000 <main>:
   0:	00000000 	nop
   4:	67f9fffc 	daddiu	t9,ra,-4
   8:	67f90000 	daddiu	t9,ra,0
	...
			10: R_MIPS_32	main
			10: R_MIPS_NONE	*ABS*
			10: R_MIPS_NONE	*ABS*

The ExprKind of main - . is ExprKind::Binary, but GNU toolchain treat it as %lo(main - .), because ORI and DADDIU only have the 16-bit signed immediate!

So just workaround ExprKind::Binary to ExprKind::Target for BAD relocation directive N64 testcase.

Why not for O32?

[xiangzhai]

Why not for O32?

Because I only have 3A3000/4000 to make check-llvm-mc-mips :)

llvm/include/llvm/MC/MCContext.h

public:
     void setSecureLogUsed(bool Value) { SecureLogUsed = Value; }
 
     void *allocate(unsigned Size, unsigned Align = 8) {
       return Allocator.Allocate(Size, Align);
     }
 
     void deallocate(void *Ptr) {}
 
+    void reportWarning(SMLoc L, const Twine &Msg);
+
     bool hadError() { return HadError; }
     void reportError(SMLoc L, const Twine &Msg);
     // Unrecoverable error has occurred. Display the best diagnostic we can
     // and bail via exit(1). For now, most MC backend errors are unrecoverable.
     // FIXME: We should really do something about that.
     LLVM_ATTRIBUTE_NORETURN void reportFatalError(SMLoc L,
                                                   const Twine &Msg);
 

llvm/lib/MC/MCContext.cpp

 
 CodeViewContext &MCContext::getCVContext() {
   if (!CVContext.get())
     CVContext.reset(new CodeViewContext);
   return *CVContext.get();
 }
 
 //===----------------------------------------------------------------------===//
+// Warning Reporting
+//===----------------------------------------------------------------------===//
+
+void MCContext::reportWarning(SMLoc Loc, const Twine &Msg) {
+  // If we have a source manager use it. Otherwise, try using the inline source
+  // manager.
+  if (SrcMgr)
+    SrcMgr->PrintMessage(Loc, SourceMgr::DK_Warning, Msg);
+  else if (InlineSrcMgr)
+    InlineSrcMgr->PrintMessage(Loc, SourceMgr::DK_Warning, Msg);
+}
+
+//===----------------------------------------------------------------------===//
 // Error Reporting
 //===----------------------------------------------------------------------===//
 
 void MCContext::reportError(SMLoc Loc, const Twine &Msg) {
   HadError = true;
 
   // If we have a source manager use it. Otherwise, try using the inline source
   // manager.

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

getSImm9AddiuspValue(const MCInst &MI, unsigned OpNo,
   if (MO.isImm()) {
     unsigned Binary = (MO.getImm() >> 2) & 0x0000ffff;
     return (((Binary & 0x8000) >> 7) | (Binary & 0x00ff));
   }
 
   return 0;
 }
 
+static bool isBinary = false;
+
 unsigned MipsMCCodeEmitter::
 getExprOpValue(const MCExpr *Expr, SmallVectorImpl<MCFixup> &Fixups,
                const MCSubtargetInfo &STI) const {
   int64_t Res;
 
   if (Expr->evaluateAsAbsolute(Res))
     return Res;
 
   MCExpr::ExprKind Kind = Expr->getKind();
   if (Kind == MCExpr::Constant) {
     return cast<MCConstantExpr>(Expr)->getValue();
   }
 
   if (Kind == MCExpr::Binary) {
+    isBinary = true;
     unsigned Res = getExprOpValue(cast<MCBinaryExpr>(Expr)->getLHS(), Fixups, STI);
     Res += getExprOpValue(cast<MCBinaryExpr>(Expr)->getRHS(), Fixups, STI);
     return Res;
   }
 
   if (Kind == MCExpr::Target) {
     const MipsMCExpr *MipsExpr = cast<MipsMCExpr>(Expr);
 
case MipsMCExpr::MEK_NEG:
           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) {
+    Ctx.reportWarning(Expr->getLoc(), "should use explicit constraints!");
     Mips::Fixups FixupKind = Mips::Fixups(0);
 
     switch(cast<MCSymbolRefExpr>(Expr)->getKind()) {
     default: llvm_unreachable("Unknown fixup kind!");
       break;
     case MCSymbolRefExpr::VK_None:
       FixupKind = Mips::fixup_Mips_32; // FIXME: This is ok for O32/N32 but not N64.
       break;
     } // switch
 
+    if (!STI.getFeatureBits()[Mips::FeatureMips64])
       Fixups.push_back(MCFixup::create(0, Expr, MCFixupKind(FixupKind)));
+    else {
+      if (isBinary)
+        Fixups.push_back(MCFixup::create(0, Expr, MCFixupKind(Mips::fixup_MIPS_PCLO16)));
+    }
     return 0;
   }
   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,
                   const MCSubtargetInfo &STI) const {
   if (MO.isReg()) {
     unsigned Reg = MO.getReg();
     unsigned RegNo = Ctx.getRegisterInfo()->getEncodingValue(Reg);
     return RegNo;
   } else if (MO.isImm()) {
     return static_cast<unsigned>(MO.getImm());
   } else if (MO.isFPImm()) {
     return static_cast<unsigned>(APFloat(MO.getFPImm())
         .bitcastToAPInt().getHiBits(32).getLimitedValue());
   }
   // MO must be an Expr.
   assert(MO.isExpr());
+  isBinary = false;
   return getExprOpValue(MO.getExpr(),Fixups, STI);
 }
 
 /// Return binary encoding of memory related operand.
 /// If the offset operand requires relocation, record the relocation.
 template <unsigned ShiftAmount>
 unsigned MipsMCCodeEmitter::getMemEncoding(const MCInst &MI, unsigned OpNo,
                                            SmallVectorImpl<MCFixup> &Fixups,

llvm/test/MC/Mips/reloc-directive-bad-n64.s

+# RUN: llvm-mc -triple mips64el-unknown-linux < %s -show-encoding \
+# RUN:     -target-abi=n64 2>&1 | FileCheck %s
+  .text
+       .global main
+main:
+       nop
+       daddiu $t9, $ra, main - .    # CHECK: :[[@LINE]]:25: warning: should use explicit constraints!
+       daddiu $t9, $ra, main        # CHECK: :[[@LINE]]:25: warning: should use explicit constraints!
+       nop
+       .word main