Re: RFC: PATCH: ld/12942: Plugin not handling correctly resolution of COMDATs.

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Tue, Jul 5, 2011 at 5:51 PM, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Jul 04, 2011 at 03:53:28PM -0700, H.J. Lu wrote:
>> Hi,
>>
>> This adds SEC_LTO_COMDAT and GNU_LTO_COMAT_SECTION_NAME. ?They are
>> used to support LTO COMDAT symbols. Any comments?
>
> This is a hard patch to review, almost as much work as writing it in
> the first place, because you don't explain why you need to do what you
> do. ?Also, with a slightly out of date gcc I couldn't reproduce the
> problem on powerpc or powerpc64, so is there something special about
> x86_64 or did gcc recently start using comdat groups for lto?

That was a compiler LTO bug which was fixed recently.

>> ? ? ? ? ? case SEC_LINK_DUPLICATES_DISCARD:
>> + ? ? ? ? ? if (info->loading_lto_outputs
>> + ? ? ? ? ? ? ? && (l_owner->flags & BFD_PLUGIN) != 0)
>
> It took me a while to convince myself this was correct, so I'd like to
> see this test commented. ?Perhaps
> ? ? ? ? ? ? ?/* If we found an LTO IR match for this comdat group on
> ? ? ? ? ? ? ? ? the first pass, replace it with the LTO output on the
> ? ? ? ? ? ? ? ? second pass. ?We can't simply choose real object
> ? ? ? ? ? ? ? ? files over IR because the first pass may contain a
> ? ? ? ? ? ? ? ? mix of LTO and normal objects and we must keep the
> ? ? ? ? ? ? ? ? first match, be it IR or real. ?*/

Thanks.  I added it.

>> + ? ? ? ? ? ? {
>> + ? ? ? ? ? ? ? /* Replace the plugin dummy with the LTO output. ?*/
>> + ? ? ? ? ? ? ? l->linked = *linked;
>> + ? ? ? ? ? ? ? return;
>> + ? ? ? ? ? ? }
>> ? ? ? ? ? ? break;
>
>> + ? ? ? l_owner = l_sec->owner;
>> + ? ? ? l_sec = l->linked.u.sec;
>
> Ordering problem above. ?Did this compile?

Fixed.

> Please explain why the following change is necessary. ?I'd like to
> know why you need to have the correct symbol on the first pass.

As the comments you mentioned above, in the first pass, we want
to keep symbol definitions with comdat keys from IR dummy BFD.
We will replace them with the definitions from LTO outputs in the
second pass.  I added some comments.

>> + ? ? ? /* A symbol with comdat key in IR dummy BFD may override
>> + ? ? ? ? ?the comdat symbol in a real BFD. ?*/
>> + ? ? ? if (link_info.loading_lto_outputs
>> + ? ? ? ? ? || (h->u.def.section->flags & SEC_LTO_COMDAT) == 0)
>> + ? ? ? ? {
>> + ? ? ? ? ? h->type = bfd_link_hash_undefweak;
>> + ? ? ? ? ? h->u.undef.abfd = h->u.def.section->owner;
>> + ? ? ? ? }
>> + ? ? ? else
>> + ? ? ? ? h->non_ir_ref = TRUE;
>

OK to install?

Thanks.

-- 
H.J.
---
bfd/

2011-07-04  H.J. Lu  <hongjiu.lu@intel.com>

	PR ld/12942
	* elf-bfd.h (_bfd_elf_section_already_linked): Replace
	"asection *" with "struct already_linked *".
	* libbfd-in.h (_bfd_nolink_section_already_linked): Likewise.
	(_bfd_generic_section_already_linked): Likewise.
	(bfd_section_already_linked_table_insert): Likewise.
	(struct already_linked): New.
	(struct bfd_section_already_linked): Use it.

	* elf.c (_bfd_elf_make_section_from_shdr): Handle
	GNU_LTO_COMDAT_SECTION_NAME.

	* elflink.c (_bfd_elf_section_already_linked): Replace.
	"asection *" with "struct already_linked *".  Replace the plugin
	dummy with the LTO output.
	* linker.c (_bfd_generic_section_already_linked): Likewise.

	* section.c (SEC_LTO_COMDAT): New.
	(GNU_LTO_COMDAT_SECTION_NAME): Likewise.

	* targets.c (struct already_linked): Add forward declaration.
	(bfd_target): Replace "struct bfd_section *" with
	"struct already_linked *" in _section_already_linked.

	* bfd-in2.h: Regenerated.
	* libbfd.h: Likewise.

include/

2011-07-04  H.J. Lu  <hongjiu.lu@intel.com>

	PR ld/12942
	* bfdlink.h (bfd_link_info): Add loading_lto_outputs.

ld/

2011-07-04  H.J. Lu  <hongjiu.lu@intel.com>

	PR ld/12942
	* ldlang.c (section_already_linked): Pass "struct already_linked *"
	to bfd_section_already_linked.
	(lang_process): Set link_info.loading_lto_outputs before
	loading LTO outputs.

	* plugin.c: Include "libbfd.h".
	(plugin_get_ir_dummy_bfd): Create GNU_LTO_COMDAT_SECTION_NAME
	section.
	(asymbol_from_plugin_symbol): Use GNU_LTO_COMDAT_SECTION_NAME
	with comdat_key.
	(add_symbols): Call bfd_section_already_linked with comdat_key.
	(plugin_notice): Set non_ir_ref to TRUE if SEC_LTO_COMDAT is
	set.

Attachment: binutils-pr12942-1.patch
Description: Text document