On Mon, 13 Jan 2020, David Malcolm wrote: > I posted the initial version of the analyzer patch kit on 2019-11-15, > shortly before the close of stage 1. > > Jeff reviewed (most of) the latest version of the kit on Friday, and > said: > > > I'm not going to have time to finish #22 or #37 -- hell, I'm not even > > supposed to be working today :-) > > > > I'd suggest committing now and we can iterate on any issues. The code > > is appropriately conditionalized, so it shouldn't affect anyone that > > doesn't ask for it and it was submitted prior to stage1 close. > > > > I would also suggest that we have a fairly loose policy for these bits > > right now. Again, they're conditionally compiled and it's effectively > > "tech preview", so if we muck something up, the after-effects are > > relatively small. > > 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've been experimenting with adding template specializations to try > to allow for memset to 0 for hash_map, but haven't been successful > (e.g. std::is_pod is C++11-only); my closest attempt so far is at: > > https://dmalcolm.fedorapeople.org/gcc/2020-01-13/0001-FIXME-experiments-with-fixing-hash_map-empty.patch > which at least expresses the memset to 0 without needing > optimization for hash_table of *pointer* types, but I wasn't able to > do the same for hash_map, today at least. > > If the hash_map::empty patch is unacceptable, I've also successfully > B&R-ed the following kludge to the analyzer, which avoids using > hash_map::empty at the single place where it's problematic. This > kludge is entirely confined to the analyzer. > > I'd like to get the analyzer code into gcc 10, but I appreciate it's > now stage 4. Jakub suggested on IRC that with approval before the > end of stage 3 (which it was), there may be some flexibility if we > get it in soon and I take steps to minimize disruption. > > 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 > > My preferred approach is option (a), but option (b) is confined to > the analyzer. > > Thoughts?
(b) we can iterate on (a)/(c) if deemed necessary but also this can be done during next stage1. > I also have a series of followup patches to the analyzer (and > diagnostic subsystem, for diagnostic_path-handling) to fix bugs > found since I posted the kit, which I've been posting to gcc-patches > since November with an "analyzer: " prefix in the subject line. > I wonder if it's OK for me to take Jeff's message about "fairly > loose policy" above as blanket pre-approval to make bug fixes to the > analyzer subdirectory? If the analyzer is defaulted to be disabled at compile-time then changes cannot disrupt anything release critical. In that case I'd be fine with you mucking at will inside analyzer/ with your own risk ask shipping something unusable. Richard. > (See also: > https://gcc.gnu.org/wiki/DavidMalcolm/StaticAnalyzer ) > > Here's the analyzer-specific kludge for the hash_map::empty issue: > > --- > gcc/analyzer/program-state.cc | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/gcc/analyzer/program-state.cc b/gcc/analyzer/program-state.cc > index 04346ae9dc8..4c6c82cdf5d 100644 > --- a/gcc/analyzer/program-state.cc > +++ b/gcc/analyzer/program-state.cc > @@ -405,7 +405,19 @@ sm_state_map::remap_svalue_ids (const svalue_id_map &map) > } > > /* Clear the existing values. */ > + /* FIXME: hash_map::empty and hash_table::empty make the assumption that > + the empty value for the key is all zeroes, which isn't the case for > + this hash_map. See > + https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00776.html > + and > + https://gcc.gnu.org/ml/gcc-patches/2020-01/msg00651.html */ > +#if 0 > m_map.empty (); > +#else > + /* Workaround: manually remove each element. */ > + while (!m_map.is_empty ()) > + m_map.remove ((*m_map.begin ()).first); > +#endif > > /* Copy over from intermediate map. */ > for (typename map_t::iterator iter = tmp_map.begin (); > -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
