Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-12 Thread Panu Matilainen 
Thanks for the patches!

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1323#issuecomment-672724645___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-12 Thread Panu Matilainen 
Merged #1323 into master.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1323#event-3646555708___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-12 Thread Panu Matilainen 
@pmatilai approved this pull request.

Approved by Mark in 
http://lists.rpm.org/pipermail/rpm-maint/2020-August/014805.html



-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1323#pullrequestreview-465691650___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-12 Thread Panu Matilainen 
Okay, thanks for pointing out that review on rpm-maint.
There seems to be something wrong with the list, it's supposed to be two-way so 
emails review are possible, and I haven't received a single message in weeks 
although clearly there has been activity.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1323#issuecomment-672720601___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-11 Thread Mark Wielaard 
On Sat, Aug 08, 2020 at 07:04:03AM +0200, Jan Kratochvil wrote:
> On Fri, 07 Aug 2020 13:55:35 +0200, Mark Wielaard wrote:
> > But I pulled
> > that branch and reviewed the actual commits (1d080e02 and c804a960).
> 
> git clone -b types g...@github.com:jankratochvil/rpm.git rpm-types
> 
> commit 8b5bbcc6d586be50b6a251256c39c3b0332b1f2b
> debugedit: Fix missing relocation of .debug_types section.
> commit 1d080e02409d181169d3aec2a19192418f253fd3
> [NFC] debugedit: Move code from edit_dwarf2() to edit_info().
> 
> 
> > It would be better to have a separate RPM_DEBUGEDIT_TYPES_SETUP that
> > does that and leave RPM_DEBUGEDIT_SETUP as is. Otherwise we change the
> > existing tests too.
> 
> done as: RPM_DEBUGEDIT_SETUP([-fdebug-types-section])

Nice. Using $1 is better than what I suggested.

Commits look good. Should go into the main branch IMHO.

Thanks,

Mark
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-07 Thread Jan Kratochvil 
It looks to me as reviewed:
http://lists.rpm.org/pipermail/rpm-maint/2020-August/014792.html
http://lists.rpm.org/pipermail/rpm-maint/2020-August/014797.html


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1323#issuecomment-670825178___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-07 Thread Jan Kratochvil 
On Fri, 07 Aug 2020 13:55:35 +0200, Mark Wielaard wrote:
> But I pulled
> that branch and reviewed the actual commits (1d080e02 and c804a960).

git clone -b types g...@github.com:jankratochvil/rpm.git rpm-types

commit 8b5bbcc6d586be50b6a251256c39c3b0332b1f2b
debugedit: Fix missing relocation of .debug_types section.
commit 1d080e02409d181169d3aec2a19192418f253fd3
[NFC] debugedit: Move code from edit_dwarf2() to edit_info().


> It would be better to have a separate RPM_DEBUGEDIT_TYPES_SETUP that
> does that and leave RPM_DEBUGEDIT_SETUP as is. Otherwise we change the
> existing tests too.

done as: RPM_DEBUGEDIT_SETUP([-fdebug-types-section])


> The only change I would like, as explained above, is to have a separate
> RPM_DEBUGEDIT_TYPES_SETUP. With that it should be good to commit to the
> main branch.


Thanks,
Jan

___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-07 Thread Mark Wielaard 
Hi Jan,

On Sat, 2020-08-01 at 11:23 +0200, Jan Kratochvil wrote:
> debugedit: Fix missing relocation of .debug_types section.
> https://github.com/rpm-software-management/rpm/pull/1323

I believe our mail review comments don't make it to that website. And
given that there are some forced updates to that pull branch. It is
sometimes hard to see which version we are discussing. But I pulled
that branch and reviewed the actual commits (1d080e02 and c804a960).

> > But here is a review inline:
> > If it is for the same lines that are moved from edit_dwarf2 () to
> > edit_info () below then it is fine, but if you do it in a separate
> > commit maybe also factor out edit_info () already? To make the next
> > commit easier to read.
> 
> Yes, that was my original intention but I made a mistake, fixed now.

commit 1d080e02 (after checking with diff -u -w) looks reasonable.
I would check that into the main branch as is.

> > This will only work for executables or shared librareis, not for
> > (ET_REL) object files (or kernel modules) because those come with more
> > than 1 (comdat) .debug_types section. This is probably fine. But if you
> > expect that .debug_types will also appear in relocatable files, then
> > you might want to look at what edit_dwarf2() does for .debug_macro,
> > which might also appear multiple times.
> 
> done

commit c804a960
Looks good. Thanks.

> > You do include a testcase for the relocatable object case:
> 
> done
> 
> 
> > I would suggest extending the testcase a little to have multiple
> > larger
> > structs, plus some small field names. e.g. add another struct in
> > foobar.h:
> 
> done

The test cases loook pretty comprehensive now. Nice.
One request though. In commit c804a960 you adjust RPM_DEBUGEDIT_SETUP
as follows:

> diff --git a/tests/debugedit.at b/tests/debugedit.at
> index cae3f4384..aa878d6fb 100644
> --- a/tests/debugedit.at
> +++ b/tests/debugedit.at
> @@ -36,11 +36,11 @@ cp "${abs_srcdir}"/data/SOURCES/foobar.h subdir_headers
>  cp "${abs_srcdir}"/data/SOURCES/baz.c .
>  
>  # First three object files (foo.o subdir_bar/bar.o and baz.o)
> -gcc -g3 -Isubdir_headers -c subdir_foo/foo.c
> +gcc -g3 -Isubdir_headers -fdebug-types-section -c subdir_foo/foo.c
>  cd subdir_bar
> -gcc -g3 -I../subdir_headers -c bar.c
> +gcc -g3 -I../subdir_headers -fdebug-types-section -c bar.c
>  cd ..
> -gcc -g3 -I$(pwd)/subdir_headers -c $(pwd)/baz.c
> +gcc -g3 -I$(pwd)/subdir_headers -fdebug-types-section -c $(pwd)/baz.c
>  
>  # Then a partially linked object file (somewhat like a kernel module).
>  # This will still have relocations between the debug sections.

It would be better to have a separate RPM_DEBUGEDIT_TYPES_SETUP that
does that and leave RPM_DEBUGEDIT_SETUP as is. Otherwise we change the
existing tests too. And I believe we want to make sure we test those
with the default debug flags.

> > Maybe we can just remove that warning.
> 
> done

Looks good. Thanks.

> > This looks OK. But just before this the sections that have been changed
> > are marked "dirty", you probably want to mark DEBUG_TYPES also dirty if
> > it has been updated in any way (otherwise it isn't guaranteed the data
> > is written to disk, although it often will be).
> 
> done

Nice catch on the dirty_section () having to follow secp->next (that
was a latent bug for the DEBUG_MACRO case).

> > To make sure you test the case where there are multiple debug line
> > table offsets in your .debug_type sections, you might want to add
> > something like the following to bar.c:
> 
> done

Nice work on the tests.

I read the whole commit c804a960 and it looks good.

The only change I would like, as explained above, is to have a separate
RPM_DEBUGEDIT_TYPES_SETUP. With that it should be good to commit to the
main branch.

Thanks,

Mark
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-01 Thread Jan Kratochvil 
@jankratochvil pushed 1 commit.

c804a960902acf5b117c61ac129363937bf746e5  debugedit: Fix missing relocation of 
.debug_types section.


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1323/files/0f56c2ff89029bd515dfb49af398427f97d348e7..c804a960902acf5b117c61ac129363937bf746e5
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-01 Thread Jan Kratochvil 
debugedit: Fix missing relocation of .debug_types section.
https://github.com/rpm-software-management/rpm/pull/1323


> But here is a review inline:
> If it is for the same lines that are moved from edit_dwarf2 () to
> edit_info () below then it is fine, but if you do it in a separate
> commit maybe also factor out edit_info () already? To make the next
> commit easier to read.

Yes, that was my original intention but I made a mistake, fixed now.


> This will only work for executables or shared librareis, not for
> (ET_REL) object files (or kernel modules) because those come with more
> than 1 (comdat) .debug_types section. This is probably fine. But if you
> expect that .debug_types will also appear in relocatable files, then
> you might want to look at what edit_dwarf2() does for .debug_macro,
> which might also appear multiple times.

done


> You do include a testcase for the relocatable object case:

done


> I would suggest extending the testcase a little to have multiple larger
> structs, plus some small field names. e.g. add another struct in
> foobar.h:

done


> Maybe we can just remove that warning.

done


> This looks OK. But just before this the sections that have been changed
> are marked "dirty", you probably want to mark DEBUG_TYPES also dirty if
> it has been updated in any way (otherwise it isn't guaranteed the data
> is written to disk, although it often will be).

done


> To make sure you test the case where there are multiple debug line
> table offsets in your .debug_type sections, you might want to add
> something like the following to bar.c:

done


Thanks,
Jan

___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-08-01 Thread Jan Kratochvil 
@jankratochvil pushed 2 commits.

1d080e02409d181169d3aec2a19192418f253fd3  [NFC] debugedit: Move code from 
edit_dwarf2() to edit_info().
0f56c2ff89029bd515dfb49af398427f97d348e7  debugedit: Fix missing relocation of 
.debug_types section.


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1323/files/6254f165671bce379ef013025a1f1d5bf5d68eb1..0f56c2ff89029bd515dfb49af398427f97d348e7
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-07-31 Thread Mark Wielaard 
Hi Florian,

On Fri, 2020-07-31 at 14:24 +0200, Florian Festi wrote:
> Mark can you have a look at this, please?

Sure. But somehow I never got this email through the mailinglist, so I
don't know how to reply so that my comments show up on that pull. Also,
it seems there are various force pushes that mess up the commit
ids/patches. But here is a review inline:

> > * [NFC] debugedit: Reindent edit_dwarf2().

This is basically:

diff --git a/tools/debugedit.c b/tools/debugedit.c
index 9f8dcd0fb..dcacb23b7 100644
--- a/tools/debugedit.c
+++ b/tools/debugedit.c
@@ -2100,8 +2100,9 @@ edit_dwarf2 (DSO *dso)
   return 1;
 }
 
-  if (debug_sections[DEBUG_INFO].data != NULL)
-{
+  if (debug_sections[DEBUG_INFO].data == NULL)
+return 0;
+
   unsigned char *ptr, *endcu, *endsec;
   uint32_t value;
   htab_t abbrev;
@@ -2480,7 +2481,6 @@ edit_dwarf2 (DSO *dso)
  macro_sec = macro_sec->next;
}
 }
-}
 
   return 0;
 }

Which is logically correct, but I don't know why you would do that.
It unnecessarily changes ~400 source lines, causing it to make it
slightly harder to track when which change was made by whom for what.
If it is for the same lines that are moved from edit_dwarf2 () to
edit_info () below then it is fine, but if you do it in a separate
commit maybe also factor out edit_info () already? To make the next
commit easier to read.

> >   * debugedit: Fix missing relocation of .debug_types section.

This will only work for executables or shared librareis, not for
(ET_REL) object files (or kernel modules) because those come with more
than 1 (comdat) .debug_types section. This is probably fine. But if you
expect that .debug_types will also appear in relocatable files, then
you might want to look at what edit_dwarf2() does for .debug_macro,
which might also appear multiple times.

You do include a testcase for the relocatable object case:

+# ===
+# Make sure -fdebug-types-section has updated strings in partial linked object.
+# ===
+AT_SETUP([debugedit .debug_types partial])
+AT_KEYWORDS([debugtypes] [debugedit])
+RPM_DEBUGEDIT_SETUP
+
+AT_DATA([expout],
+[fieldname1
+])
+
+AT_CHECK([[debugedit -b $(pwd) -d /foo/bar/baz ./foobarbaz.part.o]])
+AT_CHECK([[
+readelf --debug-dump=info ./foobarbaz.part.o \
+   | sed -n '/Abbrev Number:.*DW_TAG_type_unit/,/^$/p' \
+| sed -n 's/^.*> *DW_AT_name *:.* \(fieldname.\)$/\1/p'
+]],[0],[expout])
+
+AT_CLEANUP

Which is great, thanks for adding that (and the single object files and
exectuable testcases). The tests look good, but won't catch the
multiple .debug_types sections case (and only test indirect strings in
.debug_str, not the small strings embedded in .debug_info).

I would suggest extending the testcase a little to have multiple larger
structs, plus some small field names. e.g. add another struct in
foobar.h:

struct types_section
{
  char t;
  int n1;
};

which then gets referenced in both foo.c and bar.c as

struct types_section debug_var_foo;

and

struct types_section debug_var_bar;

That way you get at least two .debug_type (comdat) sections and one
with "static" strings.

> >  * *M* tools/debugedit.c

> diff --git a/tools/debugedit.c b/tools/debugedit.c
> index dcacb23b7..f635c0c62 100644
> --- a/tools/debugedit.c
> +++ b/tools/debugedit.c
> @@ -1459,7 +1459,7 @@ edit_dwarf2_line (DSO *dso)
> adjustments needed in the debug_list data structures. Returns true
> if line_table needs to be rewrite either the dir or file paths. */
>  static bool
> -read_dwarf2_line (DSO *dso, uint32_t off, char *comp_dir)
> +read_dwarf2_line (DSO *dso, int info_scn_ix, uint32_t off, char *comp_dir)
>  {
>unsigned char *ptr, *dir;
>unsigned char **dirt;
> @@ -1470,7 +1470,7 @@ read_dwarf2_line (DSO *dso, uint32_t off, char 
> *comp_dir)
>if (get_line_table (dso, off, ) == false
>|| table == NULL)
>  {
> -  if (table != NULL)
> +  if (table != NULL && info_scn_ix == DEBUG_INFO)
> error (0, 0, ".debug_line offset 0x%x referenced multiple times",
>off);
>return false;

Maybe we can just remove that warning. It isn't really "illegal" to
reference a debug line table multiple times. All we really care about
is that we only process it once (and only process those that are
actually used).

> @@ -1644,7 +1644,8 @@ find_new_list_offs (struct debug_lines *lines, size_t 
> idx)
> abbrev data is consumed. In phase zero data is collected, in phase one
> data might be replaced/updated.  */
>  static unsigned char *
> -edit_attributes (DSO *dso, unsigned char *ptr, struct abbrev_tag *t, int 
> phase)
> +edit_attributes (DSO *dso, int info_scn_ix, unsigned char *ptr,
> +struct abbrev_tag *t, int phase)
>  {
>int i;
>uint32_t list_offs;
> @@ -1942,7 +1943,7 @@ edit_attributes (DSO *dso, unsigned char *ptr, struct 
> abbrev_tag *t, int phase)
>   separately (at the end of phase zero after all CUs have been
>   scanned in dwarf2_edit). */
>

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-07-31 Thread Florian Festi 
Mark can you have a look at this, please?

Thanks!

Florian

On 7/29/20 11:32 PM, Jan Kratochvil wrote:
> There is a new testcase: |debugedit .debug_types exe|
> 
> 
> 
> 
> You can view, comment on, or merge this pull request online at:
> 
>   https://github.com/rpm-software-management/rpm/pull/1323
> 
> 
> Commit Summary
> 
>   * [NFC] debugedit: Reindent edit_dwarf2().
>   * debugedit: Fix missing relocation of .debug_types section.
> 
> 
> File Changes
> 
>   * *M* tests/data/SOURCES/foo.c
> 
> 
> (5)
>   * *M* tests/debugedit.at
> 
> 
> (26)
>   * *M* tools/debugedit.c
> 
> 
> (699)
> 
> 
> Patch Links:
> 
>   * https://github.com/rpm-software-management/rpm/pull/1323.patch
>   * https://github.com/rpm-software-management/rpm/pull/1323.diff
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> , or
> unsubscribe
> .
> 
> 
> ___
> Rpm-maint mailing list
> Rpm-maint@lists.rpm.org
> http://lists.rpm.org/mailman/listinfo/rpm-maint
> 


-- 
Red Hat GmbH, http://www.de.redhat.com/, Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Laurie Krebs, Michael O'Neill,
Thomas Savage

___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

Re: [Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-07-31 Thread Jan Kratochvil 
@jankratochvil pushed 1 commit.

6254f165671bce379ef013025a1f1d5bf5d68eb1  debugedit: Fix missing relocation of 
.debug_types section.


-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1323/files/a1819fb3351ed419e8f3867d27524a3451d62899..6254f165671bce379ef013025a1f1d5bf5d68eb1
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint

[Rpm-maint] [rpm-software-management/rpm] debugedit: Fix missing relocation of .debug_types section. (#1323)

2020-07-29 Thread Jan Kratochvil 
There is a new testcase: `debugedit .debug_types exe`
You can view, comment on, or merge this pull request online at:

  https://github.com/rpm-software-management/rpm/pull/1323

-- Commit Summary --

  * [NFC] debugedit: Reindent edit_dwarf2().
  * debugedit: Fix missing relocation of .debug_types section.

-- File Changes --

M tests/data/SOURCES/foo.c (5)
M tests/debugedit.at (26)
M tools/debugedit.c (699)

-- Patch Links --

https://github.com/rpm-software-management/rpm/pull/1323.patch
https://github.com/rpm-software-management/rpm/pull/1323.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1323
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint