Re: gcc_assert() and inhibit_libc
On 8/12/21 4:31 PM, Sebastian Huber wrote: This would be suitable for me, however, I am not sure if you want such a customization feature just for a niche operating system. I don't see a reason why not. Please send a patch. Martin
Re: 'hash_map>'
Hi!
On 2021-08-07T09:54:53+0100, Jonathan Wakely via Gcc wrote:
> On Sat, 7 Aug 2021, 09:08 Thomas Schwinge, wrote:
>> On 2021-08-06T19:37:58+0100, Jonathan Wakely wrote:
>> > On Fri, 6 Aug 2021, 17:58 Thomas Schwinge, wrote:
>> >> So I'm trying to do some C++... ;-)
>> >>
>> >> Given:
>> >>
>> >> /* A map from SSA names or var decls to record fields. */
>> >> typedef hash_map 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 record_field_map_t;
>> >>
>> >> Thus, that's a 'hash_map>'. (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.
>> >>
>> >> 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: [...]
>> >>
>> >> 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::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.
>>
>> That testing came back without any issues.
>>
>> > Does GCC's hash_map assume you only use it to store POD (plain old data)
>> > types
>>
>> Don't you disappoint me, C++!
>
> It's not a limitation of C++, just this data structure.
(Understood, of course. Yet, the programming language paves the way for
making it "easy" to achieve similar behavior for different kinds of data
types -- but I know, the devil's in the details, always.)
Actually, I suppose not "non-POD" is the problem here, but rather
non-trivial constructor/destructor, because the latter is how you have a
C++ class data type allocate additional resources (such as memory), which
is what's the problem here regarding the memory leak.
>> > which don't need to be destroyed, because they don't have any
>> > dynamically allocated memory or other resources?
>> >
>> > A hash_map is not a POD, because it does have dynamically allocated memory.
>>
>> ACK, that's what I tried to say above in my "layman's terms". ;-)
>>
>> > If my guess is right, then hash_map should really use a static_assert to
>> > enforce that requirement, instead of letting you use it in a way that will
>> > leak.
>>
>> Eh, yes, at the very least!
'gcc/hash-map.h':
/* Class hash_map is a hash-value based container mapping objects of
KeyId type to those of the Value type.
Both KeyId and Value may be non-trivial (non-POD) types provided
a suitabe Traits class. [...]
..., so this ought to work in principle. Indeed, if I try:
--- gcc/hash-map.h
+++ gcc/hash-map.h
@@ -38,6 +38,9 @@ template */>
class GTY((user)) hash_map
{
+ static_assert (std::is_pod::value, "non-POD KeyId");
+ static_assert (std::is_pod::value, "non-POD Value");
+
[...]
... we get a very lot of
Re: 'hash_map>'
Hi!
On 2021-08-09T12:02:02+0200, Richard Biener via Gcc wrote:
> On Fri, Aug 6, 2021 at 6:58 PM Thomas Schwinge
> wrote:
>> So I'm trying to do some C++... ;-)
>>
>> Given:
>>
>> /* A map from SSA names or var decls to record fields. */
>> typedef hash_map 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 record_field_map_t;
>>
>> Thus, that's a 'hash_map>'. (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.
>
> You could use
>
> hash_map record_field_map_p;
> vec maps;
>
> and record the index into maps which you record the actual maps.
Ugh ;-) -- yes, if I remember correctly, I've spotted that pattern in a
few GCC source files.
> Alternatively use hash_map
ACK; see commit 049eda8274b7394523238b17ab12c3e2889f253e "Avoid 'GTY' use
for 'gcc/omp-oacc-neuter-broadcast.cc:field_map'". (So, actually don't
change 'record_field_map_t' at this time.)
> Note your code will appear to work until you end up in the situation
> where record_field_map_t is resized because hash-map doesn't
> std::move elements when re-hashing.
I'm not understanding, would you please provide some more detail here?
Did you mean "resizing" instead of "re-hashing"? If the latter, then
yes, it does 'std::move' them (via 'hash_table::expand') -- see (your
own) commit 4b9d61f79c0c0185a33048ae6cc72269cf7efa31 "add move CTOR to
auto_vec, use auto_vec for get_loop_exit_edges". But I guess I
completely misunderstood?
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
Re: 'hash_map>'
Hi!
On 2021-08-12T17:15:44-0600, Martin Sebor via Gcc wrote:
> On 8/6/21 10:57 AM, Thomas Schwinge wrote:
>> So I'm trying to do some C++... ;-)
>>
>> Given:
>>
>> /* A map from SSA names or var decls to record fields. */
>> typedef hash_map 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 record_field_map_t;
>>
>> Thus, that's a 'hash_map>'. (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.
Indeed, at the time you sent this email, I already had started looking
into that one! (The Fortran test cases that I originally analyzed, which
triggered other cases of non-POD/non-trivial destructor, all didn't
result in a memory leak, because the non-trivial constructor doesn't
actually allocate any resources dynamically -- that's indeed different in
this case here.) ..., and indeed:
> (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.)
... it does leak memory at present. :-| (See attached commit log for
details for one example.)
To that effect, to document the current behavior, I propose to
"Add more self-tests for 'hash_map' with Value type with non-trivial
constructor/destructor", see attached. OK to push to master branch?
(Also cherry-pick into release branches, eventually?)
>> 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> simple_hashmap_traits, tree_node*>
>> >::hash_entry, false, xcallocator>::hash_table(unsigned long, bool, bool,
>> bool, mem_alloc_origin) (hash-table.h:275)
>> by 0x15E0120: hash_map> simple_hashmap_traits, tree_node*>
>> >::hash_map(unsigned long, bool, bool, bool) (hash-map.h:143)
>> by 0x15DEE87: hash_map> simple_hashmap_traits, tree_node*> >,
>> simple_hashmap_traits, hash_map> tree_node*, simple_hashmap_traits,
>> 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::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, th
Re: 'hash_map>'
On Mon, Aug 16, 2021 at 2:44 PM Thomas Schwinge wrote:
>
> Hi!
>
> On 2021-08-09T12:02:02+0200, Richard Biener via Gcc wrote:
> > On Fri, Aug 6, 2021 at 6:58 PM Thomas Schwinge
> > wrote:
> >> So I'm trying to do some C++... ;-)
> >>
> >> Given:
> >>
> >> /* A map from SSA names or var decls to record fields. */
> >> typedef hash_map 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 record_field_map_t;
> >>
> >> Thus, that's a 'hash_map>'. (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.
> >
> > You could use
> >
> > hash_map record_field_map_p;
> > vec maps;
> >
> > and record the index into maps which you record the actual maps.
>
> Ugh ;-) -- yes, if I remember correctly, I've spotted that pattern in a
> few GCC source files.
>
> > Alternatively use hash_map
>
> ACK; see commit 049eda8274b7394523238b17ab12c3e2889f253e "Avoid 'GTY' use
> for 'gcc/omp-oacc-neuter-broadcast.cc:field_map'". (So, actually don't
> change 'record_field_map_t' at this time.)
>
>
> > Note your code will appear to work until you end up in the situation
> > where record_field_map_t is resized because hash-map doesn't
> > std::move elements when re-hashing.
>
> I'm not understanding, would you please provide some more detail here?
>
> Did you mean "resizing" instead of "re-hashing"?
yes
> If the latter, then
> yes, it does 'std::move' them (via 'hash_table::expand') -- see (your
> own) commit 4b9d61f79c0c0185a33048ae6cc72269cf7efa31 "add move CTOR to
> auto_vec, use auto_vec for get_loop_exit_edges". But I guess I
> completely misunderstood?
Oh, indeed I fixed that ;) But then hash_table doesn't have a move CTOR ...
I bet that it also was incomplete in some other way (I ended up not actually
needing this std::move stuff for hash_map/autp_vec after all...). But the DTOR
piece should be OK with a suitable traits class I think (the ::remove
trait member
should call it).
>
>
> 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
Re: gcc_assert() and inhibit_libc
On 16/08/2021 14:33, Martin Liška wrote: On 8/12/21 4:31 PM, Sebastian Huber wrote: This would be suitable for me, however, I am not sure if you want such a customization feature just for a niche operating system. I don't see a reason why not. Please send a patch. Ok, good. I will try to figure out what can be done. One problem is that tsystem.h is included before tm.h. Independent of this Joseph S. Myers said in the recent patch review with respect to the gcov_type size that removing tm.h from the target libraries is a development goal. I guess we have a couple of options. 1. Detect the presence of __assert_func and add the result to tconfig.h. This can't be a link time check, since libgcc is build before Newlib. 2. Use __builtin_trap() instead of abort() if inhibit_libc is defined. The trap builtin is target-specific. Making this system-specific (in this case RTEMS) could be an issue. 3. Add a new target hook for the gcc_assert() failure. This has the same problem as 2. with respect to customization by system and not architecture. 4. Disable gcc_assert() if inhibit_libc is defined. 5. Use builtin __rtems__ define in tsystem.h as a hack. -- embedded brains GmbH Herr Sebastian HUBER Dornierstr. 4 82178 Puchheim Germany email: sebastian.hu...@embedded-brains.de phone: +49-89-18 94 741 - 16 fax: +49-89-18 94 741 - 08 Registergericht: Amtsgericht München Registernummer: HRB 157899 Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler Unsere Datenschutzerklärung finden Sie hier: https://embedded-brains.de/datenschutzerklaerung/
Re: [PATCH] Try LTO partial linking. (Was: Speed of compiling gimple-match.c)
PING^2 @Honza: Can you please review the change? Martin On 6/23/21 3:53 PM, Martin Liška wrote: On 5/21/21 10:29 AM, Martin Liška wrote: On 5/20/21 5:55 PM, Jan Hubicka wrote: Quick solution is to also modify partitioner to use the local symbol names when doing incremental linking (those mixing in source code and random seeds) to avoid clashes. Good hint. I added hash based on object file name (I don't want to handle proper string escaping) and -frandom-seed. What do you think about the patch? Thanks, Martin @Honza: Can you please take a look at this patch? Cheers, Martin
Re: gcc_assert() and inhibit_libc
On Mon, Aug 16, 2021 at 9:51 AM Sebastian Huber wrote: > > On 16/08/2021 14:33, Martin Liška wrote: > > On 8/12/21 4:31 PM, Sebastian Huber wrote: > >> This would be suitable for me, however, I am not sure if you want such > >> a customization feature just for a niche operating system. > > > > I don't see a reason why not. > > Please send a patch. > > Ok, good. I will try to figure out what can be done. One problem is that > tsystem.h is included before tm.h. Independent of this Joseph S. Myers > said in the recent patch review with respect to the gcov_type size that > removing tm.h from the target libraries is a development goal. > > I guess we have a couple of options. > > 1. Detect the presence of __assert_func and add the result to tconfig.h. > This can't be a link time check, since libgcc is build before Newlib. > > 2. Use __builtin_trap() instead of abort() if inhibit_libc is defined. I still think this seems the most straightforward approach. > The trap builtin is target-specific. Making this system-specific (in > this case RTEMS) could be an issue. Is that necessary? Are there interesting targets that don't have a trap insn? > 3. Add a new target hook for the gcc_assert() failure. This has the same > problem as 2. with respect to customization by system and not architecture. > > 4. Disable gcc_assert() if inhibit_libc is defined. > > 5. Use builtin __rtems__ define in tsystem.h as a hack. 6. Suppress ENABLE_RUNTIME_CHECKING when inhibit_libc. Jason
Re: gcc_assert() and inhibit_libc
On Mon, 16 Aug 2021, Sebastian Huber wrote: > Ok, good. I will try to figure out what can be done. One problem is that > tsystem.h is included before tm.h. Independent of this Joseph S. Myers said > in the recent patch review with respect to the gcov_type size that removing > tm.h from the target libraries is a development goal. That is, removing the *host* tm.h from the target libraries. libgcc_tm.h (and the target macros defined therein that are only ever defined in libgcc, not in the host parts of the compiler) is not a problem. -- Joseph S. Myers jos...@codesourcery.com
Re: gcc_assert() and inhibit_libc
On Mon, Aug 16, 2021 at 12:50:49PM -0400, Jason Merrill via Gcc wrote: > > The trap builtin is target-specific. Making this system-specific (in > > this case RTEMS) could be an issue. > > Is that necessary? Are there interesting targets that don't have a trap insn? Depends on the definition of interesting. I think avr, bpf, c6x, cr16, epiphany, fr30, frv, ft32, h8300, lm32, m32c, m32r, mcore, mmix, mn10300, moxie, msp430, or1k, pdp11, pru, rl78, rx, sh, stormy16, v850 and vax don't have trap insn, while aarch64, alpha, arc, arm, bfin, cris, csky, gcn, i386, ia64, iq2000, m68k, microblaze, mips, nds32, nios2, nvptx, pa, riscv, rs6000, s390, sparc, tilegx, tilepro, visium and xtensa have them. Jakub
Re: gcc_assert() and inhibit_libc
On 8/16/2021 11:06 AM, Jakub Jelinek via Gcc wrote: On Mon, Aug 16, 2021 at 12:50:49PM -0400, Jason Merrill via Gcc wrote: The trap builtin is target-specific. Making this system-specific (in this case RTEMS) could be an issue. Is that necessary? Are there interesting targets that don't have a trap insn? Depends on the definition of interesting. I think avr, bpf, c6x, cr16, epiphany, fr30, frv, ft32, h8300, lm32, m32c, m32r, mcore, mmix, mn10300, moxie, msp430, or1k, pdp11, pru, rl78, rx, sh, stormy16, v850 and vax don't have trap insn, while aarch64, alpha, arc, arm, bfin, cris, csky, gcn, i386, ia64, iq2000, m68k, microblaze, mips, nds32, nios2, nvptx, pa, riscv, rs6000, s390, sparc, tilegx, tilepro, visium and xtensa have them. Probably safer to say "no trap insn currently defined". I'd bet multiple targets in the first set have trap insns defined in their ISA, but not in the MD file. jeff
Re: 'hash_map>'
On 8/16/21 6:44 AM, Thomas Schwinge wrote:
Hi!
On 2021-08-12T17:15:44-0600, Martin Sebor via Gcc wrote:
On 8/6/21 10:57 AM, Thomas Schwinge wrote:
So I'm trying to do some C++... ;-)
Given:
/* A map from SSA names or var decls to record fields. */
typedef hash_map 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 record_field_map_t;
Thus, that's a 'hash_map>'. (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.
Indeed, at the time you sent this email, I already had started looking
into that one! (The Fortran test cases that I originally analyzed, which
triggered other cases of non-POD/non-trivial destructor, all didn't
result in a memory leak, because the non-trivial constructor doesn't
actually allocate any resources dynamically -- that's indeed different in
this case here.) ..., and indeed:
(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.)
... it does leak memory at present. :-| (See attached commit log for
details for one example.)
To that effect, to document the current behavior, I propose to
"Add more self-tests for 'hash_map' with Value type with non-trivial
constructor/destructor", see attached. OK to push to master branch?
(Also cherry-pick into release branches, eventually?)
Adding more tests sounds like an excellent idea. I'm not sure about
the idea of adding loopy selftests that iterate as many times as in
the patch (looks like 1234 times two?) Selftests run each time GCC
builds (i.e., even during day to day development). It seems to me
that it might be better to run such selftests only as part of
the bootstrap process.
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, 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*>
>::hash_map(unsigned long, bool, bool, bool) (hash-map.h:143)
by 0x15DEE87: hash_map, tree_node*> >,
simple_hashmap_traits, hash_map, 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::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
(tha
Expensive selftests (was: 'hash_map>')
Hi!
On 2021-08-16T14:10:00-0600, Martin Sebor wrote:
> On 8/16/21 6:44 AM, Thomas Schwinge wrote:
>> [...], to document the current behavior, I propose to
>> "Add more self-tests for 'hash_map' with Value type with non-trivial
>> constructor/destructor", see attached. OK to push to master branch?
>> (Also cherry-pick into release branches, eventually?)
(Attached again, for easy reference.)
> Adding more tests sounds like an excellent idea. I'm not sure about
> the idea of adding loopy selftests that iterate as many times as in
> the patch (looks like 1234 times two?)
Correct, and I agree it's a sensible concern, generally.
The current 1234 times two iterations is really arbitrary (should
document that in the test case), just so that we trigger a few hash table
expansions.
For 'selftest-c', we've got originally:
-fself-test: 74775 pass(es) in 0.309299 seconds
-fself-test: 74775 pass(es) in 0.366041 seconds
-fself-test: 74775 pass(es) in 0.356663 seconds
-fself-test: 74775 pass(es) in 0.355009 seconds
-fself-test: 74775 pass(es) in 0.367575 seconds
-fself-test: 74775 pass(es) in 0.320406 seconds
..., and with my changes we've got:
-fself-test: 94519 pass(es) in 0.327755 seconds
-fself-test: 94519 pass(es) in 0.369522 seconds
-fself-test: 94519 pass(es) in 0.355531 seconds
-fself-test: 94519 pass(es) in 0.362179 seconds
-fself-test: 94519 pass(es) in 0.363176 seconds
-fself-test: 94519 pass(es) in 0.318930 seconds
So it really seems to be all in the noise?
Yet:
> Selftests run each time GCC
> builds (i.e., even during day to day development). It seems to me
> that it might be better to run such selftests only as part of
> the bootstrap process.
I'd rather have thought about a '--param self-test-expensive' (or
similar), and then invoke the selftests via a new
'gcc/testsuite/selftests/expensive.exp' (or similar).
Or, adapt 'gcc/testsuite/gcc.dg/plugin/expensive_selftests_plugin.c',
that is, invoke them via the GCC plugin mechanism, which also seems to be
easy enough?
I don't have a strong opinion about where/when these tests get run, so
will happily take any suggestions.
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
>From 12fda2ece45ea477cdc9a697b5efc6e51c9da229 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge
Date: Fri, 13 Aug 2021 17:53:12 +0200
Subject: [PATCH] Add more self-tests for 'hash_map' with Value type with
non-trivial constructor/destructor
... to document the current behavior.
gcc/
* hash-map-tests.c (test_map_of_type_with_ctor_and_dtor): Extend.
(test_map_of_type_with_ctor_and_dtor_expand): Add function.
(hash_map_tests_c_tests): Call it.
---
gcc/hash-map-tests.c | 142 +++
1 file changed, 142 insertions(+)
diff --git a/gcc/hash-map-tests.c b/gcc/hash-map-tests.c
index 5b6b192cd28..3c79a13c1a8 100644
--- a/gcc/hash-map-tests.c
+++ b/gcc/hash-map-tests.c
@@ -278,6 +278,146 @@ test_map_of_type_with_ctor_and_dtor ()
ASSERT_TRUE (val_t::ndefault + val_t::ncopy == val_t::ndtor);
}
+
+
+ /* Verify basic construction and destruction of Value objects. */
+ {
+const int N_elem = 1234;
+void *a[N_elem];
+for (int i = 0; i < N_elem; ++i)
+ a[i] = &a[i];
+
+const int N_init = 44;
+
+val_t::ndefault = 0;
+val_t::ncopy = 0;
+val_t::nassign = 0;
+val_t::ndtor = 0;
+Map m (N_init);
+ASSERT_EQ (val_t::ndefault
+ + val_t::ncopy
+ + val_t::nassign
+ + val_t::ndtor, 0);
+
+for (int i = 0; i < N_elem; ++i)
+ {
+ m.get_or_insert (a[i]);
+ ASSERT_EQ (val_t::ndefault, 1 + i);
+ ASSERT_EQ (val_t::ncopy, 0);
+ ASSERT_EQ (val_t::nassign, 0);
+ ASSERT_EQ (val_t::ndtor, i);
+
+ m.remove (a[i]);
+ ASSERT_EQ (val_t::ndefault, 1 + i);
+ ASSERT_EQ (val_t::ncopy, 0);
+ ASSERT_EQ (val_t::nassign, 0);
+ ASSERT_EQ (val_t::ndtor, 1 + i);
+ }
+ }
+}
+
+/* Verify 'hash_table::expand'. */
+
+static void
+test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline)
+{
+ typedef hash_map Map;
+
+ /* Note that we are starting with a fresh 'Map'. Even if an existing one has
+ been cleared out completely, there remain 'deleted' elements, and these
+ would disturb the following logic, where we don't have access to the
+ actual 'm_n_deleted' value. */
+ size_t m_n_deleted = 0;
+
+ const int N_elem = 1234;
+ void *a[N_elem];
+ for (int i = 0; i < N_elem; ++i)
+a[i] = &a[i];
+
+ const int N_init = 4;
+
+ val_t::ndefault = 0;
+ val_t::ncopy = 0;
+ val_t::nassign = 0;
+ val_t::ndtor = 0;
+ Map m (N_init);
+
+ /* In the following, in particular related to 'expand', we're adapting from
+ the internal logic of 'hash_table', glossing over "some details" not
+ relevant
