Re: Fwd: [PATCH][GOLD] Classes to support stub generation in ARM.

[Ian Lance Taylor <iant at google dot com>]

"Doug Kwan (éæå)" <dougkwan@google.com> writes:

> 2009-10-01  Doug Kwan  <dougkwan@google.com>
>
>        * arm.cc: Update copyright comments.
>        (Target_arm): New forward class template declaration.
>        (Arm_address): New type.
>        (ARM_MAX_FWD_BRANCH_OFFSET, ARM_MAX_BWD_BRANCH_OFFSET,
>        THM_MAX_FWD_BRANCH_OFFSET, THM_MAX_BWD_BRANCH_OFFSET,
>        THM2_MAX_FWD_BRANCH_OFFSET, THM2_MAX_BWD_BRANCH_OFFSET): New
>        constants.
>        (Insn_template): Same.
>        (DEF_STUBS): New macro.
>        (Stub_type): New enum type.
>        (Stub_template): New class definition.
>        (Stub): Same.
>        (Reloc_stub): Same.
>        (Stub_factory): Same.
>        (Target_arm::Target_arm): Initialize use_blx_ and pic_veneer_.
>        (Target_arm::use_blx, Target_arm::set_use_blx, Target_arm::pic_veneer,
>        Target_arm::set_pic_veneer, Target_arm::using_thumb2,
>        Target_arm::using_thumb_only, Target_arm:;default_target): New
>        method defintions.
>        (Target_arm::use_blx_, Target_arm::pic_veneer_): New data member
>        declarations.
>        (Insn_template::size, Insn_template::alignment): New method
>        defintions.
>        (Stub_template::Stub_template): New method definition.
>        (Reloc_stub::Key::name, Reloc_stub::stub_type_for_reloc,
>        Reloc_stub::do_fixed_endian_write, Reloc_stub::do_write): Same.
>        (Stub_factory::Stub_factory): New method definition.


> +  static const Insn_template
> +  thumb16_insn(unsigned data)
> +  { return Insn_template(data, THUMB16_TYPE, elfcpp::R_ARM_NONE, 0); } 

In this function and many subsequent ones, the data parameter is
stored into a field of type uint32_t.  I think the parameter should
have type uint32_t.


> +  // Instruction specific data.
> +  uint32_t data_;

Please expand this comment a bit to indicate what type of data this
is.

> +  // Relocation type if there is a relocation or R_ARM_NOE otherwise.
> +  unsigned int r_type_;

s/NOE/NONE/

> +  // Return an array of instunction templates.
> +  const Insn_template*
> +  insns() const
> +  { return this->insns_; }

s/instunction/instruction/


> + private:
> +  // This must be defined in the child class.
> +  virtual Arm_address
> +  do_reloc_target(size_t) = 0;
> +
> +  // This must be defined in the child class.
> +  virtual void
> +  do_write(unsigned char*, section_size_type, bool) = 0;
> +  
> +  // Its template.
> +  const Stub_template* stub_template_;
> +  // Offset within the section of containing this stub.
> +  section_offset_type offset_;
> +};

gold convention is to make the do_xxx methods protected and the data
members private.


> +    // Return a hash value.
> +    size_t
> +    hash_value() const
> +    {
> +      return (this->stub_type_
> +	      ^ this->r_sym_
> +	      ^ ((this->r_sym_ != Reloc_stub::invalid_index)
> +		 ? reinterpret_cast<intptr_t>(this->u_.relobj)
> +		 : reinterpret_cast<intptr_t>(this->u_.symbol))
> +	      ^ this->addend_);
> +    }

Hash values which depend on object addresses are scary.  Are you sure
this is OK--i.e., will not affect linker output on different hosts?  I
think this needs a comment if it is OK.

> +  ~Stub_factory()
> +  {
> +    for(size_t i = 0; i <= arm_stub_type_last; ++i)
> +      delete this->stub_templates_[i];
> +  }

Don't bother writing this destructor if it will only be invoked when
gold exits.  It's cleaner but it just wastes time.


> +      copy_relocs_(elfcpp::R_ARM_COPY), dynbss_(NULL),
> +      use_blx_(true), pic_veneer_(false)
>    { }
>  
> +  // Whether we can use BLX.
> +  bool
> +  use_blx() const
> +  { return this->use_blx_; }
> +
> +  // Set use-BLX flag.
> +  void
> +  set_use_blx(bool value)
> +  { this->use_blx_ = value; }

Ideally flags should have names which are predicates.  The comment
suggests that this should be named may_use_blx throughout.


> +  // Whether we force PCI branch veneers.
> +  bool
> +  pic_veneer() const
> +  { return this->pic_veneer_; }
> +
> +  // Set PIC veneer flag.
> +  void
> +  set_pic_veneer(bool value)
> +  { this->pic_veneer_ = value; }

Similarly, it seems that this should be named should_force_pic_veneer.


> +// Stub_template methods.
> +
> +Stub_template::Stub_template(
> +    Stub_type type, const Insn_template* insns,
> +     size_t insn_count)
> +  : type_(type), insns_(insns), insn_count_(insn_count), alignment_(1),
> +    entry_in_thumb_mode_(false), relocs_()
> +{
> +  off_t offset = 0;
> +
> +  // Compute byte size and alignment of stub template.
> +  for(size_t i = 0; i < insn_count; i++)

s/for(/for (/

> +std::string
> +Reloc_stub::Key::name() const
> +{
> +  if (this->r_sym_ == invalid_index)
> +    {
> +      // Global symbol key name
> +      // <stub-type>:<symbol name>:<addend>.
> +      const std::string sym_name = this->u_.symbol->name();
> +      // We need to print two hex number and two colons.  So just add 100 bytes
> +      // to the symbol name size.
> +      size_t len = sym_name.size() + 100;
> +      char buffer[len];
> +      int c = snprintf(buffer, len, "%d:%s:%x", this->stub_type_,
> +		       sym_name.c_str(), this->addend_);
> +      gold_assert(c >0 && c < static_cast<int>(len));
> +      return std::string(buffer);

Variable length arrays are supported by gcc but are not a standard C++
feature.  Also the symbol name can be absurdly long and gold can run
multi-threaded.  This should be written using new char[].

s/c >0/c > 0/

> +    }
> +  else
> +    {
> +      // local symbol key name
> +      // <stub-type>:<object>:<r_sym>:<addend>.
> +      size_t len = 200;
> +      char buffer[len];
> +      int c = snprintf(buffer, len, "%d:%p:%u:%x", this->stub_type_,
> +		       this->u_.relobj, this->r_sym_, this->addend_);
> +      gold_assert(c > 0 && c < static_cast<int>(len));
> +      return std::string(buffer);
> +    }
> +}

In this bit, make len const so that the array is OK.


This is OK with those changes.

Sorry for the long review delay.

Ian