[PATCH 00/10] perf symbols: Improve dso__synthesize_plt_symbols()

[PATCH 00/10] perf symbols: Improve dso__synthesize_plt_symbols()

[Adrian Hunter]

Hi

This is the first of 2 patchsets to improve dso__synthesize_plt_symbols().
This patchset is really preparation for the 2nd patchset, which focuses
on getting rid of unknown symbols that show up in Intel PT traces.
The 2nd patchset is still under development.

These patches are small and staightforward. Only the new Symbols test is
slightly interesting because it provides a way to see what symbols
perf discovers for any given dso. The test fails initially, but
should pass after patch 7 "perf symbols: Add symbol for .plt header".


Adrian Hunter (10):
      perf test: Add Symbols test
      perf symbols: Factor out get_plt_sizes()
      perf symbols: Check plt_entry_size is not zero
      perf symbols: Add dso__find_symbol_nocache()
      perf symbols: Slightly simplify 'err' usage in dso__synthesize_plt_symbols()
      perf symbols: Do not check ss->dynsym twice
      perf symbols: Add symbol for .plt header
      perf symbols: Allow for .plt entries with no symbol
      perf symbols: Combine handling for SHT_RELA and SHT_REL
      perf symbols: Check SHT_RELA and SHT_REL type earlier

 tools/perf/Documentation/perf-test.txt |   3 +
 tools/perf/tests/Build                 |   1 +
 tools/perf/tests/builtin-test.c        |   3 +
 tools/perf/tests/symbols.c             | 150 ++++++++++++++++++++++++++
 tools/perf/tests/tests.h               |   3 +
 tools/perf/util/symbol-elf.c           | 190 +++++++++++++++++----------------
 tools/perf/util/symbol.c               |   5 +
 tools/perf/util/symbol.h               |   1 +
 8 files changed, 262 insertions(+), 94 deletions(-)
 create mode 100644 tools/perf/tests/symbols.c


Regards
Adrian

[PATCH 01/10] perf test: Add Symbols test

[Adrian Hunter]

Add a test to check function symbols do not overlap and are not zero
length.

The main motivation for the test is to make it easier to review changes
to PLT symbol synthesis i.e. changes to dso__synthesize_plt_symbols().

By default the test uses the perf executable as a test DSO, but a
specific DSO can be specified via a new perf test option "--dso".

The test is useful in the following ways:
 - Any DSO can be tested, even ones that do not run on the current
 architecture. For example, using cross-compiled DSOs to see how
 well perf handles different architectures.
 - With verbose > 1 (e.g. -vv), all the symbols are printed, which
 makes it easier to see issues.
 - perf removes duplicate symbols and expands zero-length symbols
 to reach the next symbol, however that is done before adding
 synthesized symbols, so the test is checking those also.

Example:

 $ perf test -v Symbols
  74: Symbols                                                         :
 --- start ---
 test child forked, pid 154918
 Testing /home/user/bin/perf
 Overlapping symbols:
  7d000-7f3a0 g _init
  7d030-7d040 g __printf_chk@plt
 test child finished with -1
 ---- end ----
 Symbols: FAILED!

Note the test fails because perf expands the _init symbol over the PLT
because there are no PLT symbols at that point, but then
dso__synthesize_plt_symbols() creates them.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/Documentation/perf-test.txt |   3 +
 tools/perf/tests/Build                 |   1 +
 tools/perf/tests/builtin-test.c        |   3 +
 tools/perf/tests/symbols.c             | 150 +++++++++++++++++++++++++
 tools/perf/tests/tests.h               |   3 +
 5 files changed, 160 insertions(+)
 create mode 100644 tools/perf/tests/symbols.c

diff --git a/tools/perf/Documentation/perf-test.txt b/tools/perf/Documentation/perf-test.txt
index b329c65d7f40..951a2f262872 100644
--- a/tools/perf/Documentation/perf-test.txt
+++ b/tools/perf/Documentation/perf-test.txt
@@ -34,3 +34,6 @@ OPTIONS
 -F::
 --dont-fork::
 	Do not fork child for each test, run all tests within single process.
+
+--dso::
+	Specify a DSO for the "Symbols" test.
diff --git a/tools/perf/tests/Build b/tools/perf/tests/Build
index 90fd1eb317bb..fb9ac5dc4079 100644
--- a/tools/perf/tests/Build
+++ b/tools/perf/tests/Build
@@ -68,6 +68,7 @@ perf-y += perf-time-to-tsc.o
 perf-y += dlfilter-test.o
 perf-y += sigtrap.o
 perf-y += event_groups.o
+perf-y += symbols.o
 
 $(OUTPUT)tests/llvm-src-base.c: tests/bpf-script-example.c tests/Build
 	$(call rule_mkdir)
diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index cfa61493c750..35cc3807cc9e 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -31,6 +31,7 @@
 #include "builtin-test-list.h"
 
 static bool dont_fork;
+const char *dso_to_test;
 
 struct test_suite *__weak arch_tests[] = {
 	NULL,
@@ -117,6 +118,7 @@ static struct test_suite *generic_tests[] = {
 	&suite__dlfilter,
 	&suite__sigtrap,
 	&suite__event_groups,
+	&suite__symbols,
 	NULL,
 };
 
@@ -521,6 +523,7 @@ int cmd_test(int argc, const char **argv)
 	OPT_BOOLEAN('F', "dont-fork", &dont_fork,
 		    "Do not fork for testcase"),
 	OPT_STRING('w', "workload", &workload, "work", "workload to run for testing"),
+	OPT_STRING(0, "dso", &dso_to_test, "dso", "dso to test"),
 	OPT_END()
 	};
 	const char * const test_subcommands[] = { "list", NULL };
diff --git a/tools/perf/tests/symbols.c b/tools/perf/tests/symbols.c
new file mode 100644
index 000000000000..057b16df6416
--- /dev/null
+++ b/tools/perf/tests/symbols.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/compiler.h>
+#include <linux/string.h>
+#include <sys/mman.h>
+#include <limits.h>
+#include "debug.h"
+#include "dso.h"
+#include "machine.h"
+#include "thread.h"
+#include "symbol.h"
+#include "map.h"
+#include "util.h"
+#include "tests.h"
+
+struct test_info {
+	struct machine *machine;
+	struct thread *thread;
+};
+
+static int init_test_info(struct test_info *ti)
+{
+	ti->machine = machine__new_host();
+	if (!ti->machine) {
+		pr_debug("machine__new_host() failed!\n");
+		return TEST_FAIL;
+	}
+
+	/* Create a dummy thread */
+	ti->thread = machine__findnew_thread(ti->machine, 100, 100);
+	if (!ti->thread) {
+		pr_debug("machine__findnew_thread() failed!\n");
+		return TEST_FAIL;
+	}
+
+	return TEST_OK;
+}
+
+static void exit_test_info(struct test_info *ti)
+{
+	thread__put(ti->thread);
+	machine__delete(ti->machine);
+}
+
+static void get_test_dso_filename(char *filename, size_t max_sz)
+{
+	if (dso_to_test)
+		strlcpy(filename, dso_to_test, max_sz);
+	else
+		perf_exe(filename, max_sz);
+}
+
+static int create_map(struct test_info *ti, char *filename, struct map **map_p)
+{
+	/* Create a dummy map at 0x100000 */
+	*map_p = map__new(ti->machine, 0x100000, 0xffffffff, 0, NULL,
+			  PROT_EXEC, 0, NULL, filename, ti->thread);
+	if (!*map_p) {
+		pr_debug("Failed to create map!");
+		return TEST_FAIL;
+	}
+
+	return TEST_OK;
+}
+
+static int test_dso(struct dso *dso)
+{
+	struct symbol *last_sym = NULL;
+	struct rb_node *nd;
+	int ret = TEST_OK;
+
+	/* dso__fprintf() prints all the symbols */
+	if (verbose > 1)
+		dso__fprintf(dso, stderr);
+
+	for (nd = rb_first_cached(&dso->symbols); nd; nd = rb_next(nd)) {
+		struct symbol *sym = rb_entry(nd, struct symbol, rb_node);
+
+		if (sym->type != STT_FUNC && sym->type != STT_GNU_IFUNC)
+			continue;
+
+		/* Check for overlapping function symbols */
+		if (last_sym && sym->start < last_sym->end) {
+			pr_debug("Overlapping symbols:\n");
+			symbol__fprintf(last_sym, stderr);
+			symbol__fprintf(sym, stderr);
+			ret = TEST_FAIL;
+		}
+		/* Check for zero-length function symbol */
+		if (sym->start == sym->end) {
+			pr_debug("Zero-length symbol:\n");
+			symbol__fprintf(sym, stderr);
+			ret = TEST_FAIL;
+		}
+		last_sym = sym;
+	}
+
+	return ret;
+}
+
+static int test_file(struct test_info *ti, char *filename)
+{
+	struct map *map = NULL;
+	int ret, nr;
+
+	pr_debug("Testing %s\n", filename);
+
+	ret = create_map(ti, filename, &map);
+	if (ret != TEST_OK)
+		return ret;
+
+	nr = dso__load(map->dso, map);
+	if (nr < 0) {
+		pr_debug("dso__load() failed!\n");
+		ret = TEST_FAIL;
+		goto out_put;
+	}
+
+	if (nr == 0) {
+		pr_debug("DSO has no symbols!\n");
+		ret = TEST_SKIP;
+		goto out_put;
+	}
+
+	ret = test_dso(map->dso);
+out_put:
+	map__put(map);
+
+	return ret;
+}
+
+static int test__symbols(struct test_suite *test __maybe_unused, int subtest __maybe_unused)
+{
+	char filename[PATH_MAX];
+	struct test_info ti;
+	int ret;
+
+	ret = init_test_info(&ti);
+	if (ret != TEST_OK)
+		return ret;
+
+	get_test_dso_filename(filename, sizeof(filename));
+
+	ret = test_file(&ti, filename);
+
+	exit_test_info(&ti);
+
+	return ret;
+}
+
+DEFINE_SUITE("Symbols", symbols);
diff --git a/tools/perf/tests/tests.h b/tools/perf/tests/tests.h
index fb4b5ad4dd0f..9a0f3904e53d 100644
--- a/tools/perf/tests/tests.h
+++ b/tools/perf/tests/tests.h
@@ -148,6 +148,7 @@ DECLARE_SUITE(perf_time_to_tsc);
 DECLARE_SUITE(dlfilter);
 DECLARE_SUITE(sigtrap);
 DECLARE_SUITE(event_groups);
+DECLARE_SUITE(symbols);
 
 /*
  * PowerPC and S390 do not support creation of instruction breakpoints using the
@@ -208,4 +209,6 @@ DECLARE_WORKLOAD(sqrtloop);
 DECLARE_WORKLOAD(brstack);
 DECLARE_WORKLOAD(datasym);
 
+extern const char *dso_to_test;
+
 #endif /* TESTS_H */
-- 
2.34.1

[PATCH 02/10] perf symbols: Factor out get_plt_sizes()

[Adrian Hunter]

Factor out get_plt_sizes() to make the code more readable and further
changes to dso__synthesize_plt_symbols() easier to follow.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/symbol-elf.c | 54 +++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 96767d1b3f1c..4605680a22a3 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -323,6 +323,33 @@ static char *demangle_sym(struct dso *dso, int kmodule, const char *elf_name)
 	return demangled;
 }
 
+static void get_plt_sizes(GElf_Ehdr *ehdr, GElf_Shdr *shdr_plt,
+			  u64 *plt_header_size, u64 *plt_entry_size)
+{
+	switch (ehdr->e_machine) {
+	case EM_ARM:
+		*plt_header_size = 20;
+		*plt_entry_size = 12;
+		return;
+	case EM_AARCH64:
+		*plt_header_size = 32;
+		*plt_entry_size = 16;
+		return;
+	case EM_SPARC:
+		*plt_header_size = 48;
+		*plt_entry_size = 12;
+		return;
+	case EM_SPARCV9:
+		*plt_header_size = 128;
+		*plt_entry_size = 32;
+		return;
+	default: /* FIXME: s390/alpha/mips/parisc/poperpc/sh/xtensa need to be checked */
+		*plt_header_size = shdr_plt->sh_entsize;
+		*plt_entry_size = shdr_plt->sh_entsize;
+		return;
+	}
+}
+
 #define elf_section__for_each_rel(reldata, pos, pos_mem, idx, nr_entries) \
 	for (idx = 0, pos = gelf_getrel(reldata, 0, &pos_mem); \
 	     idx < nr_entries; \
@@ -411,32 +438,7 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 
 	nr_rel_entries = shdr_rel_plt.sh_size / shdr_rel_plt.sh_entsize;
 	plt_offset = shdr_plt.sh_offset;
-	switch (ehdr.e_machine) {
-		case EM_ARM:
-			plt_header_size = 20;
-			plt_entry_size = 12;
-			break;
-
-		case EM_AARCH64:
-			plt_header_size = 32;
-			plt_entry_size = 16;
-			break;
-
-		case EM_SPARC:
-			plt_header_size = 48;
-			plt_entry_size = 12;
-			break;
-
-		case EM_SPARCV9:
-			plt_header_size = 128;
-			plt_entry_size = 32;
-			break;
-
-		default: /* FIXME: s390/alpha/mips/parisc/poperpc/sh/xtensa need to be checked */
-			plt_header_size = shdr_plt.sh_entsize;
-			plt_entry_size = shdr_plt.sh_entsize;
-			break;
-	}
+	get_plt_sizes(&ehdr, &shdr_plt, &plt_header_size, &plt_entry_size);
 	plt_offset += plt_header_size;
 
 	if (shdr_rel_plt.sh_type == SHT_RELA) {
-- 
2.34.1

[PATCH 03/10] perf symbols: Check plt_entry_size is not zero

[Adrian Hunter]

The code expects non-zero plt_entry_size. Check it and add a debug
message to print if it is zero.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/symbol-elf.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 4605680a22a3..c6a4e6c73990 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -323,30 +323,33 @@ static char *demangle_sym(struct dso *dso, int kmodule, const char *elf_name)
 	return demangled;
 }
 
-static void get_plt_sizes(GElf_Ehdr *ehdr, GElf_Shdr *shdr_plt,
+static bool get_plt_sizes(struct dso *dso, GElf_Ehdr *ehdr, GElf_Shdr *shdr_plt,
 			  u64 *plt_header_size, u64 *plt_entry_size)
 {
 	switch (ehdr->e_machine) {
 	case EM_ARM:
 		*plt_header_size = 20;
 		*plt_entry_size = 12;
-		return;
+		return true;
 	case EM_AARCH64:
 		*plt_header_size = 32;
 		*plt_entry_size = 16;
-		return;
+		return true;
 	case EM_SPARC:
 		*plt_header_size = 48;
 		*plt_entry_size = 12;
-		return;
+		return true;
 	case EM_SPARCV9:
 		*plt_header_size = 128;
 		*plt_entry_size = 32;
-		return;
+		return true;
 	default: /* FIXME: s390/alpha/mips/parisc/poperpc/sh/xtensa need to be checked */
 		*plt_header_size = shdr_plt->sh_entsize;
 		*plt_entry_size = shdr_plt->sh_entsize;
-		return;
+		if (*plt_entry_size)
+			return true;
+		pr_debug("Missing PLT entry size for %s\n", dso->long_name);
+		return false;
 	}
 }
 
@@ -438,7 +441,8 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 
 	nr_rel_entries = shdr_rel_plt.sh_size / shdr_rel_plt.sh_entsize;
 	plt_offset = shdr_plt.sh_offset;
-	get_plt_sizes(&ehdr, &shdr_plt, &plt_header_size, &plt_entry_size);
+	if (!get_plt_sizes(dso, &ehdr, &shdr_plt, &plt_header_size, &plt_entry_size))
+		return 0;
 	plt_offset += plt_header_size;
 
 	if (shdr_rel_plt.sh_type == SHT_RELA) {
-- 
2.34.1

[PATCH 04/10] perf symbols: Add dso__find_symbol_nocache()

[Adrian Hunter]

Symbols should not be cached when there are more symbols still to add.
Add dso__find_symbol_nocache() to facilitate that.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/symbol.c | 5 +++++
 tools/perf/util/symbol.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index a3a165ae933a..a024f06f75d8 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -556,6 +556,11 @@ struct symbol *dso__find_symbol(struct dso *dso, u64 addr)
 	return dso->last_find_result.symbol;
 }
 
+struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr)
+{
+	return symbols__find(&dso->symbols, addr);
+}
+
 struct symbol *dso__first_symbol(struct dso *dso)
 {
 	return symbols__first(&dso->symbols);
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index f735108c4d4e..2fdeb22bd02f 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -148,6 +148,7 @@ void dso__delete_symbol(struct dso *dso,
 			struct symbol *sym);
 
 struct symbol *dso__find_symbol(struct dso *dso, u64 addr);
+struct symbol *dso__find_symbol_nocache(struct dso *dso, u64 addr);
 struct symbol *dso__find_symbol_by_name(struct dso *dso, const char *name);
 
 struct symbol *symbol__next_by_name(struct symbol *sym);
-- 
2.34.1

[PATCH 05/10] perf symbols: Slightly simplify 'err' usage in dso__synthesize_plt_symbols()

[Adrian Hunter]

Return zero directly instead of needless 'goto out_elf_end' that does
the same thing. That allows 'err' to be initialized to -1 instead of
having to change its value later.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/symbol-elf.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index c6a4e6c73990..990a2c6037bb 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -384,7 +384,7 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 	GElf_Ehdr ehdr;
 	char sympltname[1024];
 	Elf *elf;
-	int nr = 0, symidx, err = 0;
+	int nr = 0, symidx, err = -1;
 
 	if (!ss->dynsym)
 		return 0;
@@ -397,7 +397,7 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 	dynsym_idx = ss->dynsym_idx;
 
 	if (scn_dynsym == NULL)
-		goto out_elf_end;
+		return 0;
 
 	scn_plt_rel = elf_section_by_name(elf, &ehdr, &shdr_rel_plt,
 					  ".rela.plt", NULL);
@@ -405,11 +405,9 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 		scn_plt_rel = elf_section_by_name(elf, &ehdr, &shdr_rel_plt,
 						  ".rel.plt", NULL);
 		if (scn_plt_rel == NULL)
-			goto out_elf_end;
+			return 0;
 	}
 
-	err = -1;
-
 	if (shdr_rel_plt.sh_link != dynsym_idx)
 		goto out_elf_end;
 
-- 
2.34.1

[PATCH 06/10] perf symbols: Do not check ss->dynsym twice

[Adrian Hunter]

ss->dynsym is checked to be not NULL twice. Remove the first check
because, in fact, there can be a plt with no dynsym, which is something
that will be dealt with later.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/symbol-elf.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 990a2c6037bb..87b82507c205 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -386,9 +386,6 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 	Elf *elf;
 	int nr = 0, symidx, err = -1;
 
-	if (!ss->dynsym)
-		return 0;
-
 	elf = ss->elf;
 	ehdr = ss->ehdr;
 
-- 
2.34.1

[PATCH 07/10] perf symbols: Add symbol for .plt header

[Adrian Hunter]

perf expands the _init symbol over .plt because there are no PLT symbols
at that point, but then dso__synthesize_plt_symbols() creates them.

Fix by truncating the previous symbol and inserting a symbol for .plt
header.

Example:

 Before:

   $ perf test --dso `which uname` -v Symbols
    74: Symbols                                                         :
   --- start ---
   test child forked, pid 191028
   Problems creating module maps, continuing anyway...
   Testing /usr/bin/uname
   Overlapping symbols:
    2000-25f0 g _init
    2040-2050 g free@plt
   test child finished with -1
   ---- end ----
   Symbols: FAILED!
   $ perf test --dso `which uname` -vv Symbols 2>/tmp/cmp1.txt

 After:

   $ perf test --dso `which uname` -v Symbols
    74: Symbols                                                         :
   --- start ---
   test child forked, pid 194291
   Testing /usr/bin/uname
   test child finished with 0
   ---- end ----
   Symbols: Ok
   $ perf test --dso `which uname` -vv Symbols 2>/tmp/cmp2.txt
   $ diff /tmp/cmp1.txt /tmp/cmp2.txt
   4,5c4
   < test child forked, pid 191031
   < Problems creating module maps, continuing anyway...
   ---
   > test child forked, pid 194296
   9c8,9
   <  2000-25f0 g _init
   ---
   >  2000-2030 g _init
   >  2030-2040 g .plt
   100,103c100
   < Overlapping symbols:
   <  2000-25f0 g _init
   <  2040-2050 g free@plt
   < test child finished with -1
   ---
   > test child finished with 0
   105c102
   < Symbols: FAILED!
   ---
   > Symbols: Ok
   $

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/symbol-elf.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 87b82507c205..a8b7c3860b2d 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -389,6 +389,27 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 	elf = ss->elf;
 	ehdr = ss->ehdr;
 
+	if (!elf_section_by_name(elf, &ehdr, &shdr_plt, ".plt", NULL))
+		return 0;
+
+	/*
+	 * A symbol from a previous section (e.g. .init) can have been expanded
+	 * by symbols__fixup_end() to overlap .plt. Truncate it before adding
+	 * a symbol for .plt header.
+	 */
+	f = dso__find_symbol_nocache(dso, shdr_plt.sh_offset);
+	if (f && f->start < shdr_plt.sh_offset && f->end > shdr_plt.sh_offset)
+		f->end = shdr_plt.sh_offset;
+
+	if (!get_plt_sizes(dso, &ehdr, &shdr_plt, &plt_header_size, &plt_entry_size))
+		return 0;
+
+	/* Add a symbol for .plt header */
+	f = symbol__new(shdr_plt.sh_offset, plt_header_size, STB_GLOBAL, STT_FUNC, ".plt");
+	if (!f)
+		goto out_elf_end;
+	symbols__insert(&dso->symbols, f);
+
 	scn_dynsym = ss->dynsym;
 	shdr_dynsym = ss->dynshdr;
 	dynsym_idx = ss->dynsym_idx;
@@ -408,9 +429,6 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 	if (shdr_rel_plt.sh_link != dynsym_idx)
 		goto out_elf_end;
 
-	if (elf_section_by_name(elf, &ehdr, &shdr_plt, ".plt", NULL) == NULL)
-		goto out_elf_end;
-
 	/*
 	 * Fetch the relocation section to find the idxes to the GOT
 	 * and the symbols in the .dynsym they refer to.
@@ -436,8 +454,6 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 
 	nr_rel_entries = shdr_rel_plt.sh_size / shdr_rel_plt.sh_entsize;
 	plt_offset = shdr_plt.sh_offset;
-	if (!get_plt_sizes(dso, &ehdr, &shdr_plt, &plt_header_size, &plt_entry_size))
-		return 0;
 	plt_offset += plt_header_size;
 
 	if (shdr_rel_plt.sh_type == SHT_RELA) {
-- 
2.34.1

[PATCH 08/10] perf symbols: Allow for .plt entries with no symbol

[Adrian Hunter]

Create a sensible name for .plt entries with no symbol.

Example:

 Before:

   $ perf test --dso /usr/lib/x86_64-linux-gnu/libc.so.6 -vv Symbols 2>/tmp/cmp1.txt

 After:

   $ perf test --dso /usr/lib/x86_64-linux-gnu/libc.so.6 -vv Symbols 2>/tmp/cmp2.txt
   $ diff /tmp/cmp1.txt /tmp/cmp2.txt
    4c4
    < test child forked, pid 53043
    ---
    > test child forked, pid 54372
    23,62c23,62
    <  280f0-28100 g @plt
    <  28100-28110 g @plt
    <  28110-28120 g @plt
    <  28120-28130 g @plt
    <  28130-28140 g @plt
    <  28140-28150 g @plt
    <  28150-28160 g @plt
    <  28160-28170 g @plt
    <  28170-28180 g @plt
    <  28180-28190 g @plt
    <  28190-281a0 g @plt
    <  281a0-281b0 g @plt
    <  281b0-281c0 g @plt
    <  281c0-281d0 g @plt
    <  281d0-281e0 g @plt
    <  281e0-281f0 g @plt
    <  281f0-28200 g @plt
    <  28200-28210 g @plt
    <  28210-28220 g @plt
    <  28220-28230 g @plt
    <  28230-28240 g @plt
    <  28240-28250 g @plt
    <  28250-28260 g @plt
    <  28260-28270 g @plt
    <  28270-28280 g @plt
    <  28280-28290 g @plt
    <  28290-282a0 g @plt
    <  282a0-282b0 g @plt
    <  282b0-282c0 g @plt
    <  282c0-282d0 g @plt
    <  282d0-282e0 g @plt
    <  282e0-282f0 g @plt
    <  282f0-28300 g @plt
    <  28300-28310 g @plt
    <  28310-28320 g @plt
    <  28320-28330 g @plt
    <  28330-28340 g @plt
    <  28340-28350 g @plt
    <  28350-28360 g @plt
    <  28360-28370 g @plt
    ---
    >  280f0-28100 g offset_0x280f0@plt
    >  28100-28110 g offset_0x28100@plt
    >  28110-28120 g offset_0x28110@plt
    >  28120-28130 g offset_0x28120@plt
    >  28130-28140 g offset_0x28130@plt
    >  28140-28150 g offset_0x28140@plt
    >  28150-28160 g offset_0x28150@plt
    >  28160-28170 g offset_0x28160@plt
    >  28170-28180 g offset_0x28170@plt
    >  28180-28190 g offset_0x28180@plt
    >  28190-281a0 g offset_0x28190@plt
    >  281a0-281b0 g offset_0x281a0@plt
    >  281b0-281c0 g offset_0x281b0@plt
    >  281c0-281d0 g offset_0x281c0@plt
    >  281d0-281e0 g offset_0x281d0@plt
    >  281e0-281f0 g offset_0x281e0@plt
    >  281f0-28200 g offset_0x281f0@plt
    >  28200-28210 g offset_0x28200@plt
    >  28210-28220 g offset_0x28210@plt
    >  28220-28230 g offset_0x28220@plt
    >  28230-28240 g offset_0x28230@plt
    >  28240-28250 g offset_0x28240@plt
    >  28250-28260 g offset_0x28250@plt
    >  28260-28270 g offset_0x28260@plt
    >  28270-28280 g offset_0x28270@plt
    >  28280-28290 g offset_0x28280@plt
    >  28290-282a0 g offset_0x28290@plt
    >  282a0-282b0 g offset_0x282a0@plt
    >  282b0-282c0 g offset_0x282b0@plt
    >  282c0-282d0 g offset_0x282c0@plt
    >  282d0-282e0 g offset_0x282d0@plt
    >  282e0-282f0 g offset_0x282e0@plt
    >  282f0-28300 g offset_0x282f0@plt
    >  28300-28310 g offset_0x28300@plt
    >  28310-28320 g offset_0x28310@plt
    >  28320-28330 g offset_0x28320@plt
    >  28330-28340 g offset_0x28330@plt
    >  28340-28350 g offset_0x28340@plt
    >  28350-28360 g offset_0x28350@plt
    >  28360-28370 g offset_0x28360@plt

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/symbol-elf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index a8b7c3860b2d..6e4a22acefba 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -470,8 +470,11 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 			demangled = demangle_sym(dso, 0, elf_name);
 			if (demangled != NULL)
 				elf_name = demangled;
-			snprintf(sympltname, sizeof(sympltname),
-				 "%s@plt", elf_name);
+			if (*elf_name)
+				snprintf(sympltname, sizeof(sympltname), "%s@plt", elf_name);
+			else
+				snprintf(sympltname, sizeof(sympltname),
+					 "offset_%#" PRIx64 "@plt", plt_offset);
 			free(demangled);
 
 			f = symbol__new(plt_offset, plt_entry_size,
@@ -496,8 +499,11 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 			demangled = demangle_sym(dso, 0, elf_name);
 			if (demangled != NULL)
 				elf_name = demangled;
-			snprintf(sympltname, sizeof(sympltname),
-				 "%s@plt", elf_name);
+			if (*elf_name)
+				snprintf(sympltname, sizeof(sympltname), "%s@plt", elf_name);
+			else
+				snprintf(sympltname, sizeof(sympltname),
+					 "offset_%#" PRIx64 "@plt", plt_offset);
 			free(demangled);
 
 			f = symbol__new(plt_offset, plt_entry_size,
-- 
2.34.1

[PATCH 09/10] perf symbols: Combine handling for SHT_RELA and SHT_REL

[Adrian Hunter]

SHT_REL and SHT_RELA are handled the same way. Simplify by combining the
handling.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/symbol-elf.c | 75 +++++++++++++-----------------------
 1 file changed, 27 insertions(+), 48 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 6e4a22acefba..e274f646ac32 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -323,6 +323,23 @@ static char *demangle_sym(struct dso *dso, int kmodule, const char *elf_name)
 	return demangled;
 }
 
+struct rel_info {
+	bool		is_rela;
+	Elf_Data	*reldata;
+	GElf_Rela	rela;
+	GElf_Rel	rel;
+};
+
+static u32 get_rel_symidx(struct rel_info *ri, u32 idx)
+{
+	if (ri->is_rela) {
+		gelf_getrela(ri->reldata, idx, &ri->rela);
+		return GELF_R_SYM(ri->rela.r_info);
+	}
+	gelf_getrel(ri->reldata, idx, &ri->rel);
+	return GELF_R_SYM(ri->rel.r_info);
+}
+
 static bool get_plt_sizes(struct dso *dso, GElf_Ehdr *ehdr, GElf_Shdr *shdr_plt,
 			  u64 *plt_header_size, u64 *plt_entry_size)
 {
@@ -353,16 +370,6 @@ static bool get_plt_sizes(struct dso *dso, GElf_Ehdr *ehdr, GElf_Shdr *shdr_plt,
 	}
 }
 
-#define elf_section__for_each_rel(reldata, pos, pos_mem, idx, nr_entries) \
-	for (idx = 0, pos = gelf_getrel(reldata, 0, &pos_mem); \
-	     idx < nr_entries; \
-	     ++idx, pos = gelf_getrel(reldata, idx, &pos_mem))
-
-#define elf_section__for_each_rela(reldata, pos, pos_mem, idx, nr_entries) \
-	for (idx = 0, pos = gelf_getrela(reldata, 0, &pos_mem); \
-	     idx < nr_entries; \
-	     ++idx, pos = gelf_getrela(reldata, idx, &pos_mem))
-
 /*
  * We need to check if we have a .dynsym, so that we can handle the
  * .plt, synthesizing its symbols, that aren't on the symtabs (be it
@@ -378,13 +385,14 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 	GElf_Shdr shdr_plt;
 	struct symbol *f;
 	GElf_Shdr shdr_rel_plt, shdr_dynsym;
-	Elf_Data *reldata, *syms, *symstrs;
+	Elf_Data *syms, *symstrs;
 	Elf_Scn *scn_plt_rel, *scn_symstrs, *scn_dynsym;
 	size_t dynsym_idx;
 	GElf_Ehdr ehdr;
 	char sympltname[1024];
 	Elf *elf;
-	int nr = 0, symidx, err = -1;
+	int nr = 0, err = -1;
+	struct rel_info ri = { .is_rela = false };
 
 	elf = ss->elf;
 	ehdr = ss->ehdr;
@@ -433,8 +441,8 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 	 * Fetch the relocation section to find the idxes to the GOT
 	 * and the symbols in the .dynsym they refer to.
 	 */
-	reldata = elf_getdata(scn_plt_rel, NULL);
-	if (reldata == NULL)
+	ri.reldata = elf_getdata(scn_plt_rel, NULL);
+	if (!ri.reldata)
 		goto out_elf_end;
 
 	syms = elf_getdata(scn_dynsym, NULL);
@@ -456,44 +464,15 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 	plt_offset = shdr_plt.sh_offset;
 	plt_offset += plt_header_size;
 
-	if (shdr_rel_plt.sh_type == SHT_RELA) {
-		GElf_Rela pos_mem, *pos;
+	ri.is_rela = shdr_rel_plt.sh_type == SHT_RELA;
 
-		elf_section__for_each_rela(reldata, pos, pos_mem, idx,
-					   nr_rel_entries) {
+	if (shdr_rel_plt.sh_type == SHT_RELA ||
+	    shdr_rel_plt.sh_type == SHT_REL) {
+		for (idx = 0; idx < nr_rel_entries; idx++) {
 			const char *elf_name = NULL;
 			char *demangled = NULL;
-			symidx = GELF_R_SYM(pos->r_info);
-			gelf_getsym(syms, symidx, &sym);
-
-			elf_name = elf_sym__name(&sym, symstrs);
-			demangled = demangle_sym(dso, 0, elf_name);
-			if (demangled != NULL)
-				elf_name = demangled;
-			if (*elf_name)
-				snprintf(sympltname, sizeof(sympltname), "%s@plt", elf_name);
-			else
-				snprintf(sympltname, sizeof(sympltname),
-					 "offset_%#" PRIx64 "@plt", plt_offset);
-			free(demangled);
 
-			f = symbol__new(plt_offset, plt_entry_size,
-					STB_GLOBAL, STT_FUNC, sympltname);
-			if (!f)
-				goto out_elf_end;
-
-			plt_offset += plt_entry_size;
-			symbols__insert(&dso->symbols, f);
-			++nr;
-		}
-	} else if (shdr_rel_plt.sh_type == SHT_REL) {
-		GElf_Rel pos_mem, *pos;
-		elf_section__for_each_rel(reldata, pos, pos_mem, idx,
-					  nr_rel_entries) {
-			const char *elf_name = NULL;
-			char *demangled = NULL;
-			symidx = GELF_R_SYM(pos->r_info);
-			gelf_getsym(syms, symidx, &sym);
+			gelf_getsym(syms, get_rel_symidx(&ri, idx), &sym);
 
 			elf_name = elf_sym__name(&sym, symstrs);
 			demangled = demangle_sym(dso, 0, elf_name);
-- 
2.34.1

[PATCH 10/10] perf symbols: Check SHT_RELA and SHT_REL type earlier

[Adrian Hunter]

Make the code more readable by checking for SHT_RELA and SHT_REL type
earlier.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/symbol-elf.c | 54 ++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index e274f646ac32..aa62735aea7b 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -434,6 +434,10 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 			return 0;
 	}
 
+	if (shdr_rel_plt.sh_type != SHT_RELA &&
+	    shdr_rel_plt.sh_type != SHT_REL)
+		return 0;
+
 	if (shdr_rel_plt.sh_link != dynsym_idx)
 		goto out_elf_end;
 
@@ -466,34 +470,30 @@ int dso__synthesize_plt_symbols(struct dso *dso, struct symsrc *ss)
 
 	ri.is_rela = shdr_rel_plt.sh_type == SHT_RELA;
 
-	if (shdr_rel_plt.sh_type == SHT_RELA ||
-	    shdr_rel_plt.sh_type == SHT_REL) {
-		for (idx = 0; idx < nr_rel_entries; idx++) {
-			const char *elf_name = NULL;
-			char *demangled = NULL;
-
-			gelf_getsym(syms, get_rel_symidx(&ri, idx), &sym);
-
-			elf_name = elf_sym__name(&sym, symstrs);
-			demangled = demangle_sym(dso, 0, elf_name);
-			if (demangled != NULL)
-				elf_name = demangled;
-			if (*elf_name)
-				snprintf(sympltname, sizeof(sympltname), "%s@plt", elf_name);
-			else
-				snprintf(sympltname, sizeof(sympltname),
-					 "offset_%#" PRIx64 "@plt", plt_offset);
-			free(demangled);
-
-			f = symbol__new(plt_offset, plt_entry_size,
-					STB_GLOBAL, STT_FUNC, sympltname);
-			if (!f)
-				goto out_elf_end;
+	for (idx = 0; idx < nr_rel_entries; idx++) {
+		const char *elf_name = NULL;
+		char *demangled = NULL;
 
-			plt_offset += plt_entry_size;
-			symbols__insert(&dso->symbols, f);
-			++nr;
-		}
+		gelf_getsym(syms, get_rel_symidx(&ri, idx), &sym);
+
+		elf_name = elf_sym__name(&sym, symstrs);
+		demangled = demangle_sym(dso, 0, elf_name);
+		if (demangled)
+			elf_name = demangled;
+		if (*elf_name)
+			snprintf(sympltname, sizeof(sympltname), "%s@plt", elf_name);
+		else
+			snprintf(sympltname, sizeof(sympltname),
+				 "offset_%#" PRIx64 "@plt", plt_offset);
+		free(demangled);
+
+		f = symbol__new(plt_offset, plt_entry_size, STB_GLOBAL, STT_FUNC, sympltname);
+		if (!f)
+			goto out_elf_end;
+
+		plt_offset += plt_entry_size;
+		symbols__insert(&dso->symbols, f);
+		++nr;
 	}
 
 	err = 0;
-- 
2.34.1

Re: [PATCH 00/10] perf symbols: Improve dso__synthesize_plt_symbols()

[Arnaldo Carvalho de Melo]

Em Fri, Jan 20, 2023 at 02:34:46PM +0200, Adrian Hunter escreveu:
> Hi
> 
> This is the first of 2 patchsets to improve dso__synthesize_plt_symbols().
> This patchset is really preparation for the 2nd patchset, which focuses
> on getting rid of unknown symbols that show up in Intel PT traces.
> The 2nd patchset is still under development.
> 
> These patches are small and staightforward. Only the new Symbols test is
> slightly interesting because it provides a way to see what symbols
> perf discovers for any given dso. The test fails initially, but
> should pass after patch 7 "perf symbols: Add symbol for .plt header".

Looks nice, I've test build, one by one and used the test with a
different --dso, etc. I'll keep it today locally for further container
tests and to give time to others to look at it, I'll have it in
tmp.perf/core soon.

- Arnaldo
 
> 
> Adrian Hunter (10):
>       perf test: Add Symbols test
>       perf symbols: Factor out get_plt_sizes()
>       perf symbols: Check plt_entry_size is not zero
>       perf symbols: Add dso__find_symbol_nocache()
>       perf symbols: Slightly simplify 'err' usage in dso__synthesize_plt_symbols()
>       perf symbols: Do not check ss->dynsym twice
>       perf symbols: Add symbol for .plt header
>       perf symbols: Allow for .plt entries with no symbol
>       perf symbols: Combine handling for SHT_RELA and SHT_REL
>       perf symbols: Check SHT_RELA and SHT_REL type earlier
> 
>  tools/perf/Documentation/perf-test.txt |   3 +
>  tools/perf/tests/Build                 |   1 +
>  tools/perf/tests/builtin-test.c        |   3 +
>  tools/perf/tests/symbols.c             | 150 ++++++++++++++++++++++++++
>  tools/perf/tests/tests.h               |   3 +
>  tools/perf/util/symbol-elf.c           | 190 +++++++++++++++++----------------
>  tools/perf/util/symbol.c               |   5 +
>  tools/perf/util/symbol.h               |   1 +
>  8 files changed, 262 insertions(+), 94 deletions(-)
>  create mode 100644 tools/perf/tests/symbols.c
> 
> 
> Regards
> Adrian

-- 

- Arnaldo

Re: [PATCH 00/10] perf symbols: Improve dso__synthesize_plt_symbols()

[Ian Rogers]

On Fri, Jan 20, 2023 at 4:35 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> Hi
>
> This is the first of 2 patchsets to improve dso__synthesize_plt_symbols().
> This patchset is really preparation for the 2nd patchset, which focuses
> on getting rid of unknown symbols that show up in Intel PT traces.
> The 2nd patchset is still under development.
>
> These patches are small and staightforward. Only the new Symbols test is
> slightly interesting because it provides a way to see what symbols
> perf discovers for any given dso. The test fails initially, but
> should pass after patch 7 "perf symbols: Add symbol for .plt header".
>
>
> Adrian Hunter (10):
>       perf test: Add Symbols test
>       perf symbols: Factor out get_plt_sizes()
>       perf symbols: Check plt_entry_size is not zero
>       perf symbols: Add dso__find_symbol_nocache()
>       perf symbols: Slightly simplify 'err' usage in dso__synthesize_plt_symbols()
>       perf symbols: Do not check ss->dynsym twice
>       perf symbols: Add symbol for .plt header
>       perf symbols: Allow for .plt entries with no symbol
>       perf symbols: Combine handling for SHT_RELA and SHT_REL
>       perf symbols: Check SHT_RELA and SHT_REL type earlier

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

>  tools/perf/Documentation/perf-test.txt |   3 +
>  tools/perf/tests/Build                 |   1 +
>  tools/perf/tests/builtin-test.c        |   3 +
>  tools/perf/tests/symbols.c             | 150 ++++++++++++++++++++++++++
>  tools/perf/tests/tests.h               |   3 +
>  tools/perf/util/symbol-elf.c           | 190 +++++++++++++++++----------------
>  tools/perf/util/symbol.c               |   5 +
>  tools/perf/util/symbol.h               |   1 +
>  8 files changed, 262 insertions(+), 94 deletions(-)
>  create mode 100644 tools/perf/tests/symbols.c
>
>
> Regards
> Adrian

Re: [PATCH 00/10] perf symbols: Improve dso__synthesize_plt_symbols()

[Arnaldo Carvalho de Melo]

Em Sat, Jan 21, 2023 at 09:53:53AM -0800, Ian Rogers escreveu:
> On Fri, Jan 20, 2023 at 4:35 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > Hi
> >
> > This is the first of 2 patchsets to improve dso__synthesize_plt_symbols().
> > This patchset is really preparation for the 2nd patchset, which focuses
> > on getting rid of unknown symbols that show up in Intel PT traces.
> > The 2nd patchset is still under development.
> >
> > These patches are small and staightforward. Only the new Symbols test is
> > slightly interesting because it provides a way to see what symbols
> > perf discovers for any given dso. The test fails initially, but
> > should pass after patch 7 "perf symbols: Add symbol for .plt header".
> >
> >
> > Adrian Hunter (10):
> >       perf test: Add Symbols test
> >       perf symbols: Factor out get_plt_sizes()
> >       perf symbols: Check plt_entry_size is not zero
> >       perf symbols: Add dso__find_symbol_nocache()
> >       perf symbols: Slightly simplify 'err' usage in dso__synthesize_plt_symbols()
> >       perf symbols: Do not check ss->dynsym twice
> >       perf symbols: Add symbol for .plt header
> >       perf symbols: Allow for .plt entries with no symbol
> >       perf symbols: Combine handling for SHT_RELA and SHT_REL
> >       perf symbols: Check SHT_RELA and SHT_REL type earlier
 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, added to the tested patches, will push to perf/core shortly.

- Arnaldo