Commit: Fix unsigned vs signed comparisons in SH code

Message ID 87o8wyivfr.fsf@redhat.com
State New
Headers show
Series
  • Commit: Fix unsigned vs signed comparisons in SH code
Related show

Commit Message

Nick Clifton Nov. 26, 2019, 2:06 p.m.
Hi Guys,

 It seems that recent versions of clang have a new warning that catches
 code like this:

    if (sym_value < (bfd_vma) -0x1000 || sym_value >= 0x1000)

  So I am checking in the patch below to fix a couple of places where
  this happens.  There may be more, but I am not using the latest
  version of clang myself.

Cheers
  Nick

bfd/ChangeLog
2019-11-26  Nick Clifton  <nickc@redhat.com>

	* elf32-sh.c (sh_elf_reloc): Use a signed_vma when checking for a
	negative relocated value.
	* coff-sh.c (sh_reloc): Likewise.

Comments

Andreas Schwab Nov. 26, 2019, 3:32 p.m. | #1
On Nov 26 2019, Nick Clifton wrote:

>  It seems that recent versions of clang have a new warning that catches

>  code like this:

>

>     if (sym_value < (bfd_vma) -0x1000 || sym_value >= 0x1000)


Why is that a unsigned vs signed comparison?  Both operands are of type
bfd_vma.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Tim Rühsen Nov. 26, 2019, 4:14 p.m. | #2
On 11/26/19 4:32 PM, Andreas Schwab wrote:
> On Nov 26 2019, Nick Clifton wrote:

> 

>>  It seems that recent versions of clang have a new warning that catches

>>  code like this:

>>

>>     if (sym_value < (bfd_vma) -0x1000 || sym_value >= 0x1000)

> 

> Why is that a unsigned vs signed comparison?  Both operands are of type

> bfd_vma.


You are right, it isn't.

It is clang-10 (possibly clang-9, but not installed here):

warning: overlapping comparisons always evaluate to true
[-Wtautological-overlap-compare]

So the comparison works likely not as intended.

Regards, Tim
Nick Clifton Nov. 26, 2019, 4:45 p.m. | #3
Hi Guys,

>> Why is that a unsigned vs signed comparison?  Both operands are of type

>> bfd_vma.

> 

> You are right, it isn't.

> 

> It is clang-10 (possibly clang-9, but not installed here):

> 

> warning: overlapping comparisons always evaluate to true

> [-Wtautological-overlap-compare]

> 

> So the comparison works likely not as intended.


Right, sorry - I had the wrong description of the warning in my subject line.

Cheers
  Nick
Alan Modra Nov. 27, 2019, 1:27 a.m. | #4
On Tue, Nov 26, 2019 at 02:06:16PM +0000, Nick Clifton wrote:
> -      if (sym_value < (bfd_vma) -0x1000 || sym_value >= 0x1000)

> +      if ((bfd_signed_vma) sym_value < -0x1000 || sym_value >= 0x1000)


Presumably the idea is to check for the range [-0x1000,0x1000).  This
still isn't correct.  sym_value >= 0x1000 is true for 0x1000 to
0xff..ffff.  (And I guess clang ought to complain that this test
subsumes the sym_value < -0x1000 test.)

Huh, but there are more bugs here.

      sym_value += (insn & 0xfff) << 1;
      if (insn & 0x800)
	sym_value -= 0x1000;
      insn = (insn & 0xf000) | (sym_value & 0xfff);

Pull a value out of the insn, shift it left, then put it back?
There's a missing shift right here.

I'm about to apply the following patch.

Using bfd_vma for insn is to avoid having to worry about sign
propagation in expressions involving insn and sym_value when bfd_vma
is not the same as unsigned long.

	* elf32-sh.c (sh_reloc): Use a bfd_vma insn.
	(sh_reloc <R_SH_IND12W>): Divide calculated relocation value
	by two before applying to insn.  Correct overflow test.
	* coff-sh.c (sh_reloc): Likewise.

diff --git a/bfd/coff-sh.c b/bfd/coff-sh.c
index 1077a20e6c..e1bfaf0a04 100644
--- a/bfd/coff-sh.c
+++ b/bfd/coff-sh.c
@@ -567,7 +567,7 @@ sh_reloc (bfd *      abfd,
 	  bfd *      output_bfd,
 	  char **    error_message ATTRIBUTE_UNUSED)
 {
-  unsigned long insn;
+  bfd_vma insn;
   bfd_vma sym_value;
   unsigned short r_type;
   bfd_vma addr = reloc_entry->address;
@@ -610,14 +610,14 @@ sh_reloc (bfd *      abfd,
 #endif
       insn = bfd_get_32 (abfd, hit_data);
       insn += sym_value + reloc_entry->addend;
-      bfd_put_32 (abfd, (bfd_vma) insn, hit_data);
+      bfd_put_32 (abfd, insn, hit_data);
       break;
 #ifdef COFF_WITH_PE
     case R_SH_IMAGEBASE:
       insn = bfd_get_32 (abfd, hit_data);
       insn += sym_value + reloc_entry->addend;
       insn -= pe_data (input_section->output_section->owner)->pe_opthdr.ImageBase;
-      bfd_put_32 (abfd, (bfd_vma) insn, hit_data);
+      bfd_put_32 (abfd, insn, hit_data);
       break;
 #endif
     case R_SH_PCDISP:
@@ -627,12 +627,10 @@ sh_reloc (bfd *      abfd,
 		    + input_section->output_offset
 		    + addr
 		    + 4);
-      sym_value += (insn & 0xfff) << 1;
-      if (insn & 0x800)
-	sym_value -= 0x1000;
-      insn = (insn & 0xf000) | (sym_value & 0xfff);
-      bfd_put_16 (abfd, (bfd_vma) insn, hit_data);
-      if ((bfd_signed_vma) sym_value < -0x1000 || sym_value >= 0x1000)
+      sym_value += (((insn & 0xfff) ^ 0x800) - 0x800) << 1;
+      insn = (insn & 0xf000) | ((sym_value >> 1) & 0xfff);
+      bfd_put_16 (abfd, insn, hit_data);
+      if (sym_value + 0x1000 >= 0x2000 || (sym_value & 1) != 0)
 	return bfd_reloc_overflow;
       break;
     default:
diff --git a/bfd/elf32-sh.c b/bfd/elf32-sh.c
index 863e2e1bfc..be4256c585 100644
--- a/bfd/elf32-sh.c
+++ b/bfd/elf32-sh.c
@@ -232,7 +232,7 @@ sh_elf_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol_in,
 	      void *data, asection *input_section, bfd *output_bfd,
 	      char **error_message ATTRIBUTE_UNUSED)
 {
-  unsigned long insn;
+  bfd_vma insn;
   bfd_vma sym_value;
   enum elf_sh_reloc_type r_type;
   bfd_vma addr = reloc_entry->address;
@@ -274,7 +274,7 @@ sh_elf_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol_in,
     case R_SH_DIR32:
       insn = bfd_get_32 (abfd, hit_data);
       insn += sym_value + reloc_entry->addend;
-      bfd_put_32 (abfd, (bfd_vma) insn, hit_data);
+      bfd_put_32 (abfd, insn, hit_data);
       break;
     case R_SH_IND12W:
       insn = bfd_get_16 (abfd, hit_data);
@@ -283,12 +283,10 @@ sh_elf_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol_in,
 		    + input_section->output_offset
 		    + addr
 		    + 4);
-      sym_value += (insn & 0xfff) << 1;
-      if (insn & 0x800)
-	sym_value -= 0x1000;
-      insn = (insn & 0xf000) | (sym_value & 0xfff);
-      bfd_put_16 (abfd, (bfd_vma) insn, hit_data);
-      if ((bfd_signed_vma) sym_value < -0x1000 || sym_value >= 0x1000)
+      sym_value += (((insn & 0xfff) ^ 0x800) - 0x800) << 1;
+      insn = (insn & 0xf000) | ((sym_value >> 1) & 0xfff);
+      bfd_put_16 (abfd, insn, hit_data);
+      if (sym_value + 0x1000 >= 0x2000 || (sym_value & 1) != 0)
 	return bfd_reloc_overflow;
       break;
     default:


-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/bfd/coff-sh.c b/bfd/coff-sh.c
index 4b6b0de542..1077a20e6c 100644
--- a/bfd/coff-sh.c
+++ b/bfd/coff-sh.c
@@ -632,7 +632,7 @@  sh_reloc (bfd *      abfd,
 	sym_value -= 0x1000;
       insn = (insn & 0xf000) | (sym_value & 0xfff);
       bfd_put_16 (abfd, (bfd_vma) insn, hit_data);
-      if (sym_value < (bfd_vma) -0x1000 || sym_value >= 0x1000)
+      if ((bfd_signed_vma) sym_value < -0x1000 || sym_value >= 0x1000)
 	return bfd_reloc_overflow;
       break;
     default:
diff --git a/bfd/elf32-sh.c b/bfd/elf32-sh.c
index 8aa49b099c..863e2e1bfc 100644
--- a/bfd/elf32-sh.c
+++ b/bfd/elf32-sh.c
@@ -288,7 +288,7 @@  sh_elf_reloc (bfd *abfd, arelent *reloc_entry, asymbol *symbol_in,
 	sym_value -= 0x1000;
       insn = (insn & 0xf000) | (sym_value & 0xfff);
       bfd_put_16 (abfd, (bfd_vma) insn, hit_data);
-      if (sym_value < (bfd_vma) -0x1000 || sym_value >= 0x1000)
+      if ((bfd_signed_vma) sym_value < -0x1000 || sym_value >= 0x1000)
 	return bfd_reloc_overflow;
       break;
     default: