Re: [PATCH] MIPS/BFD: Fix assertions with SGI IRIX files

Re: [PATCH] MIPS/BFD: Fix assertions with SGI IRIX files

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> Hello,
>
>  If an SGI IRIX ELF variation binary (also produced by binutils for some 
> target configurations) is loaded into GDB configured for the traditional 
> MIPS ELF variation, then a load of assertion failures is spat.  They 
> actually come from BFD being confused.
>
>  As no user input, however unreasonable, should ever trigger assertions 
> and this is simply a case of an unsupported binary format, here's a fix to 
> convert the offending code to do proper error reporting instead.
>
>  Verified manually with GDB, the error messages are produced.  I was 
> unable to trigger this problem with the linker, even though this piece of 
> code looks reachable within to me.

How about just dropping the assertions?  SHN_MIPS_TEXT and SHN_MIPS_DATA
are easy to handle, and it looks like the code will behave sensibly
regardless of SGI_COMPAT.  I realise you might be thinking that the user
would like to know that they have the "wrong" emulation.  But if this
is the only detectable difference, it's better just to carry on.
If there are other detectable differences that cause real problems,
we should try to diagnose the problem there instead.

A patch to remove the assertions is pre-approved if you agree that's OK.

Richard

Re: [PATCH] MIPS/BFD: Fix assertions with SGI IRIX files

[Maciej W. Rozycki]

On Fri, 19 Aug 2011, Richard Sandiford wrote:

> >  If an SGI IRIX ELF variation binary (also produced by binutils for some 
> > target configurations) is loaded into GDB configured for the traditional 
> > MIPS ELF variation, then a load of assertion failures is spat.  They 
> > actually come from BFD being confused.
> >
> >  As no user input, however unreasonable, should ever trigger assertions 
> > and this is simply a case of an unsupported binary format, here's a fix to 
> > convert the offending code to do proper error reporting instead.
> >
> >  Verified manually with GDB, the error messages are produced.  I was 
> > unable to trigger this problem with the linker, even though this piece of 
> > code looks reachable within to me.
> 
> How about just dropping the assertions?  SHN_MIPS_TEXT and SHN_MIPS_DATA
> are easy to handle, and it looks like the code will behave sensibly
> regardless of SGI_COMPAT.

 Are you sure this code is safe under the matching conditions given the 
assertions to catch them?  They were put there for a reason I presume, 
perhaps exactly because the originator was not sure about this safety.  
Do you happen to remember the story behind?  I believe it's a moderately 
recent addition as I do remember the times the SGI emulation was the only 
one we handled (circa AD 1999).

> I realise you might be thinking that the user would like to know that 
> they have the "wrong" emulation.

 No, actually not at all.  There's no point in making noise if things work 
correctly; this just confuses people.

 OTOH I think it would definitely make sense to teach tools like 
`objdump', `readelf' and `file' even how to tell the MIPS ELF flavours 
apart and report that to the user appropriately, although I realise this 
may not necessarily be straightforward or reliable.

 Also I think emulation selection could be made a bit more flexible in GDB 
-- my observation has been that extra emulations are not easily accessible 
even if built into BFD -- the logic just picks the first one that does not 
reject the binary chosen outright and this does not really work well for 
making the choice between traditional and SGI flavour MIPS ELF format.  
That's a separate problem though and frankly a marginal one as far as I'm 
concerned.

> But if this is the only detectable difference, it's better just to carry 
> on. If there are other detectable differences that cause real problems, 
> we should try to diagnose the problem there instead.

 Agreed.

> A patch to remove the assertions is pre-approved if you agree that's OK.

 OK, but are you positive that'll be no regression?  As would be silent 
mishandling compared to one accompanied with these assertions, however 
obscure they are.

  Maciej

Re: [PATCH] MIPS/BFD: Fix assertions with SGI IRIX files

[Maciej W. Rozycki]

On Wed, 31 Aug 2011, Richard Sandiford wrote:

> >> A patch to remove the assertions is pre-approved if you agree that's OK.
> >
> >  OK, but are you positive that'll be no regression?
> 
> This is binutils.  You can never be positive that a patch won't break
> something.  But the handling of SHN_MIPS_DATA and SHN_MIPS_TEXT isn't
> written in an SGI-specific way (just as the handling of the other
> SHN_MIPS_* sections isn't written in an SGI-specific way).  I couldn't see,
> and still can't see, a reason why removing the assertions is wrong.

 OK, this is good enough a justification for me, thanks for investigating.  
I meant any improper handling within BFD itself of course.  I'll make that 
change and push it at my earliest convenience.  It may take a couple of 
days.

  Maciej

Re: [PATCH] MIPS/BFD: Fix assertions with SGI IRIX files

[Maciej W. Rozycki]

On Fri, 2 Sep 2011, Maciej W. Rozycki wrote:

> > >> A patch to remove the assertions is pre-approved if you agree that's OK.
> > >
> > >  OK, but are you positive that'll be no regression?
> > 
> > This is binutils.  You can never be positive that a patch won't break
> > something.  But the handling of SHN_MIPS_DATA and SHN_MIPS_TEXT isn't
> > written in an SGI-specific way (just as the handling of the other
> > SHN_MIPS_* sections isn't written in an SGI-specific way).  I couldn't see,
> > and still can't see, a reason why removing the assertions is wrong.
> 
>  OK, this is good enough a justification for me, thanks for investigating.  
> I meant any improper handling within BFD itself of course.  I'll make that 
> change and push it at my earliest convenience.  It may take a couple of 
> days.

 It's been a couple of days a couple of times now, but there you go.  I 
have applied the change below now.

2011-10-24  Maciej W. Rozycki  <macro@codesourcery.com>

	bfd/
	* elfxx-mips.c (_bfd_mips_elf_symbol_processing): Remove 
	assertions.

  Maciej

binutils-mips-sgi-assert.diff
Index: binutils-fsf-trunk-quilt/bfd/elfxx-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/bfd/elfxx-mips.c	2011-10-24 14:48:39.655926020 +0100
+++ binutils-fsf-trunk-quilt/bfd/elfxx-mips.c	2011-10-24 14:49:14.745883975 +0100
@@ -6337,7 +6337,6 @@ _bfd_mips_elf_symbol_processing (bfd *ab
       {
 	asection *section = bfd_get_section_by_name (abfd, ".text");
 
-	BFD_ASSERT (SGI_COMPAT (abfd));
 	if (section != NULL)
 	  {
 	    asym->section = section;
@@ -6353,7 +6352,6 @@ _bfd_mips_elf_symbol_processing (bfd *ab
       {
 	asection *section = bfd_get_section_by_name (abfd, ".data");
 
-	BFD_ASSERT (SGI_COMPAT (abfd));
 	if (section != NULL)
 	  {
 	    asym->section = section;

Re: [PATCH] MIPS/BFD: Fix assertions with SGI IRIX files

[Richard Sandiford]

"Maciej W. Rozycki" <macro@codesourcery.com> writes:
> 2011-10-24  Maciej W. Rozycki  <macro@codesourcery.com>
>
> 	bfd/
> 	* elfxx-mips.c (_bfd_mips_elf_symbol_processing): Remove 
> 	assertions.

OK, thanks.

Richard

Re: [PATCH] MIPS/BFD: Fix assertions with SGI IRIX files

[Maciej W. Rozycki]

On Tue, 15 Nov 2011, Richard Sandiford wrote:

> > 	bfd/
> > 	* elfxx-mips.c (_bfd_mips_elf_symbol_processing): Remove 
> > 	assertions.
> 
> OK, thanks.

 Oh, this I had actually already applied based on your earlier 
preapproval.  I have now committed the other six changes you approved 
verbatim as well and will work on the seventh (MIPS16 mode PIC pseudo-ops) 
soon.  Thanks for the review.

  Maciej

[PATCH] MIPS/BFD: Fix assertions with SGI IRIX files

[Maciej W. Rozycki]

Hello,

 If an SGI IRIX ELF variation binary (also produced by binutils for some 
target configurations) is loaded into GDB configured for the traditional 
MIPS ELF variation, then a load of assertion failures is spat.  They 
actually come from BFD being confused.

 As no user input, however unreasonable, should ever trigger assertions 
and this is simply a case of an unsupported binary format, here's a fix to 
convert the offending code to do proper error reporting instead.

 Verified manually with GDB, the error messages are produced.  I was 
unable to trigger this problem with the linker, even though this piece of 
code looks reachable within to me.

 OK to apply?

2011-08-18  Maciej W. Rozycki  <macro@codesourcery.com>

	bfd/
	* elfxx-mips.c (_bfd_mips_elf_symbol_processing): Replace
	assertions with warning messages.

  Maciej

binutils-mips-sgi-err.diff
Index: binutils-fsf-trunk-quilt/bfd/elfxx-mips.c
===================================================================
--- binutils-fsf-trunk-quilt.orig/bfd/elfxx-mips.c	2011-08-03 14:02:30.000000000 +0100
+++ binutils-fsf-trunk-quilt/bfd/elfxx-mips.c	2011-08-03 14:04:07.000000000 +0100
@@ -6350,7 +6350,10 @@ _bfd_mips_elf_symbol_processing (bfd *ab
       {
 	asection *section = bfd_get_section_by_name (abfd, ".text");
 
-	BFD_ASSERT (SGI_COMPAT (abfd));
+	if (!SGI_COMPAT (abfd))
+	  _bfd_error_handler (_("%B: Warning: SGI IRIX format special section "
+				"%s unsupported with the selected emulation"),
+			      abfd, "MIPS_TEXT");
 	if (section != NULL)
 	  {
 	    asym->section = section;
@@ -6366,7 +6369,10 @@ _bfd_mips_elf_symbol_processing (bfd *ab
       {
 	asection *section = bfd_get_section_by_name (abfd, ".data");
 
-	BFD_ASSERT (SGI_COMPAT (abfd));
+	if (!SGI_COMPAT (abfd))
+	  _bfd_error_handler (_("%B: Warning: SGI IRIX format special section "
+				"%s unsupported with the selected emulation"),
+			      abfd, "MIPS_DATA");
 	if (section != NULL)
 	  {
 	    asym->section = section;