Re: [PATCH] gcc: pass-manager: Fix memory leak. [PR jit/63854]

["Marc Nieper-Wißkirchen" <marc@nieper-wisskirchen.de>]

Thanks for replying so quickly!

Am Do., 6. Jan. 2022 um 14:53 Uhr schrieb David Malcolm <dmalcolm@redhat.com>:

[...]

> Thanks for the patch.
>
> It looks correct to me, given that pass_manager::register_pass_name
> does an xstrdup and puts the result in the map.
>
> That said:
> - I'm not officially a reviewer for this part of gcc (though I probably
> touched this code last)

I am a newcomer to the codebase of GCC and haven't yet been able to
figure out whom to contact. I bothered you because the patch is mostly
relevant for the libgccjit frontend.

> - is it cleaner to instead change m_name_to_pass_map's key type from
> const char * to char *, to convey that the map "owns" the name?  That
> way we probably wouldn't need struct typed_const_free_remove, and (I
> hope) works better with the type system.

The problem with that approach is that we would then need a new
version of string_hash in hash-traits.h, say owned_string_hash, which
derives from pointer_hash <char> and not pointer_hash <const char>.
This would add roughly as much code as struct typed_const_free_remove.
Using the hypothetical owned_string_hash in the definition of
m_name_to_pass_map in passes.c would then produce a map taking "char
*" strings instead of "const char *" strings. This, however, would
then lead to problems in pass_manager::register_pass_name where name
is a "const char *" string (coming from outside) but
m_name_to_pass_map->get would take a "char *" string.

I don't see how to resolve this without bigger refactoring, so I think
my struct typed_const_free_remove approach is less intrusive. This
conveys at least that the key isn't changed by the hashmap operations
and that it is yet owned (because this is something that
typed_const_free_remove presupposes.

Thanks,

Marc

[...]