Patchwork riscv: remove DL_RO_DYN_SECTION

login
register
mail settings
Submitter David Abdurachmanov
Date April 9, 2019, 11:25 a.m.
Message ID <20190409112529.13930-1-david.abdurachmanov@gmail.com>
Download mbox | patch
Permalink /patch/32218/
State New
Headers show

Comments

David Abdurachmanov - April 9, 2019, 11:25 a.m.
While working on enabling D front-end (GDC) in GCC we noticed that druntime
was segfaulting if it is linked dynamically. This was tracked to
DL_RO_DYN_SECTION.

DL_RO_DYN_SECTION lines seem to be copied from MIPS file (which is the only
user of it), but the comment doesn't apply to RISC-V. There is no such
requirement in RISC-V ABI.

	* sysdeps/riscv/ldsodefs.h: Remove DL_RO_DYN_SECTION as it is not
	required by RISC-V ABI.

Signed-off-by: David Abdurachmanov <david.abdurachmanov@gmail.com>
Cc: jimw@sifive.com
Cc: palmer@sifive.com
Cc: dj@delorie.com
---
 sysdeps/riscv/ldsodefs.h | 4 ----
 1 file changed, 4 deletions(-)
Florian Weimer - April 9, 2019, 11:42 a.m.
* David Abdurachmanov:

> While working on enabling D front-end (GDC) in GCC we noticed that druntime
> was segfaulting if it is linked dynamically. This was tracked to
> DL_RO_DYN_SECTION.

Is it possible to extract a test case from that which we could put into
glibc?

Since this appears to be a user-visible bug, it needs a bug in
Bugzilla.  Would you please file one?

Thanks,
Florian
Jim Wilson - April 9, 2019, 11:21 p.m.
On Tue, Apr 9, 2019 at 4:42 AM Florian Weimer <fweimer@redhat.com> wrote:
> * David Abdurachmanov:

Do you or your employer have copyright assignments?  For a trivial
patch like this, a copyright assignment shouldn't be necessary, but
would be necessary if you want to contribute more patches.

> > While working on enabling D front-end (GDC) in GCC we noticed that druntime
> > was segfaulting if it is linked dynamically. This was tracked to
> > DL_RO_DYN_SECTION.

My analysis of this when it first came up...

I believe this was blindly copied from MIPS, and that no one on the
RISC-V side will know why it is there, because it was blindly copied
from MIPS.

Looking at the MIPS SVR4 ABI, I see
.dynamic This is the same as the generic ABI section of the same type, but
the MIPS-specific version does not include the SHF_WRITE at-
tribute.

I don't see any other reason in the spec for this to be read-only, but
making it read-only probably improves program security.  I don't see
an equivalent statement in the RISC-V ABI.

Looking at bfd/elfxx-mips.c _bfd_mips_elf_create_dynamic_sections(), we see code
  flags = (SEC_ALLOC | SEC_LOAD | SEC_HAS_CONTENTS | SEC_IN_MEMORY
           | SEC_LINKER_CREATED | SEC_READONLY);

  /* The psABI requires a read-only .dynamic section, but the VxWorks
     EABI doesn't.  */
  if (!htab->is_vxworks)
    {
      s = bfd_get_linker_section (abfd, ".dynamic");
      if (s != NULL)
        {
          if (! bfd_set_section_flags (abfd, s, flags))
            return FALSE;
        }
    }
that forces .dynamic to be read-only everywhere except vxworks.  I
don't see equivalent code in the RISC-V bfd port.

Looking at ld, in emulparams/, there are multiple mips scripts that
set TEXT_DYNAMIC, which forces .dynamic into the text segment instead
of the data segment.  This is then unset in the vxworks scripts.
Interestingly, hppa also sets TEXT_DYNAMIC, but the hppa support in
glibc is probably not well maintained.  Anyways, there is no
equivalent code for RISC-V.

So it appears unnecessary for the RISC-V glibc port.

If MIPS dynamic linking support is ever added to the druntime library,
then it will need the same change that you made for RISC-V, so in that
case it would not be a RISC-V specific change.  However, there aren't
a lot of people doing MIPS development work anymore, so this may never
come up.

Jim
PS Since I wrote my analysis, MIPS has been resurrected, so maybe
someone will care about that again.
Palmer Dabbelt - April 25, 2019, 5:54 p.m.
On Tue, 09 Apr 2019 04:25:29 PDT (-0700), david.abdurachmanov@gmail.com wrote:
> While working on enabling D front-end (GDC) in GCC we noticed that druntime
> was segfaulting if it is linked dynamically. This was tracked to
> DL_RO_DYN_SECTION.
>
> DL_RO_DYN_SECTION lines seem to be copied from MIPS file (which is the only
> user of it), but the comment doesn't apply to RISC-V. There is no such
> requirement in RISC-V ABI.
>
> 	* sysdeps/riscv/ldsodefs.h: Remove DL_RO_DYN_SECTION as it is not
> 	required by RISC-V ABI.
>
> Signed-off-by: David Abdurachmanov <david.abdurachmanov@gmail.com>
> Cc: jimw@sifive.com
> Cc: palmer@sifive.com
> Cc: dj@delorie.com
> ---
>  sysdeps/riscv/ldsodefs.h | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/sysdeps/riscv/ldsodefs.h b/sysdeps/riscv/ldsodefs.h
> index f46cf4158e..5ec607e867 100644
> --- a/sysdeps/riscv/ldsodefs.h
> +++ b/sysdeps/riscv/ldsodefs.h
> @@ -38,10 +38,6 @@ struct La_riscv_retval;
>  				       struct La_riscv_retval *,	\
>  				       const char *);
>
> -/* The RISC-V ABI specifies that the dynamic section has to be read-only.  */
> -
> -#define DL_RO_DYN_SECTION 1
> -
>  #include_next <ldsodefs.h>
>
>  #endif

Thanks.  I went ahead and pushed it without a test case so we don't lose the
fix.  I also opened a Bugzilla:

https://sourceware.org/bugzilla/show_bug.cgi?id=24484

Patch

diff --git a/sysdeps/riscv/ldsodefs.h b/sysdeps/riscv/ldsodefs.h
index f46cf4158e..5ec607e867 100644
--- a/sysdeps/riscv/ldsodefs.h
+++ b/sysdeps/riscv/ldsodefs.h
@@ -38,10 +38,6 @@  struct La_riscv_retval;
 				       struct La_riscv_retval *,	\
 				       const char *);
 
-/* The RISC-V ABI specifies that the dynamic section has to be read-only.  */
-
-#define DL_RO_DYN_SECTION 1
-
 #include_next <ldsodefs.h>
 
 #endif