This series cleans up recordmcount and then makes it into
an objtool subcommand.
The series starts with 8 cleanup patches which make recordmcount
easier to review and integrate with objtool. The final 5 patches
show the beginning steps of converting recordmcount to use objtool's
ELF code rather than its own open-coded methods of accessing ELF
files.
Matt Helsley (13):
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
objtool: Prepare to merge recordmcount
objtool: Make recordmcount into an objtool subcmd
objtool: recordmcount: Start using objtool's elf wrapper
objtool: recordmcount: Search for __mcount_loc before walking the
sections
objtool: recordmcount: Convert do_func() relhdrs
scripts/.gitignore | 1 -
scripts/Makefile | 1 -
scripts/Makefile.build | 22 +-
tools/objtool/.gitignore | 1 +
tools/objtool/Build | 1 +
tools/objtool/Makefile | 7 +-
tools/objtool/builtin-mcount.c | 72 +++++
tools/objtool/builtin-mcount.h | 23 ++
tools/objtool/builtin.h | 6 +
tools/objtool/objtool.c | 6 +
{scripts => tools/objtool}/recordmcount.c | 350 ++++++++++-----------
{scripts => tools/objtool}/recordmcount.h | 197 +++++++-----
{scripts => tools/objtool}/recordmcount.pl | 0
13 files changed, 420 insertions(+), 267 deletions(-)
create mode 100644 tools/objtool/builtin-mcount.c
create mode 100644 tools/objtool/builtin-mcount.h
rename {scripts => tools/objtool}/recordmcount.c (78%)
rename {scripts => tools/objtool}/recordmcount.h (78%)
rename {scripts => tools/objtool}/recordmcount.pl (100%)
--
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 1c8b38c0d7fe..697c567c2517 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];
@@ -436,10 +451,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);
+ Elf32_Ehdr *ehdr;
unsigned int reltype = 0;
int rc = -1;
+ ehdr = mmap_file(fname);
if (!ehdr)
goto out;
@@ -575,7 +591,8 @@ static int do_file(char const *const fname)
rc = write_file(fname);
out:
- cleanup();
+ file_append_cleanup();
+ mmap_cleanup();
return rc;
}
@@ -618,12 +635,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
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]>
---
scripts/recordmcount.c | 160 +++++++++++++++++++++--------------------
scripts/recordmcount.h | 134 +++++++++++++++++++++++++---------
2 files changed, 182 insertions(+), 112 deletions(-)
diff --git a/scripts/recordmcount.c b/scripts/recordmcount.c
index 9101b67921cb..21dda8a19c4f 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);
- fail_file();
+ 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);
- 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. */
@@ -438,11 +446,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 +464,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 +497,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 +506,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 +547,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 +570,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 +583,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 +623,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 +635,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 d9736f41009e..8423c51aa8e8 100644
--- a/scripts/recordmcount.h
+++ b/scripts/recordmcount.h
@@ -25,7 +25,9 @@
#undef mcount_adjust
#undef sift_rel_mcount
#undef nop_mcount
+#undef missing_sym
#undef find_secsym_ndx
+#undef already_has_rel_mcount
#undef __has_rel_mcount
#undef has_rel_mcount
#undef tot_relsize
@@ -55,7 +57,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
@@ -88,7 +92,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
@@ -175,7 +181,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,
@@ -203,15 +209,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);
@@ -226,7 +237,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)
@@ -240,15 +252,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 +370,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 +395,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,13 +418,17 @@ 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;
}
+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))
@@ -491,6 +520,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 +532,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 +546,53 @@ 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)) {
uint_t recval = 0;
- unsigned const recsym = find_secsym_ndx(
+ unsigned const int recsym = find_secsym_ndx(
w(relhdr->sh_info), txtname, &recval,
&shdr0[symsec_sh_link = w(relhdr->sh_link)],
ehdr);
+ if (recsym == missing_sym) {
+ result = -1;
+ goto out;
+ }
rel_entsize = _w(relhdr->sh_entsize);
mlocp = sift_rel_mcount(mlocp,
@@ -543,13 +603,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
On Wed, May 22, 2019 at 05:03:23PM -0700, Matt Helsley wrote:
> This series cleans up recordmcount and then makes it into
> an objtool subcommand.
>
> The series starts with 8 cleanup patches which make recordmcount
> easier to review and integrate with objtool. The final 5 patches
> show the beginning steps of converting recordmcount to use objtool's
> ELF code rather than its own open-coded methods of accessing ELF
> files.
Hi Matt,
Thanks for the patches. This looks like a good step in the right
direction.
What's the performance difference between the old recordmcount and the
new version which relies on elf_open()? It would be useful to compare
kernel build times, before and after.
Would it be feasible to eventually combine subcommands so that objtool
could do both ORC and mcount generation in a single invocation? I
wonder what what the interface would look like.
> Matt Helsley (13):
> 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
> objtool: Prepare to merge recordmcount
> objtool: Make recordmcount into an objtool subcmd
> objtool: recordmcount: Start using objtool's elf wrapper
> objtool: recordmcount: Search for __mcount_loc before walking the
> sections
> objtool: recordmcount: Convert do_func() relhdrs
>
> scripts/.gitignore | 1 -
> scripts/Makefile | 1 -
> scripts/Makefile.build | 22 +-
> tools/objtool/.gitignore | 1 +
> tools/objtool/Build | 1 +
> tools/objtool/Makefile | 7 +-
> tools/objtool/builtin-mcount.c | 72 +++++
> tools/objtool/builtin-mcount.h | 23 ++
> tools/objtool/builtin.h | 6 +
> tools/objtool/objtool.c | 6 +
> {scripts => tools/objtool}/recordmcount.c | 350 ++++++++++-----------
> {scripts => tools/objtool}/recordmcount.h | 197 +++++++-----
> {scripts => tools/objtool}/recordmcount.pl | 0
> 13 files changed, 420 insertions(+), 267 deletions(-)
> create mode 100644 tools/objtool/builtin-mcount.c
> create mode 100644 tools/objtool/builtin-mcount.h
> rename {scripts => tools/objtool}/recordmcount.c (78%)
> rename {scripts => tools/objtool}/recordmcount.h (78%)
> rename {scripts => tools/objtool}/recordmcount.pl (100%)
>
> --
> 2.20.1
>
--
Josh
On Tue, 28 May 2019 09:43:28 -0500
Josh Poimboeuf <[email protected]> wrote:
> Thanks for the patches. This looks like a good step in the right
> direction.
Good to hear.
>
> What's the performance difference between the old recordmcount and the
> new version which relies on elf_open()? It would be useful to compare
> kernel build times, before and after.
I'll let Matt answer these.
>
> Would it be feasible to eventually combine subcommands so that objtool
> could do both ORC and mcount generation in a single invocation? I
> wonder what what the interface would look like.
That is actually the goal of this work. To eventually be able to do
single passes to accomplish multiple tasks.
-- Steve
On Tue, May 28, 2019 at 09:43:28AM -0500, Josh Poimboeuf wrote:
> Would it be feasible to eventually combine subcommands so that objtool
> could do both ORC and mcount generation in a single invocation? I
> wonder what what the interface would look like.
objtool orc+mcount ?
That is, have '+' be a separator for cmd thingies. That would of course
require all other arguments to be shared between all commands, which is
currently already so, but I've not checked the mcount patches.
Alternatively, we ditch the command thing entirely and live off of pure
flags:
'o', "orc", "Generate ORC data"
'c', "mcount', "Generate mcount() location data"
On Wed, May 29, 2019 at 03:41:52PM +0200, Peter Zijlstra wrote:
> On Tue, May 28, 2019 at 09:43:28AM -0500, Josh Poimboeuf wrote:
> > Would it be feasible to eventually combine subcommands so that objtool
> > could do both ORC and mcount generation in a single invocation? I
> > wonder what what the interface would look like.
>
> objtool orc+mcount ?
>
> That is, have '+' be a separator for cmd thingies. That would of course
> require all other arguments to be shared between all commands, which is
> currently already so, but I've not checked the mcount patches.
The problem is that you have to combine "orc generate" with "mcount
record". Because even the subcommands have subcommands ;-)
And also sharing arguments between all subcommands isn't ideal.
Maybe could do:
objtool orc generate [orc options] + mcount record [mcount options]
> Alternatively, we ditch the command thing entirely and live off of pure
> flags:
>
> 'o', "orc", "Generate ORC data"
> 'c', "mcount', "Generate mcount() location data"
--
Josh
On Wed, May 29, 2019 at 09:11:45AM -0500, Josh Poimboeuf wrote:
> On Wed, May 29, 2019 at 03:41:52PM +0200, Peter Zijlstra wrote:
> > On Tue, May 28, 2019 at 09:43:28AM -0500, Josh Poimboeuf wrote:
> > > Would it be feasible to eventually combine subcommands so that objtool
> > > could do both ORC and mcount generation in a single invocation? I
> > > wonder what what the interface would look like.
I think it'll be feasible. I've done a bit of investigation but don't
have patches for it yet.
> >
> > objtool orc+mcount ?
> >
> > That is, have '+' be a separator for cmd thingies. That would of course
> > require all other arguments to be shared between all commands, which is
> > currently already so, but I've not checked the mcount patches.
>
> The problem is that you have to combine "orc generate" with "mcount
> record". Because even the subcommands have subcommands ;-)
>
> And also sharing arguments between all subcommands isn't ideal.
>
> Maybe could do:
>
> objtool orc generate [orc options] + mcount record [mcount options]
I think that makes more sense; it'll be easier to construct
Make recipes this way. I was thinking '+' would be something like the
getopt handling of the '--' argument where it stops argument parsing so
someting else can consume the remainder.
The really interesting part is deciding which file to operate on is
specified by the arguments to the first subcommand and subsequent subcmds
would then operate on the same object file. For example:
objtool orc generate [orc opts] foo.o + mcount record [mcount opts]
Would it be clearer what's going on if the object file(s) were specified
first and then the passes to run and their arguments came afterwards?
I'm thinking it'd go somewhat like this:
objtool foo.o [bar.o] -- check [check opts] + \
orc generate [orc opts] + \
mcount record [mcount opts]
Then objtool would iterate over the object file(s) to open,
hand off the ELF data structures into each successive pass, and
finally write any accumulated changes back.
Cheers,
-Matt
On Thu, May 30, 2019 at 04:52:19PM -0700, Matt Helsley wrote:
> > > objtool orc+mcount ?
> > >
> > > That is, have '+' be a separator for cmd thingies. That would of course
> > > require all other arguments to be shared between all commands, which is
> > > currently already so, but I've not checked the mcount patches.
> >
> > The problem is that you have to combine "orc generate" with "mcount
> > record". Because even the subcommands have subcommands ;-)
> >
> > And also sharing arguments between all subcommands isn't ideal.
> >
> > Maybe could do:
> >
> > objtool orc generate [orc options] + mcount record [mcount options]
>
> I think that makes more sense; it'll be easier to construct
> Make recipes this way. I was thinking '+' would be something like the
> getopt handling of the '--' argument where it stops argument parsing so
> someting else can consume the remainder.
>
> The really interesting part is deciding which file to operate on is
> specified by the arguments to the first subcommand and subsequent subcmds
> would then operate on the same object file. For example:
>
> objtool orc generate [orc opts] foo.o + mcount record [mcount opts]
>
> Would it be clearer what's going on if the object file(s) were specified
> first and then the passes to run and their arguments came afterwards?
> I'm thinking it'd go somewhat like this:
>
> objtool foo.o [bar.o] -- check [check opts] + \
> orc generate [orc opts] + \
> mcount record [mcount opts]
>
> Then objtool would iterate over the object file(s) to open,
> hand off the ELF data structures into each successive pass, and
> finally write any accumulated changes back.
Yeah, I forgot about the .o file. Something like that would probably
work. The ordering seems a bit funny to me. Another possibly more
readable variation would be:
objtool check [check opts] + orc generate [orc opts] + mcount record [mcount opts] -- foo.o [bar.o]
or, just use '--' as a generic separator which can be used to separate
subcommands or file names.
objtool check [check opts] -- orc generate [orc opts] -- mcount record [mcount opts] -- foo.o [bar.o]
I kind of like that. But I think any of these variations would probably
work.
--
Josh