Mark, Roland,
How do you think about this patch?
Is this an acceptable way to convert nested functions?
On Fri, Sep 18, 2015 at 11:25 AM, Chih-Hung Hsieh <c...@google.com> wrote:
> Now they should compile with clang.
>
> Used local variables are passed to new file scope functions
> as constant parameters, or pointers, or embedded in a
> 'state' structure.
>
> One simple function "report" is changed to a macro.
> It triggers a gcc false positive -Werror=maybe-uninitialized,
> so the local variables are explicitly initialized.
>
> Signed-off-by: Chih-Hung Hsieh <c...@google.com>
> ---
> libdwfl/ChangeLog | 16 ++++
> libdwfl/linux-kernel-modules.c | 120 +++++++++++++++------------
> libdwfl/relocate.c | 184
> ++++++++++++++++++++++-------------------
> 3 files changed, 182 insertions(+), 138 deletions(-)
>
> diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
> index 0ab386f..b9f42a1 100644
> --- a/libdwfl/ChangeLog
> +++ b/libdwfl/ChangeLog
> @@ -1,3 +1,19 @@
> +2015-09-18 Chih-Hung Hsieh <c...@google.com>
> +
> + * relocate.c (relocate_section): Move nested function "relocate"
> + to file scope, pass all used local variables as constants.
> + Move nested "check_badreltype" to file scope, pass addresses of
> + updated used local variables.
> + * linux-kernel-modules.c (intuit_kernel_bounds): Move nested
> function
> + "read_address" to file scope, pass all updated local variables in
> + a state structure.
> + * linux-kernel-modules.c (dwfl_linux_kernel_find_elf): Move nested
> + function "subst_name" to file scope, pass all used local variables
> + as constants.
> + * linux-kernel-modules.c (dwfl_linux_kernel_report_kernel): Replace
> + simple nested function "report" with a macro. Work around gcc
> static
> + analysis error -Werror=maybe-uninitialized.
> +
> 2015-09-09 Chih-Hung Hsieh <c...@google.com>
> Mark Wielaard <m...@redhat.com>
>
> diff --git a/libdwfl/linux-kernel-modules.c
> b/libdwfl/linux-kernel-modules.c
> index 236e2cd..dafe893 100644
> --- a/libdwfl/linux-kernel-modules.c
> +++ b/libdwfl/linux-kernel-modules.c
> @@ -440,46 +440,55 @@ dwfl_linux_kernel_report_offline (Dwfl *dwfl, const
> char *release,
> INTDEF (dwfl_linux_kernel_report_offline)
>
>
> +/* State of read_address used by intuit_kernel_bounds. */
> +struct read_address_state {
> + FILE *f;
> + char *line;
> + size_t linesz;
> + size_t n;
> + char *p;
> + const char *type;
> +};
> +
> +static inline bool
> +read_address (struct read_address_state *state, Dwarf_Addr *addr)
> +{
> + if ((state->n = getline (&state->line, &state->linesz, state->f)) < 1 ||
> + state->line[state->n - 2] == ']')
> + return false;
> + *addr = strtoull (state->line, &state->p, 16);
> + state->p += strspn (state->p, " \t");
> + state->type = strsep (&state->p, " \t\n");
> + if (state->type == NULL)
> + return false;
> + return state->p != NULL && state->p != state->line;
> +}
> +
> +
> /* Grovel around to guess the bounds of the runtime kernel image. */
> static int
> intuit_kernel_bounds (Dwarf_Addr *start, Dwarf_Addr *end, Dwarf_Addr
> *notes)
> {
> - FILE *f = fopen (KSYMSFILE, "r");
> - if (f == NULL)
> + struct read_address_state state = { NULL, NULL, 0, 0, NULL, NULL };
> +
> + state.f = fopen (KSYMSFILE, "r");
> + if (state.f == NULL)
> return errno;
>
> - (void) __fsetlocking (f, FSETLOCKING_BYCALLER);
> + (void) __fsetlocking (state.f, FSETLOCKING_BYCALLER);
>
> *notes = 0;
>
> - char *line = NULL;
> - size_t linesz = 0;
> - size_t n;
> - char *p = NULL;
> - const char *type;
> -
> - inline bool read_address (Dwarf_Addr *addr)
> - {
> - if ((n = getline (&line, &linesz, f)) < 1 || line[n - 2] == ']')
> - return false;
> - *addr = strtoull (line, &p, 16);
> - p += strspn (p, " \t");
> - type = strsep (&p, " \t\n");
> - if (type == NULL)
> - return false;
> - return p != NULL && p != line;
> - }
> -
> int result;
> do
> - result = read_address (start) ? 0 : -1;
> - while (result == 0 && strchr ("TtRr", *type) == NULL);
> + result = read_address (&state, start) ? 0 : -1;
> + while (result == 0 && strchr ("TtRr", *state.type) == NULL);
>
> if (result == 0)
> {
> *end = *start;
> - while (read_address (end))
> - if (*notes == 0 && !strcmp (p, "__start_notes\n"))
> + while (read_address (&state, end))
> + if (*notes == 0 && !strcmp (state.p, "__start_notes\n"))
> *notes = *end;
>
> Dwarf_Addr round_kernel = sysconf (_SC_PAGE_SIZE);
> @@ -489,12 +498,12 @@ intuit_kernel_bounds (Dwarf_Addr *start, Dwarf_Addr
> *end, Dwarf_Addr *notes)
> if (*start >= *end || *end - *start < round_kernel)
> result = -1;
> }
> - free (line);
> + free (state.line);
>
> if (result == -1)
> - result = ferror_unlocked (f) ? errno : ENOEXEC;
> + result = ferror_unlocked (state.f) ? errno : ENOEXEC;
>
> - fclose (f);
> + fclose (state.f);
>
> return result;
> }
> @@ -619,12 +628,11 @@ check_module_notes (Dwfl_Module *mod)
> int
> dwfl_linux_kernel_report_kernel (Dwfl *dwfl)
> {
> - Dwarf_Addr start;
> - Dwarf_Addr end;
> - inline Dwfl_Module *report (void)
> - {
> - return INTUSE(dwfl_report_module) (dwfl, KERNEL_MODNAME, start,
> end);
> - }
> + Dwarf_Addr start = 0;
> + Dwarf_Addr end = 0;
> +
> + #define report() \
> + (INTUSE(dwfl_report_module) (dwfl, KERNEL_MODNAME, start, end))
>
> /* This is a bit of a kludge. If we already reported the kernel,
> don't bother figuring it out again--it never changes. */
> @@ -657,6 +665,29 @@ dwfl_linux_kernel_report_kernel (Dwfl *dwfl)
> INTDEF (dwfl_linux_kernel_report_kernel)
>
>
> +static inline bool
> +subst_name (char from, char to,
> + const char * const module_name,
> + char * const alternate_name,
> + const size_t namelen)
> +{
> + const char *n = memchr (module_name, from, namelen);
> + if (n == NULL)
> + return false;
> + char *a = mempcpy (alternate_name, module_name, n - module_name);
> + *a++ = to;
> + ++n;
> + const char *p;
> + while ((p = memchr (n, from, namelen - (n - module_name))) != NULL)
> + {
> + a = mempcpy (a, n, p - n);
> + *a++ = to;
> + n = p + 1;
> + }
> + memcpy (a, n, namelen - (n - module_name) + 1);
> + return true;
> +}
> +
> /* Dwfl_Callbacks.find_elf for the running Linux kernel and its modules.
> */
>
> int
> @@ -713,25 +744,8 @@ dwfl_linux_kernel_find_elf (Dwfl_Module *mod,
> free (modulesdir[0]);
> return ENOMEM;
> }
> - inline bool subst_name (char from, char to)
> - {
> - const char *n = memchr (module_name, from, namelen);
> - if (n == NULL)
> - return false;
> - char *a = mempcpy (alternate_name, module_name, n - module_name);
> - *a++ = to;
> - ++n;
> - const char *p;
> - while ((p = memchr (n, from, namelen - (n - module_name))) != NULL)
> - {
> - a = mempcpy (a, n, p - n);
> - *a++ = to;
> - n = p + 1;
> - }
> - memcpy (a, n, namelen - (n - module_name) + 1);
> - return true;
> - }
> - if (!subst_name ('-', '_') && !subst_name ('_', '-'))
> + if (!subst_name ('-', '_', module_name, alternate_name, namelen) &&
> + !subst_name ('_', '-', module_name, alternate_name, namelen))
> alternate_name[0] = '\0';
>
> FTSENT *f;
> diff --git a/libdwfl/relocate.c b/libdwfl/relocate.c
> index e102e1e..2dc6737 100644
> --- a/libdwfl/relocate.c
> +++ b/libdwfl/relocate.c
> @@ -277,77 +277,18 @@ resolve_symbol (Dwfl_Module *referer, struct
> reloc_symtab_cache *symtab,
> return DWFL_E_RELUNDEF;
> }
>
> +/* Apply one relocation. Returns true for any invalid data. */
> static Dwfl_Error
> -relocate_section (Dwfl_Module *mod, Elf *relocated, const GElf_Ehdr *ehdr,
> - size_t shstrndx, struct reloc_symtab_cache *reloc_symtab,
> - Elf_Scn *scn, GElf_Shdr *shdr,
> - Elf_Scn *tscn, bool debugscn, bool partial)
> +relocate (Dwfl_Module * const mod,
> + Elf * const relocated,
> + struct reloc_symtab_cache * const reloc_symtab,
> + Elf_Data * const tdata,
> + const GElf_Ehdr * const ehdr,
> + GElf_Addr offset,
> + const GElf_Sxword *addend,
> + int rtype,
> + int symndx)
> {
> - /* First, fetch the name of the section these relocations apply to. */
> - GElf_Shdr tshdr_mem;
> - GElf_Shdr *tshdr = gelf_getshdr (tscn, &tshdr_mem);
> - const char *tname = elf_strptr (relocated, shstrndx, tshdr->sh_name);
> - if (tname == NULL)
> - return DWFL_E_LIBELF;
> -
> - if (unlikely (tshdr->sh_type == SHT_NOBITS) || unlikely (tshdr->sh_size
> == 0))
> - /* No contents to relocate. */
> - return DWFL_E_NOERROR;
> -
> - if (debugscn && ! ebl_debugscn_p (mod->ebl, tname))
> - /* This relocation section is not for a debugging section.
> - Nothing to do here. */
> - return DWFL_E_NOERROR;
> -
> - /* Fetch the section data that needs the relocations applied. */
> - Elf_Data *tdata = elf_rawdata (tscn, NULL);
> - if (tdata == NULL)
> - return DWFL_E_LIBELF;
> -
> - /* If either the section that needs the relocation applied, or the
> - section that the relocations come from overlap one of the ehdrs,
> - shdrs or phdrs data then we refuse to do the relocations. It
> - isn't illegal for ELF section data to overlap the header data,
> - but updating the (relocation) data might corrupt the in-memory
> - libelf headers causing strange corruptions or errors. */
> - size_t ehsize = gelf_fsize (relocated, ELF_T_EHDR, 1, EV_CURRENT);
> - if (unlikely (shdr->sh_offset < ehsize
> - || tshdr->sh_offset < ehsize))
> - return DWFL_E_BADELF;
> -
> - GElf_Off shdrs_start = ehdr->e_shoff;
> - size_t shnums;
> - if (elf_getshdrnum (relocated, &shnums) < 0)
> - return DWFL_E_LIBELF;
> - /* Overflows will have been checked by elf_getshdrnum/get|rawdata. */
> - size_t shentsize = gelf_fsize (relocated, ELF_T_SHDR, 1, EV_CURRENT);
> - GElf_Off shdrs_end = shdrs_start + shnums * shentsize;
> - if (unlikely ((shdrs_start < shdr->sh_offset + shdr->sh_size
> - && shdr->sh_offset < shdrs_end)
> - || (shdrs_start < tshdr->sh_offset + tshdr->sh_size
> - && tshdr->sh_offset < shdrs_end)))
> - return DWFL_E_BADELF;
> -
> - GElf_Off phdrs_start = ehdr->e_phoff;
> - size_t phnums;
> - if (elf_getphdrnum (relocated, &phnums) < 0)
> - return DWFL_E_LIBELF;
> - if (phdrs_start != 0 && phnums != 0)
> - {
> - /* Overflows will have been checked by elf_getphdrnum/get|rawdata.
> */
> - size_t phentsize = gelf_fsize (relocated, ELF_T_PHDR, 1,
> EV_CURRENT);
> - GElf_Off phdrs_end = phdrs_start + phnums * phentsize;
> - if (unlikely ((phdrs_start < shdr->sh_offset + shdr->sh_size
> - && shdr->sh_offset < phdrs_end)
> - || (phdrs_start < tshdr->sh_offset + tshdr->sh_size
> - && tshdr->sh_offset < phdrs_end)))
> - return DWFL_E_BADELF;
> - }
> -
> - /* Apply one relocation. Returns true for any invalid data. */
> - Dwfl_Error relocate (GElf_Addr offset, const GElf_Sxword *addend,
> - int rtype, int symndx)
> - {
> /* First see if this is a reloc we can handle.
> If we are skipping it, don't bother resolving the symbol. */
>
> @@ -482,7 +423,89 @@ relocate_section (Dwfl_Module *mod, Elf *relocated,
> const GElf_Ehdr *ehdr,
>
> /* We have applied this relocation! */
> return DWFL_E_NOERROR;
> - }
> +}
> +
> +static inline void
> +check_badreltype (bool *first_badreltype,
> + Dwfl_Module *mod,
> + Dwfl_Error *result)
> +{
> + if (*first_badreltype)
> + {
> + *first_badreltype = false;
> + if (ebl_get_elfmachine (mod->ebl) == EM_NONE)
> + /* This might be because ebl_openbackend failed to find
> + any libebl_CPU.so library. Diagnose that clearly. */
> + *result = DWFL_E_UNKNOWN_MACHINE;
> + }
> +}
> +
> +static Dwfl_Error
> +relocate_section (Dwfl_Module *mod, Elf *relocated, const GElf_Ehdr *ehdr,
> + size_t shstrndx, struct reloc_symtab_cache *reloc_symtab,
> + Elf_Scn *scn, GElf_Shdr *shdr,
> + Elf_Scn *tscn, bool debugscn, bool partial)
> +{
> + /* First, fetch the name of the section these relocations apply to. */
> + GElf_Shdr tshdr_mem;
> + GElf_Shdr *tshdr = gelf_getshdr (tscn, &tshdr_mem);
> + const char *tname = elf_strptr (relocated, shstrndx, tshdr->sh_name);
> + if (tname == NULL)
> + return DWFL_E_LIBELF;
> +
> + if (unlikely (tshdr->sh_type == SHT_NOBITS) || unlikely (tshdr->sh_size
> == 0))
> + /* No contents to relocate. */
> + return DWFL_E_NOERROR;
> +
> + if (debugscn && ! ebl_debugscn_p (mod->ebl, tname))
> + /* This relocation section is not for a debugging section.
> + Nothing to do here. */
> + return DWFL_E_NOERROR;
> +
> + /* Fetch the section data that needs the relocations applied. */
> + Elf_Data *tdata = elf_rawdata (tscn, NULL);
> + if (tdata == NULL)
> + return DWFL_E_LIBELF;
> +
> + /* If either the section that needs the relocation applied, or the
> + section that the relocations come from overlap one of the ehdrs,
> + shdrs or phdrs data then we refuse to do the relocations. It
> + isn't illegal for ELF section data to overlap the header data,
> + but updating the (relocation) data might corrupt the in-memory
> + libelf headers causing strange corruptions or errors. */
> + size_t ehsize = gelf_fsize (relocated, ELF_T_EHDR, 1, EV_CURRENT);
> + if (unlikely (shdr->sh_offset < ehsize
> + || tshdr->sh_offset < ehsize))
> + return DWFL_E_BADELF;
> +
> + GElf_Off shdrs_start = ehdr->e_shoff;
> + size_t shnums;
> + if (elf_getshdrnum (relocated, &shnums) < 0)
> + return DWFL_E_LIBELF;
> + /* Overflows will have been checked by elf_getshdrnum/get|rawdata. */
> + size_t shentsize = gelf_fsize (relocated, ELF_T_SHDR, 1, EV_CURRENT);
> + GElf_Off shdrs_end = shdrs_start + shnums * shentsize;
> + if (unlikely ((shdrs_start < shdr->sh_offset + shdr->sh_size
> + && shdr->sh_offset < shdrs_end)
> + || (shdrs_start < tshdr->sh_offset + tshdr->sh_size
> + && tshdr->sh_offset < shdrs_end)))
> + return DWFL_E_BADELF;
> +
> + GElf_Off phdrs_start = ehdr->e_phoff;
> + size_t phnums;
> + if (elf_getphdrnum (relocated, &phnums) < 0)
> + return DWFL_E_LIBELF;
> + if (phdrs_start != 0 && phnums != 0)
> + {
> + /* Overflows will have been checked by elf_getphdrnum/get|rawdata.
> */
> + size_t phentsize = gelf_fsize (relocated, ELF_T_PHDR, 1,
> EV_CURRENT);
> + GElf_Off phdrs_end = phdrs_start + phnums * phentsize;
> + if (unlikely ((phdrs_start < shdr->sh_offset + shdr->sh_size
> + && shdr->sh_offset < phdrs_end)
> + || (phdrs_start < tshdr->sh_offset + tshdr->sh_size
> + && tshdr->sh_offset < phdrs_end)))
> + return DWFL_E_BADELF;
> + }
>
> /* Fetch the relocation section and apply each reloc in it. */
> Elf_Data *reldata = elf_getdata (scn, NULL);
> @@ -491,17 +514,6 @@ relocate_section (Dwfl_Module *mod, Elf *relocated,
> const GElf_Ehdr *ehdr,
>
> Dwfl_Error result = DWFL_E_NOERROR;
> bool first_badreltype = true;
> - inline void check_badreltype (void)
> - {
> - if (first_badreltype)
> - {
> - first_badreltype = false;
> - if (ebl_get_elfmachine (mod->ebl) == EM_NONE)
> - /* This might be because ebl_openbackend failed to find
> - any libebl_CPU.so library. Diagnose that clearly. */
> - result = DWFL_E_UNKNOWN_MACHINE;
> - }
> - }
>
> size_t sh_entsize
> = gelf_fsize (relocated, shdr->sh_type == SHT_REL ? ELF_T_REL :
> ELF_T_RELA,
> @@ -514,10 +526,11 @@ relocate_section (Dwfl_Module *mod, Elf *relocated,
> const GElf_Ehdr *ehdr,
> GElf_Rel rel_mem, *r = gelf_getrel (reldata, relidx, &rel_mem);
> if (r == NULL)
> return DWFL_E_LIBELF;
> - result = relocate (r->r_offset, NULL,
> + result = relocate (mod, relocated, reloc_symtab, tdata, ehdr,
> + r->r_offset, NULL,
> GELF_R_TYPE (r->r_info),
> GELF_R_SYM (r->r_info));
> - check_badreltype ();
> + check_badreltype (&first_badreltype, mod, &result);
> if (partial)
> switch (result)
> {
> @@ -543,10 +556,11 @@ relocate_section (Dwfl_Module *mod, Elf *relocated,
> const GElf_Ehdr *ehdr,
> &rela_mem);
> if (r == NULL)
> return DWFL_E_LIBELF;
> - result = relocate (r->r_offset, &r->r_addend,
> + result = relocate (mod, relocated, reloc_symtab, tdata, ehdr,
> + r->r_offset, &r->r_addend,
> GELF_R_TYPE (r->r_info),
> GELF_R_SYM (r->r_info));
> - check_badreltype ();
> + check_badreltype (&first_badreltype, mod, &result);
> if (partial)
> switch (result)
> {
> --
> 2.6.0.rc0.131.gf624c3d
>
>
