[gold][aarch64] Patch for erratum-843419 (2 of 2 - fix erratum occurrences)

[Han Shen]

Hi Cary, thanks for the review. (I also just came back from an offsite...)

Attached the new patch e843419-2nd.patch. Also
"diff-against-prev-patch.patch" for ease of incremental review.

On Mon, Jun 1, 2015 at 2:39 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> Hi Cary, here is the 2nd patch for erratum 843419 - fixing the erratum.
>
> Sorry for the delay. I'm back from vacation now.
>
> -cary
>
>
> +// Stub type enum constants wrapped in a struct, so we refer to them as
> +// Stub_type::ST_XXX instead of ST_XXX.
> +struct Stub_type
> +{
> +  enum
> +    {
> +      ST_NONE = 0,
> +
> +      // Using adrp/add pair, 4 insns (including alignment) without mem access,
> +      // the fastest stub. This has a limited jump distance, which is tested by
> +      // aarch64_valid_for_adrp_p.
> +      ST_ADRP_BRANCH = 1,
> +
> +      // Using ldr-absolute-address/br-register, 4 insns with 1 mem access,
> +      // unlimited in jump distance.
> +      ST_LONG_BRANCH_ABS = 2,
> +
> +      // Using ldr/calculate-pcrel/jump, 8 insns (including alignment) with 1
> +      // mem access, slowest one. Only used in position independent
> executables.
> +      ST_LONG_BRANCH_PCREL = 3,
> +
> +      // Stub for erratum 843419 handling.
> +      ST_E_843419 = 4,
> +
> +      // Number of total stub types.
> +      ST_NUMBER = 5
> +    };
> +
> +private:
> +  // Never allow any such instance.
> +  Stub_type();
> +};
>
> Instead of using a struct to get the effect of a namespace, why
> not just use a namespace directly? (It's not clear to me that you
> really need to enclose these constants in a namespace at all. The
> entire source file is already in an anonymous namespace, so
> there's no worry about conflicting with any names from another
> source file.)

Removed wrapping struct, and define them as global enum constants.

>
> +// Simple singleton class that creates/initializes/stores all types of stub
> +// templates.
> +
> +template<bool big_endian>
> +class Stub_template_repertoire
> +{
> +public:
> +  typedef typename AArch64_insn_utilities<big_endian>::Insntype Insntype;
> +
> +  // Get singleton instance.
> +  static Stub_template_repertoire<big_endian>*
> +  get_instance()
> +  {
> +    static Stub_template_repertoire<big_endian> singleton;
> +    return &singleton;
> +  }
> +
> +  // Get stub template for a given stub type.
> +  Stub_template<big_endian>*
> +  get_stub_template(int type)
> +  { return this->stub_templates_[type]; }
> +
> +private:
> +  // Constructor - creates/initilizes all stub templates.
> +  Stub_template_repertoire();
> +
> +  // Destructor - deletes all stub templates.
> +  ~Stub_template_repertoire();
> +
> +  Stub_template_repertoire(Stub_template_repertoire&);
> +  Stub_template_repertoire& operator = (Stub_template_repertoire&);
>
> No spaces around "=".

Done

>
> I'd suggest adding a comment that you're disallowing these
> constructors.

Done

>
> +
> +  // Data that stores all insn templates.
> +  Stub_template<big_endian>* stub_templates_[Stub_type::ST_NUMBER];
> +};  // End of "class Stub_template_repertoire".
>
> This class seems overly complex. It seems to me that it would be
> simpler and clearer to make Stub_template a simple struct with
> two fields, and make your repertoire a statically-initialized
> array.

Reduced Stub_template to POD with 2 constant fields.

>
> If you decide to stick with the singleton class, however, I see
> no need for the get_instance() method -- just make
> get_stub_template() a static member, and put the singleton there.

Keep Stub_template_repertoire as a singleton class but removed get_instance.
Also in ctor, use static variable to avoid explicit deletion in
~Stub_template_repertoire dtor.

>
> +// Constructor - creates/initilizes all stub templates.
>
> "initializes"

Done

>
> +  // The stub offset. Note this has difference interpretations between an
> +  // Reloc_stub and an Erratum_stub. For Reloc_stub this is the offset from the
> +  // beginning of the containng stub_table, whereas for Erratum_stub, this is
>
> "containing"

Done

>
> +  // the offset from the end of reloc_stubs.
> +  section_offset_type offset_;
> +  // Stub type.
> +  const int type_;
> +  // Stub template that provides stub insn information.
> +  const Stub_template<big_endian>* stub_template_;
>
> This is always going to be get_stub_templates(this->type_), so why not make
> this a method instead of a data member?

Yup, this is good point. Done.

>
> +// Erratum stub class. An erratum stub differs from a reloc stub in that for
> +// each erratum occurrence, we generates an erratum stub, we never
> share erratum
> +// stubs, whereas for reloc stubs, different branches insns share a
> single reloc
> +// stub as long as the branch targets are the same.
>
> "... we generate an erratum stub. We never share ..."

Done.

>
> More to the point, reloc stubs can be shared because they're used
> to reach a specific target, whereas erratum stubs branch back to
> the original control flow.

Added this to the comment.

>
> +  // For current implemnted erratum 843419, (and 835769 which is to be
> +  // implemented soon), the first insn in the stub is always a copy of the
> +  // problmatic insn (in 843419, the mem access insn), followed by a jump-back.
>
> "problematic"
>
> +// Find all the errata for a given input sectin. The return value is a pair of
>
> "section"

Done.

>
> +         stub_b_insn_address = stub_address
> +           + 1 * AArch64_insn_utilities<big_endian>::BYTES_PER_INSN;
>
> Need parentheses around the split expression. It might look nicer if
> you declare a local const with a shorter name:
>
>     const int BPI = AArch64_insn_utilities<big_endian>::BYTES_PER_INSN
>     ...
>           stub_b_insn_address = stub_address + 1 * BPI;

Done.

Thanks,
Han
-------------- next part --------------
A non-text attachment was scrubbed...
Name: e843419-2nd.patch
Type: text/x-patch
Size: 36975 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20150605/6240b59a/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: diff-against-prev-patch.patch
Type: text/x-patch
Size: 13014 bytes
Desc: not available
URL: <https://sourceware.org/pipermail/binutils/attachments/20150605/6240b59a/attachment-0001.bin>