Re: telling symbols defined only in the linker script

[Nick Clifton <nickc at redhat dot com>]

Hi Alex,

> Here's the patch that enabled me to figure out exactly which symbol
> it was that was getting more entries than originally reserved.  It
> adds some needless overhead, so perhaps it should be conditionally
> compiled, but the overhead isn't all that significant, and it's a
> nice sanity check, so I propose that we compile it in unconditionally.

Since this is specific to the FRV backend it is OK - but I would
suggest adding a comment at a suitable point saying something like
"comment out this section if you need increased FRV linker performance
and are sure that you will not be getting corrupt relocs".


> Ok to install?

Not as it stands no:

> +#if 0
> +  /* These are set in _frv_count_got_plt_entries() or later, and this
> +     function is only called in _frv_resolve_final_relocs_info(), that
> +     runs just before it, so we don't have to worry about the fields
> +     below.  */
> +
> +  e2->plt |= e1->plt;
> +  e2->privfd |= e1->privfd;
> +  e2->lazyplt |= e1->lazyplt;
> +  e2->done |= e1->done;
> +
> +  e2->relocs32 += e1->relocs32;
> +  e2->relocsfd += e1->relocsfd;
> +  e2->relocsfdv += e1->relocsfdv;
> +  e2->fixups += e1->fixups;
> +  e2->dynrelocs += e1->dynrelocs;
> +
> +  if (abs (e1->got_entry) < abs (e2->got_entry))
> +    e2->got_entry = e1->got_entry;
> +  if (abs (e1->fdgot_entry) < abs (e2->fdgot_entry))
> +    e2->fdgot_entry = e1->fdgot_entry;
> +  if (abs (e1->fd_entry) < abs (e2->fd_entry))
> +    e2->fd_entry = e1->fd_entry;
> +
> +  if (e1->plt_entry < e2->plt_entry)
> +    e2->plt_entry = e1->plt_entry;
> +  if (e1->lzplt_entry < e2->lzplt_entry)
> +    e2->lzplt_entry = e1->lzplt_entry;
> +#endif

What is the point of adding suppressed code ?  If you want it there
for future enablement or debugging purposes then there should at least
be comment explaining this.


> +		{
> +		  info->callbacks->warning
> +		    (info, "LINKER BUG: .rofixup section size mismatch",
> +		     ".rofixup", NULL, NULL, 0);
> +		  abort ();
> +		  return FALSE;
> +		}

I do not like the idea of having the abort() here.  If at all
possible the BFD library should never abort; just print an error
message and/or return an error code.


+  BFD_ASSERT (entry->dynrelocs > 0);

+      BFD_ASSERT (entry->fixups > 0);

On similar grounds I would prefer not to see BFD_ASSERTs added to the
code.  Much better to perform an explicit test and return to the
caller without aborting.

Cheers
        Nick