Messages in this thread |  | | Date | Fri, 26 Jul 2019 13:45:23 -0400 | From | Steven Rostedt <> | Subject | Re: [PATCH v3 04/13] recordmcount: Rewrite error/success handling |
| |
On Wed, 24 Jul 2019 14:04:58 -0700 Matt Helsley <mhelsley@vmware.com> wrote:
Hi Matt,
As I'm applying these for real, I'm taking a deeper look at the patches. This one I have some questions about.
> Recordmcount uses setjmp/longjmp to manage control flow as > it reads and then writes the ELF file. This unusual control > flow is hard to follow and check in addition to being unlike > kernel coding style. > > So we rewrite these paths to use regular return values to > indicate error/success. When an error or previously-completed object > file is found we return an error code following kernel > coding conventions -- negative error values and 0 for success when > we're not returning a pointer. We return NULL for those that fail > and return non-NULL pointers otherwise. > > One oddity is already_has_rel_mcount -- there we use pointer comparison > rather than string comparison to differentiate between > previously-processed object files and returning the name of a text > section.
This is fine, but it's got a strange implementation.
> diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h > index c1e1b04b4871..909a3e4775c2 100644 > --- a/scripts/recordmcount.h > +++ b/scripts/recordmcount.h > @@ -24,7 +24,9 @@ > #undef mcount_adjust > #undef sift_rel_mcount > #undef nop_mcount > +#undef missing_sym > #undef find_secsym_ndx > +#undef already_has_rel_mcount
Why do we need these as defines? Can't you just have a single:
const char *already_has_mcount = "success";
in recordmcount.c before recordmcount.h is included?
And same for missing_sym.
Another, probably more robust way of doing this, is change find_secsym_ndx() to return 0 on success and -1 on missing symbol, and just pass a pointer by reference to fill the recsym (which doesn't have to be a constant).
I've applied the first 3 patches of this series, but stopped here. I'll continue to take a deeper look at your other patches.
Thanks!
-- Steve
> #undef __has_rel_mcount > #undef has_rel_mcount > #undef tot_relsize > @@ -54,7 +56,9 @@ > # define append_func append64 > # define sift_rel_mcount sift64_rel_mcount > # define nop_mcount nop_mcount_64 > +# define missing_sym missing_sym_64 > # define find_secsym_ndx find64_secsym_ndx > +# define already_has_rel_mcount already_has_rel_mcount_64 > # define __has_rel_mcount __has64_rel_mcount > # define has_rel_mcount has64_rel_mcount > # define tot_relsize tot64_relsize > @@ -87,7 +91,9 @@ > # define append_func append32 > # define sift_rel_mcount sift32_rel_mcount > # define nop_mcount nop_mcount_32 > +# define missing_sym missing_sym_32 > # define find_secsym_ndx find32_secsym_ndx > +# define already_has_rel_mcount already_has_rel_mcount_32 > # define __has_rel_mcount __has32_rel_mcount > # define has_rel_mcount has32_rel_mcount > # define tot_relsize tot32_relsize
> +static const unsigned int missing_sym = (unsigned int)-1; > > /* > * Find a symbol in the given section, to be used as the base for relocating > @@ -443,9 +469,11 @@ static unsigned find_secsym_ndx(unsigned const txtndx, > } > fprintf(stderr, "Cannot find symbol for section %u: %s.\n", > txtndx, txtname); > - fail_file(); > + cleanup(); > + return missing_sym; > } > > +char const *already_has_rel_mcount = "success"; /* our work here is done! */ > > /* Evade ISO C restriction: no declaration after statement in has_rel_mcount. */ > static char const * > @@ -461,7 +489,8 @@ __has_rel_mcount(Elf_Shdr const *const relhdr, /* is SHT_REL or SHT_RELA */ > if (strcmp("__mcount_loc", txtname) == 0) { > fprintf(stderr, "warning: __mcount_loc already exists: %s\n", > fname); > - succeed_file(); > + cleanup(); > + return already_has_rel_mcount; > } > if (w(txthdr->sh_type) != SHT_PROGBITS || > !(_w(txthdr->sh_flags) & SHF_EXECINSTR))
|  |