[Rpm-maint] [rpm-software-management/rpm] debugedit: Implement DWARF-5. (#1332)

[Jan Kratochvil]

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

It
- fixes a bug where errors parsing the file do not terminate the processing
- fixes a small memory leak
- simplifies the code

Otherwise extending this code for DWARF-5 in a later patch would be difficult how to code it with these existing issues.


> commit 0da448c337d481f1c50af212246ceb213a7d80cc (HEAD -> master) Author: Jan Kratochvil ***@***.***> 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 is really a mess, couldn't you learn how to do a patch review?

> This breaks the contract of the function, which says:

I forgot to update the function comment, fixed.

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

This
 - is no longer possible as for DWARF-5 one needs to parse the same line table in multiple phases
 - therefore this check is moved later into:
```
  if (phase <= table->phase_done)
    return true;
  table->phase_done = phase;
```

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

I do not think it matters and this gets fixed in later patch.

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

I have updated the function comment.

> Why do you need to call read_dwarf2_line in all phases here?

Because DWARF-5 .debug_line rewrite requires that. I have moved this change to the DWARF-5 later patch.

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

This would not work without the other fixes.


-- 
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1332#issuecomment-731388373
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20201120/80df4772/attachment.html>