Re: [patch 2/9] Nios II port, bfd support

[Alan Modra <amodra at gmail dot com>]

On Thu, Jan 24, 2013 at 12:57:50PM -0700, Sandra Loosemore wrote:
> +static bfd_vma hiadj(bfd_vma symbol_value)

static bfd_vma
hiadj (bfd_vma symbol_value)

> +{
> +  return ((((symbol_value >> 16) & 0xffff)
> +	  + ((symbol_value >> 15) & 0x01))
> +	  & 0xffff);

More elegantly (and gcc isn't clever enough to optimize the above)
  return ((symbol_value + 0x8000) >> 16) & 0xffff;

> +  /* This part is from bfd_elf_generic_reloc.  */
> +  if (output_bfd != NULL
> +      && (symbol->flags & BSF_SECTION_SYM) == 0
> +      && (!reloc_entry->howto->partial_inplace || reloc_entry->addend == 0))
> +    {
> +      reloc_entry->address += input_section->output_offset;
> +      return bfd_reloc_ok;
> +    }
> +
> +  if (output_bfd != NULL)
> +    /* FIXME: See bfd_perform_relocation.  Is this right?  */
> +    return bfd_reloc_ok;

These FIXMEs don't inspire confidence.  OK, I know these functions
aren't that important as they'll only be used when linking using the
generic linker, eg. srec output, but even a small bit of digging
should convince you that the answer is "No, this is not right".

Huh.  I see the same in elf32-fr30.c and elf32-m32r.c, so what I was
trying to prevent has already occurred.  We already have obviously
wrong code in these reloc functions that other people then copy..

To save you the digging, the reason is that output_bfd != NULL implies
relocatable linking, and in that case reloc_entry->address must
always have the adjustment you can see in the code copied from
bfd_elf_generic_reloc.  Returning bfd_reloc_ok tells
bfd_perform_relocation that everything is done.  Returning
bfd_reloc_continue there *might* be correct, so please make all of
these

  if (output_bfd != NULL)
    /* FIXME: See bfd_perform_relocation.  Is this right?  */
    return bfd_reloc_continue;

> +  htab->sdynbss = bfd_get_section_by_name (dynobj, ".dynbss");
> +  if (!htab->sdynbss)
> +    return FALSE;
> +  if (!info->shared)
> +    {
> +      htab->srelbss = bfd_get_section_by_name (dynobj, ".rela.bss");
> +      if (!htab->srelbss)
> +        return FALSE;

These should both be bfd_get_linker_section.

> +		srelgot = bfd_get_section_by_name (dynobj, ".rela.got");

and this section should be retrieved from the elf hash table, srelgot.

> +	      /* When creating a shared object, we must copy these
> +		 reloc types into the output file.  We create a reloc
> +		 section in dynobj and make room for this reloc.  */
> +	      if (sreloc == NULL)
> +		{
> +		  const char *name;
> +
> +		  name = (bfd_elf_string_from_elf_section
> +			  (abfd,
> +			   elf_elfheader (abfd)->e_shstrndx,
> +			   _bfd_elf_single_rel_hdr (sec)->sh_name));
> +		  if (name == NULL)
> +		    return FALSE;
> +
> +		  BFD_ASSERT (CONST_STRNEQ (name, ".rela")
> +			      && (strcmp (bfd_get_section_name (abfd, sec),
> +					 name + 5)
> +				  == 0));
> +
> +		  sreloc = bfd_get_section_by_name (dynobj, name);
> +		  if (sreloc == NULL)
> +		    {
> +		      sreloc = bfd_make_section_with_flags
> +			(dynobj,
> +			 name,
> +			 (SEC_ALLOC | SEC_LOAD | SEC_HAS_CONTENTS
> +			  | SEC_IN_MEMORY | SEC_LINKER_CREATED | SEC_READONLY));
> +		      if (sreloc == NULL
> +			  || !bfd_set_section_alignment (dynobj, sreloc, 2))
> +			return FALSE;
> +		    }
> +		  elf_section_data (sec)->sreloc = sreloc;
> +		}

Use _bfd_elf_get_dynamic_reloc_section here.

> +      s = bfd_get_section_by_name (h->root.u.def.section->owner,
> +				   ".rela.bss");

Again, use the elf hash table here, srelbss.

> +  sdyn = bfd_get_section_by_name (dynobj, ".dynamic");

bfd_get_linker_section

> +	    case DT_PLTGOT:
> +	      name = ".got";
> +	      goto get_vma;
> +	    case DT_JMPREL:
> +	      name = ".rela.plt";
> +	    get_vma:
> +	      s = bfd_get_section_by_name (output_bfd, name);

elf hash table, srelgot and srelplt.

> +	    case DT_PLTRELSZ:
> +	      s = bfd_get_section_by_name (output_bfd, ".rela.plt");
> +	      BFD_ASSERT (s != NULL);

ditto.

> +	    case DT_RELASZ:
> +	      /* The procedure linkage table relocs (DT_JMPREL) should
> +		 not be included in the overall relocs (DT_RELA).
> +		 Therefore, we override the DT_RELASZ entry here to
> +		 make it not include the JMPREL relocs.  Since the
> +		 linker script arranges for .rela.plt to follow all
> +		 other relocation sections, we don't have to worry
> +		 about changing the DT_RELA entry.  */
> +	      s = bfd_get_section_by_name (output_bfd, ".rela.plt");

ditto.

> +	    case DT_NIOS2_GP:
> +	      s = bfd_get_section_by_name (output_bfd, ".got");

elf hash table sgot.

> +	  s = bfd_get_section_by_name (dynobj, ".interp");

bfd_get_linker_section

> +	  /* ??? Will this section creation clash with .sbss created as
> +	     one elf32_nios2_special_sections?
> +	     And, more generally, we may not need this anymore since
> +	     .sbss is created via elf32_nios2_special_sections.  Same applies
> +	     to elf32-ppc.c and, possibly, other targets.  */
> +          htab->sbss = bfd_make_section_anyway_with_flags (dynobj, ".sbss",
> +                                                           flags);

Please remove the ??? comment.

-- 
Alan Modra
Australia Development Lab, IBM