diff mbox series

[v2,2/6] elf: lose is closely tied to _dl_map_object_from_fd

Message ID f6788964bf6412da6fcdd3be4f67eb0417e558a6.1606319495.git.szabolcs.nagy@arm.com
State New
Headers show
Series aarch64: avoid mprotect(PROT_BTI|PROT_EXEC) [BZ #26831] | expand

Commit Message

Szabolcs Nagy Nov. 27, 2020, 1:20 p.m. UTC 
Simple refactoring to keep failure handling next to
_dl_map_object_from_fd.
---
 elf/dl-load.c | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

Comments

Adhemerval Zanella Netto Dec. 10, 2020, 5:57 p.m. UTC | #1 
On 27/11/2020 10:20, Szabolcs Nagy via Libc-alpha wrote:
> Simple refactoring to keep failure handling next to
> _dl_map_object_from_fd.

It is just moving the function below _dl_process_pt_gnu_property,
and it is already close to its usage.  I don't have a strong opinion,
but I don't see much gain either.

> ---
>  elf/dl-load.c | 48 ++++++++++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index f3201e7c14..21e55deb19 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -835,30 +835,6 @@ _dl_init_paths (const char *llp, const char *source)
>  }
>  
>  
> -static void
> -__attribute__ ((noreturn, noinline))
> -lose (int code, int fd, const char *name, char *realname, struct link_map *l,
> -      const char *msg, struct r_debug *r, Lmid_t nsid)
> -{
> -  /* The file might already be closed.  */
> -  if (fd != -1)
> -    (void) __close_nocancel (fd);
> -  if (l != NULL && l->l_origin != (char *) -1l)
> -    free ((char *) l->l_origin);
> -  free (l);
> -  free (realname);
> -
> -  if (r != NULL)
> -    {
> -      r->r_state = RT_CONSISTENT;
> -      _dl_debug_state ();
> -      LIBC_PROBE (map_failed, 2, nsid, r);
> -    }
> -
> -  _dl_signal_error (code, name, NULL, msg);
> -}
> -
> -
>  /* Process PT_GNU_PROPERTY program header PH in module L after
>     PT_LOAD segments are mapped.  Only one NT_GNU_PROPERTY_TYPE_0
>     note is handled which contains processor specific properties.  */
> @@ -930,6 +906,30 @@ _dl_process_pt_gnu_property (struct link_map *l, const ElfW(Phdr) *ph)
>  }
>  
>  
> +static void
> +__attribute__ ((noreturn, noinline))
> +lose (int code, int fd, const char *name, char *realname, struct link_map *l,
> +      const char *msg, struct r_debug *r, Lmid_t nsid)
> +{
> +  /* The file might already be closed.  */
> +  if (fd != -1)
> +    (void) __close_nocancel (fd);
> +  if (l != NULL && l->l_origin != (char *) -1l)
> +    free ((char *) l->l_origin);
> +  free (l);
> +  free (realname);
> +
> +  if (r != NULL)
> +    {
> +      r->r_state = RT_CONSISTENT;
> +      _dl_debug_state ();
> +      LIBC_PROBE (map_failed, 2, nsid, r);
> +    }
> +
> +  _dl_signal_error (code, name, NULL, msg);
> +}
> +
> +
>  /* Map in the shared object NAME, actually located in REALNAME, and already
>     opened on FD.  */
>  
>
Szabolcs Nagy Dec. 11, 2020, 12:12 p.m. UTC | #2 
The 12/10/2020 14:57, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 27/11/2020 10:20, Szabolcs Nagy via Libc-alpha wrote:
> > Simple refactoring to keep failure handling next to
> > _dl_map_object_from_fd.
> 
> It is just moving the function below _dl_process_pt_gnu_property,
> and it is already close to its usage.  I don't have a strong opinion,
> but I don't see much gain either.

this function should be part of _dl_map_object_from_fd.
(it is cleaning up local state there so a change in
local variables there likely requires changes in lose
and the logic does not make sense independently).

i think historically it was separate to avoid error code
paths to blow up the code size after inlining, but now
i tried it and code size stays small: historically there
were many call sites, but now only a single call and gotos
there.

i'll rewrite this patch to just inline lose manually.

> > ---
> >  elf/dl-load.c | 48 ++++++++++++++++++++++++------------------------
> >  1 file changed, 24 insertions(+), 24 deletions(-)
> > 
> > diff --git a/elf/dl-load.c b/elf/dl-load.c
> > index f3201e7c14..21e55deb19 100644
> > --- a/elf/dl-load.c
> > +++ b/elf/dl-load.c
> > @@ -835,30 +835,6 @@ _dl_init_paths (const char *llp, const char *source)
> >  }
> >  
> >  
> > -static void
> > -__attribute__ ((noreturn, noinline))
> > -lose (int code, int fd, const char *name, char *realname, struct link_map *l,
> > -      const char *msg, struct r_debug *r, Lmid_t nsid)
> > -{
> > -  /* The file might already be closed.  */
> > -  if (fd != -1)
> > -    (void) __close_nocancel (fd);
> > -  if (l != NULL && l->l_origin != (char *) -1l)
> > -    free ((char *) l->l_origin);
> > -  free (l);
> > -  free (realname);
> > -
> > -  if (r != NULL)
> > -    {
> > -      r->r_state = RT_CONSISTENT;
> > -      _dl_debug_state ();
> > -      LIBC_PROBE (map_failed, 2, nsid, r);
> > -    }
> > -
> > -  _dl_signal_error (code, name, NULL, msg);
> > -}
> > -
> > -
> >  /* Process PT_GNU_PROPERTY program header PH in module L after
> >     PT_LOAD segments are mapped.  Only one NT_GNU_PROPERTY_TYPE_0
> >     note is handled which contains processor specific properties.  */
> > @@ -930,6 +906,30 @@ _dl_process_pt_gnu_property (struct link_map *l, const ElfW(Phdr) *ph)
> >  }
> >  
> >  
> > +static void
> > +__attribute__ ((noreturn, noinline))
> > +lose (int code, int fd, const char *name, char *realname, struct link_map *l,
> > +      const char *msg, struct r_debug *r, Lmid_t nsid)
> > +{
> > +  /* The file might already be closed.  */
> > +  if (fd != -1)
> > +    (void) __close_nocancel (fd);
> > +  if (l != NULL && l->l_origin != (char *) -1l)
> > +    free ((char *) l->l_origin);
> > +  free (l);
> > +  free (realname);
> > +
> > +  if (r != NULL)
> > +    {
> > +      r->r_state = RT_CONSISTENT;
> > +      _dl_debug_state ();
> > +      LIBC_PROBE (map_failed, 2, nsid, r);
> > +    }
> > +
> > +  _dl_signal_error (code, name, NULL, msg);
> > +}
> > +
> > +
> >  /* Map in the shared object NAME, actually located in REALNAME, and already
> >     opened on FD.  */
> >  
> > 

--
diff mbox series

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index f3201e7c14..21e55deb19 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -835,30 +835,6 @@  _dl_init_paths (const char *llp, const char *source)
 }
 
 
-static void
-__attribute__ ((noreturn, noinline))
-lose (int code, int fd, const char *name, char *realname, struct link_map *l,
-      const char *msg, struct r_debug *r, Lmid_t nsid)
-{
-  /* The file might already be closed.  */
-  if (fd != -1)
-    (void) __close_nocancel (fd);
-  if (l != NULL && l->l_origin != (char *) -1l)
-    free ((char *) l->l_origin);
-  free (l);
-  free (realname);
-
-  if (r != NULL)
-    {
-      r->r_state = RT_CONSISTENT;
-      _dl_debug_state ();
-      LIBC_PROBE (map_failed, 2, nsid, r);
-    }
-
-  _dl_signal_error (code, name, NULL, msg);
-}
-
-
 /* Process PT_GNU_PROPERTY program header PH in module L after
    PT_LOAD segments are mapped.  Only one NT_GNU_PROPERTY_TYPE_0
    note is handled which contains processor specific properties.  */
@@ -930,6 +906,30 @@  _dl_process_pt_gnu_property (struct link_map *l, const ElfW(Phdr) *ph)
 }
 
 
+static void
+__attribute__ ((noreturn, noinline))
+lose (int code, int fd, const char *name, char *realname, struct link_map *l,
+      const char *msg, struct r_debug *r, Lmid_t nsid)
+{
+  /* The file might already be closed.  */
+  if (fd != -1)
+    (void) __close_nocancel (fd);
+  if (l != NULL && l->l_origin != (char *) -1l)
+    free ((char *) l->l_origin);
+  free (l);
+  free (realname);
+
+  if (r != NULL)
+    {
+      r->r_state = RT_CONSISTENT;
+      _dl_debug_state ();
+      LIBC_PROBE (map_failed, 2, nsid, r);
+    }
+
+  _dl_signal_error (code, name, NULL, msg);
+}
+
+
 /* Map in the shared object NAME, actually located in REALNAME, and already
    opened on FD.  */