From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============7561581554665514723=="
MIME-Version: 1.0
From: Mark Wielaard <mjw@redhat.com>
To: elfutils-devel@lists.fedorahosted.org
Subject: Re: [PATCH] Move nested functions in link_map.c to file scope.
Date: Sun, 03 Jan 2016 22:25:46 +0100
Message-ID: <1451856346.13330.31.camel@redhat.com>
In-Reply-To: 1447800315-16533-1-git-send-email-chh@google.com

--===============7561581554665514723==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

On Tue, 2015-11-17 at 14:45 -0800, Chih-Hung Hsieh wrote:
> * In libdwfl/link_map.c, nested functions check64, check32,

For check64 and check32 the define plus static function extra args call
trick seems a good fit. I did push this part of the patch.

>   release_buffer, read_addrs, and consider_phdr are moved
>   to file scope to compile with clang.

But for the rest I feel it makes the code unnecessary messy. So I didn't
apply those. See below for some suggestions how to maybe get something
that doesn't create new functions with lots and lots of arguments (which
I find doesn't improve readability).

> +static inline int
> +do_release_buffer (int result, Dwfl *dwfl,
> +                   void **pbuffer, size_t *pbuffer_available,
> +                   void *memory_callback_arg,
> +                   Dwfl_Memory_Callback *memory_callback)
> +{
> +  if (*pbuffer !=3D NULL)
> +    (void) (*memory_callback) (dwfl, -1, pbuffer, pbuffer_available, 0, =
0,
> +			       memory_callback_arg);
> +  return result;
> +}
> +
> +#define release_buffer(result) \
> +  do_release_buffer (result, dwfl, &buffer, &buffer_available, \
> +                     memory_callback_arg, memory_callback)

This is just 3 lines, and it is either used as "return release_buffer
(result)" to cleanup and return a result or as release_buffer (0) just
to cleanup the buffer (if not NULL). It seems better to make the define
just the function body. Assuming your compiler does know about statement
expressions https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
Otherwise maybe just split it in two. Don't use an argument, just use it
as "cleanup_buffer()" plus a separate return line.

> +static inline bool
> +do_read_addrs (GElf_Addr vaddr, size_t n, GElf_Addr *addrs,
> +               uint_fast8_t elfdata, GElf_Addr *pread_vaddr,
> +               int elfclass, Dwfl *dwfl,
> +               void **pbuffer, size_t *pbuffer_available,
> +               void *memory_callback_arg,
> +               Dwfl_Memory_Callback *memory_callback)
> +{
> +  size_t nb =3D n * addrsize (elfclass); /* Address words -> bytes to re=
ad.  */
> +
> +  /* Read a new buffer if the old one doesn't cover these words.  */
> +  if (*pbuffer =3D=3D NULL
> +      || vaddr < *pread_vaddr
> +      || vaddr - *pread_vaddr + nb > *pbuffer_available)
> +    {
> +      do_release_buffer (0, dwfl, pbuffer, pbuffer_available,
> +                         memory_callback_arg, memory_callback);
> +
> +      *pread_vaddr =3D vaddr;
> +      int segndx =3D INTUSE(dwfl_addrsegment) (dwfl, vaddr, NULL);
> +      if (unlikely (segndx < 0)
> +	  || unlikely (! (*memory_callback) (dwfl, segndx,
> +					     pbuffer, pbuffer_available,
> +					     vaddr, nb, memory_callback_arg)))
> +	return true;
> +    }
> +
> +  Elf32_Addr (*a32)[n] =3D vaddr - *pread_vaddr + *pbuffer;
> +  Elf64_Addr (*a64)[n] =3D (void *) a32;
> +
> +  if (elfclass =3D=3D ELFCLASS32)
> +    {
> +      if (elfdata =3D=3D ELFDATA2MSB)
> +	for (size_t i =3D 0; i < n; ++i)
> +	  addrs[i] =3D BE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
> +      else
> +	for (size_t i =3D 0; i < n; ++i)
> +	  addrs[i] =3D LE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
> +    }
> +  else
> +    {
> +      if (elfdata =3D=3D ELFDATA2MSB)
> +	for (size_t i =3D 0; i < n; ++i)
> +	  addrs[i] =3D BE64 (read_8ubyte_unaligned_noncvt (&(*a64)[i]));
> +      else
> +	for (size_t i =3D 0; i < n; ++i)
> +	  addrs[i] =3D LE64 (read_8ubyte_unaligned_noncvt (&(*a64)[i]));
> +    }
> +
> +  return false;
> +}
> +
> +#define read_addrs(vaddr, n) \
> +  do_read_addrs (vaddr, n, addrs, elfdata, &read_vaddr, elfclass, dwfl, \
> +                 &buffer, &buffer_available, \
> +                 memory_callback_arg, memory_callback)

This very generic, but only used twice. Once to read 1 address and once
to read 4 addresses. Can't we make this a little simpler? Maybe just let
it handle reading one address. It might be a bit more hairy than needs
to because of the extra arguments needed to pass to the buffer cleanup.

> +static inline bool
> +do_consider_phdr (GElf_Word type, GElf_Addr vaddr, GElf_Xword filesz,
> +                  Dwfl *dwfl, GElf_Addr phdr, GElf_Addr *pdyn_bias,
> +                  GElf_Addr *pdyn_vaddr, GElf_Xword *pdyn_filesz)
> +{
> +  switch (type)
> +    {
> +    case PT_PHDR:
> +      if (*pdyn_bias =3D=3D (GElf_Addr) - 1
> +	  /* Do a sanity check on the putative address.  */
> +	  && ((vaddr & (dwfl->segment_align - 1))
> +	      =3D=3D (phdr & (dwfl->segment_align - 1))))
> +	{
> +	  *pdyn_bias =3D phdr - vaddr;
> +	  return *pdyn_vaddr !=3D 0;
> +	}
> +      break;
> +
> +    case PT_DYNAMIC:
> +      *pdyn_vaddr =3D vaddr;
> +      *pdyn_filesz =3D filesz;
> +      return *pdyn_bias !=3D (GElf_Addr) - 1;
> +    }
> +
> +  return false;
> +}
> +
> +#define consider_phdr(type, vaddr, filesz) \
> +  do_consider_phdr (type, vaddr, filesz, \
> +                    dwfl, phdr, &dyn_bias, &dyn_vaddr, &dyn_filesz)

This is just used twice in the function. Might be simpler to just inline
it twice and simplifying the switch by an if type =3D=3D PT_... check
instead of inventing a new function with 8 arguments.

Thanks,

Mark

--===============7561581554665514723==--