Re: [gold][aarch64] Fix internal error while fixing 843419

[Han Shen <shenhan at google dot com>]

Hi Cary, comments addressed and CL submitted. Thanks for your review.

Han

On Mon, Jun 29, 2015 at 11:54 AM, Cary Coutant <ccoutant@gmail.com> wrote:
>
> > The crash reason is that the insn to be moved to stub may be a
> > relocation spot, so instead of placing the origin insn (that is insn
> > before-relocation) to the stub, I have to place the relocated one.
> > Note the relocation involved is non-pc-relative, so it is safe to move
> > the relocated insn.
> >
> > Testing - Build the problematic android component, assembly examined
> > (also add this to local aarch64 test set). And passed all previous
> > tests.
> >
> > Ok for trunk?
> >
> > gold/ChangeLog:
> >         * AArch64.cc (Erratum_stub::Insn_utilities): New typedef.
> >         (Erratum_stub::update_erratum_insn): New method.
> >         (Stub_table::relocate_stubs): Modified to place relocated insn.
> >         (AArch64_relobj::fix_errata): Modified gold_assert.
>
>
> +  // For 843419, the erratum insn is ld/st xt, [xn, #uimm], which may
> +  // be a relocation spot, in this case, the erratum_insn_ recorded at
> +  // scanning phase is no long the one we want to write out to the
>
> "no longer"


Done
>
>
> +  // stub, update erratum_insn_ with relocated version. Also note that
> +  // in this case xn must not be "PC", so it is safe to move the
> +  // erratum insn from the origin place to the stub. For 835769, the
> +  // erratum insn is multiply-accumulate insn, which could not be a
> +  // relocation spot (assertion added though).
> +  void
> +  update_erratum_insn(Insntype insn)
> +  {
> +    gold_assert(this->erratum_insn_ != this->invalid_insn);
> +    switch (this->type())
> +      {
> +      case ST_E_843419:
> +       gold_assert(Insn_utilities::aarch64_ldst_uimm(insn));
> +       gold_assert(Insn_utilities::aarch64_ldst_uimm(this->erratum_insn()));
> +       gold_assert(Insn_utilities::aarch64_rd(insn) ==
> +                   Insn_utilities::aarch64_rd(this->erratum_insn()));
> +       gold_assert(Insn_utilities::aarch64_rn(insn) ==
> +                   Insn_utilities::aarch64_rn(this->erratum_insn()));
>
> If any of these assertions fail, does that indicate a failure of the
> internal logic in the linker, or could it indicate bad input? We
> should not be asserting on conditions that may be caused by bad input,
> but instead issuing a normal error message.

If any of the 4 assertion fails here, then it means the insn before
and after relocation differs in insn type or in register operands,
which is no possible. (Memory related relocation only change address
offset.)

Similar for assertion in case ST_E_835769, which asserts that a
multiply-accumulate insn is never changed during relocation pass, if
not, then it means linker has a bug.

>
>
> +         // The 1st insn of the erratum could be a relocation spot,
> +         // in this case we need to fix it with
> +         // "(*i)->erratum_insn()".
> +         elfcpp::Swap<32, big_endian>::writeval(
> +             reinterpret_cast<Insntype*>(
> +                 view + (stub_address - this->address())),
> +             (*i)->erratum_insn());
>
> I'm pretty sure that the cast to <Insntype*> here is unnecessary.

Removed and built.

>
> This is OK with these fixes. Thanks!
>
> -cary


-- 
Han Shen