[Bug symtab/29343] New: [gdb/symtab] per_cu->length set without initial_length_size

[Bug symtab/29343] New: [gdb/symtab] per_cu->length set without initial_length_size

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

            Bug ID: 29343
           Summary: [gdb/symtab] per_cu->length set without
                    initial_length_size
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: symtab
          Assignee: unassigned at sourceware dot org
          Reporter: vries at gcc dot gnu.org
  Target Milestone: ---

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;
...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] per_cu->length set without initial_length_size

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simark at simark dot ca,
                   |                            |tromey at sourceware dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] per_cu->length set without initial_length_size

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

--- Comment #1 from Tom de Vries <vries at gcc dot gnu.org> ---
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;
...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] per_cu->length set without initial_length_size

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

--- Comment #2 from Tom de Vries <vries at gcc dot gnu.org> ---
Created attachment 14200
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14200&action=edit
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] per_cu->length set without initial_length_size

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

--- Comment #3 from Tom de Vries <vries at gcc dot gnu.org> ---
(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

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] per_cu->length set without initial_length_size

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

--- Comment #4 from Tom de Vries <vries at gcc dot gnu.org> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] c++-ify comp_unit_head

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
            Summary|[gdb/symtab] per_cu->length |[gdb/symtab] c++-ify
                   |set without                 |comp_unit_head
                   |initial_length_size         |

--- Comment #5 from Tom de Vries <vries at gcc dot gnu.org> ---
Update summary.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] c++-ify comp_unit_head

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

--- Comment #6 from Tom Tromey <tromey at sourceware dot org> ---
https://sourceware.org/pipermail/gdb-patches/2022-December/195075.html

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] c++-ify comp_unit_head

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

--- Comment #7 from Tom de Vries <vries at gcc dot gnu.org> ---
(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

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] c++-ify comp_unit_head

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

--- Comment #8 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] c++-ify comp_unit_head

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
   Target Milestone|---                         |14.1
           Assignee|unassigned at sourceware dot org   |tromey at sourceware dot org
             Status|NEW                         |RESOLVED

--- Comment #9 from Tom Tromey <tromey at sourceware dot org> ---
Fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/29343] [gdb/symtab] c++-ify comp_unit_head

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29343

--- Comment #10 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
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

-- 
You are receiving this mail because:
You are on the CC list for the bug.