[PATCH] MIPS: N64, mark .interp as INITIAL_READONLY_SECTIONS

[PATCH] MIPS: N64, mark .interp as INITIAL_READONLY_SECTIONS

[YunQiang Su]

In ld/emulparams/elf64bmip-defs.sh, there is no .interp, which make
the .interp section appears after some other less important sections.

Let's add it, and mark it as INITIAL_READONLY_SECTIONS, just like
O32/N32 do.

This changes fixes ld/pr23658-2.

ld/ChangeLog:
	* emulparams/elf64bmip-defs.sh: mark .interp as
	INITIAL_READONLY_SECTIONS.
	* testsuite/ld-mips-elf/pie-n64.d: adjust addresses.
---
 ld/emulparams/elf64bmip-defs.sh    |  6 +++++-
 ld/testsuite/ld-mips-elf/pie-n64.d | 12 ++++++------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/ld/emulparams/elf64bmip-defs.sh b/ld/emulparams/elf64bmip-defs.sh
index 4165f51e9e5..19d782d8e7a 100644
--- a/ld/emulparams/elf64bmip-defs.sh
+++ b/ld/emulparams/elf64bmip-defs.sh
@@ -1,6 +1,10 @@
 source_sh ${srcdir}/emulparams/elf32bmipn32-defs.sh
 COMMONPAGESIZE="CONSTANT (COMMONPAGESIZE)"
-INITIAL_READONLY_SECTIONS="
+INITIAL_READONLY_SECTIONS=
+if test -z "${CREATE_SHLIB}"; then
+  INITIAL_READONLY_SECTIONS=".interp       ${RELOCATING-0} : { *(.interp) }"
+fi
+INITIAL_READONLY_SECTIONS="${INITIAL_READONLY_SECTIONS}
   .MIPS.abiflags      ${RELOCATING-0} : { *(.MIPS.abiflags) }
   .MIPS.xhash      ${RELOCATING-0} : { *(.MIPS.xhash) }
   .MIPS.options : { *(.MIPS.options) }
diff --git a/ld/testsuite/ld-mips-elf/pie-n64.d b/ld/testsuite/ld-mips-elf/pie-n64.d
index bf7c6b2272a..1fda2512c8e 100644
--- a/ld/testsuite/ld-mips-elf/pie-n64.d
+++ b/ld/testsuite/ld-mips-elf/pie-n64.d
@@ -2,16 +2,16 @@
 #ld: -pie
 #readelf: -d
 
-Dynamic section at offset 0x208 contains 17 entries:
+Dynamic section at offset 0x220 contains 17 entries:
   Tag * Type * Name/Value
- 0x0+00000004 \(HASH\) * 0x368
- 0x0+00000005 \(STRTAB\) * 0x3c8
- 0x0+00000006 \(SYMTAB\) * 0x380
+ 0x0+00000004 \(HASH\) * 0x380
+ 0x0+00000005 \(STRTAB\) * 0x3e0
+ 0x0+00000006 \(SYMTAB\) * 0x398
  0x0+0000000a \(STRSZ\) * 28 \(bytes\)
  0x0+0000000b \(SYMENT\) * 24 \(bytes\)
- 0x0+70000035 \(MIPS_RLD_MAP_REL\) * 0x101c8
+ 0x0+70000035 \(MIPS_RLD_MAP_REL\) * 0x101a0
  0x0+00000015 \(DEBUG\) * 0x0
- 0x0+00000003 \(PLTGOT\) * 0x10430
+ 0x0+00000003 \(PLTGOT\) * 0x10420
  0x0+70000001 \(MIPS_RLD_VERSION\) * 1
  0x0+70000005 \(MIPS_FLAGS\) * NOTPOT
  0x0+70000006 \(MIPS_BASE_ADDRESS\) * 0x0
-- 
2.30.2

Re: [PATCH] MIPS: N64, mark .interp as INITIAL_READONLY_SECTIONS

[YunQiang Su]

YunQiang Su <yunqiang.su@cipunited.com> 于2023年6月30日周五 14:46写道：
>
> In ld/emulparams/elf64bmip-defs.sh, there is no .interp, which make
> the .interp section appears after some other less important sections.
>
> Let's add it, and mark it as INITIAL_READONLY_SECTIONS, just like
> O32/N32 do.
>
> This changes fixes ld/pr23658-2.
>

ping

> ld/ChangeLog:
>         * emulparams/elf64bmip-defs.sh: mark .interp as
>         INITIAL_READONLY_SECTIONS.
>         * testsuite/ld-mips-elf/pie-n64.d: adjust addresses.
> ---
>  ld/emulparams/elf64bmip-defs.sh    |  6 +++++-
>  ld/testsuite/ld-mips-elf/pie-n64.d | 12 ++++++------
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/ld/emulparams/elf64bmip-defs.sh b/ld/emulparams/elf64bmip-defs.sh
> index 4165f51e9e5..19d782d8e7a 100644
> --- a/ld/emulparams/elf64bmip-defs.sh
> +++ b/ld/emulparams/elf64bmip-defs.sh
> @@ -1,6 +1,10 @@
>  source_sh ${srcdir}/emulparams/elf32bmipn32-defs.sh
>  COMMONPAGESIZE="CONSTANT (COMMONPAGESIZE)"
> -INITIAL_READONLY_SECTIONS="
> +INITIAL_READONLY_SECTIONS=
> +if test -z "${CREATE_SHLIB}"; then
> +  INITIAL_READONLY_SECTIONS=".interp       ${RELOCATING-0} : { *(.interp) }"
> +fi
> +INITIAL_READONLY_SECTIONS="${INITIAL_READONLY_SECTIONS}
>    .MIPS.abiflags      ${RELOCATING-0} : { *(.MIPS.abiflags) }
>    .MIPS.xhash      ${RELOCATING-0} : { *(.MIPS.xhash) }
>    .MIPS.options : { *(.MIPS.options) }
> diff --git a/ld/testsuite/ld-mips-elf/pie-n64.d b/ld/testsuite/ld-mips-elf/pie-n64.d
> index bf7c6b2272a..1fda2512c8e 100644
> --- a/ld/testsuite/ld-mips-elf/pie-n64.d
> +++ b/ld/testsuite/ld-mips-elf/pie-n64.d
> @@ -2,16 +2,16 @@
>  #ld: -pie
>  #readelf: -d
>
> -Dynamic section at offset 0x208 contains 17 entries:
> +Dynamic section at offset 0x220 contains 17 entries:
>    Tag * Type * Name/Value
> - 0x0+00000004 \(HASH\) * 0x368
> - 0x0+00000005 \(STRTAB\) * 0x3c8
> - 0x0+00000006 \(SYMTAB\) * 0x380
> + 0x0+00000004 \(HASH\) * 0x380
> + 0x0+00000005 \(STRTAB\) * 0x3e0
> + 0x0+00000006 \(SYMTAB\) * 0x398
>   0x0+0000000a \(STRSZ\) * 28 \(bytes\)
>   0x0+0000000b \(SYMENT\) * 24 \(bytes\)
> - 0x0+70000035 \(MIPS_RLD_MAP_REL\) * 0x101c8
> + 0x0+70000035 \(MIPS_RLD_MAP_REL\) * 0x101a0
>   0x0+00000015 \(DEBUG\) * 0x0
> - 0x0+00000003 \(PLTGOT\) * 0x10430
> + 0x0+00000003 \(PLTGOT\) * 0x10420
>   0x0+70000001 \(MIPS_RLD_VERSION\) * 1
>   0x0+70000005 \(MIPS_FLAGS\) * NOTPOT
>   0x0+70000006 \(MIPS_BASE_ADDRESS\) * 0x0
> --
> 2.30.2
>


-- 
YunQiang Su

Re: [PATCH] MIPS: N64, mark .interp as INITIAL_READONLY_SECTIONS

[Maciej W. Rozycki]

On Fri, 30 Jun 2023, YunQiang Su wrote:

> In ld/emulparams/elf64bmip-defs.sh, there is no .interp, which make
> the .interp section appears after some other less important sections.
> Let's add it, and mark it as INITIAL_READONLY_SECTIONS, just like
> O32/N32 do.
> 
> This changes fixes ld/pr23658-2.

 It would have helped with the review if the actual difference between the 
output expected by the test case and one produced was given along with the 
submission, perhaps in the discussion section.

 Without that I had to bend backwards to make this test case run with n64 
MIPS at all and see what is going on here as I don't have a compiler 
available for OpenBSD, which is my reference target for n64 verification.  
This revealed a minor discrepancy only between the output produced:

   03     .MIPS.abiflags .MIPS.options .dynamic .hash .dynsym .dynstr .text .interp .note.4 .note.1 .note.2 .note.3

vs expected:

 +[0-9]+ +\.interp \.note.4 \.note.1 \.note.2 \.note.3.*

and made me question whether the test case shouldn't be relaxed instead.  
Eventually I realised this is actually a fix for a regression back from 
2006, so I chose to give your change a go-ahead, even though clearly the 
current arrangement is good enough not to have caused any issues through 
17 years.

 Overall compiled tests are hard to run in a multi-target environment 
where compilers for all the exotic configurations are not necessarily 
available and therefore these tests receive less attention.  As compiler 
configurations vary these tests may even produce incorrect results despite 
no actual issue being present with binutils.  They're a compromise really 
for issues discovered in the past for which no effort could have been 
allocated to prepare specific per-target tests that rely on tools coming 
with binutils only.

 It also means we need proper coverage for section ordering, as it would 
have prevented this bug from having been introduced in the first place.

> ld/ChangeLog:
> 	* emulparams/elf64bmip-defs.sh: mark .interp as
> 	INITIAL_READONLY_SECTIONS.
> 	* testsuite/ld-mips-elf/pie-n64.d: adjust addresses.

 We'd best avoid code duplication we have here, which leads to situations 
exactly such as this one.  But that'd be for a larger rewrite.  Meanwhile 
your change seems good to me as an interim fix, with typos fixed in the 
change description.

 As I have observed above a test case is needed, so I have now pushed an 
updated version including a suitable set of tests, as recorded here: 
<https://sourceware.org/pipermail/binutils/2023-July/128674.html>.

 In the future please try to come up with testsuite cases for bug fixes 
where feasible.  I appreciate that making a test case often takes more 
effort than the bug fix itself, but we need these tests so as not to 
reintroduce similar issues in the future, such as e.g. with a rewrite I 
have suggested above.

 Thank you for your contribution.

  Maciej

Re: [PATCH] MIPS: N64, mark .interp as INITIAL_READONLY_SECTIONS

[YunQiang Su]

Maciej W. Rozycki <macro@orcam.me.uk> 于2023年7月28日周五 13:29写道：
>
> On Fri, 30 Jun 2023, YunQiang Su wrote:
>
> > In ld/emulparams/elf64bmip-defs.sh, there is no .interp, which make
> > the .interp section appears after some other less important sections.
> > Let's add it, and mark it as INITIAL_READONLY_SECTIONS, just like
> > O32/N32 do.
> >
> > This changes fixes ld/pr23658-2.
>
>  It would have helped with the review if the actual difference between the
> output expected by the test case and one produced was given along with the
> submission, perhaps in the discussion section.
>

OK. I will do like this.

>  Without that I had to bend backwards to make this test case run with n64
> MIPS at all and see what is going on here as I don't have a compiler
> available for OpenBSD, which is my reference target for n64 verification.

Ohh, you are right: we need a compiler for OpenBSD. I will work on it.

For other N64 targets, you can try Debian's cross toolchain.
If you are using Debian or Ubuntu, you can use
     apt install g++-multilib-mips64el-linux-gnuabi64
     apt install g++-multilib-mips64-linux-gnuabi64
or
    apt install g++-multilib-mipsisa64r6el-linux-gnuabi64
    apt install g++-multilib-mipsisa64r6-linux-gnuabi64
to get the cross toolchain for MIPSn64.


> This revealed a minor discrepancy only between the output produced:
>
>    03     .MIPS.abiflags .MIPS.options .dynamic .hash .dynsym .dynstr .text .interp .note.4 .note.1 .note.2 .note.3
>
> vs expected:
>
>  +[0-9]+ +\.interp \.note.4 \.note.1 \.note.2 \.note.3.*
>
> and made me question whether the test case shouldn't be relaxed instead.
> Eventually I realised this is actually a fix for a regression back from
> 2006, so I chose to give your change a go-ahead, even though clearly the
> current arrangement is good enough not to have caused any issues through
> 17 years.
>
>  Overall compiled tests are hard to run in a multi-target environment
> where compilers for all the exotic configurations are not necessarily
> available and therefore these tests receive less attention.  As compiler
> configurations vary these tests may even produce incorrect results despite
> no actual issue being present with binutils.  They're a compromise really
> for issues discovered in the past for which no effort could have been
> allocated to prepare specific per-target tests that rely on tools coming
> with binutils only.
>
>  It also means we need proper coverage for section ordering, as it would
> have prevented this bug from having been introduced in the first place.
>
> > ld/ChangeLog:
> >       * emulparams/elf64bmip-defs.sh: mark .interp as
> >       INITIAL_READONLY_SECTIONS.
> >       * testsuite/ld-mips-elf/pie-n64.d: adjust addresses.
>
>  We'd best avoid code duplication we have here, which leads to situations
> exactly such as this one.  But that'd be for a larger rewrite.  Meanwhile
> your change seems good to me as an interim fix, with typos fixed in the
> change description.
>
>  As I have observed above a test case is needed, so I have now pushed an
> updated version including a suitable set of tests, as recorded here:
> <https://sourceware.org/pipermail/binutils/2023-July/128674.html>.
>
>  In the future please try to come up with testsuite cases for bug fixes
> where feasible.  I appreciate that making a test case often takes more
> effort than the bug fix itself, but we need these tests so as not to
> reintroduce similar issues in the future, such as e.g. with a rewrite I
> have suggested above.
>
>  Thank you for your contribution.
>
>   Maciej



-- 
YunQiang Su