Re: [PATCH v2 02/19] gendwarfksyms: Add symbol list handling

[Petr Pavlu <petr.pavlu@suse.com>]

On 8/15/24 19:39, Sami Tolvanen wrote:
> Add support for passing a list of exported symbols to gendwarfksyms
> via stdin and filter out non-exported symbols from the output.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  scripts/gendwarfksyms/Makefile        |  1 +
>  scripts/gendwarfksyms/dwarf.c         | 53 ++++++++++++++-
>  scripts/gendwarfksyms/gendwarfksyms.c |  4 +-
>  scripts/gendwarfksyms/gendwarfksyms.h | 21 ++++++
>  scripts/gendwarfksyms/symbols.c       | 96 +++++++++++++++++++++++++++
>  5 files changed, 171 insertions(+), 4 deletions(-)
>  create mode 100644 scripts/gendwarfksyms/symbols.c
> 
> diff --git a/scripts/gendwarfksyms/Makefile b/scripts/gendwarfksyms/Makefile
> index c1389c161f9c..623f8fc975ea 100644
> --- a/scripts/gendwarfksyms/Makefile
> +++ b/scripts/gendwarfksyms/Makefile
> @@ -2,6 +2,7 @@ hostprogs-always-y += gendwarfksyms
>  
>  gendwarfksyms-objs += gendwarfksyms.o
>  gendwarfksyms-objs += dwarf.o
> +gendwarfksyms-objs += symbols.o
>  
>  HOST_EXTRACFLAGS := -I $(srctree)/tools/include
>  HOSTLDLIBS_gendwarfksyms := -ldw -lelf
> diff --git a/scripts/gendwarfksyms/dwarf.c b/scripts/gendwarfksyms/dwarf.c
> index 65a29d0bd8f4..71cfab0553da 100644
> --- a/scripts/gendwarfksyms/dwarf.c
> +++ b/scripts/gendwarfksyms/dwarf.c
> @@ -5,6 +5,48 @@
>  
>  #include "gendwarfksyms.h"
>  
> +static bool get_ref_die_attr(Dwarf_Die *die, unsigned int id, Dwarf_Die *value)
> +{
> +	Dwarf_Attribute da;
> +
> +	/* dwarf_formref_die returns a pointer instead of an error value. */
> +	return dwarf_attr(die, id, &da) && dwarf_formref_die(&da, value);
> +}
> +
> +static const char *get_name(Dwarf_Die *die)
> +{
> +	Dwarf_Attribute attr;
> +
> +	/* rustc uses DW_AT_linkage_name for exported symbols */
> +	if (dwarf_attr(die, DW_AT_linkage_name, &attr) ||
> +	    dwarf_attr(die, DW_AT_name, &attr)) {
> +		return dwarf_formstring(&attr);
> +	}
> +
> +	return NULL;
> +}
> +
> +static bool is_export_symbol(struct state *state, Dwarf_Die *die)
> +{
> +	Dwarf_Die *source = die;
> +	Dwarf_Die origin;
> +
> +	state->sym = NULL;

Nit: This assignment isn't strictly necessary, the value is overwritten
a few lines below and isn't used in between.

> +
> +	/* If the DIE has an abstract origin, use it for type information. */
> +	if (get_ref_die_attr(die, DW_AT_abstract_origin, &origin))
> +		source = &origin;
> +
> +	state->sym = symbol_get(get_name(die));
> +
> +	/* Look up using the origin name if there are no matches. */
> +	if (!state->sym && source != die)
> +		state->sym = symbol_get(get_name(source));
> +
> +	state->die = *source;
> +	return !!state->sym;
> +}
> +
>  /*
>   * Type string processing
>   */
> @@ -40,7 +82,7 @@ int process_die_container(struct state *state, Dwarf_Die *die,
>  }
>  
>  /*
> - * Symbol processing
> + * Exported symbol processing
>   */
>  static int process_subprogram(struct state *state, Dwarf_Die *die)
>  {
> @@ -67,10 +109,15 @@ static int process_exported_symbols(struct state *state, Dwarf_Die *die)
>  	/* Possible exported symbols */
>  	case DW_TAG_subprogram:
>  	case DW_TAG_variable:
> +		if (!is_export_symbol(state, die))
> +			return 0;
> +
> +		debug("%s", state->sym->name);
> +
>  		if (tag == DW_TAG_subprogram)
> -			check(process_subprogram(state, die));
> +			check(process_subprogram(state, &state->die));
>  		else
> -			check(process_variable(state, die));
> +			check(process_variable(state, &state->die));
>  
>  		return 0;
>  	default:
> diff --git a/scripts/gendwarfksyms/gendwarfksyms.c b/scripts/gendwarfksyms/gendwarfksyms.c
> index 27f2d6423c45..d209b237766b 100644
> --- a/scripts/gendwarfksyms/gendwarfksyms.c
> +++ b/scripts/gendwarfksyms/gendwarfksyms.c
> @@ -27,7 +27,7 @@ static const struct {
>  
>  static int usage(void)
>  {
> -	error("usage: gendwarfksyms [options] elf-object-file ...");
> +	error("usage: gendwarfksyms [options] elf-object-file ... < symbol-list");
>  	return -1;
>  }
>  
> @@ -105,6 +105,8 @@ int main(int argc, const char **argv)
>  	if (parse_options(argc, argv) < 0)
>  		return usage();
>  
> +	check(symbol_read_exports(stdin));
> +
>  	for (n = 0; n < object_count; n++) {
>  		Dwfl *dwfl;
>  		int fd;
> diff --git a/scripts/gendwarfksyms/gendwarfksyms.h b/scripts/gendwarfksyms/gendwarfksyms.h
> index 5ab7ce7d4efb..03f3e408a839 100644
> --- a/scripts/gendwarfksyms/gendwarfksyms.h
> +++ b/scripts/gendwarfksyms/gendwarfksyms.h
> @@ -7,9 +7,11 @@
>  #include <elfutils/libdw.h>
>  #include <elfutils/libdwfl.h>
>  #include <linux/hashtable.h>
> +#include <linux/jhash.h>
>  #include <inttypes.h>
>  #include <stdlib.h>
>  #include <stdio.h>
> +#include <string.h>
>  
>  #ifndef __GENDWARFKSYMS_H
>  #define __GENDWARFKSYMS_H
> @@ -56,6 +58,23 @@ extern bool debug;
>  /* Error == negative values */
>  #define checkp(expr) __check(expr, __res < 0, __res)
>  
> +/*
> + * symbols.c
> + */
> +
> +static inline u32 name_hash(const char *name)
> +{
> +	return jhash(name, strlen(name), 0);
> +}
> +
> +struct symbol {
> +	const char *name;
> +	struct hlist_node name_hash;
> +};
> +
> +extern int symbol_read_exports(FILE *file);
> +extern struct symbol *symbol_get(const char *name);

Nit: extern isn't necessary here and in other similar cases throughout
the series. It should be removed per
Documentation/process/coding-style.rst, 6.1) Function prototypes.

> +
>  /*
>   * dwarf.c
>   */
> @@ -63,6 +82,8 @@ extern bool debug;
>  struct state {
>  	Dwfl_Module *mod;
>  	Dwarf *dbg;
> +	struct symbol *sym;
> +	Dwarf_Die die;
>  };
>  
>  typedef int (*die_callback_t)(struct state *state, Dwarf_Die *die);
> diff --git a/scripts/gendwarfksyms/symbols.c b/scripts/gendwarfksyms/symbols.c
> new file mode 100644
> index 000000000000..673ad9cf9e77
> --- /dev/null
> +++ b/scripts/gendwarfksyms/symbols.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2024 Google LLC
> + */
> +
> +#include "gendwarfksyms.h"
> +
> +#define SYMBOL_HASH_BITS 15
> +static DEFINE_HASHTABLE(symbol_names, SYMBOL_HASH_BITS);
> +
> +typedef int (*symbol_callback_t)(struct symbol *, void *arg);
> +
> +static int for_each(const char *name, symbol_callback_t func, void *data)
> +{
> +	struct hlist_node *tmp;
> +	struct symbol *match;
> +
> +	if (!name || !*name)
> +		return 0;
> +
> +	hash_for_each_possible_safe(symbol_names, match, tmp, name_hash,
> +				    name_hash(name)) {
> +		if (strcmp(match->name, name))
> +			continue;
> +
> +		if (func)
> +			check(func(match, data));
> +
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static bool is_exported(const char *name)
> +{
> +	return checkp(for_each(name, NULL, NULL)) > 0;
> +}
> +
> +int symbol_read_exports(FILE *file)
> +{
> +	struct symbol *sym;
> +	char *line = NULL;
> +	char *name = NULL;
> +	size_t size = 0;
> +	int nsym = 0;
> +
> +	while (getline(&line, &size, file) > 0) {
> +		if (sscanf(line, "%ms\n", &name) != 1) {
> +			error("malformed input line: %s", line);
> +			return -1;
> +		}
> +
> +		free(line);
> +		line = NULL;
> +
> +		if (is_exported(name))
> +			continue; /* Ignore duplicates */
> +
> +		sym = malloc(sizeof(struct symbol));
> +		if (!sym) {
> +			error("malloc failed");
> +			return -1;
> +		}
> +
> +		sym->name = name;
> +		name = NULL;
> +
> +		hash_add(symbol_names, &sym->name_hash, name_hash(sym->name));
> +		++nsym;
> +
> +		debug("%s", sym->name);
> +	}
> +
> +	if (line)
> +		free(line);

The loop leaks line on a potential sscanf() error and name if the symbol
is a duplicate or malloc(sizeof(struct symbol)) fails. Additionally, it
should be possible to avoid allocating line by getline() on each
iteration.

I would change it to something like this (not tested):

int symbol_read_exports(FILE *file)
{
	struct symbol *sym;
	char *line = NULL;
	char *name = NULL;
	size_t size = 0;
	int nsym = 0;
	int ret = -1;

	while (getline(&line, &size, file) > 0) {
		if (sscanf(line, "%ms\n", &name) != 1) {
			error("malformed input line: %s", line);
			goto out;
		}

		if (is_exported(name)) {
			/* Ignore duplicates */
			free(name);
			name = NULL;
			continue;
		}

		sym = malloc(sizeof(struct symbol));
		if (!sym) {
			error("malloc failed");
			goto out;
		}

		sym->name = name;
		name = NULL;

		hash_add(symbol_names, &sym->name_hash, name_hash(sym->name));
		++nsym;

		debug("%s", sym->name);
	}

	debug("%d exported symbols", nsym);
	ret = 0;

out:
	free(line);
	free(name);
	return ret;
}

> +
> +	debug("%d exported symbols", nsym);
> +	return 0;
> +}
> +
> +static int get_symbol(struct symbol *sym, void *arg)
> +{
> +	struct symbol **res = arg;
> +
> +	*res = sym;
> +	return 0;
> +}
> +
> +struct symbol *symbol_get(const char *name)
> +{
> +	struct symbol *sym = NULL;
> +
> +	for_each(name, get_symbol, &sym);
> +	return sym;
> +}

-- 
Thanks,
Petr