[PATCH] Readelf patch to group common symbols.
Nick Clifton
nickc@redhat.com
Tue Dec 21 17:48:00 GMT 2004
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
More information about the Binutils
mailing list