diff mbox series

[1/2] MIPS: Add n32 VECs to non-vendor elf targets

Message ID 20230605102225.3566958-1-yunqiang.su@cipunited.com
State Unresolved
Headers
Series [1/2] MIPS: Add n32 VECs to non-vendor elf targets |

Checks

Context Check Description
snail/binutils-gdb-check warning Git am fail log

Commit Message

YunQiang Su June 5, 2023, 10:22 a.m. UTC 
  For triples mips*-elf without vendor, such as mipsisa32r2el-elf,
mipstx39-elf, the NUBI is used. These targets use non-trad VECs.

Currently, N32 VECs are missing from targ_selvecs in bfd/config.bfd.
Let's add them.

This patch also fixes gas testcases for these targets.

bfd:
	* config.bfd (mips*-*-elf*): add N32 VECs.
	(mips*el-*-elf*): Ditto.
---
 bfd/config.bfd                           | 4 ++--
 gas/testsuite/gas/mips/comdat-reloc-r6.d | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Maciej W. Rozycki June 5, 2023, 11:37 p.m. UTC | #1 
On Mon, 5 Jun 2023, YunQiang Su wrote:

> For triples mips*-elf without vendor, such as mipsisa32r2el-elf,
> mipstx39-elf, the NUBI is used. These targets use non-trad VECs.

 I guess you mean the IRIX variant of the psABI.  NUBI has never been 
deployed.

> Currently, N32 VECs are missing from targ_selvecs in bfd/config.bfd.
> Let's add them.

 NAK, the submitters of these targets had their reasons not to include 
n32.

> This patch also fixes gas testcases for these targets.

 This is backwards.  Any irrelevant tests just need to be disabled for 
targets that do not have n32 support enabled.

 Please follow the ${has_newabi} approach for now, or if you can be 
bothered port binutils/testsuite/binutils-all/mips/mips.exp approach, 
which I implemented a while ago that is cleaner (it has an advantage of 
also supporting targets that do not have o32 support enabled; yes, we do 
have such ones too).

> diff --git a/gas/testsuite/gas/mips/comdat-reloc-r6.d b/gas/testsuite/gas/mips/comdat-reloc-r6.d
> index 513589e73d3..f7d9a4e57c0 100644
> --- a/gas/testsuite/gas/mips/comdat-reloc-r6.d
> +++ b/gas/testsuite/gas/mips/comdat-reloc-r6.d
> @@ -26,9 +26,9 @@ Relocation section '\.rel\.text\.bar' at offset .+ contains .+ entries:
>   Offset     Info    Type            Sym\.Value  Sym\. Name
>  00000000  ......05 R_MIPS_HI16       00000000   _gp_disp
>  00000004  ......06 R_MIPS_LO16       00000000   _gp_disp
> -0000000c  0000070a R_MIPS_PC16       00000020   \.L1\^B1
> +0000000c  00000[7d]0a R_MIPS_PC16       00000020   \.L1\^B1
>  00000014  ......09 R_MIPS_GOT16      00000000   foo
>  00000024  ......09 R_MIPS_GOT16      00000000   foo
>  0000001c  ......06 R_MIPS_LO16       00000000   foo
> -00000020  0000080a R_MIPS_PC16       00000018   \.L0\^B1
> +00000020  00000[8e]0a R_MIPS_PC16       00000018   \.L0\^B1
>  #pass

 Where do these differences come from?

  Maciej
YunQiang Su June 6, 2023, 3:24 a.m. UTC | #2 
Maciej W. Rozycki <macro@orcam.me.uk> 于2023年6月6日周二 07:37写道：
>
> On Mon, 5 Jun 2023, YunQiang Su wrote:
>
> > For triples mips*-elf without vendor, such as mipsisa32r2el-elf,
> > mipstx39-elf, the NUBI is used. These targets use non-trad VECs.
>
>  I guess you mean the IRIX variant of the psABI.  NUBI has never been
> deployed.
>

Ohh, yes.

> > Currently, N32 VECs are missing from targ_selvecs in bfd/config.bfd.
> > Let's add them.
>
>  NAK, the submitters of these targets had their reasons not to include
> n32.
>

Ohh, it is reasonable. You are right.
anyway, nobody is using this triple as far as I know.
Just block these tests won't be a problem.

> > This patch also fixes gas testcases for these targets.
>
>  This is backwards.  Any irrelevant tests just need to be disabled for
> targets that do not have n32 support enabled.
>

I will update my patch.

>  Please follow the ${has_newabi} approach for now, or if you can be
> bothered port binutils/testsuite/binutils-all/mips/mips.exp approach,
> which I implemented a while ago that is cleaner (it has an advantage of
> also supporting targets that do not have o32 support enabled; yes, we do
> have such ones too).
>
> > diff --git a/gas/testsuite/gas/mips/comdat-reloc-r6.d b/gas/testsuite/gas/mips/comdat-reloc-r6.d
> > index 513589e73d3..f7d9a4e57c0 100644
> > --- a/gas/testsuite/gas/mips/comdat-reloc-r6.d
> > +++ b/gas/testsuite/gas/mips/comdat-reloc-r6.d
> > @@ -26,9 +26,9 @@ Relocation section '\.rel\.text\.bar' at offset .+ contains .+ entries:
> >   Offset     Info    Type            Sym\.Value  Sym\. Name
> >  00000000  ......05 R_MIPS_HI16       00000000   _gp_disp
> >  00000004  ......06 R_MIPS_LO16       00000000   _gp_disp
> > -0000000c  0000070a R_MIPS_PC16       00000020   \.L1\^B1
> > +0000000c  00000[7d]0a R_MIPS_PC16       00000020   \.L1\^B1
> >  00000014  ......09 R_MIPS_GOT16      00000000   foo
> >  00000024  ......09 R_MIPS_GOT16      00000000   foo
> >  0000001c  ......06 R_MIPS_LO16       00000000   foo
> > -00000020  0000080a R_MIPS_PC16       00000018   \.L0\^B1
> > +00000020  00000[8e]0a R_MIPS_PC16       00000018   \.L0\^B1
> >  #pass
>
>  Where do these differences come from?
>

It looks like that the order of symbols is different.
I will edit comdat-reloc-r6.d, use `.' for the SYM index section in r_info,
just like other symbols.

Symbol table '.symtab' contains 15 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 00000000     0 SECTION LOCAL  DEFAULT    2 .text
     2: 00000000     0 SECTION LOCAL  DEFAULT    3 .data
     3: 00000000     0 SECTION LOCAL  DEFAULT    4 .bss
     4: 00000000     0 SECTION LOCAL  DEFAULT    7 .text.foo
     5: 00000000     8 FUNC    LOCAL  DEFAULT    7 foo
     6: 00000000     0 SECTION LOCAL  DEFAULT    8 .text.bar
     7: 00000020     0 NOTYPE  LOCAL  DEFAULT    8 .L1^B1
     8: 00000018     0 NOTYPE  LOCAL  DEFAULT    8 .L^B1
     9: 00000000     0 SECTION LOCAL  DEFAULT    5 .reginfo
    10: 00000000     0 SECTION LOCAL  DEFAULT    6 .MIPS.abiflags
    11: 00000000     0 SECTION LOCAL  DEFAULT   10 .gnu.attributes
    12: 00000000     0 SECTION LOCAL  DEFAULT    1 .group
    13: 00000000    40 FUNC    GLOBAL DEFAULT    8 bar
    14: 00000000     0 OBJECT  GLOBAL DEFAULT  UND _gp_disp

vs

Symbol table '.symtab' contains 15 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 00000000     0 SECTION LOCAL  DEFAULT    2 .text
     2: 00000000     0 SECTION LOCAL  DEFAULT    3 .data
     3: 00000000     0 SECTION LOCAL  DEFAULT    4 .bss
     4: 00000000     0 SECTION LOCAL  DEFAULT    7 .text.foo
     5: 00000000     0 SECTION LOCAL  DEFAULT    8 .text.bar
     6: 00000000     0 SECTION LOCAL  DEFAULT    5 .reginfo
     7: 00000000     0 SECTION LOCAL  DEFAULT    6 .MIPS.abiflags
     8: 00000000     0 SECTION LOCAL  DEFAULT   10 .gnu.attributes
     9: 00000000     0 SECTION LOCAL  DEFAULT    1 .group
    10: 00000000     8 FUNC    LOCAL  DEFAULT    7 foo
    11: 00000000    40 FUNC    GLOBAL DEFAULT    8 bar
    12: 00000000     0 OBJECT  GLOBAL DEFAULT  UND _gp_disp
    13: 00000020     0 NOTYPE  LOCAL  DEFAULT    8 .L1^B1
    14: 00000018     0 NOTYPE  LOCAL  DEFAULT    8 .L0^B1

>   Maciej
Maciej W. Rozycki June 6, 2023, 4:57 p.m. UTC | #3 
On Tue, 6 Jun 2023, YunQiang Su wrote:

> > > diff --git a/gas/testsuite/gas/mips/comdat-reloc-r6.d b/gas/testsuite/gas/mips/comdat-reloc-r6.d
> > > index 513589e73d3..f7d9a4e57c0 100644
> > > --- a/gas/testsuite/gas/mips/comdat-reloc-r6.d
> > > +++ b/gas/testsuite/gas/mips/comdat-reloc-r6.d
> > > @@ -26,9 +26,9 @@ Relocation section '\.rel\.text\.bar' at offset .+ contains .+ entries:
> > >   Offset     Info    Type            Sym\.Value  Sym\. Name
> > >  00000000  ......05 R_MIPS_HI16       00000000   _gp_disp
> > >  00000004  ......06 R_MIPS_LO16       00000000   _gp_disp
> > > -0000000c  0000070a R_MIPS_PC16       00000020   \.L1\^B1
> > > +0000000c  00000[7d]0a R_MIPS_PC16       00000020   \.L1\^B1
> > >  00000014  ......09 R_MIPS_GOT16      00000000   foo
> > >  00000024  ......09 R_MIPS_GOT16      00000000   foo
> > >  0000001c  ......06 R_MIPS_LO16       00000000   foo
> > > -00000020  0000080a R_MIPS_PC16       00000018   \.L0\^B1
> > > +00000020  00000[8e]0a R_MIPS_PC16       00000018   \.L0\^B1
> > >  #pass
> >
> >  Where do these differences come from?
> 
> It looks like that the order of symbols is different.

 Right, the IRIX psABI wants symbols to be sorted according to a different 
rule; this is probably the most prominent divergence between the two MIPS 
psABIs.  It did not occur to me it is the symbol indices that are involved 
here.

 The difference in symbol indices would have best been mentioned in the 
change description: these descriptions are meant to make it easier to the 
reviewer to understand the change and to convince them that the change is 
indeed correct.

 And the easier a review is the quicker you will get it.  If it takes a 
minute and just reading through the submission to decide that a change is 
correct, then you you will get your change reviewed quickly.  If one has 
to wade through sources to collect bits of information so as to understand 
what the change is really about, then the likelihood of a fast review goes 
down very quickly.

 Good coding style also helps as one doesn't get distracted by mechanical 
clean-ups.

> I will edit comdat-reloc-r6.d, use `.' for the SYM index section in r_info,
> just like other symbols.

 It is the correct approach given how the original test has been written. 
It is also a fix to a recent addition, which just shows why getting things 
properly reviewed is so important.

  Maciej
diff mbox series

Patch

diff --git a/bfd/config.bfd b/bfd/config.bfd
index 78752994456..2927f35b614 100644
--- a/bfd/config.bfd
+++ b/bfd/config.bfd
@@ -923,11 +923,11 @@  case "${targ}" in
     ;;
   mips*el-*-elf* | mips*-*-chorus*)
     targ_defvec=mips_elf32_le_vec
-    targ_selvecs="mips_elf32_be_vec mips_elf64_be_vec mips_elf64_le_vec"
+    targ_selvecs="mips_elf32_be_vec mips_elf64_be_vec mips_elf64_le_vec mips_elf32_n_be_vec mips_elf32_n_le_vec"
     ;;
   mips*-*-elf* | mips*-*-rtems* | mips*-*-windiss | mips*-*-none)
     targ_defvec=mips_elf32_be_vec
-    targ_selvecs="mips_elf32_le_vec mips_elf64_be_vec mips_elf64_le_vec"
+    targ_selvecs="mips_elf32_le_vec mips_elf64_be_vec mips_elf64_le_vec mips_elf32_n_be_vec mips_elf32_n_le_vec"
     ;;
   mips64*-*-openbsd*)
     targ_defvec=mips_elf64_trad_be_vec
diff --git a/gas/testsuite/gas/mips/comdat-reloc-r6.d b/gas/testsuite/gas/mips/comdat-reloc-r6.d
index 513589e73d3..f7d9a4e57c0 100644
--- a/gas/testsuite/gas/mips/comdat-reloc-r6.d
+++ b/gas/testsuite/gas/mips/comdat-reloc-r6.d
@@ -26,9 +26,9 @@  Relocation section '\.rel\.text\.bar' at offset .+ contains .+ entries:
  Offset     Info    Type            Sym\.Value  Sym\. Name
 00000000  ......05 R_MIPS_HI16       00000000   _gp_disp
 00000004  ......06 R_MIPS_LO16       00000000   _gp_disp
-0000000c  0000070a R_MIPS_PC16       00000020   \.L1\^B1
+0000000c  00000[7d]0a R_MIPS_PC16       00000020   \.L1\^B1
 00000014  ......09 R_MIPS_GOT16      00000000   foo
 00000024  ......09 R_MIPS_GOT16      00000000   foo
 0000001c  ......06 R_MIPS_LO16       00000000   foo
-00000020  0000080a R_MIPS_PC16       00000018   \.L0\^B1
+00000020  00000[8e]0a R_MIPS_PC16       00000018   \.L0\^B1
 #pass