This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [GOLD] PowerPC64 identical code folding


On Mon, Mar 11, 2013 at 5:20 PM, Alan Modra <amodra@gmail.com> wrote:
> This makes the ICF testcase, icf_test.cc, work for PowerPC64.
> icf_test.cc has functions with recursive calls, which are recognised
> as such in icf.cc by noticing calls back to the originating section.
> On PowerPC64 a function address is the address of the descriptor in
> .opd, rather than the function entry point.  So icf.cc doesn't see the
> calls as recursive and therefore won't fold the sections.  We need to
> look through the function descriptor to notice recursive calls.  I
> also modify the symbol value/addend pair to point at the function
> entry point.  That isn't strictly necessary with current ICF and gcc
> generated code, but it seems The Right Thing To Do.
>
> You might notice I didn't say the ICF testcase passes.  It doesn't.
> That's because the address of folded_func and kept_func are different
> on PowerPC64 even though their code sections are folded.  We still
> have two different function descriptors..  I'll get around to folding
> them too one day, along with removing unused .opd entries.

Actually keeping around different function descriptors seems better
and then for PowerPC64 you can just turn ICF by default and not
violate C/C++ rules about function pointers being different.

Thanks,
Andrew

>
> OK to apply?
>
>         * gc.h (gc_process_relocs): Look through function descriptors
>         to determine shndx, symvalue and addend used by ICF.  Tidy
>         variable duplication.
>
> Index: gold/gc.h
> ===================================================================
> RCS file: /cvs/src/src/gold/gc.h,v
> retrieving revision 1.17
> diff -u -p -r1.17 gc.h
> --- gold/gc.h   9 Sep 2012 03:43:51 -0000       1.17
> +++ gold/gc.h   11 Mar 2013 22:25:02 -0000
> @@ -235,30 +235,45 @@ gc_process_relocs(
>        Reloc_types<sh_type, size, big_endian>::get_reloc_addend_noerror(&reloc);
>        Object* dst_obj;
>        unsigned int dst_indx;
> -      typename elfcpp::Elf_types<size>::Elf_Addr dst_off;
> +      typedef typename elfcpp::Elf_types<size>::Elf_Addr Address;
> +      Address dst_off;
>
>        if (r_sym < local_count)
>          {
>            gold_assert(plocal_syms != NULL);
>            typename elfcpp::Sym<size, big_endian> lsym(plocal_syms
>                                                        + r_sym * sym_size);
> -          unsigned int shndx = lsym.get_st_shndx();
> +         dst_indx = lsym.get_st_shndx();
>            bool is_ordinary;
> -          shndx = src_obj->adjust_sym_shndx(r_sym, shndx, &is_ordinary);
> +         dst_indx = src_obj->adjust_sym_shndx(r_sym, dst_indx, &is_ordinary);
>            dst_obj = src_obj;
> -          dst_indx = shndx;
> -         dst_off = lsym.get_st_value();
> +         dst_off = lsym.get_st_value() + addend;
>
>            if (is_icf_tracked)
>              {
> +             Address symvalue = dst_off - addend;
>               if (is_ordinary)
> -                (*secvec).push_back(Section_id(dst_obj, dst_indx));
> +               {
> +                 Symbol_location loc;
> +                 loc.object = dst_obj;
> +                 loc.shndx = dst_indx;
> +                 loc.offset = convert_types<off_t, Address>(dst_off);
> +                 // Look through function descriptors
> +                 parameters->target().function_location(&loc);
> +                 if (loc.shndx != dst_indx)
> +                   {
> +                     // Modify symvalue/addend to the code entry.
> +                     symvalue = loc.offset;
> +                     addend = 0;
> +                   }
> +                 (*secvec).push_back(Section_id(loc.object, loc.shndx));
> +               }
>               else
>                  (*secvec).push_back(Section_id(NULL, 0));
>                (*symvec).push_back(NULL);
> -              long long symvalue = static_cast<long long>(lsym.get_st_value());
> -              (*addendvec).push_back(std::make_pair(symvalue,
> -                                              static_cast<long long>(addend)));
> +             (*addendvec).push_back(std::make_pair(
> +                                       static_cast<long long>(symvalue),
> +                                       static_cast<long long>(addend)));
>                uint64_t reloc_offset =
>                  convert_to_section_size_type(reloc.get_r_offset());
>               (*offsetvec).push_back(reloc_offset);
> @@ -279,7 +294,7 @@ gc_process_relocs(
>              symtab->icf()->set_section_has_function_pointers(
>                src_obj, lsym.get_st_shndx());
>
> -          if (!is_ordinary || shndx == src_indx)
> +          if (!is_ordinary || dst_indx == src_indx)
>              continue;
>          }
>        else
> @@ -291,14 +306,14 @@ gc_process_relocs(
>
>            dst_obj = NULL;
>            dst_indx = 0;
> -         dst_off = 0;
>            bool is_ordinary = false;
>            if (gsym->source() == Symbol::FROM_OBJECT)
>              {
>                dst_obj = gsym->object();
>                dst_indx = gsym->shndx(&is_ordinary);
> -             dst_off = static_cast<const Sized_symbol<size>*>(gsym)->value();
>              }
> +         dst_off = static_cast<const Sized_symbol<size>*>(gsym)->value();
> +         dst_off += addend;
>
>           // When doing safe folding, check to see if this relocation is that
>           // of a function pointer being taken.
> @@ -326,17 +341,29 @@ gc_process_relocs(
>              }
>            if (is_icf_tracked)
>              {
> +             Address symvalue = dst_off - addend;
>                if (is_ordinary && gsym->source() == Symbol::FROM_OBJECT)
> -                (*secvec).push_back(Section_id(dst_obj, dst_indx));
> +               {
> +                 Symbol_location loc;
> +                 loc.object = dst_obj;
> +                 loc.shndx = dst_indx;
> +                 loc.offset = convert_types<off_t, Address>(dst_off);
> +                 // Look through function descriptors
> +                 parameters->target().function_location(&loc);
> +                 if (loc.shndx != dst_indx)
> +                   {
> +                     // Modify symvalue/addend to the code entry.
> +                     symvalue = loc.offset;
> +                     addend = 0;
> +                   }
> +                 (*secvec).push_back(Section_id(loc.object, loc.shndx));
> +               }
>               else
>                  (*secvec).push_back(Section_id(NULL, 0));
>                (*symvec).push_back(gsym);
> -              Sized_symbol<size>* sized_gsym =
> -                        static_cast<Sized_symbol<size>* >(gsym);
> -              long long symvalue =
> -                        static_cast<long long>(sized_gsym->value());
> -              (*addendvec).push_back(std::make_pair(symvalue,
> -                                        static_cast<long long>(addend)));
> +             (*addendvec).push_back(std::make_pair(
> +                                       static_cast<long long>(symvalue),
> +                                       static_cast<long long>(addend)));
>                uint64_t reloc_offset =
>                  convert_to_section_size_type(reloc.get_r_offset());
>               (*offsetvec).push_back(reloc_offset);
> @@ -353,7 +380,6 @@ gc_process_relocs(
>        if (parameters->options().gc_sections())
>          {
>           symtab->gc()->add_reference(src_obj, src_indx, dst_obj, dst_indx);
> -         dst_off += addend;
>           parameters->sized_target<size, big_endian>()
>             ->gc_add_reference(symtab, src_obj, src_indx,
>                                dst_obj, dst_indx, dst_off);
>
> --
> Alan Modra
> Australia Development Lab, IBM


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]