Page MenuHomePhabricator

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

Authored by hev on Jun 15 2020, 10:05 PM.

Details

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.

Diff Detail

Event Timeline

hev created this revision.Jun 15 2020, 10:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2020, 10:05 PM
xiangzhai added a subscriber: xiangzhai.
MaskRay added a comment.EditedJun 15 2020, 11:31 PM

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 updated this revision to Diff 270999.Jun 16 2020, 2:38 AM

Add a unit test case.

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?

Does this patch depend on D81919?

No!

hev marked 3 inline comments as done.Jun 16 2020, 5:15 AM

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.

hev updated this revision to Diff 271062.Jun 16 2020, 5:31 AM

Does this patch depend on D81919?

No!

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

hev added a comment.Jun 16 2020, 7:22 AM

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 added a comment.EditedJun 16 2020, 10:14 PM

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.

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 marked an inline comment as done.Jun 17 2020, 2:03 AM

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 added a comment.EditedJun 19 2020, 10:04 AM

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

atanasyan accepted this revision.Jun 19 2020, 11:40 AM

LGTM

Do you have commit access?

This revision is now accepted and ready to land.Jun 19 2020, 11:40 AM
hev marked an inline comment as done.Jun 19 2020, 6:12 PM

LGTM

Thank you.

Do you have commit access?

No.

MaskRay requested changes to this revision.Jun 19 2020, 7:05 PM

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

Please fix the comment first.

This revision now requires changes to proceed.Jun 19 2020, 7:05 PM
hev updated this revision to Diff 272228.Jun 19 2020, 8:18 PM
hev edited the summary of this revision. (Show Details)

@MaskRay Sorry, I missed the comment. :)

hev updated this revision to Diff 272230.Jun 19 2020, 8:42 PM
xiangzhai added a comment.EditedJun 19 2020, 9:32 PM

Do you have commit access?

Please commit it on @hev behalf.

Thanks,
Leslie Zhai

MaskRay updated this revision to Diff 272234.Jun 19 2020, 10:01 PM

Fix code style issue
Improve tests

MaskRay accepted this revision.Jun 19 2020, 10:05 PM
MaskRay retitled this revision from [MIPS] Fix incorrect relocations of instruction to [Mips] Error if a non-immediate operand is used while an immediate is expected.
MaskRay edited the summary of this revision. (Show Details)

LGTM.

This revision is now accepted and ready to land.Jun 19 2020, 10:05 PM
MaskRay updated this revision to Diff 272235.Jun 19 2020, 10:08 PM

Simplify -triple

This revision was automatically updated to reflect the committed changes.