[Rpm-maint] debugedit: Fix handling in caller for errors in called read_dwarf2_line.

[Mark Wielaard]

Hi,

I don't fully understand what this patch is intended to do. Why is it
necessary?

> commit 0da448c337d481f1c50af212246ceb213a7d80cc (HEAD -> master)
> Author: Jan Kratochvil <jan.kratochvil at redhat.com>
> Date:   Mon Aug 17 21:46:47 2020 +0200
> 
>     debugedit: Fix handling in caller for errors in called
> read_dwarf2_line.
> 
> diff --git a/tools/debugedit.c b/tools/debugedit.c
> index 8e85847d1..ff72759ca 100644
> --- a/tools/debugedit.c
> +++ b/tools/debugedit.c
> @@ -1155,7 +1155,7 @@ get_line_table (DSO *dso, size_t off, struct
> line_table **table)
>      if (lines->table[i].old_idx == off)
>        {
>         *table = &lines->table[i];
> -       return false;
> +       return true;
>        }

This breaks the contract of the function, which says:

   Gets a line_table at offset. Returns true if not yet know and
   successfully read, false otherwise.

The intention is to let the caller know whether the line_table has
already been handled or not earlier. It makes sure read_dwarf2_line
doesn't try parsing the same line table multiple times even if it is
referenced multiple times by different CUs.

Returning true in this case might make read_dwarf2_line parse and
replace the same line table multiple times.

>    if (lines->size == lines->used)
> @@ -1621,7 +1621,8 @@ read_dwarf2_line (DSO *dso, uint32_t off, char *comp_dir)
>      }
>  
>    dso->lines.debug_lines_len += 4 + table->unit_length + table->size_diff;
> -  return table->replace_dirs || table->replace_files;
> +  need_stmt_update = table->replace_dirs || table->replace_files;
> +  return true;
>  }

Again this changes the documented contract of the function which says:

   Returns true if line_table needs to be rewrite either the dir or
   file paths.

I see you change that by setting need_stmt_update early, but it is
confusing to have the function do something different than documented.

>  /* Called during phase one, after the table has been sorted. */
> @@ -1939,9 +1940,11 @@ edit_attributes (DSO *dso, unsigned char *ptr, struct abbrev_tag *t, int phase)
>       that).  Note that calculating the new size and offsets is done
>       separately (at the end of phase zero after all CUs have been
>       scanned in dwarf2_edit). */
> -  if (phase == 0 && found_list_offs
> -      && read_dwarf2_line (dso, list_offs, comp_dir))
> -    need_stmt_update = true;
> +  if (found_list_offs && ! read_dwarf2_line (dso, list_offs, comp_dir))
> +    {
> +      free (comp_dir);
> +      return NULL;
> +    }

Why do you need to call read_dwarf2_line in all phases here?
The comment just before this code explains which work is done in which
phase, but with this change that seems no longer valid.
 
>    free (comp_dir);
>  
> @@ -2059,7 +2062,10 @@ edit_info (DSO *dso, int phase, struct debug_section *sec)
>  
>           ptr = edit_attributes (dso, ptr, t, phase);
>           if (ptr == NULL)
> -           break;
> +           {
> +             htab_delete (abbrev);
> +             return 1;
> +           }
>         }
>  
>        htab_delete (abbrev);

This seems to fix a minor memory leak. Looks OK.

Cheers,

Mark