On 11/21/24 21:42, Sami Tolvanen wrote:
> Expand each structure type only once per exported symbol. This
> is necessary to support self-referential structures, which would
> otherwise result in infinite recursion, and it's sufficient for
> catching ABI changes.
>
> Types defined in .c files are opaque to external users and thus
> cannot affect the ABI. Consider type definitions in .c files to
> be declarations to prevent opaque types from changing symbol
> versions.
Thanks for adding support for skipping types defined in .c files. That
is a useful feature that genksyms has.
I was also recently thinking that it would be great if genksyms could
skip definitions that are in internal header files, for example,
kernel/events/internal.h. Perhaps something that could be added in the
future..
I've noted one nit below, but the patch looks sensible to me, feel free
to use:
Reviewed-by: Petr Pavlu <petr.pavlu@xxxxxxxx>
> @@ -79,6 +80,55 @@ static bool match_export_symbol(struct state *state, Dwarf_Die *die)
> return !!state->sym;
> }
>
> +/* DW_AT_decl_file -> struct srcfile */
> +static struct cache srcfile_cache;
> +
> +static bool is_definition_private(Dwarf_Die *die)
> +{
> + Dwarf_Word filenum;
> + Dwarf_Files *files;
> + Dwarf_Die cudie;
> + const char *s;
> + int res;
> +
> + /*
> + * Definitions in .c files cannot change the public ABI,
> + * so consider them private.
> + */
> + if (!get_udata_attr(die, DW_AT_decl_file, &filenum))
> + return false;
> +
> + res = cache_get(&srcfile_cache, filenum);
> + if (res >= 0)
> + return !!res;
> +
> + if (!dwarf_cu_die(die->cu, &cudie, NULL, NULL, NULL, NULL, NULL, NULL))
> + error("dwarf_cu_die failed: '%s'", dwarf_errmsg(-1));
> +
> + if (dwarf_getsrcfiles(&cudie, &files, NULL))
> + error("dwarf_getsrcfiles failed: '%s'", dwarf_errmsg(-1));
> +
> + s = dwarf_filesrc(files, filenum, NULL, NULL);
> + if (!s)
> + error("dwarf_filesrc failed: '%s'", dwarf_errmsg(-1));
> +
> + s = strrchr(s, '.');
> + res = s && !strcmp(s, ".c");
> + cache_set(&srcfile_cache, filenum, res);
> +
> + return !!res;
> +}
> +
> +static bool is_declaration(Dwarf_Die *die)
> +{
> + bool value;
> +
> + if (get_flag_attr(die, DW_AT_declaration, &value) && value)
> + return true;
> +
> + return is_definition_private(die);
> +}
Nit: When I read the is_declaration() function in isolation, it is not
clear to me what determining if a definition is private has to do with
the type being a declaration. I think this and related logic in
__process_structure_type() would be easier to follow if the return value
of is_declaration() was negated and the function renamed, for example,
to is_kabi_definition().
--
Thanks,
Petr
