Bug 16720 - wrong overflow check in R_MIPS_HI16
Summary: wrong overflow check in R_MIPS_HI16
Status: CLOSED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-18 07:37 UTC by ma.jiang
Modified: 2016-12-02 04:33 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
source file (112 bytes, text/plain)
2014-03-18 07:37 UTC, ma.jiang
Details
linker script (2.78 KB, text/x-csrc)
2014-03-18 07:38 UTC, ma.jiang
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ma.jiang 2014-03-18 07:37:51 UTC
Created attachment 7478 [details]
source file

There is a overflow check in mips ld.
=================================================================
	  if (r_type == R_MIPS16_HI16)
	    value = mips_elf_high (addend + gp - p - 4);
	  else
	    value = mips_elf_high (addend + gp - p);
	  overflowed_p = mips_elf_overflow_p (value, 16);
=================================================================
This check might have some problems when "addend + gp - p" is a negative number.In my cases, I got "addend + gp - p=-132666256".This number should be ok for a "R_MIPS16_HI16+R_MIPS16_LO16" as it obviously could be put into a 32bits-signed-int.
But, the ld throw a overflow error. First, it get a value=63512 from mips_elf_high, then it check if this value could be put into a 16bits-signed-int in mips_elf_overflow_p. And of course, 63512 can not be put into a 16bits-signed-int.So,a wrong overflow error is generated finally.
In my opinion, we only need to check whether "addend + gp - p"  could be put into a 32bits-signed-int in R_MIPS16_HI16. Because, a 32bits-signed-int can be expressed correctly by R_MIPS16_HI16+R_MIPS16_LO16. The code could be like:
	  bfd_vma offset;
	  if (r_type == R_MIPS16_HI16)
	  {
	  	  value = mips_elf_high (addend + gp - p - 4);
	  	  offset = addend + gp - p - 4;
	  }
	  else
	  {
	    	  value = mips_elf_high (addend + gp - p);
	  	  offset = addend + gp - p;
	  }
	  overflowed_p = mips_elf_overflow_p (offset, 32);
****************************************************************************
This bug can be reproduced by attached files, using commands like:
gcc ldtest.c -o ldtest -Wl,-T bug.lds  -static -fPIC
Comment 1 ma.jiang 2014-03-18 07:38:51 UTC
Created attachment 7479 [details]
linker script
Comment 2 ma.jiang 2014-08-05 02:01:20 UTC
still no reply?
I can not build my glibc(with -g3 option) due to this bug...
Comment 3 ma.jiang 2016-11-28 07:29:26 UTC
Hi all,
  This bug still exist in the trunk code. Using the attached files, I can still get the error "relocation truncated to fit: R_MIPS_HI16 against `_gp_disp'". This is *NOT* right, as the mips abi said that R_MIPS_HI16 need no overflow checks. Moreover, Only the o32 abi support R_MIPS_HI16 for _gp_disp, per the mips abi. R_MIPS_HI16  should never get overflowed, because under o32 abi, the address width is 32.
  So, I think we should just get rid of the overflow check for R_MIPS_HI16. Here is the patch for trunk.
--- bfd/elfxx-mips.c.orig	2016-11-28 23:09:23.343671301 +0800
+++ bfd/elfxx-mips.c	2016-11-28 23:23:49.452670956 +0800
@@ -5875,7 +5875,6 @@ mips_elf_calculate_relocation (bfd *abfd
 	    value = mips_elf_high (addend + gp - p - 1);
 	  else
 	    value = mips_elf_high (addend + gp - p);
-	  overflowed_p = mips_elf_overflow_p (value, 16);
 	}
       break;
Comment 4 cvs-commit@gcc.gnu.org 2016-12-01 12:22:35 UTC
The master branch has been updated by Nick Clifton <nickc@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=40a0bfddf07620f5321927b3231502debb3b73bc

commit 40a0bfddf07620f5321927b3231502debb3b73bc
Author: Ma Jiang <ma.jiang@zte.com.cn>
Date:   Thu Dec 1 12:21:30 2016 +0000

    Fix handling of MIPS16 HI16 relocs.
    
    	PR ld/16720
    	* elfxx-mips.c (mips_elf_calculate_relocation): Remove overflow
    	test for HI16 relocs.
Comment 5 Nick Clifton 2016-12-01 12:28:02 UTC
Hi Ma,

  Sorry for the delay in reviewing this PR.

  I agree with your analysis, so I have gone ahead and checked in your
  suggested patch.  Thanks very much for creating it!

Cheers
  Nick
Comment 6 ma.jiang 2016-12-02 04:33:41 UTC
(In reply to Nick Clifton from comment #5)
> Hi Ma,
> 
>   Sorry for the delay in reviewing this PR.
> 
>   I agree with your analysis, so I have gone ahead and checked in your
>   suggested patch.  Thanks very much for creating it!
> 
> Cheers
>   Nick
Hi Nick,
  It's happy to see this local patch go upstream. Thanks, I'll close the bug now.