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

[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