Re: [PATCH] mips: don't force the .sdata, .srdata and .lit* sections to be PROGBITS

[James Cowgill <James dot Cowgill at imgtec dot com>]

On Thu, 2014-11-06 at 20:44 +0000, Richard Sandiford wrote:
> James Cowgill <James.Cowgill@imgtec.com> writes:
> > Here's a patch for the first testsuite failure.
> >
> > Previously when writing elf files, the .sdata section was forced to be
> > PROGBITS. However in some cases (eg objcopy --only-keep-debug) the
> > section will have no data stored in the final file and should be set to
> > NOBITS. The default section type is already setup in
> > _bfd_mips_elf_special_sections so removing the assignment in
> > bfd_mips_elf_section_processing should still keep the section as the
> > right type.
> 
> Despite what the current comment says, I don't think we rely on the
> special sections array to get the sh_type right; it's only really there
> for SHF_MIPS_* stuff.  Without the special section entry we choose
> between SHT_PROGBITS and SHT_NOBITS based on the bfd section flags,
> via a combination of elf_fake_sections and:
> 
> 	  for (i = 0; i < m->count; i++)
> 	    if ((m->sections[i]->flags & (SEC_LOAD | SEC_HAS_CONTENTS)) == 0)
> 	      /* If we aren't making room for this section, then
> 		 it must be SHT_NOBITS regardless of what we've
> 		 set via struct bfd_elf_special_section.  */
> 	      elf_section_type (m->sections[i]) = SHT_NOBITS;
> 
> in assign_file_positions_for_load_sections.  E.g. for:
> 
>   SECTIONS {
>     .data : { BYTE(1); }
>     .lit4 : { . += 4; }
>     .lit8 : { . += 4; }
>     .sdata : { . += 4; }
>     .srdata : { . += 4; }
>   }
> 
> linked on its own, the last four sections are SHT_PROGBITS before the
> patch and SHT_NOBITS with just the _bfd_mips_elf_section_processing part
> of your patch applied, despite the middle three having a special section
> entry and .srdata still not having one (because I didn't apply that part).
> 
> Which is a long way of saying that I don't think we want to add .srdata
> to the special sections array, because there are no MIPS-specific flags
> or types to set.  We should get the section type right without it.

Yes you're right it doesn't make any difference to PROGBITS / NOBITS
(oops).

I think I set SHF_MIPS_GPREL for the .srdata entry in the array, so it
does have that MIPS-specific flag, but it looks like that flag gets set
for all of these sections in _bfd_mips_elf_fake_sections anyway. I just
couldn't find a reason why it wasn't in the array when the others were.

> Maybe the code you're removing was there because of some weirdness in
> the IRIX loader, or maybe not, but the IRIX target is pretty much dead
> and no modern tools should care.
> 
> So please just drop the _bfd_mips_elf_special_sections part and
> remove all three:
> 
> 	  hdr->sh_type = SHT_PROGBITS;
> 
> lines from _bfd_mips_elf_section_processing.  Now that the if bodies
> have only a single line, we should reformat them without braces too.
> I.e.:
> 
>       if (strcmp (name, ".sdata") == 0
> 	  || strcmp (name, ".lit8") == 0
> 	  || strcmp (name, ".lit4") == 0)
> 	hdr->sh_flags |= SHF_ALLOC | SHF_WRITE | SHF_MIPS_GPREL;
>       else if (strcmp (name, ".srdata") == 0)
> 	hdr->sh_flags |= SHF_ALLOC | SHF_MIPS_GPREL;
>       else if (strcmp (name, ".compact_rel") == 0)
> 	hdr->sh_flags = 0;

Ok I'll adjust it to look like that.

> (Although TBH I don't know why we need this code at all.  Which cases
> does it handle that the special section array doesn't?)

I don't know either - except it will add SHF_ALLOC to the .srdata
section unless you add that to the special sections list.

Thanks for you're help,
James