Re: [GOLD][PATCH] Handle initial TLS relocations in static linking.

[=?big5?b?RG91ZyBLd2FuICjD9q62vHcp?= <dougkwan at google dot com>]

Hi,

     This is an updated patch.  I simplified code in
Arm_output_data_got::do_write.   The original patch was also wrong in
handling undefined symbols.  This patch fixes that.  There is no
change in ChangeLog.

-Doug

2010/2/24 Ian Lance Taylor <iant@google.com>:
> "Doug Kwan (Ãö®¶¼w)" <dougkwan@google.com> writes:
>
>>> 2010-02-23  Doug Kwan  <dougkwan@google.com>
>>>
>>>        * arm.cc (Arm_output_data_got): New class.
>>>        (ARM_TCB_SIZE): New constant
>>>        (Target_arm): Use Arm_output_data_got instead of Output_data_got.
>>>        (Arm_output_section::fix_exidx_coverage): Add a parameter for layout.
>>>        If user uses a script with a SECTIONS clause, issue only a warning
>>>        for a misplaced EXIDX input section.  Otherwise, issue an error.
>>>        (Arm_relobj::do_gc_process_relocs): Exit early if we are not doing
>>>        garbage collection.
>>>        (Target_arm::got_mode_index_entry): Handle static linking.
>>>        (Target_arm::Scan::local): Ditto.
>>>        (Target_arm::Scan::global): Ditto.
>>>        (Target_arm::Relocate::relocate_tls): Handle static linking.  Fix
>>>        all incorrectly implemented relocations.
>>>        (Target_arm::fix_exidx_coverage): Pass layout to
>>>        Arm_output_section::fix_exidx_coverage.
>>>        * layout.cc (Layout::section_name_mapping): Remove trailing dots
>>>        from ".ARM.exidx." and ".ARM.extab.".
>
>> +      Symbol_value<32> symval;
>> +      const Symbol_value<32> *psymval;
>> +      bool is_defined_in_discarded_section;
>> +      Sized_relobj<32, big_endian>* object = NULL;
>> +      unsigned int shndx = elfcpp::SHN_UNDEF;
>> +       bool is_ordinary;
>> +      if (!reloc.symbol_is_global())
>> +     {
>
> The indentation looks wrong on the is_ordinary declaration.
>
>
>> +       sym = NULL;
>> +       object = reloc.relobj();
>> +       psymval = object->local_symbol(reloc.index());
>> +
>> +          // If the local symbol belongs to a section we are discarding,
>> +          // and that section is a debug section, try to find the
>> +          // corresponding kept section and map this symbol to its
>> +          // counterpart in the kept section.  The symbol must not
>> +          // correspond to a section we are folding.
>> +       shndx = psymval->input_shndx(&is_ordinary);
>> +       is_defined_in_discarded_section =
>> +         (is_ordinary
>> +          && shndx != elfcpp::SHN_UNDEF
>> +          && !object->is_section_included(shndx)
>> +          && !this->symbol_table_->is_section_folded(object, shndx));
>> +     }
>> +      else
>> +     {
>> +       const Symbol* gsym = reloc.symbol();
>> +       gold_assert(gsym != NULL);
>> +       if (gsym->is_forwarder())
>> +         gsym = this->symbol_table_->resolve_forwards(gsym);
>> +
>> +       sym = static_cast<const Sized_symbol<32>*>(gsym);
>> +       if (sym->has_symtab_index())
>> +         symval.set_output_symtab_index(sym->symtab_index());
>> +       else
>> +         symval.set_no_output_symtab_entry();
>> +       symval.set_output_value(sym->value());
>> +       psymval = &symval;
>> +
>> +       is_defined_in_discarded_section =
>> +         (gsym->is_defined_in_discarded_section()
>> +          && gsym->is_undefined());
>> +
>> +       // We need to figure out input object and section index if the
>> +       // symbol is in a discarded_section.
>> +       if (is_defined_in_discarded_section)
>> +         {
>> +           gold_assert(gsym->source() == Symbol::FROM_OBJECT
>> +                       && !gsym->in_dyn());
>> +           object =
>> +             static_cast<Sized_relobj<32, big_endian>*>(gsym->object());
>> +           shndx = gsym->shndx(&is_ordinary);
>> +         }
>> +     }
>> +
>> +      Symbol_value<32> symval2;
>> +      if (is_defined_in_discarded_section)
>> +     {
>> +       gold_assert(object != NULL && shndx != elfcpp::SHN_UNDEF);
>> +       std::string name = object->section_name(shndx);
>> +       Comdat_behavior comdat_behavior = get_comdat_behavior(name.c_str());
>> +       if (comdat_behavior == CB_PRETEND)
>> +         {
>> +           // FIXME: This case does not work for global symbols.
>> +           // We have no place to store the original section index.
>> +           // Fortunately this does not matter for comdat sections,
>> +           // only for sections explicitly discarded by a linker
>> +           // script.
>> +           bool found;
>> +           Arm_address value = object->map_to_kept_section(shndx, &found);
>> +           if (found)
>> +             symval2.set_output_value(value + psymval->input_value());
>> +           else
>> +             symval2.set_output_value(0);
>> +         }
>> +       else
>> +         {
>> +           // We don't need to issue a warning since a warning about the
>> +           // same discarded symbol will be issue in the initial TLS
>> +           // relocation.
>> +           symval2.set_output_value(0);
>> +         }
>> +       symval2.set_no_output_symtab_entry();
>> +       psymval = &symval2;
>> +     }
>
> I think this is not right.  You are checking for the comdat behaviour
> for the section where the symbol is defined.  That's not how it works.
> The comdat behaviour is for the section where the relocation is being
> applied.  The idea is that we handle references from debugging
> sections to discarded symbols in one way, and we handle references
> from normal sections to discarded symbols in a different way (from
> debugging sections we use zero, from normal sections we give a
> warning).  The section where the symbol is defined is irrelevant.
>
> In this case you know the section where the reference is coming from:
> it's the GOT section.  So get_comdat_behavior will conceptually always
> return CB_WARNING, and you should always issue a warning.  Or perhaps
> the warning will always be issued anyhow for the relocation which
> triggered adding this GOT entry, in which case you can ignore all this
> stuff here.
>
> Either way, there is some duplication with target-reloc.h, and I
> wonder if some of it can be pulled into helper functions in
> target-reloc.h.  I'm not sure it's worth it after the comdat stuff is
> cleaned up, but perhaps it is.
>
> Ian
>

Attachment: patch-static-tls-updated.txt
Description: Text document