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