[PATCH] Only warn when missing MIPS LO16 are encountered

[PATCH] Only warn when missing MIPS LO16 are encountered

[Thiemo Seufer]

Hello All,

I applied the appended patch, it downgrades missing LO16 relocations
from an error to a warning. This unbreaks builds with existing
compilers but still gives gcc developers a better chance to find and
fix the problematic bits in the optimizer.


Thiemo


2007-03-26  Thiemo Seufer  <ths@mips.com>

	* elfxx-mips.c (mips_elf_next_relocation): Don't signal an error if no
	matching relocation is found.
	(_bfd_mips_elf_relocate_section): Only warn about missing relocations.


Index: bfd/elfxx-mips.c
===================================================================
RCS file: /cvs/src/src/bfd/elfxx-mips.c,v
retrieving revision 1.197
diff -u -p -r1.197 elfxx-mips.c
--- bfd/elfxx-mips.c	21 Mar 2007 04:03:09 -0000	1.197
+++ bfd/elfxx-mips.c	26 Mar 2007 10:23:02 -0000
@@ -3615,7 +3615,6 @@ mips_elf_next_relocation (bfd *abfd ATTR
     }
 
   /* We didn't find it.  */
-  bfd_set_error (bfd_error_bad_value);
   return NULL;
 }
 
@@ -7857,10 +7856,8 @@ _bfd_mips_elf_relocate_section (bfd *out
 		      && mips_elf_local_relocation_p (input_bfd, rel,
 						      local_sections, FALSE)))
 		{
-		  bfd_vma l;
 		  const Elf_Internal_Rela *lo16_relocation;
 		  reloc_howto_type *lo16_howto;
-		  bfd_byte *lo16_location;
 		  int lo16_type;
 
 		  if (r_type == R_MIPS16_HI16)
@@ -7883,7 +7880,12 @@ _bfd_mips_elf_relocate_section (bfd *out
 		     several relocations for the same address.  In
 		     that case, the R_MIPS_LO16 relocation may occur
 		     as one of these.  We permit a similar extension
-		     in general, as that is useful for GCC.  */
+		     in general, as that is useful for GCC.
+
+		     In some cases GCC dead code elimination removes
+		     the LO16 but keeps the corresponding HI16.  This
+		     is strictly speaking a violation of the ABI but
+		     not immediately harmful.  */
 		  lo16_relocation = mips_elf_next_relocation (input_bfd,
 							      lo16_type,
 							      rel, relend);
@@ -7901,28 +7903,33 @@ _bfd_mips_elf_relocate_section (bfd *out
 			(_("%B: Can't find matching LO16 reloc against `%s' for %s at 0x%lx in section `%A'"),
 			 input_bfd, input_section, name, howto->name,
 			 rel->r_offset);
-		      return FALSE;
 		    }
+		  else
+		    {
+		      bfd_byte *lo16_location;
+		      bfd_vma l;
 
-		  lo16_location = contents + lo16_relocation->r_offset;
+		      lo16_location = contents + lo16_relocation->r_offset;
 
-		  /* Obtain the addend kept there.  */
-		  lo16_howto = MIPS_ELF_RTYPE_TO_HOWTO (input_bfd,
-							lo16_type, FALSE);
-		  _bfd_mips16_elf_reloc_unshuffle (input_bfd, lo16_type, FALSE,
-						   lo16_location);
-		  l = mips_elf_obtain_contents (lo16_howto, lo16_relocation,
-						input_bfd, contents);
-		  _bfd_mips16_elf_reloc_shuffle (input_bfd, lo16_type, FALSE,
-						 lo16_location);
-		  l &= lo16_howto->src_mask;
-		  l <<= lo16_howto->rightshift;
-		  l = _bfd_mips_elf_sign_extend (l, 16);
+		      /* Obtain the addend kept there.  */
+		      lo16_howto = MIPS_ELF_RTYPE_TO_HOWTO (input_bfd,
+							    lo16_type, FALSE);
+		      _bfd_mips16_elf_reloc_unshuffle (input_bfd, lo16_type,
+						       FALSE, lo16_location);
+		      l = mips_elf_obtain_contents (lo16_howto,
+						    lo16_relocation,
+						    input_bfd, contents);
+		      _bfd_mips16_elf_reloc_shuffle (input_bfd, lo16_type,
+						     FALSE, lo16_location);
+		      l &= lo16_howto->src_mask;
+		      l <<= lo16_howto->rightshift;
+		      l = _bfd_mips_elf_sign_extend (l, 16);
 
-		  addend <<= 16;
+		      addend <<= 16;
 
-		  /* Compute the combined addend.  */
-		  addend += l;
+		      /* Compute the combined addend.  */
+		      addend += l;
+		    }
 		}
 	      else
 		addend <<= howto->rightshift;

Re: [PATCH] Only warn when missing MIPS LO16 are encountered

[Paul Koning]

>>>>> "Thiemo" == Thiemo Seufer <ths@networkno.de> writes:

 Thiemo> Hello All, I applied the appended patch, it downgrades
 Thiemo> missing LO16 relocations from an error to a warning. This
 Thiemo> unbreaks builds with existing compilers but still gives gcc
 Thiemo> developers a better chance to find and fix the problematic
 Thiemo> bits in the optimizer.

Warnings are still a problem when you're using the "warnings are
errors" principle, as we and others do.

Does this "missing" LO16 relocation cause any problem?  If not, why
should there be a warning, and for that matter, why should this be
called "problematic bits in the optimizer"?

       paul

Re: [PATCH] Only warn when missing MIPS LO16 are encountered

[Thiemo Seufer]

Paul Koning wrote:
> >>>>> "Thiemo" == Thiemo Seufer <ths@networkno.de> writes:
> 
>  Thiemo> Hello All, I applied the appended patch, it downgrades
>  Thiemo> missing LO16 relocations from an error to a warning. This
>  Thiemo> unbreaks builds with existing compilers but still gives gcc
>  Thiemo> developers a better chance to find and fix the problematic
>  Thiemo> bits in the optimizer.
> 
> Warnings are still a problem when you're using the "warnings are
> errors" principle, as we and others do.
> 
> Does this "missing" LO16 relocation cause any problem?

It is a long-standing violation of the MIPS O32 ABI rules. It causes
unexpected cornercases for linker implementations (as seen here again)
but the resulting binaries run ok.

> If not, why
> should there be a warning, and for that matter, why should this be
> called "problematic bits in the optimizer"?

The optimizer should eliminate the whole HI/LO relocation pair.
Currently it doesn't do that in all cases.


Thiemo

Re: [PATCH] Only warn when missing MIPS LO16 are encountered

[H. J. Lu]

On Mon, Mar 26, 2007 at 04:53:56PM +0100, Thiemo Seufer wrote:
> Paul Koning wrote:
> > >>>>> "Thiemo" == Thiemo Seufer <ths@networkno.de> writes:
> > 
> >  Thiemo> Hello All, I applied the appended patch, it downgrades
> >  Thiemo> missing LO16 relocations from an error to a warning. This
> >  Thiemo> unbreaks builds with existing compilers but still gives gcc
> >  Thiemo> developers a better chance to find and fix the problematic
> >  Thiemo> bits in the optimizer.
> > 
> > Warnings are still a problem when you're using the "warnings are
> > errors" principle, as we and others do.
> > 
> > Does this "missing" LO16 relocation cause any problem?
> 
> It is a long-standing violation of the MIPS O32 ABI rules. It causes
> unexpected cornercases for linker implementations (as seen here again)
> but the resulting binaries run ok.
> 
> > If not, why
> > should there be a warning, and for that matter, why should this be
> > called "problematic bits in the optimizer"?
> 
> The optimizer should eliminate the whole HI/LO relocation pair.
> Currently it doesn't do that in all cases.

Can gas turn instruction with HI relocation without matching LO
relocation into no-op?


H.J.

RE: [PATCH] Only warn when missing MIPS LO16 are encountered

[Dave Korn]

On 26 March 2007 17:19, H. J. Lu wrote:

> On Mon, Mar 26, 2007 at 04:53:56PM +0100, Thiemo Seufer wrote:
>> Paul Koning wrote:
>>>>>>>> "Thiemo" == Thiemo Seufer <ths@networkno.de> writes:
>>> 
>>>  Thiemo> Hello All, I applied the appended patch, it downgrades
>>>  Thiemo> missing LO16 relocations from an error to a warning. This
>>>  Thiemo> unbreaks builds with existing compilers but still gives gcc
>>>  Thiemo> developers a better chance to find and fix the problematic
>>>  Thiemo> bits in the optimizer.
>>> 
>>> Warnings are still a problem when you're using the "warnings are
>>> errors" principle, as we and others do.
>>> 
>>> Does this "missing" LO16 relocation cause any problem?
>> 
>> It is a long-standing violation of the MIPS O32 ABI rules. It causes
>> unexpected cornercases for linker implementations (as seen here again)
>> but the resulting binaries run ok.
>> 
>>> If not, why
>>> should there be a warning, and for that matter, why should this be
>>> called "problematic bits in the optimizer"?
>> 
>> The optimizer should eliminate the whole HI/LO relocation pair.
>> Currently it doesn't do that in all cases.
> 
> Can gas turn instruction with HI relocation without matching LO
> relocation into no-op?

  Not without altering the program semantics, I would have thought.  At the
moment, these can only arise through 1) faulty compiler 2) hand-coded
assembly.  If we silently turn them into nops, we don't get any warning that
the compiler has emitted bad code.  If the user has hand-coded one, they
probably have some idea what they mean by it, and would probably like the
assembler to interpret it according to least-surprise.

  I think the sensible thing to do is just to treat unmatched HI16 as being
the equivalent of (addr & ~0xffff), i.e. assume that if there was a lo part,
it wouldn't carry into the high part.

    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....

Re: [PATCH] Only warn when missing MIPS LO16 are encountered

[Richard Sandiford]

"Dave Korn" <dave.korn@artimi.com> writes:
> On 26 March 2007 17:19, H. J. Lu wrote:
>> On Mon, Mar 26, 2007 at 04:53:56PM +0100, Thiemo Seufer wrote:
>>> Paul Koning wrote:
>>>>>>>>> "Thiemo" == Thiemo Seufer <ths@networkno.de> writes:
>>>> 
>>>>  Thiemo> Hello All, I applied the appended patch, it downgrades
>>>>  Thiemo> missing LO16 relocations from an error to a warning. This
>>>>  Thiemo> unbreaks builds with existing compilers but still gives gcc
>>>>  Thiemo> developers a better chance to find and fix the problematic
>>>>  Thiemo> bits in the optimizer.
>>>> 
>>>> Warnings are still a problem when you're using the "warnings are
>>>> errors" principle, as we and others do.
>>>> 
>>>> Does this "missing" LO16 relocation cause any problem?
>>> 
>>> It is a long-standing violation of the MIPS O32 ABI rules. It causes
>>> unexpected cornercases for linker implementations (as seen here again)
>>> but the resulting binaries run ok.
>>> 
>>>> If not, why
>>>> should there be a warning, and for that matter, why should this be
>>>> called "problematic bits in the optimizer"?
>>> 
>>> The optimizer should eliminate the whole HI/LO relocation pair.
>>> Currently it doesn't do that in all cases.
>> 
>> Can gas turn instruction with HI relocation without matching LO
>> relocation into no-op?
>
>   Not without altering the program semantics, I would have thought.

The problem is, %hi16 without %lo16 has no defined semantics,
so there aren't really any semantics to change.  I don't think...

> At the moment, these can only arise through 1) faulty compiler 2)
> hand-coded assembly.  If we silently turn them into nops, we don't get
> any warning that the compiler has emitted bad code.  If the user has
> hand-coded one, they probably have some idea what they mean by it, and
> would probably like the assembler to interpret it according to
> least-surprise.

...hand-coding an orphaned %hi16 is really a reasonable thing to do.

How about HJ's suggestion?  Would there be any problem with getting
gas to drop orphaned %hi16s, with an appropriate warning?  (And doing
it in addition to whatever the linker eventually does?)  The warning
could be under option control, so it could be turned off by -Werror
users.  FWIW, I think warning in the assembler would help gcc testers,
because it would also pick up problems in non-link tests.

Richard

Re: [PATCH] Only warn when missing MIPS LO16 are encountered

[Eric Christopher]

>>
>>   Not without altering the program semantics, I would have thought.
>
> The problem is, %hi16 without %lo16 has no defined semantics,
> so there aren't really any semantics to change.  I don't think...
>

Agreed.

>> At the moment, these can only arise through 1) faulty compiler 2)
>> hand-coded assembly.  If we silently turn them into nops, we don't  
>> get
>> any warning that the compiler has emitted bad code.  If the user has
>> hand-coded one, they probably have some idea what they mean by it,  
>> and
>> would probably like the assembler to interpret it according to
>> least-surprise.
>
> ...hand-coding an orphaned %hi16 is really a reasonable thing to do.
>
> How about HJ's suggestion?  Would there be any problem with getting
> gas to drop orphaned %hi16s, with an appropriate warning?  (And doing
> it in addition to whatever the linker eventually does?)  The warning
> could be under option control, so it could be turned off by -Werror
> users.  FWIW, I think warning in the assembler would help gcc testers,
> because it would also pick up problems in non-link tests.

I think this sounds like the best idea.

-eric