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

[Cary Coutant <ccoutant at gmail dot com>]

> 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.)

+// 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 "=".

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

+
+  // 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.

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.

+// Constructor - creates/initilizes all stub templates.

"initializes"

+  // 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"

+  // 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?

+// 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 ..."

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.

+  // 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"

+         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;