I ran into a race condition for per_cu->length, and while investigating this I noticed this particular requirement about length: ... /* The start offset and length of this compilation unit. NOTE: Unlike comp_unit_head.length, this length includes initial_length_size. If the DIE refers to a DWO file, this is always of the original die, not the DWO file. */ sect_offset sect_off {}; unsigned int length = 0; ... Grepping through the sources shows 3 spots were this requirement is not met: ... $ grep "cu->length = " gdb/dwarf2/read.c the_cu->length = length; this_cu->length = cu->header.get_length (); this_cu->length = cu->header.get_length (); this_cu->length = m_new_cu->header.get_length (); this_cu->length = cu_header.length + cu_header.initial_length_size; ...
Oh, my mistake, in those cases we use get_length(): ... unsigned int get_length () const { return initial_length_size + length; } ... and in the other length: ... this_cu->length = cu_header.length + cu_header.initial_length_size; ...
Created attachment 14200 [details] tentative patch This patch fixes the one use: ... - this_cu->length = cu_header.length + cu_header.initial_length_size; + this_cu->length = cu_header.get_length (); ... and then proceeds to make the length field private, to enforce using the accessor. That works reasonably well, but runs into trouble with this memset: ... @@ -23377,8 +23377,6 @@ dwarf2_per_cu_data::get_header () const const gdb_byte *info_ptr = this->section->buffer + to_underlying (this->sect_off); - memset (&m_header, 0, sizeof (m_header)); - read_comp_unit_head (&m_header, info_ptr, this->section, rcuh_kind::COMPILE); ... which I removed in the patch, but I'm not sure yet that correct.
(In reply to Tom de Vries from comment #2) > Created attachment 14200 [details] > tentative patch > > This patch fixes the one use: > ... > - this_cu->length = cu_header.length + cu_header.initial_length_size; > + this_cu->length = cu_header.get_length (); > ... I've committed this bit: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=a4ca6efe0589d0a030920a4686b692208c82a028
(In reply to Tom de Vries from comment #2) > ... > @@ -23377,8 +23377,6 @@ dwarf2_per_cu_data::get_header () const > const gdb_byte *info_ptr > = this->section->buffer + to_underlying (this->sect_off); > > - memset (&m_header, 0, sizeof (m_header)); > - > read_comp_unit_head (&m_header, info_ptr, this->section, > rcuh_kind::COMPILE); > > ... Hmm, so struct comp_unit_head is a POD, that we init using memset, sometimes. Sometimes we just initialize a bit, like this: ... /* Initialize it due to a false compiler warning. */ header.signature = -1; header.type_cu_offset_in_tu = (cu_offset) -1; ... And sometime not at all (before the reading is done). To make sure things are initialized consistently, we'd have to start using constructors, which I suppose means we no longer can use the memset. So I guess this requires a larger rewrite.
Update summary.
https://sourceware.org/pipermail/gdb-patches/2022-December/195075.html
(In reply to Tom de Vries from comment #2) > and then proceeds to make the length field private, to enforce using the > accessor. This bit submitted here: https://sourceware.org/pipermail/gdb-patches/2022-December/195095.html
The master branch has been updated by Tom Tromey <tromey@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=4d78ce772370fa48b9a749f81205076f26eba846 commit 4d78ce772370fa48b9a749f81205076f26eba846 Author: Tom Tromey <tom@tromey.com> Date: Fri Jul 15 19:05:29 2022 -0600 Add initializers to comp_unit_head PR symtab/29343 points out that it would be beneficial if comp_unit_head had a constructor and used initializers. This patch implements this. I'm unsure if this is sufficient to close the bug, but at least it's a step. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29343
Fixed.
The master branch has been updated by Tom de Vries <vries@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=d8f52a9a9ccbf7411cf4ae487d2756826f5d0bd5 commit d8f52a9a9ccbf7411cf4ae487d2756826f5d0bd5 Author: Tom de Vries <tdevries@suse.de> Date: Fri Dec 30 13:55:22 2022 +0100 [gdb/symtab] Make comp_unit_head.length private Make comp_unit_head.length private, to enforce using accessor functions. Replace accessor function get_length with get_length_with_initial and get_length_without_initial, to make it explicit which variant we're using. Tested on x86_64-linux. PR symtab/29343 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29343