<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Fri, Jan 5, 2018 at 12:14 PM Rafael Avila de Espindola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> writes:<br>
<br>
>><br>
>> > There is one other important source of indirect branches in x86 ELF<br>
>> > binaries: the PLT. These patches also include support for LLD to<br>
>> > generate PLT entries that perform a retpoline-style indirection.<br>
>><br>
>> I see that we now generate similar code for -fno-plt. We should have a<br>
>> test for that (nonlazybind at the llvm level).<br>
>><br>
><br>
> ? The LLVM code generation for PLT-calls shouldn't be impacted by retpoline<br>
> at all? It should just be the contents of the PLT section itself? Maybe I'm<br>
> just misunderstanding...<br>
><br>
> If you're wondering whether LLVM will correctly retpoline a -fno-plt call<br>
> that it directly emits, I believe it uses the same instruction patterns as<br>
> any other indirect call and so should be covered already.<br>
<br>
Yes, that is what I was wondering. It is not obvious for someone not<br>
current on the X86 backend if -fno-plt is implemented in the same code<br>
path. So the request is just to add a llvm test for -fno-plt (i.e., a<br>
test showing what nonlazybind codegens to now).<br>
<br>
Hopefully all that you need is a version of<br>
test/CodeGen/X86/fast-isel-noplt-pic.ll with<br>
<br>
+attributes #0 = { "target-features"="+retpoline" }<br></blockquote><div><br></div><div>I directly added a nonlazybind call to the retpoline test file and checked it both on 32-bit and 64-bit, and both SDISel and FastISel. Thanks for this!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
>> > +Function *X86RetpolineThunks::createThunkFunction(Module &M, StringRef<br>
>> Name) {<br>
>> > +  LLVMContext &Ctx = M.getContext();<br>
>> > +  auto Type = FunctionType::get(Type::getVoidTy(Ctx), false);<br>
>> > +  Function *F =<br>
>> > +      Function::Create(Type, GlobalValue::LinkOnceODRLinkage, Name, &M);<br>
>> > +  F->setVisibility(GlobalValue::HiddenVisibility);<br>
>><br>
>> We should probably put this in a comdat so the linker keeps only one.<br>
>><br>
><br>
> Is LinkOnceODR not sufficient? We don't need a comdat *group* here. I<br>
> thought you didn't need explicit comdats unless you're grouping multiple<br>
> globals....<br>
<br>
It is not sufficient. LLVM used to create implicit comdats, but we don't<br>
do that anymore.<br>
<br>
Given<br>
<br>
define linkonce_odr void @foo() {<br>
  ret void<br>
}<br>
<br>
llc puts it in .text or<br>
<br>
.section        .text.foo,"ax",@progbits<br>
<br>
if -function-sections is used.<br>
<br>
Given<br>
<br>
$foo = comdat any<br>
define linkonce_odr void @foo() comdat {<br>
  ret void<br>
}<br>
<br>
We always produce<br>
<br>
.section        .text.foo,"axG",@progbits,foo,comdat<br></blockquote><div><br></div><div>Sure, I've enhanced the tests to check for these sections and added the comdat generation to the code.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
>> This pass converts each indirectbr to a switch. One feature we had in<br>
>> the old clang expansion of computed gotos is that each function would<br>
>> have a single switch. This would avoid code explosion in direct threaded<br>
>> interpreters where after each opcode there is a jump that can go any<br>
>> other opcode.<br>
>><br>
><br>
> I'll see if this is easy to implement, but if it's going to take some time<br>
> I might suggest we defer this to a follow-up patch.<br>
><br>
><br>
> I think Rui got the rest of your comments, but give a shout if I've missed<br>
> anything and thanks for review!<br>
<br>
A FIXME or bug report for future improvement is fine.<br></blockquote><div><br></div><div>I'm playing with this, but if i don't get something working before an LGTM, I'll go with just a FIXME.</div><div><br></div><div><br></div><div>Thanks again!</div></div></div>