On Sat, Jan 13, 2018 at 7:56 AM, Jan Hubicka <hubi...@ucw.cz> wrote:
>> >> +/* Output a funtion with a call and return thunk for indirect branch.
>> >> + If BND_P is true, the BND prefix is needed. If REGNO != -1, the
>> >> + function address is in REGNO. Otherwise, the function address is
>> >> + on the top of stack. */
>> >> +
>> >> +static void
>> >> +output_indirect_thunk_function (bool need_bnd_p, int regno)
>> >> +{
>> >> + char name[32];
>> >> + tree decl;
>> >> +
>> >> + /* Create __x86_indirect_thunk/__x86_indirect_thunk_bnd. */
>> >> + indirect_thunk_name (name, regno, need_bnd_p);
>> >> + decl = build_decl (BUILTINS_LOCATION, FUNCTION_DECL,
>> >> + get_identifier (name),
>> >> + build_function_type_list (void_type_node, NULL_TREE));
>> >> + DECL_RESULT (decl) = build_decl (BUILTINS_LOCATION, RESULT_DECL,
>> >> + NULL_TREE, void_type_node);
>> >> + TREE_PUBLIC (decl) = 1;
>> >> + TREE_STATIC (decl) = 1;
>> >> + DECL_IGNORED_P (decl) = 1;
>> >
>> > DECL_ARTIFICIAL as well?
>>
>> This is done exactly the same way as PIC thunk. I don't think we
>> should change it here.
>> >
>> > Why do you need to output asm visibility directives by hand when you create
>> > symbol for the function anyway?
>>
>> This is done exactly the same way as PIC thunk. I don't think we
>> should change it here.
>
> I see, it is pretty ancient code. Perhaps you can at least commonize
> the uglness so we don't duplicate the ifdefs for MACHO?
I don't think we should such surgery at such late stage. I prefer to
keep it for later cleanup.
>>
>> > I would expect that you can just use similar code as
>> > cgraph_node::expand_thunk
>> > when calls output_mi_thunk and get this done in a way that is independent
>> > of
>> > target assembler.
>>
>> I took a look. I don't see an easy to do it. I'd like to keep it exactly
>> the
>> same as PIC thunk. And we change both thunks together later if needed.
>
> OK, lets keep it done same was as PIC thunk.
> Next stage1 we could clean up both.
>> > I suppose it may make sense to split the insn and at least explicitly
>> > represent
>> > the fact that we (sometimes) push the target to stack.
>>
>> Did you mean "split the function"? I will break it into
>> ix86_output_indirect_branch_via_reg and
>> ix86_output_indirect_branch_via_push.
>
> I mean define_split to insert push instruction into the instruction
> stream rather then printing it as part of the indirect jump insn.
We don't want anything added between these instructions.
Split it may lead to trouble.
>>
>> > Why the memory variant exists at first place?
>>
>> When the function address is in memory, we can push it onto stack
>> to save a register. Also it is needed to cover "call foo" to "call
>> [foo@GOT]"
>> conversion.
>> >
>> > I think you also want to update type to "many" when we do more than just
>> > indirect branch.
>>
>> Did you mean "multi"? I will change to "multi".
>
> Yes.
>
>>
>> > We do not care much about this, but it feels wrong to have attributes off
>> > reality.
>> >> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>> >> index f3e4a63ab46..ddb6035be96 100644
>> >> --- a/gcc/doc/extend.texi
>> >> +++ b/gcc/doc/extend.texi
>> >> @@ -5754,6 +5754,16 @@ Specify which floating-point unit to use. You
>> >> must specify the
>> >> @code{target("fpmath=sse+387")} because the comma would separate
>> >> different options.
>> >>
>> >> +@item indirect_branch("@var{choice}")
>> >> +@cindex @code{indirect_branch} function attribute, x86
>> >> +On x86 targets, the @code{indirect_branch} attribute causes the compiler
>> >> +to convert indirect call and jump with @var{choice}. @samp{keep}
>> >> +keeps indirect call and jump unmodified. @samp{thunk} converts indirect
>> >> +call and jump to call and return thunk. @samp{thunk-inline} converts
>> >> +indirect call and jump to inlined call and return thunk.
>> >> +@samp{thunk-extern} converts indirect call and jump to external call
>> >> +and return thunk provided in a separate object file.
>> >
>> > Please expand the documentation in a way that random user who is not aware
>> > of the
>> > issue will understand that those are security features that come at a cost.
>> >
>> >> +@opindex -mindirect-branch
>> >> +Convert indirect call and jump with @var{choice}. The default is
>> >> +@samp{keep}, which keeps indirect call and jump unmodified.
>> >> +@samp{thunk} converts indirect call and jump to call and return thunk.
>> >> +@samp{thunk-inline} converts indirect call and jump to inlined call
>> >> +and return thunk. @samp{thunk-extern} converts indirect call and jump
>> >> +to external call and return thunk provided in a separate object file.
>> >> +You can control this behavior for a specific function by using the
>> >> +function attribute @code{indirect_branch}. @xref{Function Attributes}.
>> >
>> > Similarly here.
>>
>> I will update documentation with user guide info after Intel white
>> paper is published.
>
> OK,
> thanks!
> Honza
>>
>> > Rest of the patch seems OK. We may want incrementally represent more of the
>> > indirect jump/call seqeuence in RTL, but at this point probably keeping
>> > things
>> > simple and localized is not a bad idea. This can be done incrementally.
>> >
>> > Please make updated patch and I would like to give others chance to
>> > comment today.
>> >
>> > Honza
>>
>>
>>
>> --
>> H.J.
--
H.J.
