Skip to contentSkip to conversation

perf symbol fixes

PATCH New Anton Blanchard 1 participant 5 messages Aug 24, 2011 perf events

lore
Share & tools
lore
#0 lore raw

cover perf symbol fixes

The following patches fix some bugs in our symbol code and adds heuristics to select which duplicate symbol to use.

In the process this fixes the issue on ppc where we need to handle both the old style and new style 64bit ABI.

Anton

#1 lore raw

PATCH 1/4 perf symbols: Fix ppc64 SEGV in dso__load_sym with debuginfo files

64bit PowerPC debuginfo files have an empty function descriptor section. I hit a SEGV when perf tried to use this section for symbol resolution.

To fix this we need to check the section is valid and we can do this by checking for type SHT_PROGBITS.

---

tools/perf/util/symbol.c +2 -0
Index: linux-2.6-tip/tools/perf/util/symbol.c
===================================================================
@@ -1113,6 +1113,8 @@ static int dso__load_sym(struct dso *dso
 	}
 
 	opdsec = elf_section_by_name(elf, &ehdr, &opdshdr, ".opd", &opdidx);
+	if (opdshdr.sh_type != SHT_PROGBITS)
+		opdsec = NULL;
 	if (opdsec)
 		opddata = elf_rawdata(opdsec, NULL);
 
#2 lore raw

PATCH 2/4 perf symbols: /proc/kallsyms does not sort module symbols

In reply to Anton Blanchard

kallsyms__parse assumes that /proc/kallsyms is sorted and sets the end of the previous symbol to the start of the current one.

Unfortunately module symbols are not sorted, eg:

ffffffffa0081f30 t e1000_clean_rx_irq [e1000e]
ffffffffa00817a0 t e1000_alloc_rx_buffers [e1000e]

Some symbols end up with a negative length and others have a length larger than they should. This results in confusing perf output.

We already have a function to fixup the end of zero length symbols so use that instead.

---

tools/perf/util/symbol.c +11 -22
Index: linux-2.6-tip/tools/perf/util/symbol.c
===================================================================
@@ -440,18 +440,11 @@ int kallsyms__parse(const char *filename
 	char *line = NULL;
 	size_t n;
 	int err = -1;
-	u64 prev_start = 0;
-	char prev_symbol_type = 0;
-	char *prev_symbol_name;
 	FILE *file = fopen(filename, "r");
 
 	if (file == NULL)
 		goto out_failure;
 
-	prev_symbol_name = malloc(KSYM_NAME_LEN);
-	if (prev_symbol_name == NULL)
-		goto out_close;
-
 	err = 0;
 
 	while (!feof(file)) {
@@ -482,24 +475,18 @@ int kallsyms__parse(const char *filename
 			break;
 		}
 
-		if (prev_symbol_type) {
-			u64 end = start;
-			if (end != prev_start)
-				--end;
-			err = process_symbol(arg, prev_symbol_name,
-					     prev_symbol_type, prev_start, end);
-			if (err)
-				break;
-		}
-
-		memcpy(prev_symbol_name, symbol_name, len + 1);
-		prev_symbol_type = symbol_type;
-		prev_start = start;
+		/*
+		 * module symbols are not sorted so we add all
+		 * symbols with zero length and rely on
+		 * symbols__fixup_end() to fix it up.
+		 */
+		err = process_symbol(arg, symbol_name,
+				     symbol_type, start, start);
+		if (err)
+			break;
 	}
 
-	free(prev_symbol_name);
 	free(line);
-out_close:
 	fclose(file);
 	return err;
 
@@ -705,6 +692,8 @@ int dso__load_kallsyms(struct dso *dso,
 	if (dso__load_all_kallsyms(dso, filename, map) < 0)
 		return -1;
 
+	symbols__fixup_end(&dso->symbols[map->type]);
+
 	if (dso->kernel == DSO_TYPE_GUEST_KERNEL)
 		dso->symtab_type = SYMTAB__GUEST_KALLSYMS;
 	else
#3 lore raw

PATCH 3/4 perf symbols: Preserve symbol scope when parsing /proc/kallsyms

In reply to Anton Blanchard

kallsyms__parse capitalises the symbol type, so every symbol is marked global. Remove this and fix symbol_type__is_a to handle both local and global symbols.

---

tools/perf/util/symbol.c +4 -2
Index: linux-2.6-tip/tools/perf/util/symbol.c
===================================================================
@@ -76,11 +76,13 @@ static void dso__set_sorted_by_name(stru
 
 bool symbol_type__is_a(char symbol_type, enum map_type map_type)
 {
+	symbol_type = toupper(symbol_type);
+
 	switch (map_type) {
 	case MAP__FUNCTION:
 		return symbol_type == 'T' || symbol_type == 'W';
 	case MAP__VARIABLE:
-		return symbol_type == 'D' || symbol_type == 'd';
+		return symbol_type == 'D';
 	default:
 		return false;
 	}
@@ -465,7 +467,7 @@ int kallsyms__parse(const char *filename
 		if (len + 2 >= line_len)
 			continue;
 
-		symbol_type = toupper(line[len]);
+		symbol_type = line[len];
 		len += 2;
 		symbol_name = line + len;
 		len = line_len - len;
#4 lore raw

PATCH 4/4 perf symbols: Add some heuristics for choosing the best duplicate symbol

In reply to Anton Blanchard

Try and pick the best symbol based on a few heuristics:

-  Prefer a non weak symbol over a weak one
-  Prefer a global symbol over a non global one
-  Prefer a symbol with less underscores (idea taken from kallsyms.c)
-  If all else fails, choose the symbol with the longest name

---

tools/perf/util/symbol.c +88 -0
Index: linux-2.6-tip/tools/perf/util/symbol.c
===================================================================
@@ -88,6 +88,92 @@ bool symbol_type__is_a(char symbol_type,
 	}
 }
 
+static int prefix_underscores_count(const char *str)
+{
+	const char *tail = str;
+
+	while (*tail == '_')
+		tail++;
+
+	return tail - str;
+}
+
+#define SYMBOL_A 0
+#define SYMBOL_B 1
+
+static int choose_best_symbol(struct symbol *syma, struct symbol *symb)
+{
+	s64 a;
+	s64 b;
+
+	/* Prefer a symbol with non zero length */
+	a = syma->end - syma->start;
+	b = symb->end - symb->start;
+	if ((b == 0) && (a > 0))
+		return SYMBOL_A;
+	else if ((a == 0) && (b > 0))
+		return SYMBOL_B;
+
+	/* Prefer a non weak symbol over a weak one */
+	a = syma->binding == STB_WEAK;
+	b = symb->binding == STB_WEAK;
+	if (b && !a)
+		return SYMBOL_A;
+	if (a && !b)
+		return SYMBOL_B;
+
+	/* Prefer a global symbol over a non global one */
+	a = syma->binding == STB_GLOBAL;
+	b = symb->binding == STB_GLOBAL;
+	if (a && !b)
+		return SYMBOL_A;
+	if (b && !a)
+		return SYMBOL_B;
+
+	/* Prefer a symbol with less underscores */
+	a = prefix_underscores_count(syma->name);
+	b = prefix_underscores_count(symb->name);
+	if (b > a)
+		return SYMBOL_A;
+	else if (a > b)
+		return SYMBOL_B;
+
+	/* If all else fails, choose the symbol with the longest name */
+	if (strlen(syma->name) >= strlen(symb->name))
+		return SYMBOL_A;
+	else
+		return SYMBOL_B;
+}
+
+static void symbols__fixup_duplicate(struct rb_root *symbols)
+{
+	struct rb_node *nd;
+	struct symbol *curr, *next;
+
+	nd = rb_first(symbols);
+
+	while (nd) {
+		curr = rb_entry(nd, struct symbol, rb_node);
+again:
+		nd = rb_next(&curr->rb_node);
+		next = rb_entry(nd, struct symbol, rb_node);
+
+		if (!nd)
+			break;
+
+		if (curr->start != next->start)
+			continue;
+
+		if (choose_best_symbol(curr, next) == SYMBOL_A) {
+			rb_erase(&next->rb_node, symbols);
+			goto again;
+		} else {
+			nd = rb_next(&curr->rb_node);
+			rb_erase(&curr->rb_node, symbols);
+		}
+	}
+}
+
 static void symbols__fixup_end(struct rb_root *symbols)
 {
 	struct rb_node *nd, *prevnd = rb_first(symbols);
@@ -694,6 +780,7 @@ int dso__load_kallsyms(struct dso *dso,
 	if (dso__load_all_kallsyms(dso, filename, map) < 0)
 		return -1;
 
+	symbols__fixup_duplicate(&dso->symbols[map->type]);
 	symbols__fixup_end(&dso->symbols[map->type]);
 
 	if (dso->kernel == DSO_TYPE_GUEST_KERNEL)
@@ -1271,6 +1358,7 @@ new_symbol:
 	 * For misannotated, zeroed, ASM function sizes.
 	 */
 	if (nr > 0) {
+		symbols__fixup_duplicate(&dso->symbols[map->type]);
 		symbols__fixup_end(&dso->symbols[map->type]);
 		if (kmap) {
 			/*