Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 17 Jun 2020 20:30:13 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Matt Helsley <mhelsley@vmware.com>, linux-kernel@vger.kernel.org,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Sami Tolvanen <samitolvanen@google.com>,
        Julien Thierry <jthierry@redhat.com>,
        Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>
Subject: Re: [RFC][PATCH v4 18/32] objtool: mcount: Move nop_mcount()
Message-ID: <20200617183013.GD576905@hirez.programming.kicks-ass.net>
References: <cover.1591125127.git.mhelsley@vmware.com>
 <7109ceb239a88c2901eeb7f52c29f69cdb413cd3.1591125127.git.mhelsley@vmware.com>
 <20200612132656.GQ2531@hirez.programming.kicks-ass.net>
 <20200612160534.GD2554@hirez.programming.kicks-ass.net>
 <20200617173620.GA89648@rlwimi.vmware.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200617173620.GA89648@rlwimi.vmware.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.