Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd/compile: bad codegen for 386 PIC code #15496

Closed
aclements opened this issue Apr 29, 2016 · 6 comments
Closed

cmd/compile: bad codegen for 386 PIC code #15496

aclements opened this issue Apr 29, 2016 · 6 comments
Labels
FrozenDueToAge

Comments

@aclements
Copy link
Member

  1. What version of Go are you using (go version)? Current master cd95657
  2. What operating system and processor architecture are you using (go env)? linux/386

PC-relative references in 386 code compiled with -buildmode=c-shared clobber ECX, but can occur in the middle of code sequences that are using ECX (I think because of shift instructions).

For example, heapBitsSetTypeNoScan in https://go-review.googlesource.com/#/c/22632/3 triggers this behavior, causing the runtime to crash very early because the h.bitp pointer wound up in ECX and was clobbered.

ecx is excluded from the register allocator, but code generation can pick it before register allocation happens, and then peephole optimization can spread it to many other places.

Here's an annotated heapBitsSetTypeNoScan, showing where clobbered %ecx values are being consumed. The => is on the line that crashes with a clobbered pointer.

Dump of assembler code for function runtime.heapBitsSetTypeNoScan:
   0xf7f385e0 <+0>: call   0xf7f7c720 <__x86.get_pc_thunk.cx>
   0xf7f385e5 <+5>: mov    0x8d9c3(%ecx),%ecx
   0xf7f385eb <+11>:    mov    %gs:(%ecx),%ecx
   0xf7f385ee <+14>:    cmp    0x8(%ecx),%esp
   0xf7f385f1 <+17>:    jbe    0xf7f38697 <runtime.heapBitsSetTypeNoScan+183>
   0xf7f385f7 <+23>:    sub    $0x20,%esp
   0xf7f385fa <+26>:    mov    0x24(%esp),%ecx       %ecx = x
   0xf7f385fe <+30>:    xor    %ebx,%ebx             
   0xf7f38600 <+32>:    call   0xf7f7c720 <__x86.get_pc_thunk.cx>   %ecx clobbered
   0xf7f38605 <+37>:    mov    0x9117b(%ecx),%ebx    %ebx = arena_start
   0xf7f3860b <+43>:    mov    %ecx,%ebp             SHOULD BE %ebp = x, but %ecx clobbered
   0xf7f3860d <+45>:    sub    %ebx,%ebp             %ebp = x - arena_start
   0xf7f3860f <+47>:    call   0xf7f7c720 <__x86.get_pc_thunk.cx>
   0xf7f38614 <+52>:    mov    0x9116c(%ecx),%ebx    %ebx = arena_start
   0xf7f3861a <+58>:    sub    %ebx,%ecx             SHOULD BE %ecx = x - arena_start, but %ecx clobbered
   0xf7f3861c <+60>:    shr    $0x2,%ecx             %ecx = off = (x - arena_start)/4
   0xf7f3861f <+63>:    xor    %ebx,%ebx             %ebx = 0
   0xf7f38621 <+65>:    call   0xf7f7c720 <__x86.get_pc_thunk.cx>   %ecx clobbered
   0xf7f38626 <+70>:    mov    0x9115a(%ecx),%eax    %eax = arena_start
   0xf7f3862c <+76>:    mov    %ecx,%ebp             SHOULD BE %ebp = off, but %ecx clobbered
   0xf7f3862e <+78>:    shr    $0x2,%ebp             %ebp = off/4
   0xf7f38631 <+81>:    sub    %ebp,%eax             %eax = arena_start - off/4
   0xf7f38633 <+83>:    dec    %eax                  %eax = arena_start - off/4 - 1
   0xf7f38634 <+84>:    and    $0x3,%ecx             %ecx = off & 3
   0xf7f38637 <+87>:    mov    %eax,0x18(%esp)
   0xf7f3863b <+91>:    mov    %ecx,0x1c(%esp)
   0xf7f3863f <+95>:    mov    %eax,0x8(%esp)
   0xf7f38643 <+99>:    mov    %ecx,0xc(%esp)
   0xf7f38647 <+103>:   mov    %eax,0x10(%esp)
=> 0xf7f3864b <+107>:   movzbl (%eax),%esi
   0xf7f3864e <+110>:   mov    %ecx,0x14(%esp)
   0xf7f38652 <+114>:   mov    $0x11,%ebx
   0xf7f38657 <+119>:   cmp    $0x8,%ecx
   0xf7f3865a <+122>:   jae    0xf7f38693 <runtime.heapBitsSetTypeNoScan+179>
   0xf7f3865c <+124>:   shl    %cl,%ebx
   0xf7f3865e <+126>:   xor    $0xffffffff,%ebx
   0xf7f38661 <+129>:   and    %esi,%ebx
   0xf7f38663 <+131>:   mov    %bl,(%eax)
   0xf7f38665 <+133>:   call   0xf7f51790 <runtime.printlock>
   0xf7f3866a <+138>:   call   0xf7f7c720 <__x86.get_pc_thunk.cx>
   0xf7f3866f <+143>:   lea    0x45185(%ecx),%ebx
   0xf7f38675 <+149>:   mov    %ebx,(%esp)
   0xf7f38678 <+152>:   movl   $0xc,0x4(%esp)
   0xf7f38680 <+160>:   call   0xf7f52290 <runtime.printstring>
   0xf7f38685 <+165>:   call   0xf7f51a10 <runtime.printnl>
   0xf7f3868a <+170>:   call   0xf7f51810 <runtime.printunlock>
   0xf7f3868f <+175>:   add    $0x20,%esp
   0xf7f38692 <+178>:   ret    
   0xf7f38693 <+179>:   xor    %ebx,%ebx
   0xf7f38695 <+181>:   jmp    0xf7f3865c <runtime.heapBitsSetTypeNoScan+124>
   0xf7f38697 <+183>:   call   0xf7f77240 <runtime.morestack_noctxt>
   0xf7f3869c <+188>:   jmp    0xf7f385e0 <runtime.heapBitsSetTypeNoScan>

/cc @randall77 @mwhudson
@aclements
Copy link
Member Author

I should add that getting the disassembly from gdb required adding //go:noinline to heapBitsSetTypeNoScan (otherwise it gets inlined).

To get the registerizer and peephole logs, I ran

GOARCH=386 go install -buildmode=shared -v -gcflags '-R -v' -a runtime
GOARCH=386 go install -buildmode=shared -v -gcflags '-P -v' -a runtime

The failure shows up in misc/cgo/testcshared:

##### ../misc/cgo/testcshared
FAIL test0 got 
FAIL test1 got 
FAIL test2 got 
FAIL test4 got 
re-running test4 in verbose mode
calling sigaction
calling dlopen
calling dlsym
calling RunGoroutines
./test.bash: line 165:  6712 Segmentation fault      ./testp4 ./libgo4.$libext verbose

@aclements
Copy link
Member Author

Would it work to just switch the PC-relative sequence to a less-special register, like EBX?

@randall77
Copy link
Contributor

I was thinking the same thing.

@aclements
Copy link
Member Author

aclements commented Apr 29, 2016 •
edited

Though it looks like we hard-code BX for indirect calls (Thearch.REGCALLX), so maybe that's not safe? cgen64 hard-codes AX, CX, and DX. BP is free, but according to a comment in peep.go, we can't use BP with index-offset. The block moves use SI and DI, but maybe they're safe? Maybe the easiest thing is to use BX and change what we use for indirect calls.

@gopherbot
Copy link

CL https://golang.org/cl/22652 mentions this issue.

@mwhudson
Copy link
Contributor

Pretty sure CX is least-special, for better or worse.
On 30/04/2016 11:26 am, "Austin Clements" notifications@github.com wrote:

Would it work to just switch the PC-relative sequence to a less-special
register, like EBX?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#15496 (comment)

@golang golang locked and limited conversation to collaborators Apr 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mwhudson @aclements @randall77 @gopherbot