Re: [RFC][PATCH v4 18/32] objtool: mcount: Move nop_mcount()
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.
Re: [RFC][PATCH v4 18/32] objtool: mcount: Move nop_mcount()
On Fri, Jun 12, 2020 at 06:05:34PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 12, 2020 at 03:26:57PM +0200, Peter Zijlstra wrote:
> > On Tue, Jun 02, 2020 at 12:50:11PM -0700, Matt Helsley wrote:
> > > +static int nop_mcount(struct section * const rels,
> > > + const char *const txtname)
> > > +{
> > > + struct reloc *reloc;
> > > + struct section *txts = find_section_by_index(lf, rels->sh.sh_info);
> > > + unsigned mcountsym = 0;
> > > + int once = 0;
> > > +
> > > + list_for_each_entry(reloc, &rels->reloc_list, list) {
> > > + int ret = -1;
> > > +
> > > + if (!mcountsym)
> > > + mcountsym = get_mcountsym(reloc);
> > > +
> > > + if (mcountsym == GELF_R_INFO(reloc->sym->idx, reloc->type) &&
> > > !is_fake_mcount(reloc)) {
> >
> > This makes no sense to me; why not have mcountsym be a 'struct symbol
> > *' and have get_mcountsym() return one of those.
> >
> > if (reloc->sym == mcountsym && ... )
> >
> > is much nicer, no?
(this is already incorporated in my unposted revisions but...)
>
> 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.
>
> ---
>
> 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?
> +!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. 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.
(the rest looks good)
Cheers,
-Matt Helsley
Re: [RFC][PATCH v4 18/32] objtool: mcount: Move nop_mcount()
On Fri, Jun 12, 2020 at 03:26:56PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 02, 2020 at 12:50:11PM -0700, Matt Helsley wrote:
> > +static int nop_mcount(struct section * const rels,
> > + const char *const txtname)
> > +{
> > + struct reloc *reloc;
> > + struct section *txts = find_section_by_index(lf, rels->sh.sh_info);
> > + unsigned mcountsym = 0;
> > + int once = 0;
> > +
> > + list_for_each_entry(reloc, &rels->reloc_list, list) {
> > + int ret = -1;
> > +
> > + if (!mcountsym)
> > + mcountsym = get_mcountsym(reloc);
> > +
> > + if (mcountsym == GELF_R_INFO(reloc->sym->idx, reloc->type) &&
> > !is_fake_mcount(reloc)) {
>
> This makes no sense to me; why not have mcountsym be a 'struct symbol
> *' and have get_mcountsym() return one of those.
>
> if (reloc->sym == mcountsym && ... )
>
> is much nicer, no?
Indeed! I'll change it from returning an unsigned long to struct symbol *
before I
move it out of the wrapper code.
>
> > + if (make_nop) {
> > + ret = make_nop(txts, reloc->offset);
> > + if (ret < 0)
> > + return -1;
> > + }
> > + if (warn_on_notrace_sect && !once) {
> > + printf("Section %s has mcount callers being
> > ignored\n",
> > + txtname);
> > + once = 1;
> > + /* just warn? */
> > + if (!make_nop)
> > + return 0;
> > + }
> > + }
> > +
> > + /*
> > +* If we successfully removed the mcount, mark the relocation
> > +* as a nop (don't do anything with it).
> > +*/
> > + if (!ret) {
> > + reloc->type = rel_type_nop;
> > + rels->changed = true;
>
> I have an elf_write_rela(), I'll make sure to Cc you.
Thanks! I might also make use of your patch to rewrite instructions. We
need a way to turn certain prologue instructions into nops. Would it be
more widely useful to move that functionality out of mcount and into
the objtool ELF/per-arch code or do you think it's better inside the
mcount subcommand code?
Cheers,
-Matt
Re: [RFC][PATCH v4 18/32] objtool: mcount: Move nop_mcount()
On Fri, Jun 12, 2020 at 03:26:57PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 02, 2020 at 12:50:11PM -0700, Matt Helsley wrote:
> > +static int nop_mcount(struct section * const rels,
> > + const char *const txtname)
> > +{
> > + struct reloc *reloc;
> > + struct section *txts = find_section_by_index(lf, rels->sh.sh_info);
> > + unsigned mcountsym = 0;
> > + int once = 0;
> > +
> > + list_for_each_entry(reloc, &rels->reloc_list, list) {
> > + int ret = -1;
> > +
> > + if (!mcountsym)
> > + mcountsym = get_mcountsym(reloc);
> > +
> > + if (mcountsym == GELF_R_INFO(reloc->sym->idx, reloc->type) &&
> > !is_fake_mcount(reloc)) {
>
> This makes no sense to me; why not have mcountsym be a 'struct symbol
> *' and have get_mcountsym() return one of those.
>
> if (reloc->sym == mcountsym && ... )
>
> is much nicer, no?
On top of that, I suppose we can do something like the below.
Then you can simply write:
if (reloc->sym->class == SYM_MCOUNT && ..)
---
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) ||
+ !strcmp(name, "_fentry__") ||
+ !strcmp(name, "_gnu_mcount_nc");
+}
+
+static bool is_kcov_symbol(const char *name)
+{
+ return !strncmp(name, "__sanitizer_cov_", 16);
+}
+
static int read_symbols(struct elf *elf)
{
struct section *symtab, *symtab_shndx, *sec;
@@ -410,6 +428,12 @@ static int read_symbols(struct elf *elf)
} else
sym->sec = find_section_by_index(elf, 0);
+
+ if (is_mcount_symbol(sym->name))
+ sym->class = SYM_MCOUNT;
+ else if (is_kcov_symbol(sym->name))
+ sym->class = SYM_KCOV;
+
sym->offset = sym->sym.st_value;
sym->len = sym->sym.st_size;
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index 78a2db23b8b6..3c1cccb7b5ff 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -42,6 +42,12 @@ struct section {
bool changed, text, rodata, noinstr;
};
+enum symbol_class {
+ SYM_REGULAR = 0,
+ SYM_MCOUNT,
+ SYM_KCOV,
+};
+
struct symbol {
struct list_head list;
struct rb_node node;
@@ -55,6 +61,7 @@ struct symbol {
unsigned long offset;
unsigned int len;
struct symbol *pfunc, *cfunc, *alias;
+ enum symbol_class class;
bool uaccess_safe;
};
Re: [RFC][PATCH v4 18/32] objtool: mcount: Move nop_mcount()
On Tue, Jun 02, 2020 at 12:50:11PM -0700, Matt Helsley wrote:
> +static int nop_mcount(struct section * const rels,
> + const char *const txtname)
> +{
> + struct reloc *reloc;
> + struct section *txts = find_section_by_index(lf, rels->sh.sh_info);
> + unsigned mcountsym = 0;
> + int once = 0;
> +
> + list_for_each_entry(reloc, &rels->reloc_list, list) {
> + int ret = -1;
> +
> + if (!mcountsym)
> + mcountsym = get_mcountsym(reloc);
> +
> + if (mcountsym == GELF_R_INFO(reloc->sym->idx, reloc->type) &&
> !is_fake_mcount(reloc)) {
This makes no sense to me; why not have mcountsym be a 'struct symbol
*' and have get_mcountsym() return one of those.
if (reloc->sym == mcountsym && ... )
is much nicer, no?
> + if (make_nop) {
> + ret = make_nop(txts, reloc->offset);
> + if (ret < 0)
> + return -1;
> + }
> + if (warn_on_notrace_sect && !once) {
> + printf("Section %s has mcount callers being
> ignored\n",
> +txtname);
> + once = 1;
> + /* just warn? */
> + if (!make_nop)
> + return 0;
> + }
> + }
> +
> + /*
> + * If we successfully removed the mcount, mark the relocation
> + * as a nop (don't do anything with it).
> + */
> + if (!ret) {
> + reloc->type = rel_type_nop;
> + rels->changed = true;
I have an elf_write_rela(), I'll make sure to Cc you.
> + }
> + }
> + return 0;
> +}
[RFC][PATCH v4 18/32] objtool: mcount: Move nop_mcount()
The nop_mcount() function overwrites mcount calls that should be
ignored with no-ops. This operation varies by architecture and
wordsize so we retain the function pointers used to implement
the fundamental operation while nop_mcount() itself is responsible
for walking the relocations, determining if they should be turned
into no-ops, then calling the arch-specific code. Since none of
these use the recordmcount ELF wrappers anymore we can move it out
of the wrapper.
Signed-off-by: Matt Helsley
---
tools/objtool/recordmcount.c | 47 +
tools/objtool/recordmcount.h | 50
2 files changed, 47 insertions(+), 50 deletions(-)
diff --git a/tools/objtool/recordmcount.c b/tools/objtool/recordmcount.c
index 89762908290e..88998a505859 100644
--- a/tools/objtool/recordmcount.c
+++ b/tools/objtool/recordmcount.c
@@ -398,6 +398,53 @@ static int find_section_sym_index(unsigned const txtndx,
return missing_sym;
}
+/*
+ * Read the relocation table again, but this time its called on sections
+ * that are not going to be traced. The mcount calls here will be converted
+ * into nops.
+ */
+static int nop_mcount(struct section * const rels,
+ const char *const txtname)
+{
+ struct reloc *reloc;
+ struct section *txts = find_section_by_index(lf, rels->sh.sh_info);
+ unsigned mcountsym = 0;
+ int once = 0;
+
+ list_for_each_entry(reloc, &rels->reloc_list, list) {
+ int ret = -1;
+
+ if (!mcountsym)
+ mcountsym = get_mcountsym(reloc);
+
+ if (mcountsym == GELF_R_INFO(reloc->sym->idx, reloc->type) &&
!is_fake_mcount(reloc)) {
+ if (make_nop) {
+ ret = make_nop(txts, reloc->offset);
+ if (ret < 0)
+ return -1;
+ }
+ if (warn_on_notrace_sect && !once) {
+ printf("Section %s has mcount callers being
ignored\n",
+ txtname);
+ once = 1;
+ /* just warn? */
+ if (!make_nop)
+ return 0;
+ }
+ }
+
+ /*
+* If we successfully removed the mcount, mark the relocation
+* as a nop (don't do anything with it).
+*/
+ if (!ret) {
+ reloc->type = rel_type_nop;
+ rels->changed = true;
+ }
+ }
+ return 0;
+}
+
/* 32 bit and 64 bit are very similar */
#include "recordmcount.h"
#define RECORD_MCOUNT_64
diff --git a/tools/objtool/recordmcount.h b/tools/objtool/recordmcount.h
index 6754bde0bacc..e033b600bd61 100644
--- a/tools/objtool/recordmcount.h
+++ b/tools/objtool/recordmcount.h
@@ -20,7 +20,6 @@
#undef append_func
#undef mcount_adjust
#undef sift_rel_mcount
-#undef nop_mcount
#undef has_rel_mcount
#undef tot_relsize
#undef do_func
@@ -37,7 +36,6 @@
#ifdef RECORD_MCOUNT_64
# define append_func append64
# define sift_rel_mcount sift64_rel_mcount
-# define nop_mcountnop_mcount_64
# define has_rel_mcounthas64_rel_mcount
# define tot_relsize tot64_relsize
# define do_func do64
@@ -53,7 +51,6 @@
#else
# define append_func append32
# define sift_rel_mcount sift32_rel_mcount
-# define nop_mcountnop_mcount_32
# define has_rel_mcounthas32_rel_mcount
# define tot_relsize tot32_relsize
# define do_func do32
@@ -171,53 +168,6 @@ static uint_t *sift_rel_mcount(uint_t *mlocp,
return mlocp;
}
-/*
- * Read the relocation table again, but this time its called on sections
- * that are not going to be traced. The mcount calls here will be converted
- * into nops.
- */
-static int nop_mcount(struct section * const rels,
- const char *const txtname)
-{
- struct reloc *reloc;
- struct section *txts = find_section_by_index(lf, rels->sh.sh_info);
- unsigned mcountsym = 0;
- int once = 0;
-
- list_for_each_entry(reloc, &rels->reloc_list, list) {
- int ret = -1;
-
- if (!mcountsym)
- mcountsym = get_mcountsym(reloc);
-
- if (mcountsym == GELF_R_INFO(reloc->sym->idx, reloc->type) &&
!is_fake_mcount(reloc)) {
- if (make_nop) {
- ret = make_nop(txts, reloc->offset);
- if (ret < 0)
- return -1;
- }
- if (warn_on_notrace_sect && !once) {
- printf("Section %s has mcount callers being
ignor
