Re: [PATCH] DWARF 5 support: Handle line table and file indexes

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2019-10-15 16:19, Ali Tamur wrote:
>> > -enum class file_name_index : unsigned int {};
>> > +typedef int file_name_index;
>> 
>> Is there a reason you changed these?  The reason why these were of 
>> enum
>> class type (or "strong typedef") still seems valid to me.  We don't 
>> want
>> some code accessing the vectors blindy using these indices, so it
>> encourages people to use the right accessor method instead.
> I respectfully disagree. We do "index - 1" calculation in file_name_at 
> and
> include_dir_at methods. If we are going to cast back and forth to 
> integers
> anyways, having an enum loses its appeal.

Ok, I guess that if we access the vector through these accessor 
functions, it should be safe.

I'd still suggest adding a gdb_assert to verify that in the case of 
DWARF <= 4, index should be > 0.

> @@ -11717,16 +11735,16 @@ dwarf2_cu::setup_type_unit_groups (struct
> die_info *die)
>  	 process_full_type_unit still needs to know if this is the first
>  	 time.  */
> 
> -      tu_group->num_symtabs = line_header->file_names.size ();
> +      tu_group->num_symtabs = line_header->file_names_size ();
>        tu_group->symtabs = XNEWVEC (struct symtab *,
> -				   line_header->file_names.size ());
> +				   line_header->file_names_size ());
> 
> -      for (i = 0; i < line_header->file_names.size (); ++i)
> +      auto &file_names = line_header->file_names ();
> +      for (i = 0; i < file_names.size (); ++i)
>  	{
> -	  file_entry &fe = line_header->file_names[i];
> -
> -	  dwarf2_start_subfile (this, fe.name,
> -				fe.include_dir (line_header));
> +	  file_entry *fe = &file_names[i];

In these various loops, the `fe` variable can stay a reference, unless 
there's a specific reason why you changed them to pointers.

> @@ -20286,9 +20304,9 @@ line_header::add_include_dir (const char 
> *include_dir)
>  {
>    if (dwarf_line_debug >= 2)
>      fprintf_unfiltered (gdb_stdlog, "Adding dir %zu: %s\n",
> -			include_dirs.size () + 1, include_dir);
> +			m_include_dirs.size () + 1, include_dir);

I think this debug message showing the index should be 0 based in 
DWARF5, and 1 based before.

> 
> -  include_dirs.push_back (include_dir);
> +  m_include_dirs.push_back (include_dir);
>  }
> 
>  void
> @@ -20299,9 +20317,9 @@ line_header::add_file_name (const char *name,
>  {
>    if (dwarf_line_debug >= 2)
>      fprintf_unfiltered (gdb_stdlog, "Adding file %u: %s\n",
> -			(unsigned) file_names.size () + 1, name);
> +			(unsigned) file_names_size () + 1, name);

Likewise.

> @@ -20651,18 +20677,17 @@ dwarf_decode_line_header (sect_offset
> sect_off, struct dwarf2_cu *cu)
>     Returns NULL if FILE_INDEX should be ignored, i.e., it is 
> pst->filename.  */
> 
>  static const char *
> -psymtab_include_file_name (const struct line_header *lh, int 
> file_index,
> +psymtab_include_file_name (const struct line_header *lh, const 
> file_entry *fe,

Pass `fe` by const reference instead?

> @@ -21367,13 +21392,14 @@ dwarf_decode_lines (struct line_header *lh,
> const char *comp_dir,
> 
>        /* Now that we're done scanning the Line Header Program, we can
>           create the psymtab of each included file.  */
> -      for (file_index = 0; file_index < lh->file_names.size (); 
> file_index++)
> -        if (lh->file_names[file_index].included_p == 1)
> +      auto &file_names = lh->file_names ();
> +      for (file_index = 0; file_index < file_names.size (); 
> file_index++)
> +        if (file_names[file_index].included_p == 1)

Whenever you can iterate using a range-for (when you don't need the 
iteration index), please do so:

       for (const file_entry &fe : lh->file_names ())
         {
           ...
         }

Simon