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.
This very generic, but only used twice. Once to read 1 address and once
> +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)
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.
This is just used twice in the function. Might be simpler to just inline
> +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)
it twice and simplifying the switch by an if type == PT_... check
instead of inventing a new function with 8 arguments.
Thanks,
Mark