Re: [PATCH][x86_64] Convert indirect call via GOT to direct when possible

[Sriraman Tallam <tmsriram at google dot com>]

On Mon, Jun 20, 2016 at 12:04 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> I am attaching the patch after making all the changes mentioned.
>> Please take a look.
>
> +       // If relocation type is R_X86_64_GOTPCRELX it is automatically a
> +       // candidate for conversion.
> +       if (r_type ==  elfcpp::R_X86_64_GOTPCRELX)
> +         break;
>
> If you're not calling can_convert_callq_to_direct() here, then you
> shouldn't be calling it here:

Yes, good point.  I am a little confused about this new relocation
elfcpp::R_X86_64_GOTPCRELX.  I have attached a patch that reverts to
the change you suggested where I check for conversion always.
However,  is it safe to assume that a elfcpp::R_X86_64_GOTPCRELX
relocation implies that the instruction containing the relocation is
eligible for one of the conversions *always* ?

If this is true, then I could completely remove
can_convert_callq_to_direct  and simplify the code a lot more.
Otherwise, the check is needed in both places.   Will
-relax-relocations=yes in the assembler do the checks before
converting R_X86_64_GOTPCREL to  now R_X86_64_GOTPCRELX. The attached
patch now contains a conservative check in both places to
can_convert_callq_to_direct.

Thanks
Sri


>
> +      // Convert
> +      // callq *foo@GOTPCRELX(%rip) to
> +      // addr32 callq foo
> +      // and jmpq *foo@GOTPCRELX(%rip) to
> +      // jmpq foo
> +      // nop
> +      else if (gsym != NULL
> +              && rela.get_r_offset() >= 2
> +              && Target_x86_64<size>::can_convert_callq_to_direct(gsym,
> +                                                                  r_type,
> +                                                                  0, &view))
>
> What will happen if it returns false in relocate()? You'll get no
> conversion, but also no GOT entry. Since you're only doing this for
> the GOTPCRELX relocation, you don't need can_convert_callq_to_direct()
> at all, but you will need an error case for when this is false:
>
> +    return ((*view)[r_offset - 2] == 0xff
> +            && ((*view)[r_offset - 1] == 0x15
> +                || (*view)[r_offset - 1] == 0x25));
>
> If you see a GOTPCRELX relocation, and the opcode isn't either 0xff
> 0x15 or 0xff 0x25, that's a bad input that should be diagnosed.
>
> -cary

Attachment: convert_indirect_call_patch.txt
Description: Text document