On Mon, Jan 13, 2020 at 05:10:24PM -0500, David Malcolm wrote:
> Unfortunately, I didn't resolve the hash_table/hash_map issue
> referred to here:
> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00734.html
> where r279139 on 2019-12-09 introduced the assumption that empty
> hash_table entries and empty hash_map keys are all zeroes.
>
> The analyzer uses hash_map::empty in a single place with a hash_map
> where the "empty" key value is all-ones.
>
> Unfortunately, without a fix for this issue, the analyzer can hang,
> leading to DejaGnu hanging, which I clearly don't want to push to
> master as-is.
>
> The patch here fixes the issue:
> https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00776.html
> but Jakub has expressed concern about the effect on code generated
> for the compiler.
>
> As noted here:
> https://gcc.gnu.org/ml/gcc-patches/2020-01/msg00651.html
> gcc successfully converts this back to a memset to 0 for hash_table
> at -O2, but not for hash_map, since it can't "know" that it's OK to
> clobber the values as well as the keys; it instead generates a loop
> that zeroes the keys without touching the values.
I guess in some cases the loop could be better (e.g. if the values are
large and/or keys too and mark_empty only sets something small), but I bet
in most cases the memset will be better.
> Some options:
> (a) the patch to fix hash_table::empty, and the analyzer kit
> (b) the analyzer kit with the following kludge
> (c) someone with better C++-fu than me figure out a way to get the
> memset optimization for hash_map with 0-valued empty (or to give me
> some suggestions)
> (d) not merge the analyzer for gcc 10
For (c), I see right now we have 37 mark_empty definitions:
find -type f | grep -v 'testsuite/\|ChangeLog' | xargs grep mark_empty | wc -l
37
From quick skimming of them, most of them are just zeroing one or more
fields, exceptions are profile.c, attribs.c (this one only in self-tests
and it is unclear why deleted is two NULLs and empty two ""s rather than
vice versa), and gcov.c. Also, int_hash can be zero or non-zero, depending
on template argument, e.g.
typedef hash_map<int_hash <unsigned int, -1U>, unsigned int> live_vars_map;
struct uid_hash : int_hash <int, -1, -2> {};
are not either.
Can't we add next to the mark_empty and is_empty methods also a static const
bool data member empty_zero_p or similar and use it in in the two
hash-table.h spots where this would make a difference?
In alloc_entries the
for (size_t i = 0; i < n; i++)
mark_empty (nentries[i]);
loop could be conditionalized on !Descriptor::empty_zero_p because both
allocation paths actually allocate cleared memory, and in the spot
you are talking about.
Or isn't there (e), tweak whatever hash_table traits you use so that
empty case is all zeros?
Jakub
