This is an archive of the discontinued LLVM Phabricator instance.

Differential D81919

[MIPS64] Workaround fixup_Mips_32 for getExprOpValue
AbandonedPublic

Authored by xiangzhai on Jun 16 2020, 3:08 AM.

Details

Reviewers
atanasyan
MaskRay
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

Diff Detail

Repository
rL LLVM

Event Timeline

xiangzhai created this revision.Jun 16 2020, 3:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2020, 3:08 AM
Herald added subscribers: jrtc27, hiraditya, arichardson, sdardis. · View Herald Transcript
hev added a comment.Jun 16 2020, 3:37 AM

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 added a comment.EditedJun 16 2020, 3:48 AM
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 added a comment.EditedJun 16 2020, 3:59 AM

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 added a comment.Jun 16 2020, 4:16 AM
This comment was removed by xiangzhai.
atanasyan mentioned this in D81908: [Mips] Error if a non-immediate operand is used while an immediate is expected.Jun 16 2020, 4:32 AM
atanasyan added a comment.Jun 16 2020, 4:35 AM

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 updated this revision to Diff 271067.Jun 16 2020, 5:52 AM

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 added a comment.Jun 16 2020, 7:27 AM
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 updated this revision to Diff 271252.Jun 16 2020, 6:00 PM

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 updated this revision to Diff 271277.EditedJun 16 2020, 11:31 PM

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 added a comment.Jun 17 2020, 2:19 AM
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 added a comment.Jun 17 2020, 2:45 AM

Why not for O32?

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

xiangzhai abandoned this revision.Jun 19 2020, 7:14 PM

Revision Contents

PathSize
llvm/
include/
llvm/
MC/
2 lines
lib/
MC/
13 lines
Target/
Mips/
MCTargetDesc/
6 lines
test/
MC/
Mips/
9 lines

Diff 271067

llvm/include/llvm/MC/MCContext.h

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

llvm/lib/MC/MCContext.cpp

Show First 20 Lines • Show All 600 Lines • ▼ Show 20 Lines
CodeViewContext &MCContext::getCVContext() { CodeViewContext &MCContext::getCVContext() {
if (!CVContext.get()) if (!CVContext.get())
CVContext.reset(new CodeViewContext); CVContext.reset(new CodeViewContext);
return *CVContext.get(); 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 // Error Reporting
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
void MCContext::reportError(SMLoc Loc, const Twine &Msg) { void MCContext::reportError(SMLoc Loc, const Twine &Msg) {
HadError = true; HadError = true;
// If we have a source manager use it. Otherwise, try using the inline source // If we have a source manager use it. Otherwise, try using the inline source
// manager. // manager.
Show All 18 Lines

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

Show First 20 Lines • Show All 716 Lines • ▼ Show 20 Lines case MipsMCExpr::MEK_NEG:
isMicroMips(STI) ? Mips::fixup_MICROMIPS_SUB : Mips::fixup_Mips_SUB; isMicroMips(STI) ? Mips::fixup_MICROMIPS_SUB : Mips::fixup_Mips_SUB;
break; break;
} }
Fixups.push_back(MCFixup::create(0, MipsExpr, MCFixupKind(FixupKind))); Fixups.push_back(MCFixup::create(0, MipsExpr, MCFixupKind(FixupKind)));
return 0; return 0;
} }
if (Kind == MCExpr::SymbolRef) { if (Kind == MCExpr::SymbolRef) {
Ctx.reportWarning(Expr->getLoc(), "should use explicit constraints!");
Mips::Fixups FixupKind = Mips::Fixups(0); Mips::Fixups FixupKind = Mips::Fixups(0);
switch(cast<MCSymbolRefExpr>(Expr)->getKind()) { switch(cast<MCSymbolRefExpr>(Expr)->getKind()) {
default: llvm_unreachable("Unknown fixup kind!"); default: llvm_unreachable("Unknown fixup kind!");
break; break;
case MCSymbolRefExpr::VK_None: case MCSymbolRefExpr::VK_None:
FixupKind = Mips::fixup_Mips_32; // FIXME: This is ok for O32/N32 but not N64. FixupKind = Mips::fixup_Mips_32; // FIXME: This is ok for O32/N32 but not N64.
break; break;
} // switch } // switch
if (!STI.getFeatureBits()[Mips::FeatureMips64])
Fixups.push_back(MCFixup::create(0, Expr, MCFixupKind(FixupKind))); Fixups.push_back(MCFixup::create(0, Expr, MCFixupKind(FixupKind)));
else
Fixups.push_back(MCFixup::create(0, Expr, MCFixupKind(Mips::fixup_MIPS_PCLO16)));
return 0; return 0;
} }
return 0; return 0;
} }
/// getMachineOpValue - Return binary encoding of operand. If the machine /// getMachineOpValue - Return binary encoding of operand. If the machine
/// operand requires relocation, record the relocation and return zero. /// operand requires relocation, record the relocation and return zero.
unsigned MipsMCCodeEmitter:: unsigned MipsMCCodeEmitter::
▲ Show 20 Lines • Show All 398 Lines • Show Last 20 Lines

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

  • This file was added.
# 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