[v3,1/3] Use bfd_mach_mips4000 as the default machine type for 64-bit MIPS ABIs.

Message ID 20170103184341.58346-2-jhb@FreeBSD.org
State New, archived
Headers

Commit Message

John Baldwin Jan. 3, 2017, 6:43 p.m. UTC
  If the flags word of an ELF header is empty, _bfd_elf_mips_mach always
returned bfd_mach_mips3000 which is a 32-bit MIPS ABI.  This change
uses bfd_mach_mips4000 if the ELF class identifies a 64-bit binary.

bfd/ChangeLog:

	* elf32-mips.c: Pass "abfd" to "_bfd_elf_mips_mach".
	* elf64-mips.c: Likewise.
	* elfn32-mips.c: Likewise.
	* elfxx-mips.c (_bfd_elf_mips_mach): Read "flags" from ELF header
	in "abfd".  Use "bfd_mach_mips4000" as default machine for 64-bit
	ABIs.
	* elfxx-mips.h (_bfd_elf_mips_mach): Change argument from "flags"
	to "abfd".
---
 bfd/ChangeLog     | 11 +++++++++++
 bfd/elf32-mips.c  |  2 +-
 bfd/elf64-mips.c  |  2 +-
 bfd/elfn32-mips.c |  2 +-
 bfd/elfxx-mips.c  | 11 +++++++----
 bfd/elfxx-mips.h  |  2 +-
 6 files changed, 22 insertions(+), 8 deletions(-)
  

Comments

Maciej W. Rozycki Jan. 4, 2017, 2:09 a.m. UTC | #1
On Tue, 3 Jan 2017, John Baldwin wrote:

> If the flags word of an ELF header is empty, _bfd_elf_mips_mach always
> returned bfd_mach_mips3000 which is a 32-bit MIPS ABI.  This change
> uses bfd_mach_mips4000 if the ELF class identifies a 64-bit binary.

 Since this touches the MIPS port I'll have a look at it in details when I 
am back next week.

 In particular I'm a bit concerned about the inconsistency between n64 and 
n32 it introduces by making one default to `bfd_mach_mips4000' but not the 
other, while both are 64-bit ABIs requiring a 64-bit processor to run. 
Which is also already known at the time `_bfd_elf_mips_mach' is being 
called.  So rather than changing its API entirely perhaps we just need an 
extra `need_64bit' or suchlike extra argument for the callee to select the 
BFD appropriately if the ISA is set incorrectly in the ELF object.

 Also can you please remind me why this is the case in the first place and 
how exactly such ELF objects are annotated, e.g. can we identify (limit 
the handling of) such faulty objects with the EI_OSABI marker for example? 
NB this should be explained in details for posterity in the commit message 
itself.

 Thanks,

  Maciej
  
John Baldwin Jan. 4, 2017, 3:07 a.m. UTC | #2
On Wednesday, January 04, 2017 02:09:17 AM Maciej W. Rozycki wrote:
> On Tue, 3 Jan 2017, John Baldwin wrote:
> 
> > If the flags word of an ELF header is empty, _bfd_elf_mips_mach always
> > returned bfd_mach_mips3000 which is a 32-bit MIPS ABI.  This change
> > uses bfd_mach_mips4000 if the ELF class identifies a 64-bit binary.
> 
>  Since this touches the MIPS port I'll have a look at it in details when I 
> am back next week.
> 
>  In particular I'm a bit concerned about the inconsistency between n64 and 
> n32 it introduces by making one default to `bfd_mach_mips4000' but not the 
> other, while both are 64-bit ABIs requiring a 64-bit processor to run. 
> Which is also already known at the time `_bfd_elf_mips_mach' is being 
> called.  So rather than changing its API entirely perhaps we just need an 
> extra `need_64bit' or suchlike extra argument for the callee to select the 
> BFD appropriately if the ISA is set incorrectly in the ELF object.
> 
>  Also can you please remind me why this is the case in the first place and 
> how exactly such ELF objects are annotated, e.g. can we identify (limit 
> the handling of) such faulty objects with the EI_OSABI marker for example? 
> NB this should be explained in details for posterity in the commit message 
> itself.

The specific use case is that FreeBSD/mips kernels currently write out
process core dumps with the 'flags' field in the ELF header set to zero.
This is true for o32, n64, and n32.  I originally added a workaround in
elfcore_grok_freebsd_* to use the ELF class instead of 'bits_per_word' to
determine the layout of the process info and status core dump notes, but
switched to this approach in reponse to review of the first patch.

I would be fine with making this conditional on ELFOSABI_FREEBSD.  At some
point I will probably fix FreeBSD/mips kernels to set a suitable value
in e_flags (probably just copy it over from the binary) at which point
this workaround would no longer be needed, but that doesn't fix existing
cores.
  

Patch

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 41d511948a..b3ce1e5321 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,14 @@ 
+2017-01-03  John Baldwin  <jhb@FreeBSD.org>
+
+	* elf32-mips.c: Pass "abfd" to "_bfd_elf_mips_mach".
+	* elf64-mips.c: Likewise.
+	* elfn32-mips.c: Likewise.
+	* elfxx-mips.c (_bfd_elf_mips_mach): Read "flags" from ELF header
+	in "abfd".  Use "bfd_mach_mips4000" as default machine for 64-bit
+	ABIs.
+	* elfxx-mips.h (_bfd_elf_mips_mach): Change argument from "flags"
+	to "abfd".
+
 2017-01-03  Rich Felker  <bugdal@aerifal.cx>
 
 	PR ld/21017
diff --git a/bfd/elf32-mips.c b/bfd/elf32-mips.c
index 8c1a68eba0..6586272dab 100644
--- a/bfd/elf32-mips.c
+++ b/bfd/elf32-mips.c
@@ -2295,7 +2295,7 @@  mips_elf32_object_p (bfd *abfd)
   if (SGI_COMPAT (abfd))
     elf_bad_symtab (abfd) = TRUE;
 
-  mach = _bfd_elf_mips_mach (elf_elfheader (abfd)->e_flags);
+  mach = _bfd_elf_mips_mach (abfd);
   bfd_default_set_arch_mach (abfd, bfd_arch_mips, mach);
   return TRUE;
 }
diff --git a/bfd/elf64-mips.c b/bfd/elf64-mips.c
index 5edbd4a5d3..613d782534 100644
--- a/bfd/elf64-mips.c
+++ b/bfd/elf64-mips.c
@@ -4251,7 +4251,7 @@  mips_elf64_object_p (bfd *abfd)
   if (elf64_mips_irix_compat (abfd) != ict_none)
     elf_bad_symtab (abfd) = TRUE;
 
-  mach = _bfd_elf_mips_mach (elf_elfheader (abfd)->e_flags);
+  mach = _bfd_elf_mips_mach (abfd);
   bfd_default_set_arch_mach (abfd, bfd_arch_mips, mach);
   return TRUE;
 }
diff --git a/bfd/elfn32-mips.c b/bfd/elfn32-mips.c
index c09713ad53..d35af3ac54 100644
--- a/bfd/elfn32-mips.c
+++ b/bfd/elfn32-mips.c
@@ -3513,7 +3513,7 @@  mips_elf_n32_object_p (bfd *abfd)
   if (SGI_COMPAT (abfd))
     elf_bad_symtab (abfd) = TRUE;
 
-  mach = _bfd_elf_mips_mach (elf_elfheader (abfd)->e_flags);
+  mach = _bfd_elf_mips_mach (abfd);
   bfd_default_set_arch_mach (abfd, bfd_arch_mips, mach);
   return TRUE;
 }
diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c
index ce58c436e9..17f5f99ec3 100644
--- a/bfd/elfxx-mips.c
+++ b/bfd/elfxx-mips.c
@@ -6719,8 +6719,10 @@  mips_elf_create_dynamic_relocation (bfd *output_bfd,
 /* Return the MACH for a MIPS e_flags value.  */
 
 unsigned long
-_bfd_elf_mips_mach (flagword flags)
+_bfd_elf_mips_mach (bfd *abfd)
 {
+  flagword flags = elf_elfheader (abfd)->e_flags;
+
   switch (flags & EF_MIPS_MACH)
     {
     case E_MIPS_MACH_3900:
@@ -6782,7 +6784,10 @@  _bfd_elf_mips_mach (flagword flags)
 	{
 	default:
 	case E_MIPS_ARCH_1:
-	  return bfd_mach_mips3000;
+	  if (ABI_64_P (abfd))
+	    return bfd_mach_mips4000;
+	  else
+	    return bfd_mach_mips3000;
 
 	case E_MIPS_ARCH_2:
 	  return bfd_mach_mips6000;
@@ -6815,8 +6820,6 @@  _bfd_elf_mips_mach (flagword flags)
 	  return bfd_mach_mipsisa64r6;
 	}
     }
-
-  return 0;
 }
 
 /* Return printable name for ABI.  */
diff --git a/bfd/elfxx-mips.h b/bfd/elfxx-mips.h
index 8f16cc9c53..c64f788f8b 100644
--- a/bfd/elfxx-mips.h
+++ b/bfd/elfxx-mips.h
@@ -138,7 +138,7 @@  extern bfd_reloc_status_type _bfd_mips_elf_lo16_reloc
 extern bfd_reloc_status_type _bfd_mips_elf_generic_reloc
   (bfd *, arelent *, asymbol *, void *, asection *, bfd *, char **);
 extern unsigned long _bfd_elf_mips_mach
-  (flagword);
+  (bfd *);
 extern bfd_boolean _bfd_mips_relax_section
   (bfd *, asection *, struct bfd_link_info *, bfd_boolean *);
 extern bfd_vma _bfd_mips_elf_sign_extend