recordmcount presents unnecessary challenges to reviewers:
It pretends to wrap access to the ELF file in
uread/uwrite/ulseek functions which aren't related
the way you might think (i.e. not the way read, write,
and lseek are releated to each other).
It uses setjmp/longjmp to handle errors (and success)
during processing of the object files. This makes it
hard to review what functions are doing, how globals
change over time, etc.
There are some kernel style nits.
This series addresses all of those by removing un-helper functions,
unused parameters, and rewriting the error/success handling to
better resemble regular kernel C code.
--
This series was formerly part of a v3 posted under the subject
"Cleanup recordmcount and begin objtool conversion". I am
reposting it split into two series: these cleanups of recordmcount
and a second series that begins the conversion of the cleaned-up
recordmcount into an objtool subcommand called mcount.
v4:
Addressed feedback on v3 from Steven Rostedt:
Moved already_has_mcount into recordmcount.c to avoid
unnecessary multiple definitions.
Changed return semantics of find_secsym_ndx() to avoid
need for missing_sym (and multiple definitions)
and to separate the returned symbol info
(value, index) from success/failure indication.
Fixed up local variable declaration to follow inverted
christmas tree style.
Matt Helsley (8):
recordmcount: Remove redundant strcmp
recordmcount: Remove uread()
recordmcount: Remove unused fd from uwrite() and ulseek()
recordmcount: Rewrite error/success handling
recordmcount: Kernel style function signature formatting
recordmcount: Kernel style formatting
recordmcount: Remove redundant cleanup() calls
recordmcount: Clarify what cleanup() does
scripts/recordmcount.c | 321 ++++++++++++++++++++---------------------
scripts/recordmcount.h | 150 +++++++++++++------
2 files changed, 259 insertions(+), 212 deletions(-)
--
2.20.1
Fix up the whitespace irregularity in the ELF switch
blocks.
Swapping the initial value of gpfx allows us to
simplify all but one of the one-line switch cases even
further.
Signed-off-by: Matt Helsley <[email protected]>
---
scripts/recordmcount.c | 47 ++++++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 22 deletions(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 67f9c45b824f..273ca8b42b20 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -489,15 +489,15 @@ static int do_file(char const *const fname)
push_bl_mcount_thumb = push_bl_mcount_thumb_be;
break;
} /* end switch */
- if (memcmp(ELFMAG, ehdr->e_ident, SELFMAG) != 0
- || w2(ehdr->e_type) != ET_REL
- || ehdr->e_ident[EI_VERSION] != EV_CURRENT) {
+ if (memcmp(ELFMAG, ehdr->e_ident, SELFMAG) != 0 ||
+ w2(ehdr->e_type) != ET_REL ||
+ ehdr->e_ident[EI_VERSION] != EV_CURRENT) {
fprintf(stderr, "unrecognized ET_REL file %s\n", fname);
cleanup();
goto out;
}
- gpfx = 0;
+ gpfx = '_';
switch (w2(ehdr->e_machine)) {
default:
fprintf(stderr, "unrecognized e_machine %u %s\n",
@@ -510,32 +510,35 @@ static int do_file(char const *const fname)
make_nop = make_nop_x86;
ideal_nop = ideal_nop5_x86_32;
mcount_adjust_32 = -1;
+ gpfx = 0;
+ break;
+ case EM_ARM:
+ reltype = R_ARM_ABS32;
+ altmcount = "__gnu_mcount_nc";
+ make_nop = make_nop_arm;
+ rel_type_nop = R_ARM_NONE;
+ gpfx = 0;
break;
- case EM_ARM: reltype = R_ARM_ABS32;
- altmcount = "__gnu_mcount_nc";
- make_nop = make_nop_arm;
- rel_type_nop = R_ARM_NONE;
- break;
case EM_AARCH64:
- reltype = R_AARCH64_ABS64;
- make_nop = make_nop_arm64;
- rel_type_nop = R_AARCH64_NONE;
- ideal_nop = ideal_nop4_arm64;
- gpfx = '_';
- break;
- case EM_IA_64: reltype = R_IA64_IMM64; gpfx = '_'; break;
- case EM_MIPS: /* reltype: e_class */ gpfx = '_'; break;
- case EM_PPC: reltype = R_PPC_ADDR32; gpfx = '_'; break;
- case EM_PPC64: reltype = R_PPC64_ADDR64; gpfx = '_'; break;
- case EM_S390: /* reltype: e_class */ gpfx = '_'; break;
- case EM_SH: reltype = R_SH_DIR32; break;
- case EM_SPARCV9: reltype = R_SPARC_64; gpfx = '_'; break;
+ reltype = R_AARCH64_ABS64;
+ make_nop = make_nop_arm64;
+ rel_type_nop = R_AARCH64_NONE;
+ ideal_nop = ideal_nop4_arm64;
+ break;
+ case EM_IA_64: reltype = R_IA64_IMM64; break;
+ case EM_MIPS: /* reltype: e_class */ break;
+ case EM_PPC: reltype = R_PPC_ADDR32; break;
+ case EM_PPC64: reltype = R_PPC64_ADDR64; break;
+ case EM_S390: /* reltype: e_class */ break;
+ case EM_SH: reltype = R_SH_DIR32; gpfx = 0; break;
+ case EM_SPARCV9: reltype = R_SPARC_64; break;
case EM_X86_64:
make_nop = make_nop_x86;
ideal_nop = ideal_nop5_x86_64;
reltype = R_X86_64_64;
rel_type_nop = R_X86_64_NONE;
mcount_adjust_64 = -1;
+ gpfx = 0;
break;
} /* end switch */
--
2.20.1
The uwrite() and ulseek() functions are formatted inconsistently
with the rest of the file and the kernel overall. While we're
making other changes here let's fix this.
Signed-off-by: Matt Helsley <[email protected]>
---
scripts/recordmcount.c | 21 +++++++--------------
scripts/recordmcount.h | 13 ++++++-------
2 files changed, 13 insertions(+), 21 deletions(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index c6d395b8ff29..67f9c45b824f 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -52,8 +52,7 @@ static void *file_append; /* added to the end of the file */
static size_t file_append_size; /* how much is added to end of file */
/* Per-file resource cleanup when multiple files. */
-static void
-cleanup(void)
+static void cleanup(void)
{
if (!mmap_failed)
munmap(file_map, sb.st_size);
@@ -68,8 +67,7 @@ cleanup(void)
/* ulseek, uwrite, ...: Check return value for errors. */
-static off_t
-ulseek(off_t const offset, int const whence)
+static off_t ulseek(off_t const offset, int const whence)
{
switch (whence) {
case SEEK_SET:
@@ -89,8 +87,7 @@ ulseek(off_t const offset, int const whence)
return file_ptr - file_map;
}
-static ssize_t
-uwrite(void const *const buf, size_t const count)
+static ssize_t uwrite(void const *const buf, size_t const count)
{
size_t cnt = count;
off_t idx = 0;
@@ -127,8 +124,7 @@ uwrite(void const *const buf, size_t const count)
return count;
}
-static void *
-umalloc(size_t size)
+static void * umalloc(size_t size)
{
void *const addr = malloc(size);
if (addr == 0) {
@@ -394,8 +390,7 @@ static uint32_t (*w)(uint32_t);
static uint32_t (*w2)(uint16_t);
/* Names of the sections that could contain calls to mcount. */
-static int
-is_mcounted_section_name(char const *const txtname)
+static int is_mcounted_section_name(char const *const txtname)
{
return strncmp(".text", txtname, 5) == 0 ||
strcmp(".init.text", txtname) == 0 ||
@@ -448,8 +443,7 @@ static void MIPS64_r_info(Elf64_Rel *const rp, unsigned sym, unsigned type)
}).r_info;
}
-static int
-do_file(char const *const fname)
+static int do_file(char const *const fname)
{
Elf32_Ehdr *const ehdr = mmap_file(fname);
unsigned int reltype = 0;
@@ -597,8 +591,7 @@ do_file(char const *const fname)
return rc;
}
-int
-main(int argc, char *argv[])
+int main(int argc, char *argv[])
{
const char ftrace[] = "/ftrace.o";
int ftrace_size = sizeof(ftrace) - 1;
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index 3796eb37fb12..ca9aaac89bfb 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -468,11 +468,10 @@ static int find_secsym_ndx(unsigned const txtndx,
}
/* Evade ISO C restriction: no declaration after statement in has_rel_mcount. */
-static char const *
-__has_rel_mcount(Elf_Shdr const *const relhdr, /* is SHT_REL or SHT_RELA */
- Elf_Shdr const *const shdr0,
- char const *const shstrtab,
- char const *const fname)
+static char const * __has_rel_mcount(Elf_Shdr const *const relhdr, /* reltype */
+ Elf_Shdr const *const shdr0,
+ char const *const shstrtab,
+ char const *const fname)
{
/* .sh_info depends on .sh_type == SHT_REL[,A] */
Elf_Shdr const *const txthdr = &shdr0[w(relhdr->sh_info)];
@@ -524,8 +523,8 @@ static unsigned tot_relsize(Elf_Shdr const *const shdr0,
/* Overall supervision for Elf32 ET_REL file. */
-static int
-do_func(Elf_Ehdr *const ehdr, char const *const fname, unsigned const reltype)
+static int do_func(Elf_Ehdr *const ehdr, char const *const fname,
+ unsigned const reltype)
{
Elf_Shdr *const shdr0 = (Elf_Shdr *)(_w(ehdr->e_shoff)
+ (void *)ehdr);
--
2.20.1
The strcmp is unnecessary since .text is already accepted as a
prefix in the strncmp().
Signed-off-by: Matt Helsley <[email protected]>
---
scripts/recordmcount.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 8387a9bc064a..ebe98c39f3cd 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -405,8 +405,7 @@ is_mcounted_section_name(char const *const txtname)
strcmp(".irqentry.text", txtname) == 0 ||
strcmp(".softirqentry.text", txtname) == 0 ||
strcmp(".kprobes.text", txtname) == 0 ||
- strcmp(".cpuidle.text", txtname) == 0 ||
- strcmp(".text.unlikely", txtname) == 0;
+ strcmp(".cpuidle.text", txtname) == 0;
}
/* 32 bit and 64 bit are very similar */
--
2.20.1
uread() is only used to initialize the ELF file's pseudo
private-memory mapping while uwrite() and ulseek() work within
the pseudo-mapping and extend it as necessary. Thus it is not
a complementary function to uwrite() and ulseek(). It also makes
no sense to do cleanups inside uread() when its only caller,
mmap_file(), is doing the relevant allocations and associated
initializations.
Therefore it's clearer to use a plain read() call to initialize the
data in mmap_file() and remove uread().
Signed-off-by: Matt Helsley <[email protected]>
---
scripts/recordmcount.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index ebe98c39f3cd..c0dd46344063 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -89,7 +89,7 @@ succeed_file(void)
longjmp(jmpenv, SJ_SUCCEED);
}
-/* ulseek, uread, ...: Check return value for errors. */
+/* ulseek, uwrite, ...: Check return value for errors. */
static off_t
ulseek(int const fd, off_t const offset, int const whence)
@@ -112,17 +112,6 @@ ulseek(int const fd, off_t const offset, int const whence)
return file_ptr - file_map;
}
-static size_t
-uread(int const fd, void *const buf, size_t const count)
-{
- size_t const n = read(fd, buf, count);
- if (n != count) {
- perror("read");
- fail_file();
- }
- return n;
-}
-
static size_t
uwrite(int const fd, void const *const buf, size_t const count)
{
@@ -298,7 +287,10 @@ static void *mmap_file(char const *fname)
if (file_map == MAP_FAILED) {
mmap_failed = 1;
file_map = umalloc(sb.st_size);
- uread(fd_map, file_map, sb.st_size);
+ if (read(fd_map, file_map, sb.st_size) != sb.st_size) {
+ perror(fname);
+ fail_file();
+ }
}
close(fd_map);
--
2.20.1
uwrite() works within the pseudo-mapping and extends it as necessary
without needing the file descriptor (fd) parameter passed to it.
Similarly, ulseek() doesn't need its fd parameter. These parameters
were only added because the functions bear a conceptual resemblance
to write() and lseek(). Worse, they obscure the fact that at the time
uwrite() and ulseek() are called fd_map is not a valid file descriptor.
Remove the unused file descriptor parameters that make it look like
fd_map is still valid.
Signed-off-by: Matt Helsley <[email protected]>
---
scripts/recordmcount.c | 16 ++++++++--------
scripts/recordmcount.h | 26 +++++++++++++-------------
2 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index c0dd46344063..1fe5fba99959 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -92,7 +92,7 @@ succeed_file(void)
/* ulseek, uwrite, ...: Check return value for errors. */
static off_t
-ulseek(int const fd, off_t const offset, int const whence)
+ulseek(off_t const offset, int const whence)
{
switch (whence) {
case SEEK_SET:
@@ -113,7 +113,7 @@ ulseek(int const fd, off_t const offset, int const whence)
}
static size_t
-uwrite(int const fd, void const *const buf, size_t const count)
+uwrite(void const *const buf, size_t const count)
{
size_t cnt = count;
off_t idx = 0;
@@ -183,8 +183,8 @@ static int make_nop_x86(void *map, size_t const offset)
return -1;
/* convert to nop */
- ulseek(fd_map, offset - 1, SEEK_SET);
- uwrite(fd_map, ideal_nop, 5);
+ ulseek(offset - 1, SEEK_SET);
+ uwrite(ideal_nop, 5);
return 0;
}
@@ -232,10 +232,10 @@ static int make_nop_arm(void *map, size_t const offset)
return -1;
/* Convert to nop */
- ulseek(fd_map, off, SEEK_SET);
+ ulseek(off, SEEK_SET);
do {
- uwrite(fd_map, ideal_nop, nop_size);
+ uwrite(ideal_nop, nop_size);
} while (--cnt > 0);
return 0;
@@ -252,8 +252,8 @@ static int make_nop_arm64(void *map, size_t const offset)
return -1;
/* Convert to nop */
- ulseek(fd_map, offset, SEEK_SET);
- uwrite(fd_map, ideal_nop, 4);
+ ulseek(offset, SEEK_SET);
+ uwrite(ideal_nop, 4);
return 0;
}
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index 47fca2c69a73..c1e1b04b4871 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -202,14 +202,14 @@ static void append_func(Elf_Ehdr *const ehdr,
new_e_shoff = t;
/* body for new shstrtab */
- ulseek(fd_map, sb.st_size, SEEK_SET);
- uwrite(fd_map, old_shstr_sh_offset + (void *)ehdr, old_shstr_sh_size);
- uwrite(fd_map, mc_name, 1 + strlen(mc_name));
+ ulseek(sb.st_size, SEEK_SET);
+ uwrite(old_shstr_sh_offset + (void *)ehdr, old_shstr_sh_size);
+ uwrite(mc_name, 1 + strlen(mc_name));
/* old(modified) Elf_Shdr table, word-byte aligned */
- ulseek(fd_map, t, SEEK_SET);
+ ulseek(t, SEEK_SET);
t += sizeof(Elf_Shdr) * old_shnum;
- uwrite(fd_map, old_shoff + (void *)ehdr,
+ uwrite(old_shoff + (void *)ehdr,
sizeof(Elf_Shdr) * old_shnum);
/* new sections __mcount_loc and .rel__mcount_loc */
@@ -225,7 +225,7 @@ static void append_func(Elf_Ehdr *const ehdr,
mcsec.sh_info = 0;
mcsec.sh_addralign = _w(_size);
mcsec.sh_entsize = _w(_size);
- uwrite(fd_map, &mcsec, sizeof(mcsec));
+ uwrite(&mcsec, sizeof(mcsec));
mcsec.sh_name = w(old_shstr_sh_size);
mcsec.sh_type = (sizeof(Elf_Rela) == rel_entsize)
@@ -239,15 +239,15 @@ static void append_func(Elf_Ehdr *const ehdr,
mcsec.sh_info = w(old_shnum);
mcsec.sh_addralign = _w(_size);
mcsec.sh_entsize = _w(rel_entsize);
- uwrite(fd_map, &mcsec, sizeof(mcsec));
+ uwrite(&mcsec, sizeof(mcsec));
- uwrite(fd_map, mloc0, (void *)mlocp - (void *)mloc0);
- uwrite(fd_map, mrel0, (void *)mrelp - (void *)mrel0);
+ uwrite(mloc0, (void *)mlocp - (void *)mloc0);
+ uwrite(mrel0, (void *)mrelp - (void *)mrel0);
ehdr->e_shoff = _w(new_e_shoff);
ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum)); /* {.rel,}__mcount_loc */
- ulseek(fd_map, 0, SEEK_SET);
- uwrite(fd_map, ehdr, sizeof(*ehdr));
+ ulseek(0, SEEK_SET);
+ uwrite(ehdr, sizeof(*ehdr));
}
static unsigned get_mcountsym(Elf_Sym const *const sym0,
@@ -396,8 +396,8 @@ static void nop_mcount(Elf_Shdr const *const relhdr,
Elf_Rel rel;
rel = *(Elf_Rel *)relp;
Elf_r_info(&rel, Elf_r_sym(relp), rel_type_nop);
- ulseek(fd_map, (void *)relp - (void *)ehdr, SEEK_SET);
- uwrite(fd_map, &rel, sizeof(rel));
+ ulseek((void *)relp - (void *)ehdr, SEEK_SET);
+ uwrite(&rel, sizeof(rel));
}
relp = (Elf_Rel const *)(rel_entsize + (void *)relp);
}
--
2.20.1
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.
Signed-off-by: Matt Helsley <[email protected]>
--
v3.5 -- Moved already_has_mcount into recordmcount.c to avoid
unnecessary multiple definitions
Changed return semantics of find_secsym_ndx() to avoid
need for missing_sym (and multiple definitions)
and to separate the returned symbol info
(value, index) from success/failure indication
---
scripts/recordmcount.c | 162 +++++++++++++++++++++--------------------
scripts/recordmcount.h | 141 ++++++++++++++++++++++++-----------
2 files changed, 184 insertions(+), 119 deletions(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 1fe5fba99959..c6d395b8ff29 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -27,7 +27,6 @@
#include <getopt.h>
#include <elf.h>
#include <fcntl.h>
-#include <setjmp.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -43,7 +42,6 @@ static int fd_map; /* File descriptor for file being modified. */
static int mmap_failed; /* Boolean flag. */
static char gpfx; /* prefix for global symbol name (sometimes '_') */
static struct stat sb; /* Remember .st_size, etc. */
-static jmp_buf jmpenv; /* setjmp/longjmp per-file error escape */
static const char *altmcount; /* alternate mcount symbol name */
static int warn_on_notrace_sect; /* warn when section has mcount not being recorded */
static void *file_map; /* pointer of the mapped file */
@@ -53,13 +51,6 @@ static void *file_ptr; /* current file pointer location */
static void *file_append; /* added to the end of the file */
static size_t file_append_size; /* how much is added to end of file */
-/* setjmp() return values */
-enum {
- SJ_SETJMP = 0, /* hardwired first return */
- SJ_FAIL,
- SJ_SUCCEED
-};
-
/* Per-file resource cleanup when multiple files. */
static void
cleanup(void)
@@ -75,20 +66,6 @@ cleanup(void)
file_updated = 0;
}
-static void __attribute__((noreturn))
-fail_file(void)
-{
- cleanup();
- longjmp(jmpenv, SJ_FAIL);
-}
-
-static void __attribute__((noreturn))
-succeed_file(void)
-{
- cleanup();
- longjmp(jmpenv, SJ_SUCCEED);
-}
-
/* ulseek, uwrite, ...: Check return value for errors. */
static off_t
@@ -107,12 +84,12 @@ ulseek(off_t const offset, int const whence)
}
if (file_ptr < file_map) {
fprintf(stderr, "lseek: seek before file\n");
- fail_file();
+ return -1;
}
return file_ptr - file_map;
}
-static size_t
+static ssize_t
uwrite(void const *const buf, size_t const count)
{
size_t cnt = count;
@@ -129,7 +106,8 @@ uwrite(void const *const buf, size_t const count)
}
if (!file_append) {
perror("write");
- fail_file();
+ cleanup();
+ return -1;
}
if (file_ptr < file_end) {
cnt = file_end - file_ptr;
@@ -155,7 +133,8 @@ umalloc(size_t size)
void *const addr = malloc(size);
if (addr == 0) {
fprintf(stderr, "malloc failed: %zu bytes\n", size);
- fail_file();
+ cleanup();
+ return NULL;
}
return addr;
}
@@ -183,8 +162,10 @@ static int make_nop_x86(void *map, size_t const offset)
return -1;
/* convert to nop */
- ulseek(offset - 1, SEEK_SET);
- uwrite(ideal_nop, 5);
+ if (ulseek(offset - 1, SEEK_SET) < 0)
+ return -1;
+ if (uwrite(ideal_nop, 5) < 0)
+ return -1;
return 0;
}
@@ -232,10 +213,12 @@ static int make_nop_arm(void *map, size_t const offset)
return -1;
/* Convert to nop */
- ulseek(off, SEEK_SET);
+ if (ulseek(off, SEEK_SET) < 0)
+ return -1;
do {
- uwrite(ideal_nop, nop_size);
+ if (uwrite(ideal_nop, nop_size) < 0)
+ return -1;
} while (--cnt > 0);
return 0;
@@ -252,8 +235,10 @@ static int make_nop_arm64(void *map, size_t const offset)
return -1;
/* Convert to nop */
- ulseek(offset, SEEK_SET);
- uwrite(ideal_nop, 4);
+ if (ulseek(offset, SEEK_SET) < 0)
+ return -1;
+ if (uwrite(ideal_nop, 4) < 0)
+ return -1;
return 0;
}
@@ -272,14 +257,23 @@ static int make_nop_arm64(void *map, size_t const offset)
*/
static void *mmap_file(char const *fname)
{
+ file_map = NULL;
+ sb.st_size = 0;
fd_map = open(fname, O_RDONLY);
- if (fd_map < 0 || fstat(fd_map, &sb) < 0) {
+ if (fd_map < 0) {
+ perror(fname);
+ cleanup();
+ return NULL;
+ }
+ if (fstat(fd_map, &sb) < 0) {
perror(fname);
- fail_file();
+ cleanup();
+ goto out;
}
if (!S_ISREG(sb.st_mode)) {
fprintf(stderr, "not a regular file: %s\n", fname);
- fail_file();
+ cleanup();
+ goto out;
}
file_map = mmap(0, sb.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE,
fd_map, 0);
@@ -287,11 +281,18 @@ static void *mmap_file(char const *fname)
if (file_map == MAP_FAILED) {
mmap_failed = 1;
file_map = umalloc(sb.st_size);
+ if (!file_map) {
+ perror(fname);
+ goto out;
+ }
if (read(fd_map, file_map, sb.st_size) != sb.st_size) {
perror(fname);
- fail_file();
+ free(file_map);
+ file_map = NULL;
+ goto out;
}
}
+out:
close(fd_map);
file_end = file_map + sb.st_size;
@@ -299,13 +300,13 @@ static void *mmap_file(char const *fname)
return file_map;
}
-static void write_file(const char *fname)
+static int write_file(const char *fname)
{
char tmp_file[strlen(fname) + 4];
size_t n;
if (!file_updated)
- return;
+ return 0;
sprintf(tmp_file, "%s.rc", fname);
@@ -317,25 +318,32 @@ static void write_file(const char *fname)
fd_map = open(tmp_file, O_WRONLY | O_TRUNC | O_CREAT, sb.st_mode);
if (fd_map < 0) {
perror(fname);
- fail_file();
+ cleanup();
+ return -1;
}
n = write(fd_map, file_map, sb.st_size);
if (n != sb.st_size) {
perror("write");
- fail_file();
+ cleanup();
+ close(fd_map);
+ return -1;
}
if (file_append_size) {
n = write(fd_map, file_append, file_append_size);
if (n != file_append_size) {
perror("write");
- fail_file();
+ cleanup();
+ close(fd_map);
+ return -1;
}
}
close(fd_map);
if (rename(tmp_file, fname) < 0) {
perror(fname);
- fail_file();
+ cleanup();
+ return -1;
}
+ return 0;
}
/* w8rev, w8nat, ...: Handle endianness. */
@@ -400,6 +408,8 @@ is_mcounted_section_name(char const *const txtname)
strcmp(".cpuidle.text", txtname) == 0;
}
+static char const *already_has_rel_mcount = "success"; /* our work here is done! */
+
/* 32 bit and 64 bit are very similar */
#include "recordmcount.h"
#define RECORD_MCOUNT_64
@@ -438,11 +448,15 @@ static void MIPS64_r_info(Elf64_Rel *const rp, unsigned sym, unsigned type)
}).r_info;
}
-static void
+static int
do_file(char const *const fname)
{
Elf32_Ehdr *const ehdr = mmap_file(fname);
unsigned int reltype = 0;
+ int rc = -1;
+
+ if (!ehdr)
+ goto out;
w = w4nat;
w2 = w2nat;
@@ -452,8 +466,8 @@ do_file(char const *const fname)
default:
fprintf(stderr, "unrecognized ELF data encoding %d: %s\n",
ehdr->e_ident[EI_DATA], fname);
- fail_file();
- break;
+ cleanup();
+ goto out;
case ELFDATA2LSB:
if (*(unsigned char const *)&endian != 1) {
/* main() is big endian, file.o is little endian. */
@@ -485,7 +499,8 @@ do_file(char const *const fname)
|| w2(ehdr->e_type) != ET_REL
|| ehdr->e_ident[EI_VERSION] != EV_CURRENT) {
fprintf(stderr, "unrecognized ET_REL file %s\n", fname);
- fail_file();
+ cleanup();
+ goto out;
}
gpfx = 0;
@@ -493,8 +508,8 @@ do_file(char const *const fname)
default:
fprintf(stderr, "unrecognized e_machine %u %s\n",
w2(ehdr->e_machine), fname);
- fail_file();
- break;
+ cleanup();
+ goto out;
case EM_386:
reltype = R_386_32;
rel_type_nop = R_386_NONE;
@@ -534,20 +549,22 @@ do_file(char const *const fname)
default:
fprintf(stderr, "unrecognized ELF class %d %s\n",
ehdr->e_ident[EI_CLASS], fname);
- fail_file();
- break;
+ cleanup();
+ goto out;
case ELFCLASS32:
if (w2(ehdr->e_ehsize) != sizeof(Elf32_Ehdr)
|| w2(ehdr->e_shentsize) != sizeof(Elf32_Shdr)) {
fprintf(stderr,
"unrecognized ET_REL file: %s\n", fname);
- fail_file();
+ cleanup();
+ goto out;
}
if (w2(ehdr->e_machine) == EM_MIPS) {
reltype = R_MIPS_32;
is_fake_mcount32 = MIPS32_is_fake_mcount;
}
- do32(ehdr, fname, reltype);
+ if (do32(ehdr, fname, reltype) < 0)
+ goto out;
break;
case ELFCLASS64: {
Elf64_Ehdr *const ghdr = (Elf64_Ehdr *)ehdr;
@@ -555,7 +572,8 @@ do_file(char const *const fname)
|| w2(ghdr->e_shentsize) != sizeof(Elf64_Shdr)) {
fprintf(stderr,
"unrecognized ET_REL file: %s\n", fname);
- fail_file();
+ cleanup();
+ goto out;
}
if (w2(ghdr->e_machine) == EM_S390) {
reltype = R_390_64;
@@ -567,13 +585,16 @@ do_file(char const *const fname)
Elf64_r_info = MIPS64_r_info;
is_fake_mcount64 = MIPS64_is_fake_mcount;
}
- do64(ghdr, fname, reltype);
+ if (do64(ghdr, fname, reltype) < 0)
+ goto out;
break;
}
} /* end switch */
- write_file(fname);
+ rc = write_file(fname);
+out:
cleanup();
+ return rc;
}
int
@@ -604,7 +625,6 @@ main(int argc, char *argv[])
/* Process each file in turn, allowing deep failure. */
for (i = optind; i < argc; i++) {
char *file = argv[i];
- int const sjval = setjmp(jmpenv);
int len;
/*
@@ -617,28 +637,16 @@ main(int argc, char *argv[])
strcmp(file + (len - ftrace_size), ftrace) == 0)
continue;
- switch (sjval) {
- default:
- fprintf(stderr, "internal error: %s\n", file);
- exit(1);
- break;
- case SJ_SETJMP: /* normal sequence */
- /* Avoid problems if early cleanup() */
- fd_map = -1;
- mmap_failed = 1;
- file_map = NULL;
- file_ptr = NULL;
- file_updated = 0;
- do_file(file);
- break;
- case SJ_FAIL: /* error in do_file or below */
+ /* Avoid problems if early cleanup() */
+ fd_map = -1;
+ mmap_failed = 1;
+ file_map = NULL;
+ file_ptr = NULL;
+ file_updated = 0;
+ if (do_file(file)) {
fprintf(stderr, "%s: failed\n", file);
++n_error;
- break;
- case SJ_SUCCEED: /* premature success */
- /* do nothing */
- break;
- } /* end switch */
+ }
}
return !!n_error;
}
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index c1e1b04b4871..3796eb37fb12 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -174,7 +174,7 @@ static int MIPS_is_fake_mcount(Elf_Rel const *rp)
}
/* Append the new shstrtab, Elf_Shdr[], __mcount_loc and its relocations. */
-static void append_func(Elf_Ehdr *const ehdr,
+static int append_func(Elf_Ehdr *const ehdr,
Elf_Shdr *const shstr,
uint_t const *const mloc0,
uint_t const *const mlocp,
@@ -202,15 +202,20 @@ static void append_func(Elf_Ehdr *const ehdr,
new_e_shoff = t;
/* body for new shstrtab */
- ulseek(sb.st_size, SEEK_SET);
- uwrite(old_shstr_sh_offset + (void *)ehdr, old_shstr_sh_size);
- uwrite(mc_name, 1 + strlen(mc_name));
+ if (ulseek(sb.st_size, SEEK_SET) < 0)
+ return -1;
+ if (uwrite(old_shstr_sh_offset + (void *)ehdr, old_shstr_sh_size) < 0)
+ return -1;
+ if (uwrite(mc_name, 1 + strlen(mc_name)) < 0)
+ return -1;
/* old(modified) Elf_Shdr table, word-byte aligned */
- ulseek(t, SEEK_SET);
+ if (ulseek(t, SEEK_SET) < 0)
+ return -1;
t += sizeof(Elf_Shdr) * old_shnum;
- uwrite(old_shoff + (void *)ehdr,
- sizeof(Elf_Shdr) * old_shnum);
+ if (uwrite(old_shoff + (void *)ehdr,
+ sizeof(Elf_Shdr) * old_shnum) < 0)
+ return -1;
/* new sections __mcount_loc and .rel__mcount_loc */
t += 2*sizeof(mcsec);
@@ -225,7 +230,8 @@ static void append_func(Elf_Ehdr *const ehdr,
mcsec.sh_info = 0;
mcsec.sh_addralign = _w(_size);
mcsec.sh_entsize = _w(_size);
- uwrite(&mcsec, sizeof(mcsec));
+ if (uwrite(&mcsec, sizeof(mcsec)) < 0)
+ return -1;
mcsec.sh_name = w(old_shstr_sh_size);
mcsec.sh_type = (sizeof(Elf_Rela) == rel_entsize)
@@ -239,15 +245,22 @@ static void append_func(Elf_Ehdr *const ehdr,
mcsec.sh_info = w(old_shnum);
mcsec.sh_addralign = _w(_size);
mcsec.sh_entsize = _w(rel_entsize);
- uwrite(&mcsec, sizeof(mcsec));
- uwrite(mloc0, (void *)mlocp - (void *)mloc0);
- uwrite(mrel0, (void *)mrelp - (void *)mrel0);
+ if (uwrite(&mcsec, sizeof(mcsec)) < 0)
+ return -1;
+
+ if (uwrite(mloc0, (void *)mlocp - (void *)mloc0) < 0)
+ return -1;
+ if (uwrite(mrel0, (void *)mrelp - (void *)mrel0) < 0)
+ return -1;
ehdr->e_shoff = _w(new_e_shoff);
ehdr->e_shnum = w2(2 + w2(ehdr->e_shnum)); /* {.rel,}__mcount_loc */
- ulseek(0, SEEK_SET);
- uwrite(ehdr, sizeof(*ehdr));
+ if (ulseek(0, SEEK_SET) < 0)
+ return -1;
+ if (uwrite(ehdr, sizeof(*ehdr)) < 0)
+ return -1;
+ return 0;
}
static unsigned get_mcountsym(Elf_Sym const *const sym0,
@@ -351,9 +364,9 @@ static uint_t *sift_rel_mcount(uint_t *mlocp,
* that are not going to be traced. The mcount calls here will be converted
* into nops.
*/
-static void nop_mcount(Elf_Shdr const *const relhdr,
- Elf_Ehdr const *const ehdr,
- const char *const txtname)
+static int nop_mcount(Elf_Shdr const *const relhdr,
+ Elf_Ehdr const *const ehdr,
+ const char *const txtname)
{
Elf_Shdr *const shdr0 = (Elf_Shdr *)(_w(ehdr->e_shoff)
+ (void *)ehdr);
@@ -376,15 +389,18 @@ static void nop_mcount(Elf_Shdr const *const relhdr,
mcountsym = get_mcountsym(sym0, relp, str0);
if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
- if (make_nop)
+ if (make_nop) {
ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_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;
+ return 0;
}
}
@@ -396,14 +412,16 @@ static void nop_mcount(Elf_Shdr const *const relhdr,
Elf_Rel rel;
rel = *(Elf_Rel *)relp;
Elf_r_info(&rel, Elf_r_sym(relp), rel_type_nop);
- ulseek((void *)relp - (void *)ehdr, SEEK_SET);
- uwrite(&rel, sizeof(rel));
+ if (ulseek((void *)relp - (void *)ehdr, SEEK_SET) < 0)
+ return -1;
+ if (uwrite(&rel, sizeof(rel)) < 0)
+ return -1;
}
relp = (Elf_Rel const *)(rel_entsize + (void *)relp);
}
+ return 0;
}
-
/*
* Find a symbol in the given section, to be used as the base for relocating
* the table of offsets of calls to mcount. A local or global symbol suffices,
@@ -414,9 +432,10 @@ static void nop_mcount(Elf_Shdr const *const relhdr,
* Num: Value Size Type Bind Vis Ndx Name
* 2: 00000000 0 SECTION LOCAL DEFAULT 1
*/
-static unsigned find_secsym_ndx(unsigned const txtndx,
+static int find_secsym_ndx(unsigned const txtndx,
char const *const txtname,
uint_t *const recvalp,
+ unsigned int *sym_index,
Elf_Shdr const *const symhdr,
Elf_Ehdr const *const ehdr)
{
@@ -438,15 +457,16 @@ static unsigned find_secsym_ndx(unsigned const txtndx,
continue;
*recvalp = _w(symp->st_value);
- return symp - sym0;
+ *sym_index = symp - sym0;
+ return 0;
}
}
fprintf(stderr, "Cannot find symbol for section %u: %s.\n",
txtndx, txtname);
- fail_file();
+ cleanup();
+ return -1;
}
-
/* Evade ISO C restriction: no declaration after statement in has_rel_mcount. */
static char const *
__has_rel_mcount(Elf_Shdr const *const relhdr, /* is SHT_REL or SHT_RELA */
@@ -461,7 +481,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))
@@ -491,6 +512,10 @@ static unsigned tot_relsize(Elf_Shdr const *const shdr0,
for (; nhdr; --nhdr, ++shdrp) {
txtname = has_rel_mcount(shdrp, shdr0, shstrtab, fname);
+ if (txtname == already_has_rel_mcount) {
+ totrelsz = 0;
+ break;
+ }
if (txtname && is_mcounted_section_name(txtname))
totrelsz += _w(shdrp->sh_size);
}
@@ -499,7 +524,7 @@ static unsigned tot_relsize(Elf_Shdr const *const shdr0,
/* Overall supervision for Elf32 ET_REL file. */
-static void
+static int
do_func(Elf_Ehdr *const ehdr, char const *const fname, unsigned const reltype)
{
Elf_Shdr *const shdr0 = (Elf_Shdr *)(_w(ehdr->e_shoff)
@@ -513,26 +538,54 @@ do_func(Elf_Ehdr *const ehdr, char const *const fname, unsigned const reltype)
unsigned k;
/* Upper bound on space: assume all relevant relocs are for mcount. */
- unsigned const totrelsz = tot_relsize(shdr0, nhdr, shstrtab, fname);
- Elf_Rel *const mrel0 = umalloc(totrelsz);
- Elf_Rel * mrelp = mrel0;
+ unsigned totrelsz;
- /* 2*sizeof(address) <= sizeof(Elf_Rel) */
- uint_t *const mloc0 = umalloc(totrelsz>>1);
- uint_t * mlocp = mloc0;
+ Elf_Rel * mrel0;
+ Elf_Rel * mrelp;
+
+ uint_t * mloc0;
+ uint_t * mlocp;
unsigned rel_entsize = 0;
unsigned symsec_sh_link = 0;
+ int result = 0;
+
+ totrelsz = tot_relsize(shdr0, nhdr, shstrtab, fname);
+ if (totrelsz == 0)
+ return 0;
+ mrel0 = umalloc(totrelsz);
+ mrelp = mrel0;
+ if (!mrel0)
+ return -1;
+
+ /* 2*sizeof(address) <= sizeof(Elf_Rel) */
+ mloc0 = umalloc(totrelsz>>1);
+ mlocp = mloc0;
+ if (!mloc0) {
+ free(mrel0);
+ return -1;
+ }
+
for (relhdr = shdr0, k = nhdr; k; --k, ++relhdr) {
char const *const txtname = has_rel_mcount(relhdr, shdr0,
shstrtab, fname);
+ if (txtname == already_has_rel_mcount) {
+ result = 0;
+ file_updated = 0;
+ goto out; /* Nothing to be done; don't append! */
+ }
if (txtname && is_mcounted_section_name(txtname)) {
+ unsigned int recsym;
uint_t recval = 0;
- unsigned const recsym = find_secsym_ndx(
- w(relhdr->sh_info), txtname, &recval,
- &shdr0[symsec_sh_link = w(relhdr->sh_link)],
- ehdr);
+
+ symsec_sh_link = w(relhdr->sh_link);
+ result = find_secsym_ndx(w(relhdr->sh_info), txtname,
+ &recval, &recsym,
+ &shdr0[symsec_sh_link],
+ ehdr);
+ if (result)
+ goto out;
rel_entsize = _w(relhdr->sh_entsize);
mlocp = sift_rel_mcount(mlocp,
@@ -543,13 +596,17 @@ do_func(Elf_Ehdr *const ehdr, char const *const fname, unsigned const reltype)
* This section is ignored by ftrace, but still
* has mcount calls. Convert them to nops now.
*/
- nop_mcount(relhdr, ehdr, txtname);
+ if (nop_mcount(relhdr, ehdr, txtname) < 0) {
+ result = -1;
+ goto out;
+ }
}
}
- if (mloc0 != mlocp) {
- append_func(ehdr, shstr, mloc0, mlocp, mrel0, mrelp,
- rel_entsize, symsec_sh_link);
- }
+ if (!result && mloc0 != mlocp)
+ result = append_func(ehdr, shstr, mloc0, mlocp, mrel0, mrelp,
+ rel_entsize, symsec_sh_link);
+out:
free(mrel0);
free(mloc0);
+ return result;
}
--
2.20.1
cleanup() mostly frees/unmaps the malloc'd/privately-mapped
copy of the ELF file recordmcount is working on, which is
set up in mmap_file(). It also deals with positioning within
the pseduo prive-mapping of the file and appending to the ELF
file.
Split into two steps:
mmap_cleanup() for the mapping itself
file_append_cleanup() for allocations storing the
appended ELF data.
Also, move the global variable initializations out of the main,
per-object-file loop and nearer to the alloc/init (mmap_file())
and two cleanup functions so we can more clearly see how they're
related.
Signed-off-by: Matt Helsley <[email protected]>
---
scripts/recordmcount.c | 151 ++++++++++++++++++++++-------------------
1 file changed, 81 insertions(+), 70 deletions(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 5677fcc88a72..612268eabef4 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -48,21 +48,26 @@ static void *file_map; /* pointer of the mapped file */
static void *file_end; /* pointer to the end of the mapped file */
static int file_updated; /* flag to state file was changed */
static void *file_ptr; /* current file pointer location */
+
static void *file_append; /* added to the end of the file */
static size_t file_append_size; /* how much is added to end of file */
/* Per-file resource cleanup when multiple files. */
-static void cleanup(void)
+static void file_append_cleanup(void)
+{
+ free(file_append);
+ file_append = NULL;
+ file_append_size = 0;
+ file_updated = 0;
+}
+
+static void mmap_cleanup(void)
{
if (!mmap_failed)
munmap(file_map, sb.st_size);
else
free(file_map);
file_map = NULL;
- free(file_append);
- file_append = NULL;
- file_append_size = 0;
- file_updated = 0;
}
/* ulseek, uwrite, ...: Check return value for errors. */
@@ -103,7 +108,8 @@ static ssize_t uwrite(void const *const buf, size_t const count)
}
if (!file_append) {
perror("write");
- cleanup();
+ file_append_cleanup();
+ mmap_cleanup();
return -1;
}
if (file_ptr < file_end) {
@@ -129,12 +135,76 @@ static void * umalloc(size_t size)
void *const addr = malloc(size);
if (addr == 0) {
fprintf(stderr, "malloc failed: %zu bytes\n", size);
- cleanup();
+ file_append_cleanup();
+ mmap_cleanup();
return NULL;
}
return addr;
}
+/*
+ * Get the whole file as a programming convenience in order to avoid
+ * malloc+lseek+read+free of many pieces. If successful, then mmap
+ * avoids copying unused pieces; else just read the whole file.
+ * Open for both read and write; new info will be appended to the file.
+ * Use MAP_PRIVATE so that a few changes to the in-memory ElfXX_Ehdr
+ * do not propagate to the file until an explicit overwrite at the last.
+ * This preserves most aspects of consistency (all except .st_size)
+ * for simultaneous readers of the file while we are appending to it.
+ * However, multiple writers still are bad. We choose not to use
+ * locking because it is expensive and the use case of kernel build
+ * makes multiple writers unlikely.
+ */
+static void *mmap_file(char const *fname)
+{
+ /* Avoid problems if early cleanup() */
+ fd_map = -1;
+ mmap_failed = 1;
+ file_map = NULL;
+ file_ptr = NULL;
+ file_updated = 0;
+ sb.st_size = 0;
+
+ fd_map = open(fname, O_RDONLY);
+ if (fd_map < 0) {
+ perror(fname);
+ return NULL;
+ }
+ if (fstat(fd_map, &sb) < 0) {
+ perror(fname);
+ goto out;
+ }
+ if (!S_ISREG(sb.st_mode)) {
+ fprintf(stderr, "not a regular file: %s\n", fname);
+ goto out;
+ }
+ file_map = mmap(0, sb.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE,
+ fd_map, 0);
+ if (file_map == MAP_FAILED) {
+ mmap_failed = 1;
+ file_map = umalloc(sb.st_size);
+ if (!file_map) {
+ perror(fname);
+ goto out;
+ }
+ if (read(fd_map, file_map, sb.st_size) != sb.st_size) {
+ perror(fname);
+ free(file_map);
+ file_map = NULL;
+ goto out;
+ }
+ } else
+ mmap_failed = 0;
+out:
+ close(fd_map);
+ fd_map = -1;
+
+ file_end = file_map + sb.st_size;
+
+ return file_map;
+}
+
+
static unsigned char ideal_nop5_x86_64[5] = { 0x0f, 0x1f, 0x44, 0x00, 0x00 };
static unsigned char ideal_nop5_x86_32[5] = { 0x3e, 0x8d, 0x74, 0x26, 0x00 };
static unsigned char *ideal_nop;
@@ -238,61 +308,6 @@ static int make_nop_arm64(void *map, size_t const offset)
return 0;
}
-/*
- * Get the whole file as a programming convenience in order to avoid
- * malloc+lseek+read+free of many pieces. If successful, then mmap
- * avoids copying unused pieces; else just read the whole file.
- * Open for both read and write; new info will be appended to the file.
- * Use MAP_PRIVATE so that a few changes to the in-memory ElfXX_Ehdr
- * do not propagate to the file until an explicit overwrite at the last.
- * This preserves most aspects of consistency (all except .st_size)
- * for simultaneous readers of the file while we are appending to it.
- * However, multiple writers still are bad. We choose not to use
- * locking because it is expensive and the use case of kernel build
- * makes multiple writers unlikely.
- */
-static void *mmap_file(char const *fname)
-{
- file_map = NULL;
- sb.st_size = 0;
- fd_map = open(fname, O_RDONLY);
- if (fd_map < 0) {
- perror(fname);
- return NULL;
- }
- if (fstat(fd_map, &sb) < 0) {
- perror(fname);
- goto out;
- }
- if (!S_ISREG(sb.st_mode)) {
- fprintf(stderr, "not a regular file: %s\n", fname);
- goto out;
- }
- file_map = mmap(0, sb.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE,
- fd_map, 0);
- mmap_failed = 0;
- if (file_map == MAP_FAILED) {
- mmap_failed = 1;
- file_map = umalloc(sb.st_size);
- if (!file_map) {
- perror(fname);
- goto out;
- }
- if (read(fd_map, file_map, sb.st_size) != sb.st_size) {
- perror(fname);
- free(file_map);
- file_map = NULL;
- goto out;
- }
- }
-out:
- close(fd_map);
-
- file_end = file_map + sb.st_size;
-
- return file_map;
-}
-
static int write_file(const char *fname)
{
char tmp_file[strlen(fname) + 4];
@@ -438,10 +453,11 @@ static void MIPS64_r_info(Elf64_Rel *const rp, unsigned sym, unsigned type)
static int do_file(char const *const fname)
{
- Elf32_Ehdr *const ehdr = mmap_file(fname);
unsigned int reltype = 0;
+ Elf32_Ehdr *ehdr;
int rc = -1;
+ ehdr = mmap_file(fname);
if (!ehdr)
goto out;
@@ -577,7 +593,8 @@ static int do_file(char const *const fname)
rc = write_file(fname);
out:
- cleanup();
+ file_append_cleanup();
+ mmap_cleanup();
return rc;
}
@@ -620,12 +637,6 @@ int main(int argc, char *argv[])
strcmp(file + (len - ftrace_size), ftrace) == 0)
continue;
- /* Avoid problems if early cleanup() */
- fd_map = -1;
- mmap_failed = 1;
- file_map = NULL;
- file_ptr = NULL;
- file_updated = 0;
if (do_file(file)) {
fprintf(stderr, "%s: failed\n", file);
++n_error;
--
2.20.1
Redundant cleanup calls were introduced when transitioning from
the old error/success handling via setjmp/longjmp -- the longjmp
ensured the cleanup() call only happened once but replacing
the success_file()/fail_file() calls with cleanup() meant that
multiple cleanup() calls can happen as we return from function
calls.
In do_file(), looking just before and after the "goto out" jumps we
can see that multiple cleanups() are being performed. We remove
cleanup() calls from the nested functions because it makes the code
easier to review -- the resources being cleaned up are generally
allocated and initialized in the callers so freeing them there
makes more sense.
Other redundant cleanup() calls:
mmap_file() is only called from do_file() and, if mmap_file() fails,
then we goto out and do cleanup() there too.
write_file() is only called from do_file() and do_file()
calls cleanup() unconditionally after returning from write_file()
therefore the cleanup() calls in write_file() are not necessary.
find_secsym_ndx(), called from do_func()'s for-loop, when we are
cleaning up here it's obvious that we break out of the loop and
do another cleanup().
__has_rel_mcount() is called from two parts of do_func()
and calls cleanup(). In theory we move them into do_func(), however
these in turn prove redundant so another simplification step
removes them as well.
Signed-off-by: Matt Helsley <[email protected]>
---
scripts/recordmcount.c | 13 -------------
scripts/recordmcount.h | 2 --
2 files changed, 15 deletions(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 273ca8b42b20..5677fcc88a72 100644
--- a/scripts/recordmcount.c
+++ b/scripts/recordmcount.c
@@ -258,17 +258,14 @@ static void *mmap_file(char const *fname)
fd_map = open(fname, O_RDONLY);
if (fd_map < 0) {
perror(fname);
- cleanup();
return NULL;
}
if (fstat(fd_map, &sb) < 0) {
perror(fname);
- cleanup();
goto out;
}
if (!S_ISREG(sb.st_mode)) {
fprintf(stderr, "not a regular file: %s\n", fname);
- cleanup();
goto out;
}
file_map = mmap(0, sb.st_size, PROT_READ|PROT_WRITE, MAP_PRIVATE,
@@ -314,13 +311,11 @@ static int write_file(const char *fname)
fd_map = open(tmp_file, O_WRONLY | O_TRUNC | O_CREAT, sb.st_mode);
if (fd_map < 0) {
perror(fname);
- cleanup();
return -1;
}
n = write(fd_map, file_map, sb.st_size);
if (n != sb.st_size) {
perror("write");
- cleanup();
close(fd_map);
return -1;
}
@@ -328,7 +323,6 @@ static int write_file(const char *fname)
n = write(fd_map, file_append, file_append_size);
if (n != file_append_size) {
perror("write");
- cleanup();
close(fd_map);
return -1;
}
@@ -336,7 +330,6 @@ static int write_file(const char *fname)
close(fd_map);
if (rename(tmp_file, fname) < 0) {
perror(fname);
- cleanup();
return -1;
}
return 0;
@@ -460,7 +453,6 @@ static int do_file(char const *const fname)
default:
fprintf(stderr, "unrecognized ELF data encoding %d: %s\n",
ehdr->e_ident[EI_DATA], fname);
- cleanup();
goto out;
case ELFDATA2LSB:
if (*(unsigned char const *)&endian != 1) {
@@ -493,7 +485,6 @@ static int do_file(char const *const fname)
w2(ehdr->e_type) != ET_REL ||
ehdr->e_ident[EI_VERSION] != EV_CURRENT) {
fprintf(stderr, "unrecognized ET_REL file %s\n", fname);
- cleanup();
goto out;
}
@@ -502,7 +493,6 @@ static int do_file(char const *const fname)
default:
fprintf(stderr, "unrecognized e_machine %u %s\n",
w2(ehdr->e_machine), fname);
- cleanup();
goto out;
case EM_386:
reltype = R_386_32;
@@ -546,14 +536,12 @@ static int do_file(char const *const fname)
default:
fprintf(stderr, "unrecognized ELF class %d %s\n",
ehdr->e_ident[EI_CLASS], fname);
- cleanup();
goto out;
case ELFCLASS32:
if (w2(ehdr->e_ehsize) != sizeof(Elf32_Ehdr)
|| w2(ehdr->e_shentsize) != sizeof(Elf32_Shdr)) {
fprintf(stderr,
"unrecognized ET_REL file: %s\n", fname);
- cleanup();
goto out;
}
if (w2(ehdr->e_machine) == EM_MIPS) {
@@ -569,7 +557,6 @@ static int do_file(char const *const fname)
|| w2(ghdr->e_shentsize) != sizeof(Elf64_Shdr)) {
fprintf(stderr,
"unrecognized ET_REL file: %s\n", fname);
- cleanup();
goto out;
}
if (w2(ghdr->e_machine) == EM_S390) {
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index ca9aaac89bfb..8f0a278ce0af 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -463,7 +463,6 @@ static int find_secsym_ndx(unsigned const txtndx,
}
fprintf(stderr, "Cannot find symbol for section %u: %s.\n",
txtndx, txtname);
- cleanup();
return -1;
}
@@ -480,7 +479,6 @@ static char const * __has_rel_mcount(Elf_Shdr const *const relhdr, /* reltype */
if (strcmp("__mcount_loc", txtname) == 0) {
fprintf(stderr, "warning: __mcount_loc already exists: %s\n",
fname);
- cleanup();
return already_has_rel_mcount;
}
if (w(txthdr->sh_type) != SHT_PROGBITS ||
--
2.20.1
On Wed, 31 Jul 2019 11:24:08 -0700
Matt Helsley <[email protected]> wrote:
> recordmcount presents unnecessary challenges to reviewers:
>
> It pretends to wrap access to the ELF file in
> uread/uwrite/ulseek functions which aren't related
> the way you might think (i.e. not the way read, write,
> and lseek are releated to each other).
>
> It uses setjmp/longjmp to handle errors (and success)
> during processing of the object files. This makes it
> hard to review what functions are doing, how globals
> change over time, etc.
>
> There are some kernel style nits.
>
> This series addresses all of those by removing un-helper functions,
> unused parameters, and rewriting the error/success handling to
> better resemble regular kernel C code.
>
I applied these patches to my queue.
I pushed them to my repo on branch ftrace/core
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
But note, that this branch will rebase, probably on top of v5.3-rc3
when it comes out.
-- Steve
Hello,
On Wed, Jul 31, 2019 at 11:24:12AM -0700, Matt Helsley wrote:
> 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.
>
> Signed-off-by: Matt Helsley <[email protected]>
This is 3f1df12019f333442b12c3b5d110b8fc43eb0b36 in mainline now.
I have the following problem:
$ make ARCH=arm arch/arm/crypto/
Using /home/uwe/gsrc/linux as source for kernel
GEN Makefile
CALL /home/uwe/gsrc/linux/scripts/checksyscalls.sh
CALL /home/uwe/gsrc/linux/scripts/atomic/check-atomics.sh
CC arch/arm/crypto/aes-cipher-glue.o
arch/arm/crypto/aes-cipher-glue.o: failed
make[2]: *** [/home/uwe/gsrc/linux/scripts/Makefile.build:281: arch/arm/crypto/aes-cipher-glue.o] Error 1
make[2]: *** Deleting file 'arch/arm/crypto/aes-cipher-glue.o'
make[1]: *** [/home/uwe/gsrc/linux/Makefile:1792: arch/arm/crypto/] Error 2
make: *** [/home/uwe/gsrc/linux/Makefile:179: sub-make] Error 2
and bisection points to 3f1df12019f333442b12c3b5d110b8fc43eb0b36.
This doesn't affect all files, most compile just fine. After calling the
compiler by hand to get aes-cipher-glue.o back I have:
uwe@taurus:~/arietta/kbuild$ ./scripts/recordmcount "arch/arm/crypto/aes-cipher-glue.o"
arch/arm/crypto/aes-cipher-glue.o: failed
I didn't debug this further, if you have problems reproducing or need more
infos tell me. The defconfig I'm using is attached.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Wed, 9 Oct 2019 12:46:26 +0200
Uwe Kleine-König <[email protected]> wrote:
> uwe@taurus:~/arietta/kbuild$ ./scripts/recordmcount "arch/arm/crypto/aes-cipher-glue.o"
> arch/arm/crypto/aes-cipher-glue.o: failed
Thanks for the report.
>
> I didn't debug this further, if you have problems reproducing or need more
> infos tell me. The defconfig I'm using is attached.
>
Does this fix it for you?
-- Steve
diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
index 3796eb37fb12..6dbec46b7703 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -389,11 +389,8 @@ static int nop_mcount(Elf_Shdr const *const relhdr,
mcountsym = get_mcountsym(sym0, relp, str0);
if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
- if (make_nop) {
+ if (make_nop)
ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_offset));
- if (ret < 0)
- return -1;
- }
if (warn_on_notrace_sect && !once) {
printf("Section %s has mcount callers being ignored\n",
txtname);
On Wed, Oct 09, 2019 at 11:05:38AM -0400, Steven Rostedt wrote:
> On Wed, 9 Oct 2019 12:46:26 +0200
> Uwe Kleine-K?nig <[email protected]> wrote:
>
>
> > uwe@taurus:~/arietta/kbuild$ ./scripts/recordmcount "arch/arm/crypto/aes-cipher-glue.o"
> > arch/arm/crypto/aes-cipher-glue.o: failed
>
> Thanks for the report.
>
> >
> > I didn't debug this further, if you have problems reproducing or need more
> > infos tell me. The defconfig I'm using is attached.
> >
>
> Does this fix it for you?
>
> -- Steve
>
> diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
> index 3796eb37fb12..6dbec46b7703 100644
> --- a/scripts/recordmcount.h
> +++ b/scripts/recordmcount.h
> @@ -389,11 +389,8 @@ static int nop_mcount(Elf_Shdr const *const relhdr,
> mcountsym = get_mcountsym(sym0, relp, str0);
>
> if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
> - if (make_nop) {
> + if (make_nop)
> ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_offset));
> - if (ret < 0)
> - return -1;
> - }
Yes, this patch fixes building for me.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |
On Wed, 9 Oct 2019 17:22:18 +0200
Uwe Kleine-König <[email protected]> wrote:
> > diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
> > index 3796eb37fb12..6dbec46b7703 100644
> > --- a/scripts/recordmcount.h
> > +++ b/scripts/recordmcount.h
> > @@ -389,11 +389,8 @@ static int nop_mcount(Elf_Shdr const *const relhdr,
> > mcountsym = get_mcountsym(sym0, relp, str0);
> >
> > if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
> > - if (make_nop) {
> > + if (make_nop)
> > ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_offset));
> > - if (ret < 0)
> > - return -1;
> > - }
>
> Yes, this patch fixes building for me.
May I add to my patch:
Reported-by: Uwe Kleine-König <[email protected]>
Tested-by: Uwe Kleine-König <[email protected]>
?
-- Steve
On Thu, Oct 10, 2019 at 12:23:21PM -0400, Steven Rostedt wrote:
> On Wed, 9 Oct 2019 17:22:18 +0200
> Uwe Kleine-K?nig <[email protected]> wrote:
>
> > > diff --git a/scripts/recordmcount.h b/scripts/recordmcount.h
> > > index 3796eb37fb12..6dbec46b7703 100644
> > > --- a/scripts/recordmcount.h
> > > +++ b/scripts/recordmcount.h
> > > @@ -389,11 +389,8 @@ static int nop_mcount(Elf_Shdr const *const relhdr,
> > > mcountsym = get_mcountsym(sym0, relp, str0);
> > >
> > > if (mcountsym == Elf_r_sym(relp) && !is_fake_mcount(relp)) {
> > > - if (make_nop) {
> > > + if (make_nop)
> > > ret = make_nop((void *)ehdr, _w(shdr->sh_offset) + _w(relp->r_offset));
> > > - if (ret < 0)
> > > - return -1;
> > > - }
> >
> > Yes, this patch fixes building for me.
>
> May I add to my patch:
>
> Reported-by: Uwe Kleine-K?nig <[email protected]>
> Tested-by: Uwe Kleine-K?nig <[email protected]>
Yeah, sure.
Thanks
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |