<p></p>
<blockquote>
<p>Hi, I don't fully understand what this patch is intended to do. Why is it necessary?</p>
</blockquote>
<p>It</p>
<ul>
<li>fixes a bug where errors parsing the file do not terminate the processing</li>
<li>fixes a small memory leak</li>
<li>simplifies the code</li>
</ul>
<p>Otherwise extending this code for DWARF-5 in a later patch would be difficult how to code it with these existing issues.</p>
<blockquote>
<p>commit 0da448c337d481f1c50af212246ceb213a7d80cc (HEAD -> master) Author: Jan Kratochvil <em><strong>@</strong></em>.***> 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; }</p>
</blockquote>
<p>This is really a mess, couldn't you learn how to do a patch review?</p>
<blockquote>
<p>This breaks the contract of the function, which says:</p>
</blockquote>
<p>I forgot to update the function comment, fixed.</p>
<blockquote>
<p>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.</p>
</blockquote>
<p>This</p>
<ul>
<li>is no longer possible as for DWARF-5 one needs to parse the same line table in multiple phases</li>
<li>therefore this check is moved later into:</li>
</ul>
<pre><code>  if (phase <= table->phase_done)
    return true;
  table->phase_done = phase;
</code></pre>
<blockquote>
<p>Returning true in this case might make read_dwarf2_line parse and replace the same line table multiple times.</p>
</blockquote>
<p>I do not think it matters and this gets fixed in later patch.</p>
<blockquote>
<p>Again this changes the documented contract of the function which says:</p>
</blockquote>
<p>I have updated the function comment.</p>
<blockquote>
<p>Why do you need to call read_dwarf2_line in all phases here?</p>
</blockquote>
<p>Because DWARF-5 .debug_line rewrite requires that. I have moved this change to the DWARF-5 later patch.</p>
<blockquote>
<p>This seems to fix a minor memory leak. Looks OK.</p>
</blockquote>
<p>This would not work without the other fixes.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you commented.<br />Reply to this email directly, <a href="https://github.com/rpm-software-management/rpm/pull/1332#issuecomment-731388373">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ADLPZUZEUJM6GXS2GLFRC5TSQ3FSBANCNFSM4QBBKIWA">unsubscribe</a>.<img src="https://github.com/notifications/beacon/ADLPZU3FDB4ZWHNVZFE7SN3SQ3FSBA5CNFSM4QBBKIWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOFOMBTVI.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/rpm-software-management/rpm/pull/1332#issuecomment-731388373",
"url": "https://github.com/rpm-software-management/rpm/pull/1332#issuecomment-731388373",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>