This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [PATCH] Move nested functions in link_map.c to file scope.
- From: Chih-Hung Hsieh <chh at google dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Wed, 06 Jan 2016 11:35:46 -0800
- Subject: Re: [PATCH] Move nested functions in link_map.c to file scope.
Mark,
Thanks for pushing several of my patches during the holidays.
I have merged all of them and latest elfutils changes into Android open
source project.
We are getting closer to compile all needed elfutils files with clang/llvm
for Android.
I will continue to fix the remain elfutils issues after fixing a few other
urgent unrelated Android problems. Thanks.
On Sun, Jan 3, 2016 at 1:25 PM, Mark Wielaard <mjw@redhat.com> wrote:
> 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 != 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 = n * addrsize (elfclass); /* Address words -> bytes to
> read. */
> > +
> > + /* Read a new buffer if the old one doesn't cover these words. */
> > + if (*pbuffer == 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 = vaddr;
> > + int segndx = 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] = vaddr - *pread_vaddr + *pbuffer;
> > + Elf64_Addr (*a64)[n] = (void *) a32;
> > +
> > + if (elfclass == ELFCLASS32)
> > + {
> > + if (elfdata == ELFDATA2MSB)
> > + for (size_t i = 0; i < n; ++i)
> > + addrs[i] = BE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
> > + else
> > + for (size_t i = 0; i < n; ++i)
> > + addrs[i] = LE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
> > + }
> > + else
> > + {
> > + if (elfdata == ELFDATA2MSB)
> > + for (size_t i = 0; i < n; ++i)
> > + addrs[i] = BE64 (read_8ubyte_unaligned_noncvt (&(*a64)[i]));
> > + else
> > + for (size_t i = 0; i < n; ++i)
> > + addrs[i] = 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 == (GElf_Addr) - 1
> > + /* Do a sanity check on the putative address. */
> > + && ((vaddr & (dwfl->segment_align - 1))
> > + == (phdr & (dwfl->segment_align - 1))))
> > + {
> > + *pdyn_bias = phdr - vaddr;
> > + return *pdyn_vaddr != 0;
> > + }
> > + break;
> > +
> > + case PT_DYNAMIC:
> > + *pdyn_vaddr = vaddr;
> > + *pdyn_filesz = filesz;
> > + return *pdyn_bias != (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 == PT_... check
> instead of inventing a new function with 8 arguments.
>
> Thanks,
>
> Mark
>
>
Mark,
Thanks for pushing several of my patches during the holidays.
I have merged all of them and latest elfutils changes into Android open source project.
We are getting closer to compile all needed elfutils files with clang/llvm for Android.
I will continue to fix the remain elfutils issues after fixing a few other urgent unrelated Android problems. Thanks.
On Sun, Jan 3, 2016 at 1:25 PM, Mark Wielaard
<mjw@redhat.com> wrote:
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 != 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 = n * addrsize (elfclass); /* Address words -> bytes to read. */
> +
> + /* Read a new buffer if the old one doesn't cover these words. */
> +Â if (*pbuffer == 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 = vaddr;
> +Â Â Â int segndx = 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] = vaddr - *pread_vaddr + *pbuffer;
> +Â Elf64_Addr (*a64)[n] = (void *) a32;
> +
> +Â if (elfclass == ELFCLASS32)
> +Â Â {
> +Â Â Â if (elfdata == ELFDATA2MSB)
> +Â Â Â for (size_t i = 0; i < n; ++i)
> +Â Â Â Â addrs[i] = BE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
> +Â Â Â else
> +Â Â Â for (size_t i = 0; i < n; ++i)
> +Â Â Â Â addrs[i] = LE32 (read_4ubyte_unaligned_noncvt (&(*a32)[i]));
> +Â Â }
> +Â else
> +Â Â {
> +Â Â Â if (elfdata == ELFDATA2MSB)
> +Â Â Â for (size_t i = 0; i < n; ++i)
> +Â Â Â Â addrs[i] = BE64 (read_8ubyte_unaligned_noncvt (&(*a64)[i]));
> +Â Â Â else
> +Â Â Â for (size_t i = 0; i < n; ++i)
> +Â Â Â Â addrs[i] = 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 == (GElf_Addr) - 1
> +    /* Do a sanity check on the putative address. */
> +Â Â Â Â && ((vaddr & (dwfl->segment_align - 1))
> +Â Â Â Â Â Â == (phdr & (dwfl->segment_align - 1))))
> +Â Â Â {
> +Â Â Â Â *pdyn_bias = phdr - vaddr;
> +Â Â Â Â return *pdyn_vaddr != 0;
> +Â Â Â }
> +Â Â Â break;
> +
> +Â Â case PT_DYNAMIC:
> +Â Â Â *pdyn_vaddr = vaddr;
> +Â Â Â *pdyn_filesz = filesz;
> +Â Â Â return *pdyn_bias != (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 == PT_... check
instead of inventing a new function with 8 arguments.
Thanks,
Mark