Re: [PATCH 1/2] dwarf-reader: refactor try_reading_first_ksymtab_entry_using{pre,}_v4_19_format

["Matthias Maennich via libabigail" <libabigail@sourceware.org>]

Hi!

On Thu, Oct 24, 2019 at 05:18:50PM +0200, Dodji Seketeli wrote:
> Hello,
>
> Matthias Maennich <maennich@google.com> a écrit:
>
> > Avoid code duplication and increase maintainebility of these helper
> > functions. As their only difference was the application of position
> > relative relocations, consolidate them and add a flag for exactly this
> > feature.
> >
> > This is purely stylistic and not changing functionality.
> >
> > 	* src/abg-dwarf-reader.cc(try_reading_first_ksymtab_entry):
> > 	New function to consolidate functionality for
> > 	try_reading_first_ksymtab_entry_using_{pre,}v4_19_format functions.
> > 	(try_reading_first_ksymtab_entry_using_v4_19_format,
> > 	try_reading_first_ksymtab_entry_using_pre_v4_19_format):
> > 	refactor to use try_reading_first_ksymtab_entry
> >
> > Signed-off-by: Matthias Maennich <maennich@google.com>
> > ---
> >  src/abg-dwarf-reader.cc | 104 ++++++++++++++++++++++++----------------
> >  1 file changed, 63 insertions(+), 41 deletions(-)
> >
> > diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
> > index f2ebede11582..31bf5011777b 100644
> > --- a/src/abg-dwarf-reader.cc
> > +++ b/src/abg-dwarf-reader.cc
> > @@ -7584,67 +7584,89 @@ public:
> >      return true;
> >    }
> >
> > +  /// Try reading the first __ksymtab section entry.
> > +  ///
> > +  /// We lookup the symbol from the raw section passed as an argument. For
> > +  /// that, consider endianess and adjust for potential Elf relocations before
> > +  /// looking up the symbol in the .symtab section.
> > +  //
> > +  /// Optionally, support position relative relocations by considering the
> > +  /// ksymtab entry as 32 bit and applying the relocation relative to the
> > +  /// section header (i.e. the symbol position as we are reading the first
> > +  /// symbol).
> > +  ///
> > +  /// @return the symbol resulting from the lookup of the symbol address we
> > +  /// got from reading the first entry of the ksymtab or null if no such entry
> > +  /// could be found.
> > +  elf_symbol_sptr
> > +  try_reading_first_ksymtab_entry(
> > +      Elf_Scn* section, const bool position_relative_relocations) const
> > +  {
>
> The identation of this doesn't look right, compared to the rest of the
> code.
>
> I should rather look like:
>
> elf_symbol_sptr
> try_reading_first_ksymtab_entry(Elf_Scn* section,
>                                const bool position_relative_relocations) const
> {
>
> I have seen that you fixed this in the subsequent patch of the series,
> so  no biggy.  Well, this is no biggy anyhow :-)

Yeah, that was probably sneaking in when moving the code between
commits. You are right with your suggestion.
> Also, the 'const' on the scalar parameter
> 'position_relative_relocations' seems useless.

Well, we could argue about that, but either is fine for me. :-)

> So, I am thinking about committing this into master (with the comment
> above), independently from the second patch (I have some comments about
> that one).  Or do you prefer that both patch get in at once, after we
> finish the discussion on the second one?

I think this refactoring makes sense in any case independent of the
second patch, so feel free to apply this to master in any case.

Cheers,
Matthias

> Thanks a lot for working on this.
>
> Cheers,
>
> --
> 		Dodji