Re: [RFC][PATCH v4 18/32] objtool: mcount: Move nop_mcount()

[Peter Zijlstra]

On Wed, Jun 17, 2020 at 10:36:20AM -0700, Matt Helsley wrote:
> On Fri, Jun 12, 2020 at 06:05:34PM +0200, Peter Zijlstra wrote:

> > On top of that, I suppose we can do something like the below.
> > 
> > Then you can simply write:
> > 
> > 	if (reloc->sym->class == SYM_MCOUNT && ..)
> 
> This looks like a good way to move towards a "single pass" through the ELF data
> for mcount.
> 
> What order do you want to see this patch go in? Before this series (i.e. perhaps
> just a kcov SYM_ class to start)? Early or late in this series? After?
> 
> Right now I'm thinking of putting this on the end of my series because
> I'm focusing on converting recordmcount in the series and this isn't
> strictly necessary but is definitely nicer.

No particular thoughts about where, so at the end would be fine.


> > ---
> > 
> > diff --git a/kernel/locking/Makefile b/kernel/locking/Makefile
> > index 45452facff3b..94e4b8fcf9c1 100644
> > --- a/kernel/locking/Makefile
> > +++ b/kernel/locking/Makefile
> > @@ -1,7 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  # Any varying coverage in these files is non-deterministic
> >  # and is generally not a function of system call inputs.
> > -KCOV_INSTRUMENT		:= n
> > +# KCOV_INSTRUMENT		:= n
> >  
> >  obj-y += mutex.o semaphore.o rwsem.o percpu-rwsem.o
> >  
> > diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> > index 432417a83902..133c0c285be6 100644
> > --- a/tools/objtool/elf.c
> > +++ b/tools/objtool/elf.c
> > @@ -341,6 +341,24 @@ static int read_sections(struct elf *elf)
> >  	return 0;
> >  }
> >  
> > +static bool is_mcount_symbol(const char *name)
> > +{
> > +	if (name[0] == '.')
> > +		name++;
> > +
> > +	if (name[0] == '_')
> > +		name++;
> > +
> > +	return !strcmp(name, "mcount", 6) ||
> 
> Looks like you intended this to be a strncmp() but I don't see a reason to
> use strncmp(). Am I missing something?

typing hard :-)

> > +	       !strcmp(name, "_fentry__") ||
> > +	       !strcmp(name, "_gnu_mcount_nc");
> > +}
> 
> This mashes all of the arch-specific mcount name checks together. I
> don't see a problem with that because I doubt there will be a collision
> with other functions. 

This, I assumed it would just work.

> Just to be careful I looked through the Clang and
> GCC sources, though I only dug through the history of Clang's output --
> GCC's history with respect to mcount symbol names across architectures is
> much harder to trace so I only looked at the current sources.

Fair enough; thanks for the due-dilligence.