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

[Dodji Seketeli <dodji@seketeli.org>]

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 :-)

Also, the 'const' on the scalar parameter
'position_relative_relocations' seems useless.

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?

Thanks a lot for working on this.

Cheers,

-- 
		Dodji