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.kratoch...@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
_______________________________________________
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
