Undelivered Mail Returned to Sender

[MAILER-DAEMON@zimbra2.kalray.eu (Mail Delivery System)]

[-- Attachment #1: Notification --]
[-- Type: text/plain, Size: 511 bytes --]

This is the mail system at host zimbra2.kalray.eu.

I'm sorry to have to inform you that your message could not
be delivered to one or more recipients. It's attached below.

For further assistance, please send mail to postmaster.

If you do so, please include this problem report. You can
delete your own text from the attached returned message.

                   The mail system

<bddinechin@kalray.eu>: host zimbra2.kalray.eu[192.168.40.202] said: 452 4.2.2
    Over quota (in reply to end of DATA command)

[-- Attachment #2: Delivery report --]
[-- Type: message/delivery-status, Size: 374 bytes --]

[-- Attachment #3: Undelivered Message --]
[-- Type: message/rfc822, Size: 10964 bytes --]

From: Martin Sebor via Gcc <gcc@gcc.gnu.org>
To: Thomas Schwinge <thomas@codesourcery.com>, gcc@gcc.gnu.org
Subject: Re: 'hash_map<tree, hash_map<tree, tree>>'
Date: Thu, 12 Aug 2021 17:15:44 -0600
Message-ID: <af8fa221-b555-c192-bd99-6eb73db3935f@gmail.com>

On 8/6/21 10:57 AM, Thomas Schwinge wrote:
> Hi!
> 
> So I'm trying to do some C++...  ;-)
> 
> Given:
> 
>      /* A map from SSA names or var decls to record fields.  */
>      typedef hash_map<tree, tree> field_map_t;
> 
>      /* For each propagation record type, this is a map from SSA names or var decls
>         to propagate, to the field in the record type that should be used for
>         transmission and reception.  */
>      typedef hash_map<tree, field_map_t> record_field_map_t;
> 
> Thus, that's a 'hash_map<tree, hash_map<tree, tree>>'.  (I may do that,
> right?)  Looking through GCC implementation files, very most of all uses
> of 'hash_map' boil down to pointer key ('tree', for example) and
> pointer/integer value.

Right.  Because most GCC containers rely exclusively on GCC's own
uses for testing, if your use case is novel in some way, chances
are it might not work as intended in all circumstances.

I've wrestled with hash_map a number of times.  A use case that's
close to yours (i.e., a non-trivial value type) is in cp/parser.c:
see class_to_loc_map_t.  (I don't remember if I tested it for leaks
though.  It's used to implement -Wmismatched-tags so compiling
a few tests under Valgrind should show if it does leak.)

> 
> Then:
> 
>      record_field_map_t field_map ([...]); // see below
>      for ([...])
>        {
>          tree record_type = [...];
>          [...]
>          bool existed;
>          field_map_t &fields
>            = field_map.get_or_insert (record_type, &existed);
>          gcc_checking_assert (!existed);
>          [...]
>          for ([...])
>            fields.put ([...], [...]);
>          [...]
>        }
>      [stuff that looks up elements from 'field_map']
>      field_map.empty ();
> 
> This generally works.
> 
> If I instantiate 'record_field_map_t field_map (40);', Valgrind is happy.
> If however I instantiate 'record_field_map_t field_map (13);' (where '13'
> would be the default for 'hash_map'), Valgrind complains:
> 
>      2,080 bytes in 10 blocks are definitely lost in loss record 828 of 876
>         at 0x483DD99: calloc (vg_replace_malloc.c:762)
>         by 0x175F010: xcalloc (xmalloc.c:162)
>         by 0xAF4A2C: hash_table<hash_map<tree_node*, tree_node*, simple_hashmap_traits<default_hash_traits<tree_node*>, tree_node*> >::hash_entry, false, xcallocator>::hash_table(unsigned long, bool, bool, bool, mem_alloc_origin) (hash-table.h:275)
>         by 0x15E0120: hash_map<tree_node*, tree_node*, simple_hashmap_traits<default_hash_traits<tree_node*>, tree_node*> >::hash_map(unsigned long, bool, bool, bool) (hash-map.h:143)
>         by 0x15DEE87: hash_map<tree_node*, hash_map<tree_node*, tree_node*, simple_hashmap_traits<default_hash_traits<tree_node*>, tree_node*> >, simple_hashmap_traits<default_hash_traits<tree_node*>, hash_map<tree_node*, tree_node*, simple_hashmap_traits<default_hash_traits<tree_node*>, tree_node*> > > >::get_or_insert(tree_node* const&, bool*) (hash-map.h:205)
>         by 0x15DD52C: execute_omp_oacc_neuter_broadcast() (omp-oacc-neuter-broadcast.cc:1371)
>         [...]
> 
> (That's with '#pragma GCC optimize "O0"' at the top of the 'gcc/*.cc'
> file.)
> 
> My suspicion was that it is due to the 'field_map' getting resized as it
> incrementally grows (and '40' being big enough for that to never happen),
> and somehow the non-POD (?) value objects not being properly handled
> during that.  Working my way a bit through 'gcc/hash-map.*' and
> 'gcc/hash-table.*' (but not claiming that I understand all that, off
> hand), it seems as if my theory is right: I'm able to plug this memory
> leak as follows:
> 
>      --- gcc/hash-table.h
>      +++ gcc/hash-table.h
>      @@ -820,6 +820,8 @@ hash_table<Descriptor, Lazy, Allocator>::expand ()
>               {
>                 value_type *q = find_empty_slot_for_expand (Descriptor::hash (x));
>            new ((void*) q) value_type (std::move (x));
>      +     //BAD Descriptor::remove (x); // (doesn't make sense and) a ton of "Invalid read [...] inside a block of size [...] free'd"
>      +     x.~value_type (); //GOOD This seems to work!  -- but does it make sense?
>               }
> 
>             p++;
> 
> However, that doesn't exactly look like a correct fix, does it?  I'd
> expect such a manual destructor call in combination with placement new
> (that is being used here, obviously) -- but this is after 'std::move'?
> However, this also survives a smoke-test-like run of parts of the GCC
> testsuite, bootstrap and complete run now ongoing.

If explicitly calling the dtor on the moved object is the right thing
to do I'd expect to see such invocations elsewhere in hash_table but
I don't.  It does seem like removed elements ought to be destroyed,
but it also seems like the destruction should go through some traits
class (e.g., call Descriptor::remove and mark_deleted or do some
similar dance), and be called from other member functions that move
elements.

Martin


> 
> 
> Grüße
>   Thomas
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
> 



To declare a filtering error, please use the following link : https://www.security-mail.net/reporter.php?mid=9960.6115abaf.e5515.0&r=benoit.dinechin%40kalray.eu&s=gcc-bounces%2Bbenoit.dinechin%3Dkalray.eu%40gcc.gnu.org&o=Re%3A+%27hash_map%3Ctree%2C+hash_map%3Ctree%2C+tree%3E%3E%27&verdict=C&c=5ce25cdf0f1a5a418e600a7ee6f6fd6fdcca4695