Re: [PATCH v3 04/13] recordmcount: Rewrite error/success handling

[Steven Rostedt <rostedt@goodmis.org>]

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))