Mips target in gold - revision 3 - part 2

[Sasa Stankovic]

I forgot to remove the call to Mips_output_section_reginfo::set_always_keeps_input_sections(). Attached is the updated patch where the call is removed.

Regards,
Sasa
________________________________________
From: Sasa Stankovic
Sent: Wednesday, June 04, 2014 11:00 PM
To: Cary Coutant
Cc: Rich Fuhler; binutils@sourceware.org; Petar Jovanovic
Subject: RE: Mips target in gold - revision 3 - part 2

Hi Cary,

Attached is the first part of the updated patch. Sorry for the delay; I didn't respond immediately after your last message because I thought there will be more reviews.

> >   // Fill the unused space with zeroes.
> >   // TODO(sasa): Can we strip unused bytes during the relaxation?
> >   unsigned char* end = oview + oview_size;
> >   while (pov < end)
> >     {
> >       elfcpp::Swap<32, big_endian>::writeval(pov, 0);
> >       pov += 4;
> >     }
>
> You can just use memset here:
>
>  if (unused > 0)
>    memset(pov, 0, unused);

Done.

> One more issue I just noticed...
>
> In Mips_output_section_reginfo::do_write:
>
> >       const unsigned char* section_contents =
> >         relobj->section_contents(shndx, &section_size, false);
> >
> >       gprmask |= elfcpp::Swap<size, big_endian>::readval(section_contents);
> >       cprmask1 |= elfcpp::Swap<size, big_endian>::readval(section_contents + 4);
> >       cprmask2 |= elfcpp::Swap<size, big_endian>::readval(section_contents + 8);
> >       cprmask3 |=
> >         elfcpp::Swap<size, big_endian>::readval(section_contents + 12);
> >       cprmask4 |=
> >         elfcpp::Swap<size, big_endian>::readval(section_contents + 16);
>
> Going back to the input sections at this point could be a performance
> issue, especially for very large links. This is essentially another
> pass over the input files, and if we've run out of file descriptors,
> we'll have to close and reopen all the input files one more time each.
> It would be better to read these values and accumulate the five masks
> in do_read_symbols(), and you won't have to call
> set_always_keeps_input_sections() for Mips_output_section_reginfo.

Done.

Regards,
Sasa
________________________________________
From: Cary Coutant [ccoutant@google.com]
Sent: Tuesday, April 15, 2014 10:46 PM
To: Sasa Stankovic
Cc: Rich Fuhler; binutils@sourceware.org; iant@google.com; Petar Jovanovic
Subject: Re: Mips target in gold - revision 3 - part 2

>> If I understand the code correctly, in this case the HI16 reloc
>> will remain on the got16_addends_ list forever. Is there a point
>> where you know it's safe to remove it? Can you clear the list
>> at the end of each section?
>
> If LO16 is removed by GCC, than HI16 reloc is paired with the next found LO16
> reloc (this means that two or more HI16 reloc will be paired with a single LO16 reloc).
> This is what GNU ld does - it always tries to find LO16 reloc for HI16 reloc, and if it
> can't find it, error is reported.
>
> When trying to find LO16 reloc, GNU ld examines next relocations in the section until
> it finds it, or reports an error.  I implemented it differently in Gold - when HI16 or GOT16
> reloc is found, it is recorded in the got16_addends_ list, and the normal scaning of
> relocations continues.  Then when the LO16 reloc is found, the got16_addends_ list is
> examined and all pending matching HI16 and GOT16 relocs are removed.  The error when
> HI16 or GOT16 reloc doesn't have the LO16 part is detected by checking whether the
> section of the pending HI16 or GOT16 relocation is different from the section of the current
> LO16 relocation. The check whether the final section that is scanned has HI16 or GOT16
> without the LO16 part is in the do_finalize_sections method - I just check whether the
> got16_addends_ list is empty - if it isn't, error is reported.

Ah, I see.

>> >   typename std::list<got16_addend<size, big_endian> > got16_addends_;
>>
>> Do you realy want to use a linked list here? Wouldn't a
>> vector be better?
>
> I used std::list because erase is called on it.

Sorry, I missed that. And now I understand that you don't expect the
list to ever grow very long.

>   // Check whether the final section that was scaned has HI16 or GOT16

Typo: "scanned".

>   // Fill the unused space with zeroes.
>   // TODO(sasa): Can we strip unused bytes during the relaxation?
>   unsigned char* end = oview + oview_size;
>   while (pov < end)
>     {
>       elfcpp::Swap<32, big_endian>::writeval(pov, 0);
>       pov += 4;
>     }

You can just use memset here:

  if (unused > 0)
    memset(pov, 0, unused);

If that's OK with you, I'll commit it with that change.

Thanks for working on this, and for putting up with the long review process!

-cary
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mips1.patch
Type: text/x-patch
Size: 187105 bytes
Desc: mips1.patch
URL: <https://sourceware.org/pipermail/binutils/attachments/20140604/601cffcc/attachment.bin>