Re: [PATCH] Readelf patch to group common symbols.

[Nick Clifton <nickc at redhat dot com>]

Hi Prafulla,

Thank you very much for submitting the revised patch.  Unfortunately now 
that I have looked more closely at it I have some other serious concerns 
with it, vis:

  * Not reusing code:
      The patch basically repeats the contents of the 
display_debug_info() function twice more in 
get_symbol_base_type_and_size() and display_common_symbol().  This is a 
very bad idea.  If so much code is in common with three routines it 
should be extracted and made into a single function called by three 
parents.  There are other instances of code duplication in the patch as 
well.

  * Target specific coding:
      It appears that this patch has been developed for a target which 
prefixes its symbols with a leading underscore, because in your 
get_symbol_size() function there is this code at the start:

  +  char symb_name[32] = "_";
  +  strcat((char *)symb_name,symbol_name);

The code should be fixed so that it will work with targets which are not 
prefixing their symbols.  (Note, because of gcc's -fleading-underscore 
switch you cannot make a static decision on a per-target basis).

  * Not made against current sources:
     The patch does not appear to have been created against the current 
mainline binutils sources in the CVS repository.  This makes it very 
hard for me to apply and test the patch.

  * Not making use of existing code:
      The patch includes code to print the name of a common symbol and 
then pad the name out to 20 spaces.  The print_symbol() function can 
already do this in a much more effiicient manner.

  * Performing redundant strcmps:
      The patch uses strcmp to check the name of a tag or attribute 
against a fixed string.  This is very inefficient.  It would be much 
better to just perform a direct comparison.  ie:

	(!strcmp("DW_AT_name",get_AT_name (attr->attribute))

should be:

        (attr->attribute == DW_AT_name)

Cheers
  Nick