[PATCH v3 0/9] perf: Default use of build IDs and improvements

[PATCH v3 0/9] perf: Default use of build IDs and improvements

[Ian Rogers]

Build ID mmap2 events have been available since Linux v5.12 and avoid
certain races. Enable these by default as discussed in:
https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/

The dso_id is used to indentify a DSO that may change by being
overwritten. The inode generation isn't present in /proc/pid/maps and
so was already only optionally filled in. With build ID mmap events
the other major, minor and inode varialbes aren't filled in. Change
the dso_id implementation to make optional values explicit, rather
than injecting a dso_id we want to improve it during find operations,
add the buildid to the dso_id for sorting and so that matching fails
when build IDs vary between DSOs.

Mark the callchain for buildids and not just the sample IP, fixing
missing DSOs.

Fix sample__for_each_callchain_node to populate the map even when
symbols aren't computed.

Other minor bits of build_id clean up.

v3: Ensure the struct build_id is initialized empty prior to use as
    read paths may fail (Namhyung).

v2: Make marking DSOs still the default even with the defaulted build
    ID mmap. The command line option still disables this to avoid
    regressions. Add callchain patches and jitdump fix.

Ian Rogers (9):
  perf callchain: Always populate the addr_location map when adding IP
  perf build-id: Reduce size of "size" variable
  perf build-id: Truncate to avoid overflowing the build_id data
  perf build-id: Change sprintf functions to snprintf
  perf build-id: Mark DSO in sample callchains
  perf build-id: Ensure struct build_id is empty before use
  perf dso: Move build_id to dso_id
  perf jitdump: Directly mark the jitdump DSO
  perf record: Make --buildid-mmap the default

 tools/perf/bench/inject-buildid.c             |   2 +-
 tools/perf/builtin-buildid-cache.c            |  20 ++--
 tools/perf/builtin-buildid-list.c             |   6 +-
 tools/perf/builtin-inject.c                   |  36 +++---
 tools/perf/builtin-record.c                   |  33 ++++--
 tools/perf/builtin-report.c                   |  11 +-
 tools/perf/include/perf/perf_dlfilter.h       |   2 +-
 tools/perf/tests/pe-file-parsing.c            |   2 +-
 tools/perf/tests/sdt.c                        |   2 +-
 tools/perf/tests/symbols.c                    |   4 +-
 tools/perf/util/build-id.c                    |  63 ++++++----
 tools/perf/util/build-id.h                    |   8 +-
 tools/perf/util/debuginfo.c                   |   2 +-
 tools/perf/util/disasm.c                      |   2 +-
 tools/perf/util/dso.c                         | 111 ++++++++++--------
 tools/perf/util/dso.h                         |  75 ++++++------
 tools/perf/util/dsos.c                        |  20 ++--
 tools/perf/util/event.c                       |   2 +-
 tools/perf/util/header.c                      |   2 +-
 tools/perf/util/jitdump.c                     |  21 +++-
 tools/perf/util/machine.c                     |  34 +++---
 tools/perf/util/map.c                         |  15 ++-
 tools/perf/util/map.h                         |   5 +-
 tools/perf/util/probe-event.c                 |   3 +-
 tools/perf/util/probe-file.c                  |   4 +-
 tools/perf/util/probe-finder.c                |   3 +-
 .../scripting-engines/trace-event-python.c    |   7 +-
 tools/perf/util/sort.c                        |  27 +++--
 tools/perf/util/symbol-minimal.c              |   2 +-
 tools/perf/util/symbol.c                      |   7 +-
 tools/perf/util/symbol_conf.h                 |   2 +-
 tools/perf/util/synthetic-events.c            |  44 ++++---
 tools/perf/util/thread.c                      |   8 +-
 tools/perf/util/thread.h                      |   2 +-
 34 files changed, 337 insertions(+), 250 deletions(-)

-- 
2.49.0.901.g37484f566f-goog

[PATCH v3 1/9] perf callchain: Always populate the addr_location map when adding IP

[Ian Rogers]

Dropping symbols also meant the callchain maps wasn't populated, but
the callchain map is needed to find the DSO. Plumb the symbols option
better, falling back to thread__find_map rather than
thread__find_symbol when symbols are disabled.

Fixes: 02b2705017d2 ("perf callchain: Allow symbols to be optional when resolving a callchain")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/machine.c | 6 ++++--
 tools/perf/util/thread.c  | 8 ++++++--
 tools/perf/util/thread.h  | 2 +-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 2531b373f2cf..b048165b10c1 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1976,7 +1976,7 @@ static void ip__resolve_ams(struct thread *thread,
 	 * Thus, we have to try consecutively until we find a match
 	 * or else, the symbol is unknown
 	 */
-	thread__find_cpumode_addr_location(thread, ip, &al);
+	thread__find_cpumode_addr_location(thread, ip, /*symbols=*/true, &al);
 
 	ams->addr = ip;
 	ams->al_addr = al.addr;
@@ -2078,7 +2078,7 @@ static int add_callchain_ip(struct thread *thread,
 	al.sym = NULL;
 	al.srcline = NULL;
 	if (!cpumode) {
-		thread__find_cpumode_addr_location(thread, ip, &al);
+		thread__find_cpumode_addr_location(thread, ip, symbols, &al);
 	} else {
 		if (ip >= PERF_CONTEXT_MAX) {
 			switch (ip) {
@@ -2106,6 +2106,8 @@ static int add_callchain_ip(struct thread *thread,
 		}
 		if (symbols)
 			thread__find_symbol(thread, *cpumode, ip, &al);
+		else
+			thread__find_map(thread, *cpumode, ip, &al);
 	}
 
 	if (al.sym != NULL) {
diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
index 89585f53c1d5..10a01f8fbd40 100644
--- a/tools/perf/util/thread.c
+++ b/tools/perf/util/thread.c
@@ -410,7 +410,7 @@ int thread__fork(struct thread *thread, struct thread *parent, u64 timestamp, bo
 }
 
 void thread__find_cpumode_addr_location(struct thread *thread, u64 addr,
-					struct addr_location *al)
+					bool symbols, struct addr_location *al)
 {
 	size_t i;
 	const u8 cpumodes[] = {
@@ -421,7 +421,11 @@ void thread__find_cpumode_addr_location(struct thread *thread, u64 addr,
 	};
 
 	for (i = 0; i < ARRAY_SIZE(cpumodes); i++) {
-		thread__find_symbol(thread, cpumodes[i], addr, al);
+		if (symbols)
+			thread__find_symbol(thread, cpumodes[i], addr, al);
+		else
+			thread__find_map(thread, cpumodes[i], addr, al);
+
 		if (al->map)
 			break;
 	}
diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
index cd574a896418..2b90bbed7a61 100644
--- a/tools/perf/util/thread.h
+++ b/tools/perf/util/thread.h
@@ -126,7 +126,7 @@ struct symbol *thread__find_symbol_fb(struct thread *thread, u8 cpumode,
 				      u64 addr, struct addr_location *al);
 
 void thread__find_cpumode_addr_location(struct thread *thread, u64 addr,
-					struct addr_location *al);
+					bool symbols, struct addr_location *al);
 
 int thread__memcpy(struct thread *thread, struct machine *machine,
 		   void *buf, u64 ip, int len, bool *is64bit);
-- 
2.49.0.901.g37484f566f-goog

[PATCH v3 2/9] perf build-id: Reduce size of "size" variable

[Ian Rogers]

Later clean up of the dso_id to include a build_id will suffer from
alignment and size issues. The size can only hold up to a value of
BUILD_ID_SIZE (20) and the mmap2 event uses a byte for the value.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/build-id.h         | 2 +-
 tools/perf/util/synthetic-events.c | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
index a212497bfdb0..e3e0a446ff0c 100644
--- a/tools/perf/util/build-id.h
+++ b/tools/perf/util/build-id.h
@@ -13,7 +13,7 @@
 
 struct build_id {
 	u8	data[BUILD_ID_SIZE];
-	size_t	size;
+	u8	size;
 };
 
 struct dso;
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 2fc4d0537840..68bb7c5fe1b1 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -2248,7 +2248,9 @@ int perf_event__synthesize_build_id(const struct perf_tool *tool,
 
 	memset(&ev, 0, len);
 
-	ev.build_id.size = min(bid->size, sizeof(ev.build_id.build_id));
+	ev.build_id.size = bid->size;
+	if (ev.build_id.size > sizeof(ev.build_id.build_id))
+		ev.build_id.size = sizeof(ev.build_id.build_id);
 	memcpy(ev.build_id.build_id, bid->data, ev.build_id.size);
 	ev.build_id.header.type = PERF_RECORD_HEADER_BUILD_ID;
 	ev.build_id.header.misc = misc | PERF_RECORD_MISC_BUILD_ID_SIZE;
@@ -2308,7 +2310,9 @@ int perf_event__synthesize_mmap2_build_id(const struct perf_tool *tool,
 	ev.mmap2.len = len;
 	ev.mmap2.pgoff = pgoff;
 
-	ev.mmap2.build_id_size = min(bid->size, sizeof(ev.mmap2.build_id));
+	ev.mmap2.build_id_size = bid->size;
+	if (ev.mmap2.build_id_size > sizeof(ev.mmap2.build_id))
+		ev.build_id.size = sizeof(ev.mmap2.build_id);
 	memcpy(ev.mmap2.build_id, bid->data, ev.mmap2.build_id_size);
 
 	ev.mmap2.prot = prot;
-- 
2.49.0.901.g37484f566f-goog

[PATCH v3 3/9] perf build-id: Truncate to avoid overflowing the build_id data

[Ian Rogers]

Warning when the build_id data would be overflowed would lead to
memory corruption, switch to truncation.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/build-id.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index e763e8d99a43..5bc2040bdd0d 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -951,7 +951,10 @@ bool perf_session__read_build_ids(struct perf_session *session, bool with_hits)
 
 void build_id__init(struct build_id *bid, const u8 *data, size_t size)
 {
-	WARN_ON(size > BUILD_ID_SIZE);
+	if (size > BUILD_ID_SIZE) {
+		pr_debug("Truncating build_id size from %zd\n", size);
+		size = BUILD_ID_SIZE;
+	}
 	memcpy(bid->data, data, size);
 	bid->size = size;
 }
-- 
2.49.0.901.g37484f566f-goog

[PATCH v3 4/9] perf build-id: Change sprintf functions to snprintf

[Ian Rogers]

Pass in a size argument rather than implying all build id strings must
be SBUILD_ID_SIZE.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-buildid-cache.c            | 12 +++----
 tools/perf/builtin-buildid-list.c             |  6 ++--
 tools/perf/util/build-id.c                    | 33 ++++++++-----------
 tools/perf/util/build-id.h                    |  6 ++--
 tools/perf/util/disasm.c                      |  2 +-
 tools/perf/util/dso.c                         |  4 +--
 tools/perf/util/dsos.c                        |  2 +-
 tools/perf/util/event.c                       |  2 +-
 tools/perf/util/header.c                      |  2 +-
 tools/perf/util/map.c                         |  2 +-
 tools/perf/util/probe-file.c                  |  4 +--
 .../scripting-engines/trace-event-python.c    |  7 ++--
 tools/perf/util/symbol.c                      |  2 +-
 13 files changed, 38 insertions(+), 46 deletions(-)

diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index b0511d16aeb6..3f7739b21148 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -31,7 +31,7 @@
 #include <linux/string.h>
 #include <linux/err.h>
 
-static int build_id_cache__kcore_buildid(const char *proc_dir, char *sbuildid)
+static int build_id_cache__kcore_buildid(const char *proc_dir, char *sbuildid, size_t sbuildid_size)
 {
 	char root_dir[PATH_MAX];
 	char *p;
@@ -42,7 +42,7 @@ static int build_id_cache__kcore_buildid(const char *proc_dir, char *sbuildid)
 	if (!p)
 		return -1;
 	*p = '\0';
-	return sysfs__sprintf_build_id(root_dir, sbuildid);
+	return sysfs__snprintf_build_id(root_dir, sbuildid, sbuildid_size);
 }
 
 static int build_id_cache__kcore_dir(char *dir, size_t sz)
@@ -128,7 +128,7 @@ static int build_id_cache__add_kcore(const char *filename, bool force)
 		return -1;
 	*p = '\0';
 
-	if (build_id_cache__kcore_buildid(from_dir, sbuildid) < 0)
+	if (build_id_cache__kcore_buildid(from_dir, sbuildid, sizeof(sbuildid)) < 0)
 		return -1;
 
 	scnprintf(to_dir, sizeof(to_dir), "%s/%s/%s",
@@ -187,7 +187,7 @@ static int build_id_cache__add_file(const char *filename, struct nsinfo *nsi)
 		return -1;
 	}
 
-	build_id__sprintf(&bid, sbuild_id);
+	build_id__snprintf(&bid, sbuild_id, sizeof(sbuild_id));
 	err = build_id_cache__add_s(sbuild_id, filename, nsi,
 				    false, false);
 	pr_debug("Adding %s %s: %s\n", sbuild_id, filename,
@@ -211,7 +211,7 @@ static int build_id_cache__remove_file(const char *filename, struct nsinfo *nsi)
 		return -1;
 	}
 
-	build_id__sprintf(&bid, sbuild_id);
+	build_id__snprintf(&bid, sbuild_id, sizeof(sbuild_id));
 	err = build_id_cache__remove_s(sbuild_id);
 	pr_debug("Removing %s %s: %s\n", sbuild_id, filename,
 		 err ? "FAIL" : "Ok");
@@ -317,7 +317,7 @@ static int build_id_cache__update_file(const char *filename, struct nsinfo *nsi)
 	}
 	err = 0;
 
-	build_id__sprintf(&bid, sbuild_id);
+	build_id__snprintf(&bid, sbuild_id, sizeof(sbuild_id));
 	if (build_id_cache__cached(sbuild_id))
 		err = build_id_cache__remove_s(sbuild_id);
 
diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
index 52dfacaff8e3..ba8ba0303920 100644
--- a/tools/perf/builtin-buildid-list.c
+++ b/tools/perf/builtin-buildid-list.c
@@ -31,7 +31,7 @@ static int buildid__map_cb(struct map *map, void *arg __maybe_unused)
 
 	memset(bid_buf, 0, sizeof(bid_buf));
 	if (dso__has_build_id(dso))
-		build_id__sprintf(dso__bid_const(dso), bid_buf);
+		build_id__snprintf(dso__bid_const(dso), bid_buf, sizeof(bid_buf));
 	printf("%s %16" PRIx64 " %16" PRIx64, bid_buf, map__start(map), map__end(map));
 	if (dso_long_name != NULL)
 		printf(" %s", dso_long_name);
@@ -57,7 +57,7 @@ static int sysfs__fprintf_build_id(FILE *fp)
 	char sbuild_id[SBUILD_ID_SIZE];
 	int ret;
 
-	ret = sysfs__sprintf_build_id("/", sbuild_id);
+	ret = sysfs__snprintf_build_id("/", sbuild_id, sizeof(sbuild_id));
 	if (ret != sizeof(sbuild_id))
 		return ret < 0 ? ret : -EINVAL;
 
@@ -69,7 +69,7 @@ static int filename__fprintf_build_id(const char *name, FILE *fp)
 	char sbuild_id[SBUILD_ID_SIZE];
 	int ret;
 
-	ret = filename__sprintf_build_id(name, sbuild_id);
+	ret = filename__snprintf_build_id(name, sbuild_id, sizeof(sbuild_id));
 	if (ret != sizeof(sbuild_id))
 		return ret < 0 ? ret : -EINVAL;
 
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 5bc2040bdd0d..aa35dceace90 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -67,24 +67,17 @@ int build_id__mark_dso_hit(const struct perf_tool *tool __maybe_unused,
 	return 0;
 }
 
-int build_id__sprintf(const struct build_id *build_id, char *bf)
+int build_id__snprintf(const struct build_id *build_id, char *bf, size_t bf_size)
 {
-	char *bid = bf;
-	const u8 *raw = build_id->data;
-	size_t i;
-
-	bf[0] = 0x0;
+	size_t offs = 0;
 
-	for (i = 0; i < build_id->size; ++i) {
-		sprintf(bid, "%02x", *raw);
-		++raw;
-		bid += 2;
-	}
+	for (size_t i = 0; i < build_id->size && offs < bf_size; ++i)
+		offs += snprintf(bf + offs, bf_size - offs, "%02x", build_id->data[i]);
 
-	return (bid - bf) + 1;
+	return offs;
 }
 
-int sysfs__sprintf_build_id(const char *root_dir, char *sbuild_id)
+int sysfs__snprintf_build_id(const char *root_dir, char *sbuild_id, size_t sbuild_id_size)
 {
 	char notes[PATH_MAX];
 	struct build_id bid;
@@ -99,10 +92,10 @@ int sysfs__sprintf_build_id(const char *root_dir, char *sbuild_id)
 	if (ret < 0)
 		return ret;
 
-	return build_id__sprintf(&bid, sbuild_id);
+	return build_id__snprintf(&bid, sbuild_id, sbuild_id_size);
 }
 
-int filename__sprintf_build_id(const char *pathname, char *sbuild_id)
+int filename__snprintf_build_id(const char *pathname, char *sbuild_id, size_t sbuild_id_size)
 {
 	struct build_id bid;
 	int ret;
@@ -111,7 +104,7 @@ int filename__sprintf_build_id(const char *pathname, char *sbuild_id)
 	if (ret < 0)
 		return ret;
 
-	return build_id__sprintf(&bid, sbuild_id);
+	return build_id__snprintf(&bid, sbuild_id, sbuild_id_size);
 }
 
 /* asnprintf consolidates asprintf and snprintf */
@@ -212,9 +205,9 @@ static bool build_id_cache__valid_id(char *sbuild_id)
 		return false;
 
 	if (!strcmp(pathname, DSO__NAME_KALLSYMS))
-		ret = sysfs__sprintf_build_id("/", real_sbuild_id);
+		ret = sysfs__snprintf_build_id("/", real_sbuild_id, sizeof(real_sbuild_id));
 	else if (pathname[0] == '/')
-		ret = filename__sprintf_build_id(pathname, real_sbuild_id);
+		ret = filename__snprintf_build_id(pathname, real_sbuild_id, sizeof(real_sbuild_id));
 	else
 		ret = -EINVAL;	/* Should we support other special DSO cache? */
 	if (ret >= 0)
@@ -243,7 +236,7 @@ char *__dso__build_id_filename(const struct dso *dso, char *bf, size_t size,
 	if (!dso__has_build_id(dso))
 		return NULL;
 
-	build_id__sprintf(dso__bid_const(dso), sbuild_id);
+	build_id__snprintf(dso__bid_const(dso), sbuild_id, sizeof(sbuild_id));
 	linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
 	if (!linkname)
 		return NULL;
@@ -769,7 +762,7 @@ static int build_id_cache__add_b(const struct build_id *bid,
 {
 	char sbuild_id[SBUILD_ID_SIZE];
 
-	build_id__sprintf(bid, sbuild_id);
+	build_id__snprintf(bid, sbuild_id, sizeof(sbuild_id));
 
 	return __build_id_cache__add_s(sbuild_id, name, nsi, is_kallsyms,
 				       is_vdso, proper_name, root_dir);
diff --git a/tools/perf/util/build-id.h b/tools/perf/util/build-id.h
index e3e0a446ff0c..47e621cebe1b 100644
--- a/tools/perf/util/build-id.h
+++ b/tools/perf/util/build-id.h
@@ -21,10 +21,10 @@ struct feat_fd;
 struct nsinfo;
 
 void build_id__init(struct build_id *bid, const u8 *data, size_t size);
-int build_id__sprintf(const struct build_id *build_id, char *bf);
+int build_id__snprintf(const struct build_id *build_id, char *bf, size_t bf_size);
 bool build_id__is_defined(const struct build_id *bid);
-int sysfs__sprintf_build_id(const char *root_dir, char *sbuild_id);
-int filename__sprintf_build_id(const char *pathname, char *sbuild_id);
+int sysfs__snprintf_build_id(const char *root_dir, char *sbuild_id, size_t sbuild_id_size);
+int filename__snprintf_build_id(const char *pathname, char *sbuild_id, size_t sbuild_id_size);
 char *build_id_cache__kallsyms_path(const char *sbuild_id, char *bf,
 				    size_t size);
 
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 8f0eb56c6fc6..96e6a5d6eacc 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -1218,7 +1218,7 @@ int symbol__strerror_disassemble(struct map_symbol *ms, int errnum, char *buf, s
 		char *build_id_msg = NULL;
 
 		if (dso__has_build_id(dso)) {
-			build_id__sprintf(dso__bid(dso), bf + 15);
+			build_id__snprintf(dso__bid(dso), bf + 15, sizeof(bf) - 15);
 			build_id_msg = bf;
 		}
 		scnprintf(buf, buflen,
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 8619b6eea62d..776506b93b8b 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -217,7 +217,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
 			break;
 		}
 
-		build_id__sprintf(dso__bid_const(dso), build_id_hex);
+		build_id__snprintf(dso__bid_const(dso), build_id_hex, sizeof(build_id_hex));
 		len = __symbol__join_symfs(filename, size, "/usr/lib/debug/.build-id/");
 		snprintf(filename + len, size - len, "%.2s/%s.debug",
 			 build_id_hex, build_id_hex + 2);
@@ -1697,7 +1697,7 @@ static size_t dso__fprintf_buildid(struct dso *dso, FILE *fp)
 {
 	char sbuild_id[SBUILD_ID_SIZE];
 
-	build_id__sprintf(dso__bid(dso), sbuild_id);
+	build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
 	return fprintf(fp, "%s", sbuild_id);
 }
 
diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index e0998e2a7c4e..b2172632b3cd 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -370,7 +370,7 @@ static int dsos__fprintf_buildid_cb(struct dso *dso, void *data)
 
 	if (args->skip && args->skip(dso, args->parm))
 		return 0;
-	build_id__sprintf(dso__bid(dso), sbuild_id);
+	build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
 	args->ret += fprintf(args->fp, "%-40s %s\n", sbuild_id, dso__long_name(dso));
 	return 0;
 }
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index c23b77f8f854..34176050ebac 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -329,7 +329,7 @@ size_t perf_event__fprintf_mmap2(union perf_event *event, FILE *fp)
 
 		build_id__init(&bid, event->mmap2.build_id,
 			       event->mmap2.build_id_size);
-		build_id__sprintf(&bid, sbuild_id);
+		build_id__snprintf(&bid, sbuild_id, sizeof(sbuild_id));
 
 		return fprintf(fp, " %d/%d: [%#" PRI_lx64 "(%#" PRI_lx64 ") @ %#" PRI_lx64
 				   " <%s>]: %c%c%c%c %s\n",
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index e3cdc3b7b4ab..38e903307e8e 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -2291,7 +2291,7 @@ static int __event_process_build_id(struct perf_record_header_build_id *bev,
 			free(m.name);
 		}
 
-		build_id__sprintf(dso__bid(dso), sbuild_id);
+		build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
 		pr_debug("build id event received for %s: %s [%zu]\n",
 			 dso__long_name(dso), sbuild_id, size);
 		dso__put(dso);
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index d729438b7d65..0f6b185f9589 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -354,7 +354,7 @@ int map__load(struct map *map)
 		if (dso__has_build_id(dso)) {
 			char sbuild_id[SBUILD_ID_SIZE];
 
-			build_id__sprintf(dso__bid(dso), sbuild_id);
+			build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
 			pr_debug("%s with build id %s not found", name, sbuild_id);
 		} else
 			pr_debug("Failed to open %s", name);
diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index ec8ac242fedb..5069fb61f48c 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -448,10 +448,10 @@ static int probe_cache__open(struct probe_cache *pcache, const char *target,
 	if (!target || !strcmp(target, DSO__NAME_KALLSYMS)) {
 		target = DSO__NAME_KALLSYMS;
 		is_kallsyms = true;
-		ret = sysfs__sprintf_build_id("/", sbuildid);
+		ret = sysfs__snprintf_build_id("/", sbuildid, sizeof(sbuildid));
 	} else {
 		nsinfo__mountns_enter(nsi, &nsc);
-		ret = filename__sprintf_build_id(target, sbuildid);
+		ret = filename__snprintf_build_id(target, sbuildid, sizeof(sbuildid));
 		nsinfo__mountns_exit(&nsc);
 	}
 
diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
index 520729e78965..68e92552d954 100644
--- a/tools/perf/util/scripting-engines/trace-event-python.c
+++ b/tools/perf/util/scripting-engines/trace-event-python.c
@@ -780,14 +780,13 @@ static void set_sym_in_dict(PyObject *dict, struct addr_location *al,
 			    const char *sym_field, const char *symoff_field,
 			    const char *map_pgoff)
 {
-	char sbuild_id[SBUILD_ID_SIZE];
-
 	if (al->map) {
+		char sbuild_id[SBUILD_ID_SIZE];
 		struct dso *dso = map__dso(al->map);
 
 		pydict_set_item_string_decref(dict, dso_field,
 					      _PyUnicode_FromString(dso__name(dso)));
-		build_id__sprintf(dso__bid(dso), sbuild_id);
+		build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
 		pydict_set_item_string_decref(dict, dso_bid_field,
 			_PyUnicode_FromString(sbuild_id));
 		pydict_set_item_string_decref(dict, dso_map_start,
@@ -1238,7 +1237,7 @@ static int python_export_dso(struct db_export *dbe, struct dso *dso,
 	char sbuild_id[SBUILD_ID_SIZE];
 	PyObject *t;
 
-	build_id__sprintf(dso__bid(dso), sbuild_id);
+	build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
 
 	t = tuple_new(5);
 
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 11540219481b..30750ed313ca 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -2143,7 +2143,7 @@ static char *dso__find_kallsyms(struct dso *dso, struct map *map)
 			goto proc_kallsyms;
 	}
 
-	build_id__sprintf(dso__bid(dso), sbuild_id);
+	build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
 
 	/* Find kallsyms in build-id cache with kcore */
 	scnprintf(path, sizeof(path), "%s/%s/%s",
-- 
2.49.0.901.g37484f566f-goog

[PATCH v3 5/9] perf build-id: Mark DSO in sample callchains

[Ian Rogers]

Previously only the sample IP's map DSO would be marked hit for the
purposes of populating the build ID cache. Walk the call chain to mark
all IPs and DSOs.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/build-id.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index aa35dceace90..3386fa8e1e7e 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -42,10 +42,20 @@
 
 static bool no_buildid_cache;
 
+static int mark_dso_hit_callback(struct callchain_cursor_node *node, void *data __maybe_unused)
+{
+	struct map *map = node->ms.map;
+
+	if (map)
+		dso__set_hit(map__dso(map));
+
+	return 0;
+}
+
 int build_id__mark_dso_hit(const struct perf_tool *tool __maybe_unused,
 			   union perf_event *event,
 			   struct perf_sample *sample,
-			   struct evsel *evsel __maybe_unused,
+			   struct evsel *evsel,
 			   struct machine *machine)
 {
 	struct addr_location al;
@@ -63,6 +73,11 @@ int build_id__mark_dso_hit(const struct perf_tool *tool __maybe_unused,
 		dso__set_hit(map__dso(al.map));
 
 	addr_location__exit(&al);
+
+	sample__for_each_callchain_node(thread, evsel, sample, PERF_MAX_STACK_DEPTH,
+					/*symbols=*/false, mark_dso_hit_callback, /*data=*/NULL);
+
+
 	thread__put(thread);
 	return 0;
 }
-- 
2.49.0.901.g37484f566f-goog

Re: [PATCH v3 5/9] perf build-id: Mark DSO in sample callchains

[Namhyung Kim]

On Mon, Apr 28, 2025 at 02:34:04PM -0700, Ian Rogers wrote:
> Previously only the sample IP's map DSO would be marked hit for the
> purposes of populating the build ID cache. Walk the call chain to mark
> all IPs and DSOs.

I think this is correct, but I'm afraid it'd also increase the processing
time.  Do you happen to have any numbers?

Thanks,
Namhyung

> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/build-id.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> index aa35dceace90..3386fa8e1e7e 100644
> --- a/tools/perf/util/build-id.c
> +++ b/tools/perf/util/build-id.c
> @@ -42,10 +42,20 @@
>  
>  static bool no_buildid_cache;
>  
> +static int mark_dso_hit_callback(struct callchain_cursor_node *node, void *data __maybe_unused)
> +{
> +	struct map *map = node->ms.map;
> +
> +	if (map)
> +		dso__set_hit(map__dso(map));
> +
> +	return 0;
> +}
> +
>  int build_id__mark_dso_hit(const struct perf_tool *tool __maybe_unused,
>  			   union perf_event *event,
>  			   struct perf_sample *sample,
> -			   struct evsel *evsel __maybe_unused,
> +			   struct evsel *evsel,
>  			   struct machine *machine)
>  {
>  	struct addr_location al;
> @@ -63,6 +73,11 @@ int build_id__mark_dso_hit(const struct perf_tool *tool __maybe_unused,
>  		dso__set_hit(map__dso(al.map));
>  
>  	addr_location__exit(&al);
> +
> +	sample__for_each_callchain_node(thread, evsel, sample, PERF_MAX_STACK_DEPTH,
> +					/*symbols=*/false, mark_dso_hit_callback, /*data=*/NULL);
> +
> +
>  	thread__put(thread);
>  	return 0;
>  }
> -- 
> 2.49.0.901.g37484f566f-goog
> 

Re: [PATCH v3 5/9] perf build-id: Mark DSO in sample callchains

[Ian Rogers]

On Wed, May 28, 2025 at 1:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Apr 28, 2025 at 02:34:04PM -0700, Ian Rogers wrote:
> > Previously only the sample IP's map DSO would be marked hit for the
> > purposes of populating the build ID cache. Walk the call chain to mark
> > all IPs and DSOs.
>
> I think this is correct, but I'm afraid it'd also increase the processing
> time.  Do you happen to have any numbers?

It increases time spent processing the data file but to get a large
data file I had to run for multiple seconds and I struggled to get the
performance cost of this to be in the milliseconds (ie a tiny fraction
of the record time). Ultimately I found the change imperceptible and
couldn't think of a good command line to make it perceptible.

If the time is spent populating ~/.debug because more DSOs are marked
then this is fixing a bug and isn't a problem with the patch.

My personal opinion is that it is somewhat surprising `perf record` is
post-processing the perf.data file at all, and -B and -N would be my
expected defaults - just as --buildid-mmap implies --no-buildid (-B).
I didn't want to modify existing behaviors in these changes, however,
in this case I was just trying to make the existing behavior correct,
similar to fixing the same bug in `perf inject`.

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/build-id.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
> > index aa35dceace90..3386fa8e1e7e 100644
> > --- a/tools/perf/util/build-id.c
> > +++ b/tools/perf/util/build-id.c
> > @@ -42,10 +42,20 @@
> >
> >  static bool no_buildid_cache;
> >
> > +static int mark_dso_hit_callback(struct callchain_cursor_node *node, void *data __maybe_unused)
> > +{
> > +     struct map *map = node->ms.map;
> > +
> > +     if (map)
> > +             dso__set_hit(map__dso(map));
> > +
> > +     return 0;
> > +}
> > +
> >  int build_id__mark_dso_hit(const struct perf_tool *tool __maybe_unused,
> >                          union perf_event *event,
> >                          struct perf_sample *sample,
> > -                        struct evsel *evsel __maybe_unused,
> > +                        struct evsel *evsel,
> >                          struct machine *machine)
> >  {
> >       struct addr_location al;
> > @@ -63,6 +73,11 @@ int build_id__mark_dso_hit(const struct perf_tool *tool __maybe_unused,
> >               dso__set_hit(map__dso(al.map));
> >
> >       addr_location__exit(&al);
> > +
> > +     sample__for_each_callchain_node(thread, evsel, sample, PERF_MAX_STACK_DEPTH,
> > +                                     /*symbols=*/false, mark_dso_hit_callback, /*data=*/NULL);
> > +
> > +
> >       thread__put(thread);
> >       return 0;
> >  }
> > --
> > 2.49.0.901.g37484f566f-goog
> >

Re: [PATCH v3 5/9] perf build-id: Mark DSO in sample callchains

[Namhyung Kim]

On Wed, May 28, 2025 at 01:54:41PM -0700, Ian Rogers wrote:
> On Wed, May 28, 2025 at 1:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Apr 28, 2025 at 02:34:04PM -0700, Ian Rogers wrote:
> > > Previously only the sample IP's map DSO would be marked hit for the
> > > purposes of populating the build ID cache. Walk the call chain to mark
> > > all IPs and DSOs.
> >
> > I think this is correct, but I'm afraid it'd also increase the processing
> > time.  Do you happen to have any numbers?
> 
> It increases time spent processing the data file but to get a large
> data file I had to run for multiple seconds and I struggled to get the
> performance cost of this to be in the milliseconds (ie a tiny fraction
> of the record time). Ultimately I found the change imperceptible and
> couldn't think of a good command line to make it perceptible.

The worst case would be dwarf unwinding.  Maybe we can skip the
processing if it takes too long..

> 
> If the time is spent populating ~/.debug because more DSOs are marked
> then this is fixing a bug and isn't a problem with the patch.

Right, it's a good thing.

> 
> My personal opinion is that it is somewhat surprising `perf record` is
> post-processing the perf.data file at all, and -B and -N would be my
> expected defaults - just as --buildid-mmap implies --no-buildid (-B).

Otherwise nobody will run perf buildid-cache to add the info. :)


> I didn't want to modify existing behaviors in these changes, however,
> in this case I was just trying to make the existing behavior correct,
> similar to fixing the same bug in `perf inject`.

Thanks for your work,
Namhyung

Re: [PATCH v3 5/9] perf build-id: Mark DSO in sample callchains

[Ian Rogers]

On Wed, May 28, 2025 at 3:16 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, May 28, 2025 at 01:54:41PM -0700, Ian Rogers wrote:
> > On Wed, May 28, 2025 at 1:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > On Mon, Apr 28, 2025 at 02:34:04PM -0700, Ian Rogers wrote:
> > > > Previously only the sample IP's map DSO would be marked hit for the
> > > > purposes of populating the build ID cache. Walk the call chain to mark
> > > > all IPs and DSOs.
> > >
> > > I think this is correct, but I'm afraid it'd also increase the processing
> > > time.  Do you happen to have any numbers?
> >
> > It increases time spent processing the data file but to get a large
> > data file I had to run for multiple seconds and I struggled to get the
> > performance cost of this to be in the milliseconds (ie a tiny fraction
> > of the record time). Ultimately I found the change imperceptible and
> > couldn't think of a good command line to make it perceptible.
>
> The worst case would be dwarf unwinding.  Maybe we can skip the
> processing if it takes too long..

This doesn't sound unreasonable but is somewhat beyond the scope of
what I wanted to do here, which relates to migrating from inodes to
buildids as identifiers for DSOs. It would be useful to get a bug
report on this being too slow.

> >
> > If the time is spent populating ~/.debug because more DSOs are marked
> > then this is fixing a bug and isn't a problem with the patch.
>
> Right, it's a good thing.
>
> >
> > My personal opinion is that it is somewhat surprising `perf record` is
> > post-processing the perf.data file at all, and -B and -N would be my
> > expected defaults - just as --buildid-mmap implies --no-buildid (-B).
>
> Otherwise nobody will run perf buildid-cache to add the info. :)

Right, but we know it is high overhead as we run a number of tests with -B:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/record_sideband.sh?h=perf-tools-next#n25
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/test_arm_spe.sh?h=perf-tools-next#n94

I agree that populating the buildid cache immediately after perf
record minimizes a gap where DSOs may change prior to perf report. If
the DSO changes midway through perf record it doesn't help as the
buildid information is only computed at the end of perf record.

Isn't there an argument that because we have build IDs we don't care
about the record to report race any more as debuginfod can find the
prior version by means of the build ID? What I mean:

Current perf default way:
1) Run perf record with mmap2 inode rather the buildid data
1.1) To avoid any DSOs in the perf.data from not being available
populate the buildid header in the perf.data and the buildid cache at
the end of perf record
2) Replace some DSO that's got a sample in the perf.data with a different DSO
3) Run perf report
3.1) The DSO's buildid is known via the header and the buildid cache
already contains the DSO

Current way with -N:
1) Run perf record with mmap2 inode rather the buildid data
1.1) To ensure DSOs have buildid data populate the buildid header in
the perf.data  at the end of perf record
2) Replace some DSO that's got a sample in the perf.data with a different DSO
3) Run perf report
3.1) The DSO's buildid is known via the header, debuginfod can
populate the buildid cache as needed

With --buildid-mmap:
1) Run perf record with mmap2 buildid data
1.1) No need to post process file to gather buildids
2) Replace some DSO that's got a sample in the perf.data with a different DSO
3) Run perf report
3.1) The DSO's buildid is known via the mmap2 events, debuginfod can
populate the buildid cache as needed

With the current way and the current way with -N there is a race with
the DSO changing midway through perf record. The buildid mmap closes
the race.
With -N and --buildid-mmap the buildid cache is populated based on use
by a tool trying to read the DSO, rather than ahead of time at the end
of perf record.

Is the risk of the race that much of an issue? I'm not sure, that's
why I'd say default to using -B (if we didn't switch to buildid mmaps
by default, but this series does that). You could opt into to covering
the race by adding a flag so the data is processed at the end of perf
record. You could use perf inject to add the build IDs.

As you say there's the cost at the end of perf record and I'm not sure
it is worth it, which is why I'd expect the default to be to opt into
having the cost. With --buildid-mmap I'm not seeing a race to cover
but this series populates the buildid cache as I know you've argued
that perf record should do this by default.

Thanks,
Ian

> > I didn't want to modify existing behaviors in these changes, however,
> > in this case I was just trying to make the existing behavior correct,
> > similar to fixing the same bug in `perf inject`.
>
> Thanks for your work,
> Namhyung
>

Re: [PATCH v3 5/9] perf build-id: Mark DSO in sample callchains

[Namhyung Kim]

On Wed, May 28, 2025 at 04:11:13PM -0700, Ian Rogers wrote:
> On Wed, May 28, 2025 at 3:16 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Wed, May 28, 2025 at 01:54:41PM -0700, Ian Rogers wrote:
> > > On Wed, May 28, 2025 at 1:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > On Mon, Apr 28, 2025 at 02:34:04PM -0700, Ian Rogers wrote:
> > > > > Previously only the sample IP's map DSO would be marked hit for the
> > > > > purposes of populating the build ID cache. Walk the call chain to mark
> > > > > all IPs and DSOs.
> > > >
> > > > I think this is correct, but I'm afraid it'd also increase the processing
> > > > time.  Do you happen to have any numbers?
> > >
> > > It increases time spent processing the data file but to get a large
> > > data file I had to run for multiple seconds and I struggled to get the
> > > performance cost of this to be in the milliseconds (ie a tiny fraction
> > > of the record time). Ultimately I found the change imperceptible and
> > > couldn't think of a good command line to make it perceptible.
> >
> > The worst case would be dwarf unwinding.  Maybe we can skip the
> > processing if it takes too long..
> 
> This doesn't sound unreasonable but is somewhat beyond the scope of
> what I wanted to do here, which relates to migrating from inodes to
> buildids as identifiers for DSOs. It would be useful to get a bug
> report on this being too slow.
> 
> > >
> > > If the time is spent populating ~/.debug because more DSOs are marked
> > > then this is fixing a bug and isn't a problem with the patch.
> >
> > Right, it's a good thing.
> >
> > >
> > > My personal opinion is that it is somewhat surprising `perf record` is
> > > post-processing the perf.data file at all, and -B and -N would be my
> > > expected defaults - just as --buildid-mmap implies --no-buildid (-B).
> >
> > Otherwise nobody will run perf buildid-cache to add the info. :)
> 
> Right, but we know it is high overhead as we run a number of tests with -B:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/record_sideband.sh?h=perf-tools-next#n25
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/shell/test_arm_spe.sh?h=perf-tools-next#n94
> 
> I agree that populating the buildid cache immediately after perf
> record minimizes a gap where DSOs may change prior to perf report. If
> the DSO changes midway through perf record it doesn't help as the
> buildid information is only computed at the end of perf record.
> 
> Isn't there an argument that because we have build IDs we don't care
> about the record to report race any more as debuginfod can find the
> prior version by means of the build ID? What I mean:
> 
> Current perf default way:
> 1) Run perf record with mmap2 inode rather the buildid data
> 1.1) To avoid any DSOs in the perf.data from not being available
> populate the buildid header in the perf.data and the buildid cache at
> the end of perf record
> 2) Replace some DSO that's got a sample in the perf.data with a different DSO
> 3) Run perf report
> 3.1) The DSO's buildid is known via the header and the buildid cache
> already contains the DSO
> 
> Current way with -N:
> 1) Run perf record with mmap2 inode rather the buildid data
> 1.1) To ensure DSOs have buildid data populate the buildid header in
> the perf.data  at the end of perf record
> 2) Replace some DSO that's got a sample in the perf.data with a different DSO
> 3) Run perf report
> 3.1) The DSO's buildid is known via the header, debuginfod can
> populate the buildid cache as needed
> 
> With --buildid-mmap:
> 1) Run perf record with mmap2 buildid data
> 1.1) No need to post process file to gather buildids
> 2) Replace some DSO that's got a sample in the perf.data with a different DSO
> 3) Run perf report
> 3.1) The DSO's buildid is known via the mmap2 events, debuginfod can
> populate the buildid cache as needed
> 
> With the current way and the current way with -N there is a race with
> the DSO changing midway through perf record. The buildid mmap closes
> the race.
> With -N and --buildid-mmap the buildid cache is populated based on use
> by a tool trying to read the DSO, rather than ahead of time at the end
> of perf record.
> 
> Is the risk of the race that much of an issue? I'm not sure, that's
> why I'd say default to using -B (if we didn't switch to buildid mmaps
> by default, but this series does that). You could opt into to covering
> the race by adding a flag so the data is processed at the end of perf
> record. You could use perf inject to add the build IDs.
> 
> As you say there's the cost at the end of perf record and I'm not sure
> it is worth it, which is why I'd expect the default to be to opt into
> having the cost. With --buildid-mmap I'm not seeing a race to cover
> but this series populates the buildid cache as I know you've argued
> that perf record should do this by default.

Yeah, I just wanted to keep the original behavior to save the used
binaries in the build-ID cache.  But we can use debuginfod if it's
available as you said.

I think it's a separate topic whether we prefer the build-ID cache or
debuginfod.  We should be able to change the default behavior with a
config option later.  So let's keep the caching for now.

Thanks,
Namhyung

[PATCH v3 6/9] perf build-id: Ensure struct build_id is empty before use

[Ian Rogers]

If a build ID is read then not all code paths may ensure it is empty
before use. Initialize the build_id to be zero-ed unless there is
clear initialization such as a call to build_id__init.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/bench/inject-buildid.c  | 2 +-
 tools/perf/builtin-buildid-cache.c | 8 ++++----
 tools/perf/tests/pe-file-parsing.c | 2 +-
 tools/perf/tests/sdt.c             | 2 +-
 tools/perf/util/build-id.c         | 6 +++---
 tools/perf/util/debuginfo.c        | 2 +-
 tools/perf/util/probe-event.c      | 3 ++-
 tools/perf/util/probe-finder.c     | 3 ++-
 tools/perf/util/symbol-minimal.c   | 2 +-
 tools/perf/util/symbol.c           | 5 +++--
 tools/perf/util/synthetic-events.c | 2 +-
 11 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/tools/perf/bench/inject-buildid.c b/tools/perf/bench/inject-buildid.c
index f55c07e4be94..aad572a78d7f 100644
--- a/tools/perf/bench/inject-buildid.c
+++ b/tools/perf/bench/inject-buildid.c
@@ -80,7 +80,7 @@ static int add_dso(const char *fpath, const struct stat *sb __maybe_unused,
 		   int typeflag, struct FTW *ftwbuf __maybe_unused)
 {
 	struct bench_dso *dso = &dsos[nr_dsos];
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 
 	if (typeflag == FTW_D || typeflag == FTW_SL)
 		return 0;
diff --git a/tools/perf/builtin-buildid-cache.c b/tools/perf/builtin-buildid-cache.c
index 3f7739b21148..e936a34b7d37 100644
--- a/tools/perf/builtin-buildid-cache.c
+++ b/tools/perf/builtin-buildid-cache.c
@@ -175,7 +175,7 @@ static int build_id_cache__add_kcore(const char *filename, bool force)
 static int build_id_cache__add_file(const char *filename, struct nsinfo *nsi)
 {
 	char sbuild_id[SBUILD_ID_SIZE];
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	int err;
 	struct nscookie nsc;
 
@@ -198,7 +198,7 @@ static int build_id_cache__add_file(const char *filename, struct nsinfo *nsi)
 static int build_id_cache__remove_file(const char *filename, struct nsinfo *nsi)
 {
 	char sbuild_id[SBUILD_ID_SIZE];
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	struct nscookie nsc;
 
 	int err;
@@ -275,7 +275,7 @@ static int build_id_cache__purge_all(void)
 static bool dso__missing_buildid_cache(struct dso *dso, int parm __maybe_unused)
 {
 	char filename[PATH_MAX];
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 
 	if (!dso__build_id_filename(dso, filename, sizeof(filename), false))
 		return true;
@@ -303,7 +303,7 @@ static int build_id_cache__fprintf_missing(struct perf_session *session, FILE *f
 static int build_id_cache__update_file(const char *filename, struct nsinfo *nsi)
 {
 	char sbuild_id[SBUILD_ID_SIZE];
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	struct nscookie nsc;
 
 	int err;
diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
index fff58b220c07..30c7da79e109 100644
--- a/tools/perf/tests/pe-file-parsing.c
+++ b/tools/perf/tests/pe-file-parsing.c
@@ -24,7 +24,7 @@ static int run_dir(const char *d)
 {
 	char filename[PATH_MAX];
 	char debugfile[PATH_MAX];
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	char debuglink[PATH_MAX];
 	char expect_build_id[] = {
 		0x5a, 0x0f, 0xd8, 0x82, 0xb5, 0x30, 0x84, 0x22,
diff --git a/tools/perf/tests/sdt.c b/tools/perf/tests/sdt.c
index 919712899251..de4c7916b40d 100644
--- a/tools/perf/tests/sdt.c
+++ b/tools/perf/tests/sdt.c
@@ -28,7 +28,7 @@ static int target_function(void)
 static int build_id_cache__add_file(const char *filename)
 {
 	char sbuild_id[SBUILD_ID_SIZE];
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	int err;
 
 	err = filename__read_build_id(filename, &bid);
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 3386fa8e1e7e..1abd5a670665 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -95,7 +95,7 @@ int build_id__snprintf(const struct build_id *build_id, char *bf, size_t bf_size
 int sysfs__snprintf_build_id(const char *root_dir, char *sbuild_id, size_t sbuild_id_size)
 {
 	char notes[PATH_MAX];
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	int ret;
 
 	if (!root_dir)
@@ -112,7 +112,7 @@ int sysfs__snprintf_build_id(const char *root_dir, char *sbuild_id, size_t sbuil
 
 int filename__snprintf_build_id(const char *pathname, char *sbuild_id, size_t sbuild_id_size)
 {
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	int ret;
 
 	ret = filename__read_build_id(pathname, &bid);
@@ -849,7 +849,7 @@ static int filename__read_build_id_ns(const char *filename,
 
 static bool dso__build_id_mismatch(struct dso *dso, const char *name)
 {
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	bool ret = false;
 
 	mutex_lock(dso__lock(dso));
diff --git a/tools/perf/util/debuginfo.c b/tools/perf/util/debuginfo.c
index b5deea7cbdf2..a44c70f93156 100644
--- a/tools/perf/util/debuginfo.c
+++ b/tools/perf/util/debuginfo.c
@@ -103,7 +103,7 @@ struct debuginfo *debuginfo__new(const char *path)
 	char buf[PATH_MAX], nil = '\0';
 	struct dso *dso;
 	struct debuginfo *dinfo = NULL;
-	struct build_id bid;
+	struct build_id bid = { .size = 0};
 
 	/* Try to open distro debuginfo files */
 	dso = dso__new(path);
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 307ad6242a4e..3c87aa6f7d3b 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -1063,7 +1063,6 @@ static int sprint_line_description(char *sbuf, size_t size, struct line_range *l
 static int __show_line_range(struct line_range *lr, const char *module,
 			     bool user)
 {
-	struct build_id bid;
 	int l = 1;
 	struct int_node *ln;
 	struct debuginfo *dinfo;
@@ -1088,6 +1087,8 @@ static int __show_line_range(struct line_range *lr, const char *module,
 			ret = -ENOENT;
 	}
 	if (dinfo->build_id) {
+		struct build_id bid;
+
 		build_id__init(&bid, dinfo->build_id, BUILD_ID_SIZE);
 		build_id__sprintf(&bid, sbuild_id);
 	}
diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
index 3cc7c40f5097..1bbb91758316 100644
--- a/tools/perf/util/probe-finder.c
+++ b/tools/perf/util/probe-finder.c
@@ -848,7 +848,6 @@ static int probe_point_lazy_walker(const char *fname, int lineno,
 /* Find probe points from lazy pattern  */
 static int find_probe_point_lazy(Dwarf_Die *sp_die, struct probe_finder *pf)
 {
-	struct build_id bid;
 	char sbuild_id[SBUILD_ID_SIZE] = "";
 	int ret = 0;
 	char *fpath;
@@ -858,6 +857,8 @@ static int find_probe_point_lazy(Dwarf_Die *sp_die, struct probe_finder *pf)
 
 		comp_dir = cu_get_comp_dir(&pf->cu_die);
 		if (pf->dbg->build_id) {
+			struct build_id bid;
+
 			build_id__init(&bid, pf->dbg->build_id, BUILD_ID_SIZE);
 			build_id__sprintf(&bid, sbuild_id);
 		}
diff --git a/tools/perf/util/symbol-minimal.c b/tools/perf/util/symbol-minimal.c
index c6f369b5d893..6e3d79730538 100644
--- a/tools/perf/util/symbol-minimal.c
+++ b/tools/perf/util/symbol-minimal.c
@@ -343,7 +343,7 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 		  struct symsrc *runtime_ss __maybe_unused,
 		  int kmodule __maybe_unused)
 {
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	int ret;
 
 	ret = fd__is_64_bit(ss->fd);
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 30750ed313ca..41420d5f4001 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1804,7 +1804,6 @@ int dso__load(struct dso *dso, struct map *map)
 	struct symsrc *syms_ss = NULL, *runtime_ss = NULL;
 	bool kmod;
 	bool perfmap;
-	struct build_id bid;
 	struct nscookie nsc;
 	char newmapname[PATH_MAX];
 	const char *map_path = dso__long_name(dso);
@@ -1865,6 +1864,8 @@ int dso__load(struct dso *dso, struct map *map)
 	 */
 	if (!dso__has_build_id(dso) &&
 	    is_regular_file(dso__long_name(dso))) {
+		struct build_id bid = { .size = 0, };
+
 		__symbol__join_symfs(name, PATH_MAX, dso__long_name(dso));
 		if (filename__read_build_id(name, &bid) > 0)
 			dso__set_build_id(dso, &bid);
@@ -2113,7 +2114,7 @@ static bool filename__readable(const char *file)
 
 static char *dso__find_kallsyms(struct dso *dso, struct map *map)
 {
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	char sbuild_id[SBUILD_ID_SIZE];
 	bool is_host = false;
 	char path[PATH_MAX];
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 68bb7c5fe1b1..b7ebee95536f 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -368,7 +368,7 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
 					     struct machine *machine,
 					     bool is_kernel)
 {
-	struct build_id bid;
+	struct build_id bid = { .size = 0, };
 	struct nsinfo *nsi;
 	struct nscookie nc;
 	struct dso *dso = NULL;
-- 
2.49.0.901.g37484f566f-goog

[PATCH v3 7/9] perf dso: Move build_id to dso_id

[Ian Rogers]

The dso_id previously contained the major, minor, inode and inode
generation information from a mmap2 event - the inode generation would
be zero when reading from /proc/pid/maps. The build_id was in the
dso. With build ID mmap2 events these fields wouldn't be initialized
which would largely mean the special empty case where any dso would
match for equality. This isn't desirable as if a dso is replaced we
want the comparison to yield a difference.

To support detecting the difference between DSOs based on build_id,
move the build_id out of the DSO and into the dso_id. The dso_id is
also stored in the DSO so nothing is lost. Capture in the dso_id what
parts have been initialized and rename dso_id__inject to
dso_id__improve_id so that it is clear the dso_id is being improved
upon with additional information. With the build_id in the dso_id, use
memcmp to compare for equality.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-buildid-list.c       |   2 +-
 tools/perf/builtin-inject.c             |  36 ++++----
 tools/perf/builtin-report.c             |  11 ++-
 tools/perf/include/perf/perf_dlfilter.h |   2 +-
 tools/perf/tests/symbols.c              |   4 +-
 tools/perf/util/build-id.c              |   4 +-
 tools/perf/util/dso.c                   | 109 +++++++++++++-----------
 tools/perf/util/dso.h                   |  75 ++++++++--------
 tools/perf/util/dsos.c                  |  18 ++--
 tools/perf/util/machine.c               |  28 +++---
 tools/perf/util/map.c                   |  13 ++-
 tools/perf/util/map.h                   |   5 +-
 tools/perf/util/sort.c                  |  27 +++---
 tools/perf/util/synthetic-events.c      |  18 ++--
 14 files changed, 197 insertions(+), 155 deletions(-)

diff --git a/tools/perf/builtin-buildid-list.c b/tools/perf/builtin-buildid-list.c
index ba8ba0303920..151cd84b6dfe 100644
--- a/tools/perf/builtin-buildid-list.c
+++ b/tools/perf/builtin-buildid-list.c
@@ -31,7 +31,7 @@ static int buildid__map_cb(struct map *map, void *arg __maybe_unused)
 
 	memset(bid_buf, 0, sizeof(bid_buf));
 	if (dso__has_build_id(dso))
-		build_id__snprintf(dso__bid_const(dso), bid_buf, sizeof(bid_buf));
+		build_id__snprintf(dso__bid(dso), bid_buf, sizeof(bid_buf));
 	printf("%s %16" PRIx64 " %16" PRIx64, bid_buf, map__start(map), map__end(map));
 	if (dso_long_name != NULL)
 		printf(" %s", dso_long_name);
diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 11e49cafa3af..bdd51fc1685c 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -587,15 +587,17 @@ static int perf_event__repipe_mmap2(const struct perf_tool *tool,
 				struct perf_sample *sample,
 				struct machine *machine)
 {
-	struct dso_id id;
-	struct dso_id *dso_id = NULL;
+	struct dso_id id = dso_id_empty;
 
-	if (!(event->header.misc & PERF_RECORD_MISC_MMAP_BUILD_ID)) {
+	if (event->header.misc & PERF_RECORD_MISC_MMAP_BUILD_ID) {
+		build_id__init(&id.build_id, event->mmap2.build_id, event->mmap2.build_id_size);
+	} else {
 		id.maj = event->mmap2.maj;
 		id.min = event->mmap2.min;
 		id.ino = event->mmap2.ino;
 		id.ino_generation = event->mmap2.ino_generation;
-		dso_id = &id;
+		id.mmap2_valid = true;
+		id.mmap2_ino_generation_valid = true;
 	}
 
 	return perf_event__repipe_common_mmap(
@@ -603,7 +605,7 @@ static int perf_event__repipe_mmap2(const struct perf_tool *tool,
 		event->mmap2.pid, event->mmap2.tid,
 		event->mmap2.start, event->mmap2.len, event->mmap2.pgoff,
 		event->mmap2.flags, event->mmap2.prot,
-		event->mmap2.filename, dso_id,
+		event->mmap2.filename, &id,
 		perf_event__process_mmap2);
 }
 
@@ -671,19 +673,20 @@ static int perf_event__repipe_tracing_data(struct perf_session *session,
 static int dso__read_build_id(struct dso *dso)
 {
 	struct nscookie nsc;
+	struct build_id bid = { .size = 0, };
 
 	if (dso__has_build_id(dso))
 		return 0;
 
 	mutex_lock(dso__lock(dso));
 	nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
-	if (filename__read_build_id(dso__long_name(dso), dso__bid(dso)) > 0)
-		dso__set_has_build_id(dso);
+	if (filename__read_build_id(dso__long_name(dso), &bid) > 0)
+		dso__set_build_id(dso, &bid);
 	else if (dso__nsinfo(dso)) {
 		char *new_name = dso__filename_with_chroot(dso, dso__long_name(dso));
 
-		if (new_name && filename__read_build_id(new_name, dso__bid(dso)) > 0)
-			dso__set_has_build_id(dso);
+		if (new_name && filename__read_build_id(new_name, &bid) > 0)
+			dso__set_build_id(dso, &bid);
 		free(new_name);
 	}
 	nsinfo__mountns_exit(&nsc);
@@ -732,23 +735,26 @@ static bool perf_inject__lookup_known_build_id(struct perf_inject *inject,
 					       struct dso *dso)
 {
 	struct str_node *pos;
-	int bid_len;
 
 	strlist__for_each_entry(pos, inject->known_build_ids) {
+		struct build_id bid;
 		const char *build_id, *dso_name;
+		size_t bid_len;
 
 		build_id = skip_spaces(pos->s);
 		dso_name = strchr(build_id, ' ');
 		bid_len = dso_name - pos->s;
+		if (bid_len > sizeof(bid.data))
+			bid_len = sizeof(bid.data);
 		dso_name = skip_spaces(dso_name);
 		if (strcmp(dso__long_name(dso), dso_name))
 			continue;
-		for (int ix = 0; 2 * ix + 1 < bid_len; ++ix) {
-			dso__bid(dso)->data[ix] = (hex(build_id[2 * ix]) << 4 |
-						  hex(build_id[2 * ix + 1]));
+		for (size_t ix = 0; 2 * ix + 1 < bid_len; ++ix) {
+			bid.data[ix] = (hex(build_id[2 * ix]) << 4 |
+					hex(build_id[2 * ix + 1]));
 		}
-		dso__bid(dso)->size = bid_len / 2;
-		dso__set_has_build_id(dso);
+		bid.size = bid_len / 2;
+		dso__set_build_id(dso, &bid);
 		return true;
 	}
 	return false;
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index f0299c7ee025..98b1f73c28da 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -858,17 +858,24 @@ static int maps__fprintf_task_cb(struct map *map, void *data)
 	struct maps__fprintf_task_args *args = data;
 	const struct dso *dso = map__dso(map);
 	u32 prot = map__prot(map);
+	const struct dso_id *dso_id = dso__id_const(dso);
 	int ret;
+	char buf[SBUILD_ID_SIZE];
+
+	if (dso_id->mmap2_valid)
+		snprintf(buf, sizeof(buf), "%" PRIu64, dso_id->ino);
+	else
+		build_id__snprintf(&dso_id->build_id, buf, sizeof(buf));
 
 	ret = fprintf(args->fp,
-		"%*s  %" PRIx64 "-%" PRIx64 " %c%c%c%c %08" PRIx64 " %" PRIu64 " %s\n",
+		"%*s  %" PRIx64 "-%" PRIx64 " %c%c%c%c %08" PRIx64 " %s %s\n",
 		args->indent, "", map__start(map), map__end(map),
 		prot & PROT_READ ? 'r' : '-',
 		prot & PROT_WRITE ? 'w' : '-',
 		prot & PROT_EXEC ? 'x' : '-',
 		map__flags(map) ? 's' : 'p',
 		map__pgoff(map),
-		dso__id_const(dso)->ino, dso__name(dso));
+		buf, dso__name(dso));
 
 	if (ret < 0)
 		return ret;
diff --git a/tools/perf/include/perf/perf_dlfilter.h b/tools/perf/include/perf/perf_dlfilter.h
index 16fc4568ac53..2d3540ed3c58 100644
--- a/tools/perf/include/perf/perf_dlfilter.h
+++ b/tools/perf/include/perf/perf_dlfilter.h
@@ -87,7 +87,7 @@ struct perf_dlfilter_al {
 	__u8  is_64_bit; /* Only valid if dso is not NULL */
 	__u8  is_kernel_ip; /* True if in kernel space */
 	__u32 buildid_size;
-	__u8 *buildid;
+	const __u8 *buildid;
 	/* Below members are only populated by resolve_ip() */
 	__u8 filtered; /* True if this sample event will be filtered out */
 	const char *comm;
diff --git a/tools/perf/tests/symbols.c b/tools/perf/tests/symbols.c
index ee20a366f32f..b07fdf831868 100644
--- a/tools/perf/tests/symbols.c
+++ b/tools/perf/tests/symbols.c
@@ -96,8 +96,8 @@ static int create_map(struct test_info *ti, char *filename, struct map **map_p)
 	dso__put(dso);
 
 	/* Create a dummy map at 0x100000 */
-	*map_p = map__new(ti->machine, 0x100000, 0xffffffff, 0, NULL,
-			  PROT_EXEC, 0, NULL, filename, ti->thread);
+	*map_p = map__new(ti->machine, 0x100000, 0xffffffff, 0, &dso_id_empty,
+			  PROT_EXEC, /*flags=*/0, filename, ti->thread);
 	if (!*map_p) {
 		pr_debug("Failed to create map!");
 		return TEST_FAIL;
diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c
index 1abd5a670665..e2b295fe4d2f 100644
--- a/tools/perf/util/build-id.c
+++ b/tools/perf/util/build-id.c
@@ -251,7 +251,7 @@ char *__dso__build_id_filename(const struct dso *dso, char *bf, size_t size,
 	if (!dso__has_build_id(dso))
 		return NULL;
 
-	build_id__snprintf(dso__bid_const(dso), sbuild_id, sizeof(sbuild_id));
+	build_id__snprintf(dso__bid(dso), sbuild_id, sizeof(sbuild_id));
 	linkname = build_id_cache__linkname(sbuild_id, NULL, 0);
 	if (!linkname)
 		return NULL;
@@ -334,7 +334,7 @@ static int machine__write_buildid_table_cb(struct dso *dso, void *data)
 	}
 
 	in_kernel = dso__kernel(dso) || is_kernel_module(name, PERF_RECORD_MISC_CPUMODE_UNKNOWN);
-	return write_buildid(name, name_len, dso__bid(dso), args->machine->pid,
+	return write_buildid(name, name_len, &dso__id(dso)->build_id, args->machine->pid,
 			     in_kernel ? args->kmisc : args->umisc, args->fd);
 }
 
diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index 776506b93b8b..075f74c1bdf0 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -217,7 +217,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
 			break;
 		}
 
-		build_id__snprintf(dso__bid_const(dso), build_id_hex, sizeof(build_id_hex));
+		build_id__snprintf(dso__bid(dso), build_id_hex, sizeof(build_id_hex));
 		len = __symbol__join_symfs(filename, size, "/usr/lib/debug/.build-id/");
 		snprintf(filename + len, size - len, "%.2s/%s.debug",
 			 build_id_hex, build_id_hex + 2);
@@ -1379,64 +1379,76 @@ static void dso__set_long_name_id(struct dso *dso, const char *name, bool name_a
 
 static int __dso_id__cmp(const struct dso_id *a, const struct dso_id *b)
 {
-	if (a->maj > b->maj) return -1;
-	if (a->maj < b->maj) return 1;
+	if (a->mmap2_valid && b->mmap2_valid) {
+		if (a->maj > b->maj) return -1;
+		if (a->maj < b->maj) return 1;
 
-	if (a->min > b->min) return -1;
-	if (a->min < b->min) return 1;
+		if (a->min > b->min) return -1;
+		if (a->min < b->min) return 1;
 
-	if (a->ino > b->ino) return -1;
-	if (a->ino < b->ino) return 1;
-
-	/*
-	 * Synthesized MMAP events have zero ino_generation, avoid comparing
-	 * them with MMAP events with actual ino_generation.
-	 *
-	 * I found it harmful because the mismatch resulted in a new
-	 * dso that did not have a build ID whereas the original dso did have a
-	 * build ID. The build ID was essential because the object was not found
-	 * otherwise. - Adrian
-	 */
-	if (a->ino_generation && b->ino_generation) {
+		if (a->ino > b->ino) return -1;
+		if (a->ino < b->ino) return 1;
+	}
+	if (a->mmap2_ino_generation_valid && b->mmap2_ino_generation_valid) {
 		if (a->ino_generation > b->ino_generation) return -1;
 		if (a->ino_generation < b->ino_generation) return 1;
 	}
-
+	if (build_id__is_defined(&a->build_id) && build_id__is_defined(&b->build_id)) {
+		if (a->build_id.size != b->build_id.size)
+			return a->build_id.size < b->build_id.size ? -1 : 1;
+		return memcmp(a->build_id.data, b->build_id.data, a->build_id.size);
+	}
 	return 0;
 }
 
-bool dso_id__empty(const struct dso_id *id)
-{
-	if (!id)
-		return true;
-
-	return !id->maj && !id->min && !id->ino && !id->ino_generation;
-}
+const struct dso_id dso_id_empty = {
+	{
+		.maj = 0,
+		.min = 0,
+		.ino = 0,
+		.ino_generation = 0,
+	},
+	.mmap2_valid = false,
+	.mmap2_ino_generation_valid = false,
+	{
+		.size = 0,
+	}
+};
 
-void __dso__inject_id(struct dso *dso, const struct dso_id *id)
+void __dso__improve_id(struct dso *dso, const struct dso_id *id)
 {
 	struct dsos *dsos = dso__dsos(dso);
 	struct dso_id *dso_id = dso__id(dso);
+	bool changed = false;
 
 	/* dsos write lock held by caller. */
 
-	dso_id->maj = id->maj;
-	dso_id->min = id->min;
-	dso_id->ino = id->ino;
-	dso_id->ino_generation = id->ino_generation;
-
-	if (dsos)
+	if (id->mmap2_valid && !dso_id->mmap2_valid) {
+		dso_id->maj = id->maj;
+		dso_id->min = id->min;
+		dso_id->ino = id->ino;
+		dso_id->mmap2_valid = true;
+		changed = true;
+	}
+	if (id->mmap2_ino_generation_valid && !dso_id->mmap2_ino_generation_valid) {
+		dso_id->ino_generation = id->ino_generation;
+		dso_id->mmap2_ino_generation_valid = true;
+		changed = true;
+	}
+	if (build_id__is_defined(&id->build_id) && !build_id__is_defined(&dso_id->build_id)) {
+		dso_id->build_id = id->build_id;
+		changed = true;
+	}
+	if (changed && dsos)
 		dsos->sorted = false;
 }
 
 int dso_id__cmp(const struct dso_id *a, const struct dso_id *b)
 {
-	/*
-	 * The second is always dso->id, so zeroes if not set, assume passing
-	 * NULL for a means a zeroed id
-	 */
-	if (dso_id__empty(a) || dso_id__empty(b))
+	if (a == &dso_id_empty || b == &dso_id_empty) {
+		/* There is no valid data to compare so the comparison always returns identical. */
 		return 0;
+	}
 
 	return __dso_id__cmp(a, b);
 }
@@ -1533,7 +1545,6 @@ struct dso *dso__new_id(const char *name, const struct dso_id *id)
 		dso->loaded = 0;
 		dso->rel = 0;
 		dso->sorted_by_name = 0;
-		dso->has_build_id = 0;
 		dso->has_srcline = 1;
 		dso->a2l_fails = 1;
 		dso->kernel = DSO_SPACE__USER;
@@ -1638,15 +1649,14 @@ int dso__swap_init(struct dso *dso, unsigned char eidata)
 	return 0;
 }
 
-void dso__set_build_id(struct dso *dso, struct build_id *bid)
+void dso__set_build_id(struct dso *dso, const struct build_id *bid)
 {
-	RC_CHK_ACCESS(dso)->bid = *bid;
-	RC_CHK_ACCESS(dso)->has_build_id = 1;
+	dso__id(dso)->build_id = *bid;
 }
 
-bool dso__build_id_equal(const struct dso *dso, struct build_id *bid)
+bool dso__build_id_equal(const struct dso *dso, const struct build_id *bid)
 {
-	const struct build_id *dso_bid = dso__bid_const(dso);
+	const struct build_id *dso_bid = dso__bid(dso);
 
 	if (dso_bid->size > bid->size && dso_bid->size == BUILD_ID_SIZE) {
 		/*
@@ -1665,18 +1675,20 @@ bool dso__build_id_equal(const struct dso *dso, struct build_id *bid)
 void dso__read_running_kernel_build_id(struct dso *dso, struct machine *machine)
 {
 	char path[PATH_MAX];
+	struct build_id bid = { .size = 0, };
 
 	if (machine__is_default_guest(machine))
 		return;
 	sprintf(path, "%s/sys/kernel/notes", machine->root_dir);
-	if (sysfs__read_build_id(path, dso__bid(dso)) == 0)
-		dso__set_has_build_id(dso);
+	sysfs__read_build_id(path, &bid);
+	dso__set_build_id(dso, &bid);
 }
 
 int dso__kernel_module_get_build_id(struct dso *dso,
 				    const char *root_dir)
 {
 	char filename[PATH_MAX];
+	struct build_id bid = { .size = 0, };
 	/*
 	 * kernel module short names are of the form "[module]" and
 	 * we need just "module" here.
@@ -1687,9 +1699,8 @@ int dso__kernel_module_get_build_id(struct dso *dso,
 		 "%s/sys/module/%.*s/notes/.note.gnu.build-id",
 		 root_dir, (int)strlen(name) - 1, name);
 
-	if (sysfs__read_build_id(filename, dso__bid(dso)) == 0)
-		dso__set_has_build_id(dso);
-
+	sysfs__read_build_id(filename, &bid);
+	dso__set_build_id(dso, &bid);
 	return 0;
 }
 
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index c87564471f9b..3457d713d3c5 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -185,14 +185,33 @@ enum dso_load_errno {
 #define DSO__DATA_CACHE_SIZE 4096
 #define DSO__DATA_CACHE_MASK ~(DSO__DATA_CACHE_SIZE - 1)
 
-/*
- * Data about backing storage DSO, comes from PERF_RECORD_MMAP2 meta events
+/**
+ * struct dso_id
+ *
+ * Data about backing storage DSO, comes from PERF_RECORD_MMAP2 meta events,
+ * reading from /proc/pid/maps or synthesis of build_ids from DSOs. Possibly
+ * incomplete at any particular use.
  */
 struct dso_id {
-	u32	maj;
-	u32	min;
-	u64	ino;
-	u64	ino_generation;
+	/* Data related to the mmap2 event or read from /proc/pid/maps. */
+	struct {
+		u32	maj;
+		u32	min;
+		u64	ino;
+		u64	ino_generation;
+	};
+	/** @mmap2_valid: Are the maj, min and ino fields valid? */
+	bool	mmap2_valid;
+	/**
+	 * @mmap2_ino_generation_valid: Is the ino_generation valid? Generally
+	 * false for /proc/pid/maps mmap event.
+	 */
+	bool	mmap2_ino_generation_valid;
+	/**
+	 * @build_id: A possibly populated build_id. build_id__is_defined checks
+	 * whether it is populated.
+	 */
+	struct build_id build_id;
 };
 
 struct dso_cache {
@@ -243,7 +262,6 @@ DECLARE_RC_STRUCT(dso) {
 		u64		addr;
 		struct symbol	*symbol;
 	} last_find_result;
-	struct build_id	 bid;
 	u64		 text_offset;
 	u64		 text_end;
 	const char	 *short_name;
@@ -276,7 +294,6 @@ DECLARE_RC_STRUCT(dso) {
 	enum dso_swap_type	needs_swap:2;
 	bool			is_kmod:1;
 	u8		 adjust_symbols:1;
-	u8		 has_build_id:1;
 	u8		 header_build_id:1;
 	u8		 has_srcline:1;
 	u8		 hit:1;
@@ -292,6 +309,9 @@ DECLARE_RC_STRUCT(dso) {
 };
 
 extern struct mutex _dso__data_open_lock;
+extern const struct dso_id dso_id_empty;
+
+int dso_id__cmp(const struct dso_id *a, const struct dso_id *b);
 
 /* dso__for_each_symbol - iterate over the symbols of given type
  *
@@ -362,31 +382,11 @@ static inline void dso__set_auxtrace_cache(struct dso *dso, struct auxtrace_cach
 	RC_CHK_ACCESS(dso)->auxtrace_cache = cache;
 }
 
-static inline struct build_id *dso__bid(struct dso *dso)
-{
-	return &RC_CHK_ACCESS(dso)->bid;
-}
-
-static inline const struct build_id *dso__bid_const(const struct dso *dso)
-{
-	return &RC_CHK_ACCESS(dso)->bid;
-}
-
 static inline struct dso_bpf_prog *dso__bpf_prog(struct dso *dso)
 {
 	return &RC_CHK_ACCESS(dso)->bpf_prog;
 }
 
-static inline bool dso__has_build_id(const struct dso *dso)
-{
-	return RC_CHK_ACCESS(dso)->has_build_id;
-}
-
-static inline void dso__set_has_build_id(struct dso *dso)
-{
-	RC_CHK_ACCESS(dso)->has_build_id = true;
-}
-
 static inline bool dso__has_srcline(const struct dso *dso)
 {
 	return RC_CHK_ACCESS(dso)->has_srcline;
@@ -462,6 +462,16 @@ static inline const struct dso_id *dso__id_const(const struct dso *dso)
 	return &RC_CHK_ACCESS(dso)->id;
 }
 
+static inline const struct build_id *dso__bid(const struct dso *dso)
+{
+	return &dso__id_const(dso)->build_id;
+}
+
+static inline bool dso__has_build_id(const struct dso *dso)
+{
+	return build_id__is_defined(dso__bid(dso));
+}
+
 static inline struct rb_root_cached *dso__inlined_nodes(struct dso *dso)
 {
 	return &RC_CHK_ACCESS(dso)->inlined_nodes;
@@ -699,9 +709,6 @@ static inline void dso__set_text_offset(struct dso *dso, u64 val)
 	RC_CHK_ACCESS(dso)->text_offset = val;
 }
 
-int dso_id__cmp(const struct dso_id *a, const struct dso_id *b);
-bool dso_id__empty(const struct dso_id *id);
-
 struct dso *dso__new_id(const char *name, const struct dso_id *id);
 struct dso *dso__new(const char *name);
 void dso__delete(struct dso *dso);
@@ -709,7 +716,7 @@ void dso__delete(struct dso *dso);
 int dso__cmp_id(struct dso *a, struct dso *b);
 void dso__set_short_name(struct dso *dso, const char *name, bool name_allocated);
 void dso__set_long_name(struct dso *dso, const char *name, bool name_allocated);
-void __dso__inject_id(struct dso *dso, const struct dso_id *id);
+void __dso__improve_id(struct dso *dso, const struct dso_id *id);
 
 int dso__name_len(const struct dso *dso);
 
@@ -739,8 +746,8 @@ void dso__sort_by_name(struct dso *dso);
 
 int dso__swap_init(struct dso *dso, unsigned char eidata);
 
-void dso__set_build_id(struct dso *dso, struct build_id *bid);
-bool dso__build_id_equal(const struct dso *dso, struct build_id *bid);
+void dso__set_build_id(struct dso *dso, const struct build_id *bid);
+bool dso__build_id_equal(const struct dso *dso, const struct build_id *bid);
 void dso__read_running_kernel_build_id(struct dso *dso,
 				       struct machine *machine);
 int dso__kernel_module_get_build_id(struct dso *dso, const char *root_dir);
diff --git a/tools/perf/util/dsos.c b/tools/perf/util/dsos.c
index b2172632b3cd..e239a3fa2896 100644
--- a/tools/perf/util/dsos.c
+++ b/tools/perf/util/dsos.c
@@ -72,6 +72,7 @@ static int dsos__read_build_ids_cb(struct dso *dso, void *data)
 {
 	struct dsos__read_build_ids_cb_args *args = data;
 	struct nscookie nsc;
+	struct build_id bid = { .size = 0, };
 
 	if (args->with_hits && !dso__hit(dso) && !dso__is_vdso(dso))
 		return 0;
@@ -80,15 +81,15 @@ static int dsos__read_build_ids_cb(struct dso *dso, void *data)
 		return 0;
 	}
 	nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
-	if (filename__read_build_id(dso__long_name(dso), dso__bid(dso)) > 0) {
+	if (filename__read_build_id(dso__long_name(dso), &bid) > 0) {
+		dso__set_build_id(dso, &bid);
 		args->have_build_id = true;
-		dso__set_has_build_id(dso);
 	} else if (errno == ENOENT && dso__nsinfo(dso)) {
 		char *new_name = dso__filename_with_chroot(dso, dso__long_name(dso));
 
-		if (new_name && filename__read_build_id(new_name, dso__bid(dso)) > 0) {
+		if (new_name && filename__read_build_id(new_name, &bid) > 0) {
+			dso__set_build_id(dso, &bid);
 			args->have_build_id = true;
-			dso__set_has_build_id(dso);
 		}
 		free(new_name);
 	}
@@ -284,7 +285,7 @@ struct dso *dsos__find(struct dsos *dsos, const char *name, bool cmp_short)
 	struct dso *res;
 
 	down_read(&dsos->lock);
-	res = __dsos__find_id(dsos, name, NULL, cmp_short, /*write_locked=*/false);
+	res = __dsos__find_id(dsos, name, &dso_id_empty, cmp_short, /*write_locked=*/false);
 	up_read(&dsos->lock);
 	return res;
 }
@@ -341,8 +342,8 @@ static struct dso *__dsos__findnew_id(struct dsos *dsos, const char *name, const
 {
 	struct dso *dso = __dsos__find_id(dsos, name, id, false, /*write_locked=*/true);
 
-	if (dso && dso_id__empty(dso__id(dso)) && !dso_id__empty(id))
-		__dso__inject_id(dso, id);
+	if (dso)
+		__dso__improve_id(dso, id);
 
 	return dso ? dso : __dsos__addnew_id(dsos, name, id);
 }
@@ -433,7 +434,8 @@ struct dso *dsos__findnew_module_dso(struct dsos *dsos,
 
 	down_write(&dsos->lock);
 
-	dso = __dsos__find_id(dsos, m->name, NULL, /*cmp_short=*/true, /*write_locked=*/true);
+	dso = __dsos__find_id(dsos, m->name, &dso_id_empty, /*cmp_short=*/true,
+			      /*write_locked=*/true);
 	if (dso) {
 		up_write(&dsos->lock);
 		return dso;
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index b048165b10c1..04062148a9ec 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1696,21 +1696,21 @@ int machine__process_mmap2_event(struct machine *machine,
 {
 	struct thread *thread;
 	struct map *map;
-	struct dso_id dso_id = {
-		.maj = event->mmap2.maj,
-		.min = event->mmap2.min,
-		.ino = event->mmap2.ino,
-		.ino_generation = event->mmap2.ino_generation,
-	};
-	struct build_id __bid, *bid = NULL;
+	struct dso_id dso_id = dso_id_empty;
 	int ret = 0;
 
 	if (dump_trace)
 		perf_event__fprintf_mmap2(event, stdout);
 
 	if (event->header.misc & PERF_RECORD_MISC_MMAP_BUILD_ID) {
-		bid = &__bid;
-		build_id__init(bid, event->mmap2.build_id, event->mmap2.build_id_size);
+		build_id__init(&dso_id.build_id, event->mmap2.build_id, event->mmap2.build_id_size);
+	} else {
+		dso_id.maj = event->mmap2.maj;
+		dso_id.min = event->mmap2.min;
+		dso_id.ino = event->mmap2.ino;
+		dso_id.ino_generation = event->mmap2.ino_generation;
+		dso_id.mmap2_valid = true;
+		dso_id.mmap2_ino_generation_valid = true;
 	}
 
 	if (sample->cpumode == PERF_RECORD_MISC_GUEST_KERNEL ||
@@ -1722,7 +1722,7 @@ int machine__process_mmap2_event(struct machine *machine,
 		};
 
 		strlcpy(xm.name, event->mmap2.filename, KMAP_NAME_LEN);
-		ret = machine__process_kernel_mmap_event(machine, &xm, bid);
+		ret = machine__process_kernel_mmap_event(machine, &xm, &dso_id.build_id);
 		if (ret < 0)
 			goto out_problem;
 		return 0;
@@ -1736,7 +1736,7 @@ int machine__process_mmap2_event(struct machine *machine,
 	map = map__new(machine, event->mmap2.start,
 			event->mmap2.len, event->mmap2.pgoff,
 			&dso_id, event->mmap2.prot,
-			event->mmap2.flags, bid,
+			event->mmap2.flags,
 			event->mmap2.filename, thread);
 
 	if (map == NULL)
@@ -1794,8 +1794,8 @@ int machine__process_mmap_event(struct machine *machine, union perf_event *event
 		prot = PROT_EXEC;
 
 	map = map__new(machine, event->mmap.start,
-			event->mmap.len, event->mmap.pgoff,
-			NULL, prot, 0, NULL, event->mmap.filename, thread);
+		       event->mmap.len, event->mmap.pgoff,
+		       &dso_id_empty, prot, /*flags=*/0, event->mmap.filename, thread);
 
 	if (map == NULL)
 		goto out_problem_map;
@@ -3157,7 +3157,7 @@ struct dso *machine__findnew_dso_id(struct machine *machine, const char *filenam
 
 struct dso *machine__findnew_dso(struct machine *machine, const char *filename)
 {
-	return machine__findnew_dso_id(machine, filename, NULL);
+	return machine__findnew_dso_id(machine, filename, &dso_id_empty);
 }
 
 char *machine__resolve_kernel_addr(void *vmachine, unsigned long long *addrp, char **modp)
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 0f6b185f9589..b46c68c24d1c 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -120,8 +120,8 @@ static void map__init(struct map *map, u64 start, u64 end, u64 pgoff,
 }
 
 struct map *map__new(struct machine *machine, u64 start, u64 len,
-		     u64 pgoff, struct dso_id *id,
-		     u32 prot, u32 flags, struct build_id *bid,
+		     u64 pgoff, const struct dso_id *id,
+		     u32 prot, u32 flags,
 		     char *filename, struct thread *thread)
 {
 	struct map *result;
@@ -132,7 +132,7 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
 	map = zalloc(sizeof(*map));
 	if (ADD_RC_CHK(result, map)) {
 		char newfilename[PATH_MAX];
-		struct dso *dso, *header_bid_dso;
+		struct dso *dso;
 		int anon, no_dso, vdso, android;
 
 		android = is_android_lib(filename);
@@ -189,16 +189,15 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
 		dso__set_nsinfo(dso, nsi);
 		mutex_unlock(dso__lock(dso));
 
-		if (build_id__is_defined(bid)) {
-			dso__set_build_id(dso, bid);
-		} else {
+		if (!build_id__is_defined(&id->build_id)) {
 			/*
 			 * If the mmap event had no build ID, search for an existing dso from the
 			 * build ID header by name. Otherwise only the dso loaded at the time of
 			 * reading the header will have the build ID set and all future mmaps will
 			 * have it missing.
 			 */
-			header_bid_dso = dsos__find(&machine->dsos, filename, false);
+			struct dso *header_bid_dso = dsos__find(&machine->dsos, filename, false);
+
 			if (header_bid_dso && dso__header_build_id(header_bid_dso)) {
 				dso__set_build_id(dso, dso__bid(header_bid_dso));
 				dso__set_header_build_id(dso, 1);
diff --git a/tools/perf/util/map.h b/tools/perf/util/map.h
index 4262f5a143be..9cadf533a561 100644
--- a/tools/perf/util/map.h
+++ b/tools/perf/util/map.h
@@ -173,11 +173,10 @@ struct thread;
 	__map__for_each_symbol_by_name(map, sym_name, (pos), idx)
 
 struct dso_id;
-struct build_id;
 
 struct map *map__new(struct machine *machine, u64 start, u64 len,
-		     u64 pgoff, struct dso_id *id, u32 prot, u32 flags,
-		     struct build_id *bid, char *filename, struct thread *thread);
+		     u64 pgoff, const struct dso_id *id, u32 prot, u32 flags,
+		     char *filename, struct thread *thread);
 struct map *map__new2(u64 start, struct dso *dso);
 void map__delete(struct map *map);
 struct map *map__clone(struct map *map);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 594b75ca95bf..d20b980d5052 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1709,22 +1709,27 @@ sort__dcacheline_cmp(struct hist_entry *left, struct hist_entry *right)
 	if (rc)
 		return rc;
 	/*
-	 * Addresses with no major/minor numbers are assumed to be
+	 * Addresses with no major/minor numbers or build ID are assumed to be
 	 * anonymous in userspace.  Sort those on pid then address.
 	 *
 	 * The kernel and non-zero major/minor mapped areas are
 	 * assumed to be unity mapped.  Sort those on address.
 	 */
+	if (left->cpumode != PERF_RECORD_MISC_KERNEL && (map__flags(l_map) & MAP_SHARED) == 0) {
+		const struct dso_id *dso_id = dso__id_const(l_dso);
 
-	if ((left->cpumode != PERF_RECORD_MISC_KERNEL) &&
-	    (!(map__flags(l_map) & MAP_SHARED)) && !dso__id(l_dso)->maj && !dso__id(l_dso)->min &&
-	     !dso__id(l_dso)->ino && !dso__id(l_dso)->ino_generation) {
-		/* userspace anonymous */
+		if (!dso_id->mmap2_valid)
+			dso_id = dso__id_const(r_dso);
 
-		if (thread__pid(left->thread) > thread__pid(right->thread))
-			return -1;
-		if (thread__pid(left->thread) < thread__pid(right->thread))
-			return 1;
+		if (!build_id__is_defined(&dso_id->build_id) &&
+		    (!dso_id->mmap2_valid || (dso_id->maj == 0 && dso_id->min == 0))) {
+			/* userspace anonymous */
+
+			if (thread__pid(left->thread) > thread__pid(right->thread))
+				return -1;
+			if (thread__pid(left->thread) < thread__pid(right->thread))
+				return 1;
+		}
 	}
 
 addr:
@@ -1749,6 +1754,7 @@ static int hist_entry__dcacheline_snprintf(struct hist_entry *he, char *bf,
 	if (he->mem_info) {
 		struct map *map = mem_info__daddr(he->mem_info)->ms.map;
 		struct dso *dso = map ? map__dso(map) : NULL;
+		const struct dso_id *dso_id = dso ? dso__id_const(dso) : &dso_id_empty;
 
 		addr = cl_address(mem_info__daddr(he->mem_info)->al_addr, chk_double_cl);
 		ms = &mem_info__daddr(he->mem_info)->ms;
@@ -1757,8 +1763,7 @@ static int hist_entry__dcacheline_snprintf(struct hist_entry *he, char *bf,
 		if ((he->cpumode != PERF_RECORD_MISC_KERNEL) &&
 		     map && !(map__prot(map) & PROT_EXEC) &&
 		     (map__flags(map) & MAP_SHARED) &&
-		     (dso__id(dso)->maj || dso__id(dso)->min || dso__id(dso)->ino ||
-		      dso__id(dso)->ino_generation))
+		     (!dso_id->mmap2_valid || (dso_id->maj == 0 && dso_id->min == 0)))
 			level = 's';
 		else if (!map)
 			level = 'X';
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index b7ebee95536f..69b98023ce74 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -372,7 +372,7 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
 	struct nsinfo *nsi;
 	struct nscookie nc;
 	struct dso *dso = NULL;
-	struct dso_id id;
+	struct dso_id dso_id = dso_id_empty;
 	int rc;
 
 	if (is_kernel) {
@@ -380,12 +380,18 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
 		goto out;
 	}
 
-	id.maj = event->maj;
-	id.min = event->min;
-	id.ino = event->ino;
-	id.ino_generation = event->ino_generation;
+	if (event->header.misc & PERF_RECORD_MISC_MMAP_BUILD_ID) {
+		build_id__init(&dso_id.build_id, event->build_id, event->build_id_size);
+	} else {
+		dso_id.maj = event->maj;
+		dso_id.min = event->min;
+		dso_id.ino = event->ino;
+		dso_id.ino_generation = event->ino_generation;
+		dso_id.mmap2_valid = true;
+		dso_id.mmap2_ino_generation_valid = true;
+	};
 
-	dso = dsos__findnew_id(&machine->dsos, event->filename, &id);
+	dso = dsos__findnew_id(&machine->dsos, event->filename, &dso_id);
 	if (dso && dso__has_build_id(dso)) {
 		bid = *dso__bid(dso);
 		rc = 0;
-- 
2.49.0.901.g37484f566f-goog

[PATCH v3 8/9] perf jitdump: Directly mark the jitdump DSO

[Ian Rogers]

The DSO being generated was being accessed through a thread's maps,
this is unnecessary as the dso can just be directly found. This avoids
problems with passing a NULL evsel which may be inspected to determine
properties of a callchain when using the buildid DSO marking code.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/jitdump.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
index 624964f01b5f..b062b1f234b6 100644
--- a/tools/perf/util/jitdump.c
+++ b/tools/perf/util/jitdump.c
@@ -14,9 +14,9 @@
 #include <sys/mman.h>
 #include <linux/stringify.h>
 
-#include "build-id.h"
 #include "event.h"
 #include "debug.h"
+#include "dso.h"
 #include "evlist.h"
 #include "namespaces.h"
 #include "symbol.h"
@@ -531,9 +531,22 @@ static int jit_repipe_code_load(struct jit_buf_desc *jd, union jr_entry *jr)
 	/*
 	 * mark dso as use to generate buildid in the header
 	 */
-	if (!ret)
-		build_id__mark_dso_hit(tool, event, &sample, NULL, jd->machine);
-
+	if (!ret) {
+		struct dso_id dso_id = {
+			{
+				.maj = event->mmap2.maj,
+				.min = event->mmap2.min,
+				.ino = event->mmap2.ino,
+				.ino_generation = event->mmap2.ino_generation,
+			},
+			.mmap2_valid = true,
+			.mmap2_ino_generation_valid = true,
+		};
+		struct dso *dso = machine__findnew_dso_id(jd->machine, filename, &dso_id);
+
+		if (dso)
+			dso__set_hit(dso);
+	}
 out:
 	perf_sample__exit(&sample);
 	free(event);
-- 
2.49.0.901.g37484f566f-goog

[PATCH v3 9/9] perf record: Make --buildid-mmap the default

[Ian Rogers]

Support for build IDs in mmap2 perf events has been present since
Linux v5.12:
https://lore.kernel.org/lkml/20210219194619.1780437-1-acme@kernel.org/
Build ID mmap events don't avoid the need to inject build IDs for DSO
touched by samples as the build ID cache is populated by perf
record. They can avoid some cases of symbol mis-resolution caused by
the file system changing from when a sample occurred and when the DSO
is sought. To disable build ID scanning

Unlike the --buildid-mmap option, this doesn't disable the build ID
cache but it does disable the processing of samples looking for DSOs
to inject build IDs for. To disable the build ID cache the -B
(--no-buildid) option should be used.

Making this option the default was raised on the list in:
https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-record.c        | 33 +++++++++++++++++++-----------
 tools/perf/util/symbol_conf.h      |  2 +-
 tools/perf/util/synthetic-events.c | 16 +++++++--------
 3 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ba20bf7c011d..7b64013ba8c0 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -169,6 +169,7 @@ struct record {
 	bool			no_buildid_cache_set;
 	bool			buildid_all;
 	bool			buildid_mmap;
+	bool			buildid_mmap_set;
 	bool			timestamp_filename;
 	bool			timestamp_boundary;
 	bool			off_cpu;
@@ -1795,6 +1796,7 @@ record__finish_output(struct record *rec)
 			data->dir.files[i].size = lseek(data->dir.files[i].fd, 0, SEEK_CUR);
 	}
 
+	/* Buildid scanning disabled or build ID in kernel and synthesized map events. */
 	if (!rec->no_buildid) {
 		process_buildids(rec);
 
@@ -2966,6 +2968,8 @@ static int perf_record_config(const char *var, const char *value, void *cb)
 			rec->no_buildid = true;
 		else if (!strcmp(value, "mmap"))
 			rec->buildid_mmap = true;
+		else if (!strcmp(value, "no-mmap"))
+			rec->buildid_mmap = false;
 		else
 			return -1;
 		return 0;
@@ -3349,6 +3353,7 @@ static struct record record = {
 		.ctl_fd_ack          = -1,
 		.synth               = PERF_SYNTH_ALL,
 	},
+	.buildid_mmap = true,
 };
 
 const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
@@ -3514,8 +3519,8 @@ static struct option __record_options[] = {
 		   "file", "vmlinux pathname"),
 	OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
 		    "Record build-id of all DSOs regardless of hits"),
-	OPT_BOOLEAN(0, "buildid-mmap", &record.buildid_mmap,
-		    "Record build-id in map events"),
+	OPT_BOOLEAN_SET(0, "buildid-mmap", &record.buildid_mmap, &record.buildid_mmap_set,
+			"Legacy record build-id in map events option which is now the default. Behaves indentically to --no-buildid. Disable with --no-buildid-mmap"),
 	OPT_BOOLEAN(0, "timestamp-filename", &record.timestamp_filename,
 		    "append timestamp to output filename"),
 	OPT_BOOLEAN(0, "timestamp-boundary", &record.timestamp_boundary,
@@ -4042,19 +4047,23 @@ int cmd_record(int argc, const char **argv)
 		record.opts.record_switch_events = true;
 	}
 
+	if (!rec->buildid_mmap) {
+		pr_debug("Disabling build id in synthesized mmap2 events.\n");
+		symbol_conf.no_buildid_mmap2 = true;
+	} else if (rec->buildid_mmap_set) {
+		/*
+		 * Explicitly passing --buildid-mmap disables buildid processing
+		 * and cache generation.
+		 */
+		rec->no_buildid = true;
+	}
+	if (rec->buildid_mmap && !perf_can_record_build_id()) {
+		pr_warning("Missing support for build id in kernel mmap events. Disable this warning with --no-buildid-mmap\n");
+		rec->buildid_mmap = false;
+	}
 	if (rec->buildid_mmap) {
-		if (!perf_can_record_build_id()) {
-			pr_err("Failed: no support to record build id in mmap events, update your kernel.\n");
-			err = -EINVAL;
-			goto out_opts;
-		}
-		pr_debug("Enabling build id in mmap2 events.\n");
-		/* Enable mmap build id synthesizing. */
-		symbol_conf.buildid_mmap2 = true;
 		/* Enable perf_event_attr::build_id bit. */
 		rec->opts.build_id = true;
-		/* Disable build id cache. */
-		rec->no_buildid = true;
 	}
 
 	if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
index cd9aa82c7d5a..7a80d2c14d9b 100644
--- a/tools/perf/util/symbol_conf.h
+++ b/tools/perf/util/symbol_conf.h
@@ -43,7 +43,7 @@ struct symbol_conf {
 			report_individual_block,
 			inline_name,
 			disable_add2line_warn,
-			buildid_mmap2,
+			no_buildid_mmap2,
 			guest_code,
 			lazy_load_kernel_maps,
 			keep_exited_threads,
diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
index 69b98023ce74..638d7dd7fa4b 100644
--- a/tools/perf/util/synthetic-events.c
+++ b/tools/perf/util/synthetic-events.c
@@ -532,7 +532,7 @@ int perf_event__synthesize_mmap_events(const struct perf_tool *tool,
 		event->mmap2.pid = tgid;
 		event->mmap2.tid = pid;
 
-		if (symbol_conf.buildid_mmap2)
+		if (!symbol_conf.no_buildid_mmap2)
 			perf_record_mmap2__read_build_id(&event->mmap2, machine, false);
 
 		if (perf_tool__process_synth_event(tool, event, machine, process) != 0) {
@@ -690,7 +690,7 @@ static int perf_event__synthesize_modules_maps_cb(struct map *map, void *data)
 		return 0;
 
 	dso = map__dso(map);
-	if (symbol_conf.buildid_mmap2) {
+	if (!symbol_conf.no_buildid_mmap2) {
 		size = PERF_ALIGN(dso__long_name_len(dso) + 1, sizeof(u64));
 		event->mmap2.header.type = PERF_RECORD_MMAP2;
 		event->mmap2.header.size = (sizeof(event->mmap2) -
@@ -734,9 +734,9 @@ int perf_event__synthesize_modules(const struct perf_tool *tool, perf_event__han
 		.process = process,
 		.machine = machine,
 	};
-	size_t size = symbol_conf.buildid_mmap2
-		? sizeof(args.event->mmap2)
-		: sizeof(args.event->mmap);
+	size_t size = symbol_conf.no_buildid_mmap2
+		? sizeof(args.event->mmap)
+		: sizeof(args.event->mmap2);
 
 	args.event = zalloc(size + machine->id_hdr_size);
 	if (args.event == NULL) {
@@ -1124,8 +1124,8 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
 						struct machine *machine)
 {
 	union perf_event *event;
-	size_t size = symbol_conf.buildid_mmap2 ?
-			sizeof(event->mmap2) : sizeof(event->mmap);
+	size_t size = symbol_conf.no_buildid_mmap2 ?
+			sizeof(event->mmap) : sizeof(event->mmap2);
 	struct map *map = machine__kernel_map(machine);
 	struct kmap *kmap;
 	int err;
@@ -1159,7 +1159,7 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
 		event->header.misc = PERF_RECORD_MISC_GUEST_KERNEL;
 	}
 
-	if (symbol_conf.buildid_mmap2) {
+	if (!symbol_conf.no_buildid_mmap2) {
 		size = snprintf(event->mmap2.filename, sizeof(event->mmap2.filename),
 				"%s%s", machine->mmap_name, kmap->ref_reloc_sym->name) + 1;
 		size = PERF_ALIGN(size, sizeof(u64));
-- 
2.49.0.901.g37484f566f-goog

Re: [PATCH v3 9/9] perf record: Make --buildid-mmap the default

[Namhyung Kim]

On Mon, Apr 28, 2025 at 02:34:08PM -0700, Ian Rogers wrote:
> Support for build IDs in mmap2 perf events has been present since
> Linux v5.12:
> https://lore.kernel.org/lkml/20210219194619.1780437-1-acme@kernel.org/
> Build ID mmap events don't avoid the need to inject build IDs for DSO
> touched by samples as the build ID cache is populated by perf
> record. They can avoid some cases of symbol mis-resolution caused by
> the file system changing from when a sample occurred and when the DSO
> is sought. To disable build ID scanning
> 
> Unlike the --buildid-mmap option, this doesn't disable the build ID
> cache but it does disable the processing of samples looking for DSOs
> to inject build IDs for. To disable the build ID cache the -B
> (--no-buildid) option should be used.
> 
> Making this option the default was raised on the list in:
> https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-record.c        | 33 +++++++++++++++++++-----------
>  tools/perf/util/symbol_conf.h      |  2 +-
>  tools/perf/util/synthetic-events.c | 16 +++++++--------
>  3 files changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index ba20bf7c011d..7b64013ba8c0 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -169,6 +169,7 @@ struct record {
>  	bool			no_buildid_cache_set;
>  	bool			buildid_all;
>  	bool			buildid_mmap;
> +	bool			buildid_mmap_set;
>  	bool			timestamp_filename;
>  	bool			timestamp_boundary;
>  	bool			off_cpu;
> @@ -1795,6 +1796,7 @@ record__finish_output(struct record *rec)
>  			data->dir.files[i].size = lseek(data->dir.files[i].fd, 0, SEEK_CUR);
>  	}
>  
> +	/* Buildid scanning disabled or build ID in kernel and synthesized map events. */
>  	if (!rec->no_buildid) {
>  		process_buildids(rec);
>  
> @@ -2966,6 +2968,8 @@ static int perf_record_config(const char *var, const char *value, void *cb)
>  			rec->no_buildid = true;
>  		else if (!strcmp(value, "mmap"))
>  			rec->buildid_mmap = true;
> +		else if (!strcmp(value, "no-mmap"))
> +			rec->buildid_mmap = false;
>  		else
>  			return -1;
>  		return 0;
> @@ -3349,6 +3353,7 @@ static struct record record = {
>  		.ctl_fd_ack          = -1,
>  		.synth               = PERF_SYNTH_ALL,
>  	},
> +	.buildid_mmap = true,
>  };
>  
>  const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
> @@ -3514,8 +3519,8 @@ static struct option __record_options[] = {
>  		   "file", "vmlinux pathname"),
>  	OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
>  		    "Record build-id of all DSOs regardless of hits"),
> -	OPT_BOOLEAN(0, "buildid-mmap", &record.buildid_mmap,
> -		    "Record build-id in map events"),
> +	OPT_BOOLEAN_SET(0, "buildid-mmap", &record.buildid_mmap, &record.buildid_mmap_set,
> +			"Legacy record build-id in map events option which is now the default. Behaves indentically to --no-buildid. Disable with --no-buildid-mmap"),
>  	OPT_BOOLEAN(0, "timestamp-filename", &record.timestamp_filename,
>  		    "append timestamp to output filename"),
>  	OPT_BOOLEAN(0, "timestamp-boundary", &record.timestamp_boundary,
> @@ -4042,19 +4047,23 @@ int cmd_record(int argc, const char **argv)
>  		record.opts.record_switch_events = true;
>  	}
>  
> +	if (!rec->buildid_mmap) {
> +		pr_debug("Disabling build id in synthesized mmap2 events.\n");
> +		symbol_conf.no_buildid_mmap2 = true;
> +	} else if (rec->buildid_mmap_set) {
> +		/*
> +		 * Explicitly passing --buildid-mmap disables buildid processing
> +		 * and cache generation.
> +		 */
> +		rec->no_buildid = true;
> +	}
> +	if (rec->buildid_mmap && !perf_can_record_build_id()) {
> +		pr_warning("Missing support for build id in kernel mmap events. Disable this warning with --no-buildid-mmap\n");
> +		rec->buildid_mmap = false;
> +	}
>  	if (rec->buildid_mmap) {
> -		if (!perf_can_record_build_id()) {
> -			pr_err("Failed: no support to record build id in mmap events, update your kernel.\n");
> -			err = -EINVAL;
> -			goto out_opts;
> -		}
> -		pr_debug("Enabling build id in mmap2 events.\n");
> -		/* Enable mmap build id synthesizing. */
> -		symbol_conf.buildid_mmap2 = true;
>  		/* Enable perf_event_attr::build_id bit. */
>  		rec->opts.build_id = true;
> -		/* Disable build id cache. */
> -		rec->no_buildid = true;
>  	}
>  
>  	if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
> diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> index cd9aa82c7d5a..7a80d2c14d9b 100644
> --- a/tools/perf/util/symbol_conf.h
> +++ b/tools/perf/util/symbol_conf.h
> @@ -43,7 +43,7 @@ struct symbol_conf {
>  			report_individual_block,
>  			inline_name,
>  			disable_add2line_warn,
> -			buildid_mmap2,
> +			no_buildid_mmap2,
>  			guest_code,
>  			lazy_load_kernel_maps,
>  			keep_exited_threads,
> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 69b98023ce74..638d7dd7fa4b 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -532,7 +532,7 @@ int perf_event__synthesize_mmap_events(const struct perf_tool *tool,
>  		event->mmap2.pid = tgid;
>  		event->mmap2.tid = pid;
>  
> -		if (symbol_conf.buildid_mmap2)
> +		if (!symbol_conf.no_buildid_mmap2)

I find the double negation confusing.  Can we keep it positive?

Thanks,
Namhyung


>  			perf_record_mmap2__read_build_id(&event->mmap2, machine, false);
>  
>  		if (perf_tool__process_synth_event(tool, event, machine, process) != 0) {
> @@ -690,7 +690,7 @@ static int perf_event__synthesize_modules_maps_cb(struct map *map, void *data)
>  		return 0;
>  
>  	dso = map__dso(map);
> -	if (symbol_conf.buildid_mmap2) {
> +	if (!symbol_conf.no_buildid_mmap2) {
>  		size = PERF_ALIGN(dso__long_name_len(dso) + 1, sizeof(u64));
>  		event->mmap2.header.type = PERF_RECORD_MMAP2;
>  		event->mmap2.header.size = (sizeof(event->mmap2) -
> @@ -734,9 +734,9 @@ int perf_event__synthesize_modules(const struct perf_tool *tool, perf_event__han
>  		.process = process,
>  		.machine = machine,
>  	};
> -	size_t size = symbol_conf.buildid_mmap2
> -		? sizeof(args.event->mmap2)
> -		: sizeof(args.event->mmap);
> +	size_t size = symbol_conf.no_buildid_mmap2
> +		? sizeof(args.event->mmap)
> +		: sizeof(args.event->mmap2);
>  
>  	args.event = zalloc(size + machine->id_hdr_size);
>  	if (args.event == NULL) {
> @@ -1124,8 +1124,8 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
>  						struct machine *machine)
>  {
>  	union perf_event *event;
> -	size_t size = symbol_conf.buildid_mmap2 ?
> -			sizeof(event->mmap2) : sizeof(event->mmap);
> +	size_t size = symbol_conf.no_buildid_mmap2 ?
> +			sizeof(event->mmap) : sizeof(event->mmap2);
>  	struct map *map = machine__kernel_map(machine);
>  	struct kmap *kmap;
>  	int err;
> @@ -1159,7 +1159,7 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
>  		event->header.misc = PERF_RECORD_MISC_GUEST_KERNEL;
>  	}
>  
> -	if (symbol_conf.buildid_mmap2) {
> +	if (!symbol_conf.no_buildid_mmap2) {
>  		size = snprintf(event->mmap2.filename, sizeof(event->mmap2.filename),
>  				"%s%s", machine->mmap_name, kmap->ref_reloc_sym->name) + 1;
>  		size = PERF_ALIGN(size, sizeof(u64));
> -- 
> 2.49.0.901.g37484f566f-goog
> 

Re: [PATCH v3 9/9] perf record: Make --buildid-mmap the default

[Ian Rogers]

On Wed, May 28, 2025 at 1:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Apr 28, 2025 at 02:34:08PM -0700, Ian Rogers wrote:
> > Support for build IDs in mmap2 perf events has been present since
> > Linux v5.12:
> > https://lore.kernel.org/lkml/20210219194619.1780437-1-acme@kernel.org/
> > Build ID mmap events don't avoid the need to inject build IDs for DSO
> > touched by samples as the build ID cache is populated by perf
> > record. They can avoid some cases of symbol mis-resolution caused by
> > the file system changing from when a sample occurred and when the DSO
> > is sought. To disable build ID scanning
> >
> > Unlike the --buildid-mmap option, this doesn't disable the build ID
> > cache but it does disable the processing of samples looking for DSOs
> > to inject build IDs for. To disable the build ID cache the -B
> > (--no-buildid) option should be used.
> >
> > Making this option the default was raised on the list in:
> > https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/builtin-record.c        | 33 +++++++++++++++++++-----------
> >  tools/perf/util/symbol_conf.h      |  2 +-
> >  tools/perf/util/synthetic-events.c | 16 +++++++--------
> >  3 files changed, 30 insertions(+), 21 deletions(-)
> >
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index ba20bf7c011d..7b64013ba8c0 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -169,6 +169,7 @@ struct record {
> >       bool                    no_buildid_cache_set;
> >       bool                    buildid_all;
> >       bool                    buildid_mmap;
> > +     bool                    buildid_mmap_set;
> >       bool                    timestamp_filename;
> >       bool                    timestamp_boundary;
> >       bool                    off_cpu;
> > @@ -1795,6 +1796,7 @@ record__finish_output(struct record *rec)
> >                       data->dir.files[i].size = lseek(data->dir.files[i].fd, 0, SEEK_CUR);
> >       }
> >
> > +     /* Buildid scanning disabled or build ID in kernel and synthesized map events. */
> >       if (!rec->no_buildid) {
> >               process_buildids(rec);
> >
> > @@ -2966,6 +2968,8 @@ static int perf_record_config(const char *var, const char *value, void *cb)
> >                       rec->no_buildid = true;
> >               else if (!strcmp(value, "mmap"))
> >                       rec->buildid_mmap = true;
> > +             else if (!strcmp(value, "no-mmap"))
> > +                     rec->buildid_mmap = false;
> >               else
> >                       return -1;
> >               return 0;
> > @@ -3349,6 +3353,7 @@ static struct record record = {
> >               .ctl_fd_ack          = -1,
> >               .synth               = PERF_SYNTH_ALL,
> >       },
> > +     .buildid_mmap = true,
> >  };
> >
> >  const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
> > @@ -3514,8 +3519,8 @@ static struct option __record_options[] = {
> >                  "file", "vmlinux pathname"),
> >       OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
> >                   "Record build-id of all DSOs regardless of hits"),
> > -     OPT_BOOLEAN(0, "buildid-mmap", &record.buildid_mmap,
> > -                 "Record build-id in map events"),
> > +     OPT_BOOLEAN_SET(0, "buildid-mmap", &record.buildid_mmap, &record.buildid_mmap_set,
> > +                     "Legacy record build-id in map events option which is now the default. Behaves indentically to --no-buildid. Disable with --no-buildid-mmap"),
> >       OPT_BOOLEAN(0, "timestamp-filename", &record.timestamp_filename,
> >                   "append timestamp to output filename"),
> >       OPT_BOOLEAN(0, "timestamp-boundary", &record.timestamp_boundary,
> > @@ -4042,19 +4047,23 @@ int cmd_record(int argc, const char **argv)
> >               record.opts.record_switch_events = true;
> >       }
> >
> > +     if (!rec->buildid_mmap) {
> > +             pr_debug("Disabling build id in synthesized mmap2 events.\n");
> > +             symbol_conf.no_buildid_mmap2 = true;
> > +     } else if (rec->buildid_mmap_set) {
> > +             /*
> > +              * Explicitly passing --buildid-mmap disables buildid processing
> > +              * and cache generation.
> > +              */
> > +             rec->no_buildid = true;
> > +     }
> > +     if (rec->buildid_mmap && !perf_can_record_build_id()) {
> > +             pr_warning("Missing support for build id in kernel mmap events. Disable this warning with --no-buildid-mmap\n");
> > +             rec->buildid_mmap = false;
> > +     }
> >       if (rec->buildid_mmap) {
> > -             if (!perf_can_record_build_id()) {
> > -                     pr_err("Failed: no support to record build id in mmap events, update your kernel.\n");
> > -                     err = -EINVAL;
> > -                     goto out_opts;
> > -             }
> > -             pr_debug("Enabling build id in mmap2 events.\n");
> > -             /* Enable mmap build id synthesizing. */
> > -             symbol_conf.buildid_mmap2 = true;
> >               /* Enable perf_event_attr::build_id bit. */
> >               rec->opts.build_id = true;
> > -             /* Disable build id cache. */
> > -             rec->no_buildid = true;
> >       }
> >
> >       if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
> > diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> > index cd9aa82c7d5a..7a80d2c14d9b 100644
> > --- a/tools/perf/util/symbol_conf.h
> > +++ b/tools/perf/util/symbol_conf.h
> > @@ -43,7 +43,7 @@ struct symbol_conf {
> >                       report_individual_block,
> >                       inline_name,
> >                       disable_add2line_warn,
> > -                     buildid_mmap2,
> > +                     no_buildid_mmap2,
> >                       guest_code,
> >                       lazy_load_kernel_maps,
> >                       keep_exited_threads,
> > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > index 69b98023ce74..638d7dd7fa4b 100644
> > --- a/tools/perf/util/synthetic-events.c
> > +++ b/tools/perf/util/synthetic-events.c
> > @@ -532,7 +532,7 @@ int perf_event__synthesize_mmap_events(const struct perf_tool *tool,
> >               event->mmap2.pid = tgid;
> >               event->mmap2.tid = pid;
> >
> > -             if (symbol_conf.buildid_mmap2)
> > +             if (!symbol_conf.no_buildid_mmap2)
>
> I find the double negation confusing.  Can we keep it positive?

Agreed double negation is broadly bad. Here I changed buildid_mmap2 to
no_buildid_mmap2 to convey that using build ID with mmap2 events was
the default and needed opting out of. Getting rid of the 'no_' is of
course possible, it will mean all symbol_confs will need to initialize
the value to true (which has minor binary size implications), I also
think it makes the intention of the code harder to understand.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> >                       perf_record_mmap2__read_build_id(&event->mmap2, machine, false);
> >
> >               if (perf_tool__process_synth_event(tool, event, machine, process) != 0) {
> > @@ -690,7 +690,7 @@ static int perf_event__synthesize_modules_maps_cb(struct map *map, void *data)
> >               return 0;
> >
> >       dso = map__dso(map);
> > -     if (symbol_conf.buildid_mmap2) {
> > +     if (!symbol_conf.no_buildid_mmap2) {
> >               size = PERF_ALIGN(dso__long_name_len(dso) + 1, sizeof(u64));
> >               event->mmap2.header.type = PERF_RECORD_MMAP2;
> >               event->mmap2.header.size = (sizeof(event->mmap2) -
> > @@ -734,9 +734,9 @@ int perf_event__synthesize_modules(const struct perf_tool *tool, perf_event__han
> >               .process = process,
> >               .machine = machine,
> >       };
> > -     size_t size = symbol_conf.buildid_mmap2
> > -             ? sizeof(args.event->mmap2)
> > -             : sizeof(args.event->mmap);
> > +     size_t size = symbol_conf.no_buildid_mmap2
> > +             ? sizeof(args.event->mmap)
> > +             : sizeof(args.event->mmap2);
> >
> >       args.event = zalloc(size + machine->id_hdr_size);
> >       if (args.event == NULL) {
> > @@ -1124,8 +1124,8 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
> >                                               struct machine *machine)
> >  {
> >       union perf_event *event;
> > -     size_t size = symbol_conf.buildid_mmap2 ?
> > -                     sizeof(event->mmap2) : sizeof(event->mmap);
> > +     size_t size = symbol_conf.no_buildid_mmap2 ?
> > +                     sizeof(event->mmap) : sizeof(event->mmap2);
> >       struct map *map = machine__kernel_map(machine);
> >       struct kmap *kmap;
> >       int err;
> > @@ -1159,7 +1159,7 @@ static int __perf_event__synthesize_kernel_mmap(const struct perf_tool *tool,
> >               event->header.misc = PERF_RECORD_MISC_GUEST_KERNEL;
> >       }
> >
> > -     if (symbol_conf.buildid_mmap2) {
> > +     if (!symbol_conf.no_buildid_mmap2) {
> >               size = snprintf(event->mmap2.filename, sizeof(event->mmap2.filename),
> >                               "%s%s", machine->mmap_name, kmap->ref_reloc_sym->name) + 1;
> >               size = PERF_ALIGN(size, sizeof(u64));
> > --
> > 2.49.0.901.g37484f566f-goog
> >

Re: [PATCH v3 9/9] perf record: Make --buildid-mmap the default

[Namhyung Kim]

On Wed, May 28, 2025 at 01:47:47PM -0700, Ian Rogers wrote:
> On Wed, May 28, 2025 at 1:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Apr 28, 2025 at 02:34:08PM -0700, Ian Rogers wrote:
> > > Support for build IDs in mmap2 perf events has been present since
> > > Linux v5.12:
> > > https://lore.kernel.org/lkml/20210219194619.1780437-1-acme@kernel.org/
> > > Build ID mmap events don't avoid the need to inject build IDs for DSO
> > > touched by samples as the build ID cache is populated by perf
> > > record. They can avoid some cases of symbol mis-resolution caused by
> > > the file system changing from when a sample occurred and when the DSO
> > > is sought. To disable build ID scanning
> > >
> > > Unlike the --buildid-mmap option, this doesn't disable the build ID
> > > cache but it does disable the processing of samples looking for DSOs
> > > to inject build IDs for. To disable the build ID cache the -B
> > > (--no-buildid) option should be used.
> > >
> > > Making this option the default was raised on the list in:
> > > https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/builtin-record.c        | 33 +++++++++++++++++++-----------
> > >  tools/perf/util/symbol_conf.h      |  2 +-
> > >  tools/perf/util/synthetic-events.c | 16 +++++++--------
> > >  3 files changed, 30 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > > index ba20bf7c011d..7b64013ba8c0 100644
> > > --- a/tools/perf/builtin-record.c
> > > +++ b/tools/perf/builtin-record.c
> > > @@ -169,6 +169,7 @@ struct record {
> > >       bool                    no_buildid_cache_set;
> > >       bool                    buildid_all;
> > >       bool                    buildid_mmap;
> > > +     bool                    buildid_mmap_set;
> > >       bool                    timestamp_filename;
> > >       bool                    timestamp_boundary;
> > >       bool                    off_cpu;
> > > @@ -1795,6 +1796,7 @@ record__finish_output(struct record *rec)
> > >                       data->dir.files[i].size = lseek(data->dir.files[i].fd, 0, SEEK_CUR);
> > >       }
> > >
> > > +     /* Buildid scanning disabled or build ID in kernel and synthesized map events. */
> > >       if (!rec->no_buildid) {
> > >               process_buildids(rec);
> > >
> > > @@ -2966,6 +2968,8 @@ static int perf_record_config(const char *var, const char *value, void *cb)
> > >                       rec->no_buildid = true;
> > >               else if (!strcmp(value, "mmap"))
> > >                       rec->buildid_mmap = true;
> > > +             else if (!strcmp(value, "no-mmap"))
> > > +                     rec->buildid_mmap = false;
> > >               else
> > >                       return -1;
> > >               return 0;
> > > @@ -3349,6 +3353,7 @@ static struct record record = {
> > >               .ctl_fd_ack          = -1,
> > >               .synth               = PERF_SYNTH_ALL,
> > >       },
> > > +     .buildid_mmap = true,
> > >  };
> > >
> > >  const char record_callchain_help[] = CALLCHAIN_RECORD_HELP
> > > @@ -3514,8 +3519,8 @@ static struct option __record_options[] = {
> > >                  "file", "vmlinux pathname"),
> > >       OPT_BOOLEAN(0, "buildid-all", &record.buildid_all,
> > >                   "Record build-id of all DSOs regardless of hits"),
> > > -     OPT_BOOLEAN(0, "buildid-mmap", &record.buildid_mmap,
> > > -                 "Record build-id in map events"),
> > > +     OPT_BOOLEAN_SET(0, "buildid-mmap", &record.buildid_mmap, &record.buildid_mmap_set,
> > > +                     "Legacy record build-id in map events option which is now the default. Behaves indentically to --no-buildid. Disable with --no-buildid-mmap"),
> > >       OPT_BOOLEAN(0, "timestamp-filename", &record.timestamp_filename,
> > >                   "append timestamp to output filename"),
> > >       OPT_BOOLEAN(0, "timestamp-boundary", &record.timestamp_boundary,
> > > @@ -4042,19 +4047,23 @@ int cmd_record(int argc, const char **argv)
> > >               record.opts.record_switch_events = true;
> > >       }
> > >
> > > +     if (!rec->buildid_mmap) {
> > > +             pr_debug("Disabling build id in synthesized mmap2 events.\n");
> > > +             symbol_conf.no_buildid_mmap2 = true;
> > > +     } else if (rec->buildid_mmap_set) {
> > > +             /*
> > > +              * Explicitly passing --buildid-mmap disables buildid processing
> > > +              * and cache generation.
> > > +              */
> > > +             rec->no_buildid = true;
> > > +     }
> > > +     if (rec->buildid_mmap && !perf_can_record_build_id()) {
> > > +             pr_warning("Missing support for build id in kernel mmap events. Disable this warning with --no-buildid-mmap\n");
> > > +             rec->buildid_mmap = false;
> > > +     }
> > >       if (rec->buildid_mmap) {
> > > -             if (!perf_can_record_build_id()) {
> > > -                     pr_err("Failed: no support to record build id in mmap events, update your kernel.\n");
> > > -                     err = -EINVAL;
> > > -                     goto out_opts;
> > > -             }
> > > -             pr_debug("Enabling build id in mmap2 events.\n");
> > > -             /* Enable mmap build id synthesizing. */
> > > -             symbol_conf.buildid_mmap2 = true;
> > >               /* Enable perf_event_attr::build_id bit. */
> > >               rec->opts.build_id = true;
> > > -             /* Disable build id cache. */
> > > -             rec->no_buildid = true;
> > >       }
> > >
> > >       if (rec->opts.record_cgroup && !perf_can_record_cgroup()) {
> > > diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> > > index cd9aa82c7d5a..7a80d2c14d9b 100644
> > > --- a/tools/perf/util/symbol_conf.h
> > > +++ b/tools/perf/util/symbol_conf.h
> > > @@ -43,7 +43,7 @@ struct symbol_conf {
> > >                       report_individual_block,
> > >                       inline_name,
> > >                       disable_add2line_warn,
> > > -                     buildid_mmap2,
> > > +                     no_buildid_mmap2,
> > >                       guest_code,
> > >                       lazy_load_kernel_maps,
> > >                       keep_exited_threads,
> > > diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> > > index 69b98023ce74..638d7dd7fa4b 100644
> > > --- a/tools/perf/util/synthetic-events.c
> > > +++ b/tools/perf/util/synthetic-events.c
> > > @@ -532,7 +532,7 @@ int perf_event__synthesize_mmap_events(const struct perf_tool *tool,
> > >               event->mmap2.pid = tgid;
> > >               event->mmap2.tid = pid;
> > >
> > > -             if (symbol_conf.buildid_mmap2)
> > > +             if (!symbol_conf.no_buildid_mmap2)
> >
> > I find the double negation confusing.  Can we keep it positive?
> 
> Agreed double negation is broadly bad. Here I changed buildid_mmap2 to
> no_buildid_mmap2 to convey that using build ID with mmap2 events was
> the default and needed opting out of. Getting rid of the 'no_' is of
> course possible, it will mean all symbol_confs will need to initialize
> the value to true (which has minor binary size implications), I also
> think it makes the intention of the code harder to understand.

Well I think perf record is the only place to set it.  But I won't
insist on it strongly.  Let's go with negative if you like.

Thanks,
Namhyung

Re: [PATCH v3 0/9] perf: Default use of build IDs and improvements

[Ian Rogers]

On Mon, Apr 28, 2025 at 2:34 PM Ian Rogers <irogers@google.com> wrote:
>
> Build ID mmap2 events have been available since Linux v5.12 and avoid
> certain races. Enable these by default as discussed in:
> https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/
>
> The dso_id is used to indentify a DSO that may change by being
> overwritten. The inode generation isn't present in /proc/pid/maps and
> so was already only optionally filled in. With build ID mmap events
> the other major, minor and inode varialbes aren't filled in. Change
> the dso_id implementation to make optional values explicit, rather
> than injecting a dso_id we want to improve it during find operations,
> add the buildid to the dso_id for sorting and so that matching fails
> when build IDs vary between DSOs.
>
> Mark the callchain for buildids and not just the sample IP, fixing
> missing DSOs.
>
> Fix sample__for_each_callchain_node to populate the map even when
> symbols aren't computed.
>
> Other minor bits of build_id clean up.
>
> v3: Ensure the struct build_id is initialized empty prior to use as
>     read paths may fail (Namhyung).
>
> v2: Make marking DSOs still the default even with the defaulted build
>     ID mmap. The command line option still disables this to avoid
>     regressions. Add callchain patches and jitdump fix.

Ping. Thanks,
Ian

> Ian Rogers (9):
>   perf callchain: Always populate the addr_location map when adding IP
>   perf build-id: Reduce size of "size" variable
>   perf build-id: Truncate to avoid overflowing the build_id data
>   perf build-id: Change sprintf functions to snprintf
>   perf build-id: Mark DSO in sample callchains
>   perf build-id: Ensure struct build_id is empty before use
>   perf dso: Move build_id to dso_id
>   perf jitdump: Directly mark the jitdump DSO
>   perf record: Make --buildid-mmap the default
>
>  tools/perf/bench/inject-buildid.c             |   2 +-
>  tools/perf/builtin-buildid-cache.c            |  20 ++--
>  tools/perf/builtin-buildid-list.c             |   6 +-
>  tools/perf/builtin-inject.c                   |  36 +++---
>  tools/perf/builtin-record.c                   |  33 ++++--
>  tools/perf/builtin-report.c                   |  11 +-
>  tools/perf/include/perf/perf_dlfilter.h       |   2 +-
>  tools/perf/tests/pe-file-parsing.c            |   2 +-
>  tools/perf/tests/sdt.c                        |   2 +-
>  tools/perf/tests/symbols.c                    |   4 +-
>  tools/perf/util/build-id.c                    |  63 ++++++----
>  tools/perf/util/build-id.h                    |   8 +-
>  tools/perf/util/debuginfo.c                   |   2 +-
>  tools/perf/util/disasm.c                      |   2 +-
>  tools/perf/util/dso.c                         | 111 ++++++++++--------
>  tools/perf/util/dso.h                         |  75 ++++++------
>  tools/perf/util/dsos.c                        |  20 ++--
>  tools/perf/util/event.c                       |   2 +-
>  tools/perf/util/header.c                      |   2 +-
>  tools/perf/util/jitdump.c                     |  21 +++-
>  tools/perf/util/machine.c                     |  34 +++---
>  tools/perf/util/map.c                         |  15 ++-
>  tools/perf/util/map.h                         |   5 +-
>  tools/perf/util/probe-event.c                 |   3 +-
>  tools/perf/util/probe-file.c                  |   4 +-
>  tools/perf/util/probe-finder.c                |   3 +-
>  .../scripting-engines/trace-event-python.c    |   7 +-
>  tools/perf/util/sort.c                        |  27 +++--
>  tools/perf/util/symbol-minimal.c              |   2 +-
>  tools/perf/util/symbol.c                      |   7 +-
>  tools/perf/util/symbol_conf.h                 |   2 +-
>  tools/perf/util/synthetic-events.c            |  44 ++++---
>  tools/perf/util/thread.c                      |   8 +-
>  tools/perf/util/thread.h                      |   2 +-
>  34 files changed, 337 insertions(+), 250 deletions(-)
>
> --
> 2.49.0.901.g37484f566f-goog
>

Re: [PATCH v3 0/9] perf: Default use of build IDs and improvements

[Arnaldo Carvalho de Melo]

On Tue, May 27, 2025 at 01:48:43PM -0700, Ian Rogers wrote:
> On Mon, Apr 28, 2025 at 2:34 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Build ID mmap2 events have been available since Linux v5.12 and avoid
> > certain races. Enable these by default as discussed in:
> > https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/
> >
> > The dso_id is used to indentify a DSO that may change by being
> > overwritten. The inode generation isn't present in /proc/pid/maps and
> > so was already only optionally filled in. With build ID mmap events
> > the other major, minor and inode varialbes aren't filled in. Change
> > the dso_id implementation to make optional values explicit, rather
> > than injecting a dso_id we want to improve it during find operations,
> > add the buildid to the dso_id for sorting and so that matching fails
> > when build IDs vary between DSOs.
> >
> > Mark the callchain for buildids and not just the sample IP, fixing
> > missing DSOs.
> >
> > Fix sample__for_each_callchain_node to populate the map even when
> > symbols aren't computed.
> >
> > Other minor bits of build_id clean up.
> >
> > v3: Ensure the struct build_id is initialized empty prior to use as
> >     read paths may fail (Namhyung).
> >
> > v2: Make marking DSOs still the default even with the defaulted build
> >     ID mmap. The command line option still disables this to avoid
> >     regressions. Add callchain patches and jitdump fix.
> 
> Ping. Thanks,
> Ian

⬢ [acme@toolbx perf-tools-next]$        git am ./v3_20250428_irogers_perf_default_use_of_build_ids_and_improvements.mbx
Applying: perf callchain: Always populate the addr_location map when adding IP
Applying: perf build-id: Reduce size of "size" variable
Applying: perf build-id: Truncate to avoid overflowing the build_id data
Applying: perf build-id: Change sprintf functions to snprintf
Applying: perf build-id: Mark DSO in sample callchains
Applying: perf build-id: Ensure struct build_id is empty before use
Applying: perf dso: Move build_id to dso_id
Applying: perf jitdump: Directly mark the jitdump DSO
Applying: perf record: Make --buildid-mmap the default
error: patch failed: tools/perf/builtin-record.c:3349
error: tools/perf/builtin-record.c: patch does not apply
Patch failed at 0009 perf record: Make --buildid-mmap the default
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
⬢ [acme@toolbx perf-tools-next]$

- Arnaldo

Re: [PATCH v3 0/9] perf: Default use of build IDs and improvements

[Ian Rogers]

On Wed, May 28, 2025 at 11:48 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Tue, May 27, 2025 at 01:48:43PM -0700, Ian Rogers wrote:
> > On Mon, Apr 28, 2025 at 2:34 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > Build ID mmap2 events have been available since Linux v5.12 and avoid
> > > certain races. Enable these by default as discussed in:
> > > https://lore.kernel.org/linux-perf-users/CAP-5=fXP7jN_QrGUcd55_QH5J-Y-FCaJ6=NaHVtyx0oyNh8_-Q@mail.gmail.com/
> > >
> > > The dso_id is used to indentify a DSO that may change by being
> > > overwritten. The inode generation isn't present in /proc/pid/maps and
> > > so was already only optionally filled in. With build ID mmap events
> > > the other major, minor and inode varialbes aren't filled in. Change
> > > the dso_id implementation to make optional values explicit, rather
> > > than injecting a dso_id we want to improve it during find operations,
> > > add the buildid to the dso_id for sorting and so that matching fails
> > > when build IDs vary between DSOs.
> > >
> > > Mark the callchain for buildids and not just the sample IP, fixing
> > > missing DSOs.
> > >
> > > Fix sample__for_each_callchain_node to populate the map even when
> > > symbols aren't computed.
> > >
> > > Other minor bits of build_id clean up.
> > >
> > > v3: Ensure the struct build_id is initialized empty prior to use as
> > >     read paths may fail (Namhyung).
> > >
> > > v2: Make marking DSOs still the default even with the defaulted build
> > >     ID mmap. The command line option still disables this to avoid
> > >     regressions. Add callchain patches and jitdump fix.
> >
> > Ping. Thanks,
> > Ian
>
> ⬢ [acme@toolbx perf-tools-next]$        git am ./v3_20250428_irogers_perf_default_use_of_build_ids_and_improvements.mbx
> Applying: perf callchain: Always populate the addr_location map when adding IP
> Applying: perf build-id: Reduce size of "size" variable
> Applying: perf build-id: Truncate to avoid overflowing the build_id data
> Applying: perf build-id: Change sprintf functions to snprintf
> Applying: perf build-id: Mark DSO in sample callchains
> Applying: perf build-id: Ensure struct build_id is empty before use
> Applying: perf dso: Move build_id to dso_id
> Applying: perf jitdump: Directly mark the jitdump DSO
> Applying: perf record: Make --buildid-mmap the default
> error: patch failed: tools/perf/builtin-record.c:3349
> error: tools/perf/builtin-record.c: patch does not apply
> Patch failed at 0009 perf record: Make --buildid-mmap the default
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> hint: When you have resolved this problem, run "git am --continue".
> hint: If you prefer to skip this patch, run "git am --skip" instead.
> hint: To restore the original branch and stop patching, run "git am --abort".
> hint: Disable this message with "git config set advice.mergeConflict false"
> ⬢ [acme@toolbx perf-tools-next]$

Thanks, I'll send the rebase in v4. I saw you had a branch on
perf-tools-next.git.

Thanks,
Ian

> - Arnaldo