LLC Bug x86 with thread local storage

Hello,

This bug affects all LLVM versions from 2.6 to trunk :
http://llvm.org/bugs/show_bug.cgi?id=5081

The workaround I found is to add this :

Index: lib/Target/X86/X86Instr64bit.td

I think it just needs support in all of those cases for the tls immediate type (for example your test would have still failed in kernel mode). I've gone ahead and added that in r106433 and it passes the testcase in a few different codegen modes, but don't have any way of testing on linux at the moment so if you can test the codegen that would be appreciated.

Thanks!

-eric

Actually, now llc works with the testcase but the generated code is wrong if you try to compile it with "as".

$ as select.s
select.s: Assembler messages:
select.s:8: Error: relocated field and relocation type differ in signedness

select.s
8: movl $tm_nest_level@TPOFF, %ecx
9: movq %fs:0, %rax

This is why I set MOV64ri32 and not MOV64ri64i32 in my patch (thus this is why I asked for correctness).

Thank you,

Patrick Marlier.

>>
>>> Hello,
>>>
>>> This bug affects all LLVM versions from 2.6 to trunk :
>>> http://llvm.org/bugs/show_bug.cgi?id=5081
>>>
>>> The workaround I found is to add this :
>>>
>>> Index: lib/Target/X86/X86Instr64bit.td
>>> ===================================================================
>>> --- lib/Target/X86/X86Instr64bit.td (revision 105882)
>>> +++ lib/Target/X86/X86Instr64bit.td (working copy)
>>> @@ -1832,6 +1832,8 @@
>>> (MOV64ri64i32 tjumptable :$dst)>, Requires<[SmallCode]>;
>>> def : Pat<(i64 (X86Wrapper tglobaladdr :$dst)),
>>> (MOV64ri64i32 tglobaladdr :$dst)>, Requires<[SmallCode]>;
>>> +def : Pat<(i64 (X86Wrapper tglobaltlsaddr :$dst)),
>>> + (MOV64ri32 tglobaltlsaddr :$dst)>, Requires<[SmallCode]>;
>>> def : Pat<(i64 (X86Wrapper texternalsym:$dst)),
>>> (MOV64ri64i32 texternalsym:$dst)>, Requires<[SmallCode]>;
>>> def : Pat<(i64 (X86Wrapper tblockaddress:$dst)),
>>>
>>> Unfortunately, I am 100% confident with this modification since I am not an expert of LLVM and I doubt a bit about the conversion for 32 bit to 64bit.
>>>
>>> If someone could approve it or help me with this bug it will be wonderful.
>>>
>> I think it just needs support in all of those cases for the tls immediate type (for example your test would have still failed in kernel mode). I've gone ahead and added that in r106433 and it passes the testcase in a few different codegen modes, but don't have any way of testing on linux at the moment so if you can test the codegen that would be appreciated.
>>
>> Thanks!
>>
>> -eric
> Actually, now llc works with the testcase but the generated code is
> wrong if you try to compile it with "as".
>
> $ as select.s
> select.s: Assembler messages:
> select.s:8: Error: relocated field and relocation type differ in signedness
>
> select.s
> 8: movl $tm_nest_level@TPOFF, %ecx
> 9: movq %fs:0, %rax

Which one is correct ?
- movl $tm_nest_level@TPOFF, %ecx
or
- movq $tm_nest_level@TPOFF, %rcx
or
- movl tm_nest_level@TPOFF, %ecx

Otherwise, Is there a way to remove this $ character?

I found that it is here in lib/Target/X86/AsmPrinter/X86ATTInstPrinter.cpp

void X86ATTInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
                                      raw_ostream &O) {
   const MCOperand &Op = MI->getOperand(OpNo);
   if (Op.isReg()) {
     O << '%' << getRegisterName(Op.getReg());
   } else if (Op.isImm()) {
     ...
   } else {
     assert(Op.isExpr() && "unknown operand kind in printOperand");
// HERE I remove the '$' to make it work
     O << '$' << *Op.getExpr();
   }

I hope someone is good in this thing!

Thank you for your help,

Patrick Marlier.

Which one is correct ?
- movl $tm_nest_level@TPOFF, %ecx
or
- movq $tm_nest_level@TPOFF, %rcx
or
- movl tm_nest_level@TPOFF, %ecx

I believe this is initial exec and so from:

http://people.redhat.com/drepper/tls.pdf

it would be movl tm_nest_level@TPOFF, %ecx

Otherwise, Is there a way to remove this $ character?

I found that it is here in lib/Target/X86/AsmPrinter/X86ATTInstPrinter.cpp

void X86ATTInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
                                    raw_ostream &O) {
const MCOperand &Op = MI->getOperand(OpNo);
if (Op.isReg()) {
   O << '%' << getRegisterName(Op.getReg());
} else if (Op.isImm()) {
   ...
} else {
   assert(Op.isExpr() && "unknown operand kind in printOperand");
// HERE I remove the '$' to make it work
   O << '$' << *Op.getExpr();
}

Hrm. Something is wonky here. Can you file a testcase with a .i file I can compile please?

-eric

Is the testcase from Eli Friedman not enough?

http://llvm.org/bugs/show_bug.cgi?id=5081
Eli Friedman 2009-10-02 20:15:35 CDT
Reduced testcase:

target triple = "x86_64-unknown-linux-gnu"
@tm_nest_level = internal thread_local global i32 0
define i64 @z() nounwind {
   ret i64 and (i64 ptrtoint (i32* @tm_nest_level to i64), i64 100)
}

Because actually, the C application where it happens is really huge and I don't know if I will be able to provide you a really simple testcase.

Tell me if it is really required I can try to do it tomorrow.

Patrick Marlier.

Is the testcase from Eli Friedman not enough?

Hey look, a testcase.

http://llvm.org/bugs/show_bug.cgi?id=5081
Eli Friedman 2009-10-02 20:15:35 CDT
Reduced testcase:

target triple = "x86_64-unknown-linux-gnu"
@tm_nest_level = internal thread_local global i32 0
define i64 @z() nounwind {
ret i64 and (i64 ptrtoint (i32* @tm_nest_level to i64), i64 100)
}

Because actually, the C application where it happens is really huge and I don't know if I will be able to provide you a really simple testcase.

Tell me if it is really required I can try to do it tomorrow.

Nah, that's fine. I just didn't get anything on this and I don't have a linux machine I can assemble on.

-eric

As a correction, I don't know that any of these is correct.
I need to look into this a bit more. I think that it requires the
relocation to be off of a register, i.e.

  movabs $foo@tpoff, %rax
  add $foo@tpoff, %rax
        mov foo@tpoff(%rbx), %eax

are all legal, while:

  mov foo@tpoff(%ebx), %eax
      call foo@tpoff
       mov $foo@tpoff, %eax
       mov $foo@tpoff, %ax
       mov $foo@tpoff, %al

are all illegal.

So we appear to be lowering something, and trying to make
an illegal move out of it.

Unfortunately we're really short on testcases here. :slight_smile: I've made
an attempt that works for the current test and passes our set of tests
in:

Sending lib/Target/X86/X86Instr64bit.td
Sending test/CodeGen/X86/x86-64-tls-1.ll
Transmitting file data ..
Committed revision 107860.

Going to need some more testcases soon though if you run into
any more problems :slight_smile:

-eric

I tested on our application and now it compiles :slight_smile:

Thank you so much!
Have an nice day,

Patrick Marlier.