Hi Petr,
On Tue, Sep 10, 2024 at 7:58 AM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote:
>
> On 8/15/24 19:39, Sami Tolvanen wrote:
> > @@ -122,6 +135,16 @@ int main(int argc, const char **argv)
> >
> > check(symbol_read_exports(stdin));
> >
> > + if (symtypes_file) {
> > + symfile = fopen(symtypes_file, "w+");
>
> The file is sufficient to open only for writing.
True, I'll drop the +.
> > + if (symfile)
> > + fclose(symfile);
> > +
> > return 0;
> > }
>
> The fclose() call should be wrapped in check() to catch a situation when
> flushing the stream potentially failed.
Ack.
> > +/* See dwarf.c:is_declaration */
> > +#define SYMBOL_DECLONLY_PREFIX "__gendwarfksyms_declonly_"
> > +#define SYMBOL_DECLONLY_PREFIX_LEN (sizeof(SYMBOL_DECLONLY_PREFIX) - 1)
>
> Nit: These defines should go into the patch 15/19 "gendwarfksyms: Add
> support for declaration-only data structures".
Yeah, I noticed these too. Will fix in the next version.
> > +struct type_expansion {
> > + char *name;
> > + struct type_list *expanded;
> > + struct type_list *last;
> > + size_t len;
> > + struct hlist_node hash;
> > +};
>
> I found the manipulation of type_expansion.expanded and
> type_expansion.last somewhat strange.
>
> The list starts already with one element in type_expansion_init(). This
> is apparently to make the last pointer valid. This element is however
> empty and gets only assigned on the first call to type_list_append().
> Other elements are then added normally, always assigned.
>
> Perhaps consider using a regular list implementation, similarly to what
> was discussed under the patch 06/19 "gendwarfksyms: Add a cache for
> processed DIEs".
Agreed, I'll switch this to a regular list in v3.
> > + /* Wrap names with spaces in single quotes */
> > + if (strstr(cache->fqn, " ")) {
> > + format = "%c#'%s'";
> > + len += 2;
> > + } else {
> > + format = "%c#%s";
> > + }
> > +
> > + name = malloc(len);
> > + if (!name) {
> > + error("malloc failed");
> > + return NULL;
> > + }
> > +
> > + if (snprintf(name, len, format, prefix, cache->fqn) >= len) {
> > + error("snprintf failed for '%s' (length %zu)", cache->fqn,
> > + len);
> > + free(name);
> > + return NULL;
> > + }
>
> This could be quite simplified:
>
> const char *quote = strstr(cache->fqn, " ") != NULL ? "'" : "";
> if (asprintf(&name, "%c#%s%s%s", prefix, quote, cache->fqn, quote) < 0)
> [...]
Good point, I'll change this too. Thanks for taking a look!
Sami
