| Message ID | CAFULd4buH86HbNw6QBDpfArhSR4H+AgrPRXcbr8p-nfa8UBEcw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [gotools] : Fix TestCrashDumpsAllThreads testcase failure | expand |
On Sun, Jan 21, 2018 at 3:13 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > > The default "go build" compile options over-optimize the auxiliary > executable, built in TestCrashDumpsAllThreads testcase > (libgo/go/runtime/crash_unix_test.go). This over-optimization results > in removal of the trivial summing loop and in the inlining of the > main.loop function into main.$thunk0. > > The testcase expects backtrace that shows several instances of > main.loop in the backtrace dump. When main.loop gets inlined into > thunk, its name is not dumped anymore. > > The solution is to pass "-O0" to gccgo, which inhibits unwanted inlining. > > Patch was bootstrapped and regression tested on x86_64-linux-gnu and > alphev68-linux-gnu, where for the later target the patch fixed the > mentioned failure. That sounds like a bug somewhere. Even when one function gets inlined into another, its name should still be dumped. This is implemented by report_inlined_functions in libbacktrace/dwarf.c. While something like your patch may be needed, I'd like to more clearly understand why libbacktrace isn't working. Ian
On Tue, Jan 23, 2018 at 5:49 AM, Ian Lance Taylor <iant@golang.org> wrote: > On Sun, Jan 21, 2018 at 3:13 PM, Uros Bizjak <ubizjak@gmail.com> wrote: >> >> The default "go build" compile options over-optimize the auxiliary >> executable, built in TestCrashDumpsAllThreads testcase >> (libgo/go/runtime/crash_unix_test.go). This over-optimization results >> in removal of the trivial summing loop and in the inlining of the >> main.loop function into main.$thunk0. >> >> The testcase expects backtrace that shows several instances of >> main.loop in the backtrace dump. When main.loop gets inlined into >> thunk, its name is not dumped anymore. >> >> The solution is to pass "-O0" to gccgo, which inhibits unwanted inlining. >> >> Patch was bootstrapped and regression tested on x86_64-linux-gnu and >> alphev68-linux-gnu, where for the later target the patch fixed the >> mentioned failure. > > That sounds like a bug somewhere. Even when one function gets inlined > into another, its name should still be dumped. This is implemented by > report_inlined_functions in libbacktrace/dwarf.c. While something > like your patch may be needed, I'd like to more clearly understand why > libbacktrace isn't working. If I manually compile the testcase from crash_unix_test.go wth gccgo -O2 (the asm is attached), the main.loop function is not even present in the dump. Even gdb session shows: (gdb) b main.loop Breakpoint 1 at 0x1200021f8: file main.go, line 11. (gdb) r Starting program: /space/homedirs/uros/a.out [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/libthread_db.so.1". [New Thread 0x200028831e0 (LWP 23072)] [New Thread 0x2000308b1e0 (LWP 23073)] [New Thread 0x20003c931e0 (LWP 23074)] [New Thread 0x20008fff1e0 (LWP 23075)] [New Thread 0x2000a7ff1e0 (LWP 23076)] [Switching to Thread 0x20003c931e0 (LWP 23074)] Thread 4 "a.out" hit Breakpoint 1, main.$thunk0 () at main.go:25 25 go loop(i, chans[i]) (gdb) bt #0 main.$thunk0 () at main.go:25 #1 0x000002000100bf54 in runtime.kickoff () at /space/homedirs/uros/gcc-svn/trunk/libgo/go/runtime/proc.go:1161 #2 0x0000020001a4f934 in ?? () from /lib/libc.so.6.1 While it is possible to put breakpoint at main.loop, the gdb backtrace omits function name and displays only thunk. When -O0 is used, the gdb backtrace shows: (gdb) bt #0 main.loop () at main.go:11 #1 0x000000012000237c in main.$thunk0 () at main.go:25 #2 0x000002000100bf54 in runtime.kickoff () at /space/homedirs/uros/gcc-svn/trunk/libgo/go/runtime/proc.go:1161 #3 0x0000020001a4f934 in ?? () from /lib/libc.so.6.1 I don't think this is a bug in libbacktrace, the function simply isn't there. Uros.
On Tue, Jan 23, 2018 at 8:32 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Tue, Jan 23, 2018 at 5:49 AM, Ian Lance Taylor <iant@golang.org> wrote: >> On Sun, Jan 21, 2018 at 3:13 PM, Uros Bizjak <ubizjak@gmail.com> wrote: >>> >>> The default "go build" compile options over-optimize the auxiliary >>> executable, built in TestCrashDumpsAllThreads testcase >>> (libgo/go/runtime/crash_unix_test.go). This over-optimization results >>> in removal of the trivial summing loop and in the inlining of the >>> main.loop function into main.$thunk0. >>> >>> The testcase expects backtrace that shows several instances of >>> main.loop in the backtrace dump. When main.loop gets inlined into >>> thunk, its name is not dumped anymore. >>> >>> The solution is to pass "-O0" to gccgo, which inhibits unwanted inlining. >>> >>> Patch was bootstrapped and regression tested on x86_64-linux-gnu and >>> alphev68-linux-gnu, where for the later target the patch fixed the >>> mentioned failure. >> >> That sounds like a bug somewhere. Even when one function gets inlined >> into another, its name should still be dumped. This is implemented by >> report_inlined_functions in libbacktrace/dwarf.c. While something >> like your patch may be needed, I'd like to more clearly understand why >> libbacktrace isn't working. > > If I manually compile the testcase from crash_unix_test.go wth gccgo > -O2 (the asm is attached), the main.loop function is not even present > in the dump. Digging deeper into the source, here is the problem: When inlined, the thunk reads: 0000000000000260 <main.$thunk0>: 260: 00 00 bb 27 ldah gp,0(t12) 260: GPDISP .text+0x4 264: 00 00 bd 23 lda gp,0(gp) 268: 08 00 10 a6 ldq a0,8(a0) 26c: 00 00 7d a7 ldq t12,0(gp) 26c: ELF_LITERAL runtime.closechan 270: f0 ff de 23 lda sp,-16(sp) 274: 00 00 5e b7 stq ra,0(sp) 278: 00 40 5b 6b jsr ra,(t12),27c <main.$thunk0+0x1c> 278: LITUSE .text+0x3 278: HINT runtime.closechan 27c: 00 80 5f 24 ldah t1,-32768 280: 01 05 e2 47 not t1,t0 284: 00 00 fe 2f unop 288: 1f 04 ff 47 nop 28c: 00 00 fe 2f unop 290: ff ff 21 20 lda t0,-1(t0) 294: fe ff 3f f4 bne t0,290 <main.$thunk0+0x30> 298: 01 05 e2 47 not t1,t0 29c: fc ff ff c3 br 290 <main.$thunk0+0x30> and the (very tight) loop consists only of insns at 0x290 and 0x294. However, the assembly dump of the loop reads: $L23: lda $1,-1($1) $LBB53: $LBB51: $LBB49: $LBB47: .loc 1 13 31 bne $1,$L23 $LBE47: $LBE49: $LBE51: $LBE53: with $Ldebug_ranges0: .8byte $LBB45-$Ltext0 .8byte $LBE45-$Ltext0 .8byte $LBB50-$Ltext0 .8byte $LBE50-$Ltext0 .8byte $LBB51-$Ltext0 .8byte $LBE51-$Ltext0 .8byte 0 .8byte 0 So, those $LBB labels are at the wrong place. During runtime, we get (oh, why can't we display PC in hex?!): SIGQUIT: quit PC=4831845072 m=0 sigcode=0 goroutine 7 [running]: runtime.sighandler /space/homedirs/uros/gcc-svn/trunk/libgo/go/runtime/signal_sighandler.go:104 runtime.sigtrampgo /space/homedirs/uros/gcc-svn/trunk/libgo/go/runtime/signal_unix.go:312 runtime.sigtramp /space/homedirs/uros/gcc-svn/trunk/libgo/runtime/go-signal.c:54 runtime.kickoff /space/homedirs/uros/gcc-svn/trunk/libgo/go/runtime/proc.go:1161 0x120001ad0 0000000120001aa0 <main.$thunk0>: 120001aa0: 02 00 bb 27 ldah gp,2(t12) 120001aa4: f0 a5 bd 23 lda gp,-23056(gp) 120001aa8: 08 00 10 a6 ldq a0,8(a0) 120001aac: c8 80 7d a7 ldq t12,-32568(gp) 120001ab0: f0 ff de 23 lda sp,-16(sp) 120001ab4: 00 00 5e b7 stq ra,0(sp) 120001ab8: 00 40 5b 6b jsr ra,(t12),120001abc <main.$thunk0+0x1c> 120001abc: 00 80 5f 24 ldah t1,-32768 120001ac0: 01 05 e2 47 not t1,t0 120001ac4: 00 00 fe 2f unop 120001ac8: 1f 04 ff 47 nop 120001acc: 00 00 fe 2f unop > 120001ad0: ff ff 21 20 lda t0,-1(t0) 120001ad4: fe ff 3f f4 bne t0,120001ad0 <main.$thunk0+0x30> 120001ad8: 01 05 e2 47 not t1,t0 120001adc: fc ff ff c3 br 120001ad0 <main.$thunk0+0x30> which is obviously out of corresponding debug range. Contents of the .debug_ranges section: Offset Begin End 00000000 0000000120001a7c 0000000120001a8c 00000000 0000000120001a90 0000000120001a9c 00000000 <End of list> 00000000 0000000120001a7c 0000000120001a8c 00000000 0000000120001a90 0000000120001a9c 00000000 <End of list> 00000030 0000000120001aa8 0000000120001ab0 00000030 0000000120001ab8 0000000120001abc 00000030 0000000120001ad4 0000000120001ad8 <<< this one! 00000030 <End of list> Uros.
On Tue, Jan 23, 2018 at 11:38 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Tue, Jan 23, 2018 at 8:32 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> On Tue, Jan 23, 2018 at 5:49 AM, Ian Lance Taylor <iant@golang.org> wrote: >>> On Sun, Jan 21, 2018 at 3:13 PM, Uros Bizjak <ubizjak@gmail.com> wrote: >>>> >>>> The default "go build" compile options over-optimize the auxiliary >>>> executable, built in TestCrashDumpsAllThreads testcase >>>> (libgo/go/runtime/crash_unix_test.go). This over-optimization results >>>> in removal of the trivial summing loop and in the inlining of the >>>> main.loop function into main.$thunk0. >>>> >>>> The testcase expects backtrace that shows several instances of >>>> main.loop in the backtrace dump. When main.loop gets inlined into >>>> thunk, its name is not dumped anymore. >>>> >>>> The solution is to pass "-O0" to gccgo, which inhibits unwanted inlining. >>>> >>>> Patch was bootstrapped and regression tested on x86_64-linux-gnu and >>>> alphev68-linux-gnu, where for the later target the patch fixed the >>>> mentioned failure. >>> >>> That sounds like a bug somewhere. Even when one function gets inlined >>> into another, its name should still be dumped. This is implemented by >>> report_inlined_functions in libbacktrace/dwarf.c. While something >>> like your patch may be needed, I'd like to more clearly understand why >>> libbacktrace isn't working. >> >> If I manually compile the testcase from crash_unix_test.go wth gccgo >> -O2 (the asm is attached), the main.loop function is not even present >> in the dump. > > Digging deeper into the source, here is the problem: [...] > So, those $LBB labels are at the wrong place. During runtime, we get > (oh, why can't we display PC in hex?!): Following testcase: --cut here-- package main func loop(i int, c chan bool) { close(c) for { for j := 0; j < 0x7fffffff; j++ { } } } --cut here-- can be compiled with a crosscompiler to alpha-linux-gnu: ~/gcc-build-alpha/gcc/go1 -O2 -fkeep-static-functions l.go to generate assembly, where $LBB2 is placed after "lda" insn. main.loop: ... $L2: lda $1,-1($1) $LBB2: $LM6: bne $1,$L2 br $31,$L3 $LBE2: ... and in ._final, we can see that (note 52) is in wrong place. It should be placed in front of (insn 13). (code_label 14 6 12 2 (nil) [1 uses]) (note 12 14 13 [bb 4] NOTE_INSN_BASIC_BLOCK) (insn:TI 13 12 52 (set (reg:DI 1 $1 [orig:70 ivtmp_1 ] [70]) (plus:DI (reg:DI 1 $1 [orig:70 ivtmp_1 ] [70]) (const_int -1 [0xffffffffffffffff]))) 7 {*adddi_internal} (nil)) (note 52 13 15 0x7f6b5f2791e0 NOTE_INSN_BLOCK_BEG) (jump_insn:TI 15 52 45 (set (pc) (if_then_else (ne (reg:DI 1 $1 [orig:70 ivtmp_1 ] [70]) (const_int 0 [0])) (label_ref:DI 14) (pc))) "l.go":6 169 {*bcc_normal} (int_list:REG_BR_PROB 1062895956 (nil)) -> 14) (note 45 15 46 [bb 5] NOTE_INSN_BASIC_BLOCK) (jump_insn:TI 46 45 47 (set (pc) (label_ref 16)) 200 {jump} (nil) -> 16) (barrier 47 46 32) (note 32 47 54 NOTE_INSN_DELETED) (note 54 32 0 0x7f6b5f2791e0 NOTE_INSN_BLOCK_END) I will open a PR: Uros.
On Tue, Jan 23, 2018 at 1:54 PM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Tue, Jan 23, 2018 at 11:38 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> On Tue, Jan 23, 2018 at 8:32 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >>> On Tue, Jan 23, 2018 at 5:49 AM, Ian Lance Taylor <iant@golang.org> wrote: >>>> On Sun, Jan 21, 2018 at 3:13 PM, Uros Bizjak <ubizjak@gmail.com> wrote: >>>>> >>>>> The default "go build" compile options over-optimize the auxiliary >>>>> executable, built in TestCrashDumpsAllThreads testcase >>>>> (libgo/go/runtime/crash_unix_test.go). This over-optimization results >>>>> in removal of the trivial summing loop and in the inlining of the >>>>> main.loop function into main.$thunk0. >>>>> >>>>> The testcase expects backtrace that shows several instances of >>>>> main.loop in the backtrace dump. When main.loop gets inlined into >>>>> thunk, its name is not dumped anymore. >>>>> >>>>> The solution is to pass "-O0" to gccgo, which inhibits unwanted inlining. >>>>> >>>>> Patch was bootstrapped and regression tested on x86_64-linux-gnu and >>>>> alphev68-linux-gnu, where for the later target the patch fixed the >>>>> mentioned failure. >>>> >>>> That sounds like a bug somewhere. Even when one function gets inlined >>>> into another, its name should still be dumped. This is implemented by >>>> report_inlined_functions in libbacktrace/dwarf.c. While something >>>> like your patch may be needed, I'd like to more clearly understand why >>>> libbacktrace isn't working. >>> >>> If I manually compile the testcase from crash_unix_test.go wth gccgo >>> -O2 (the asm is attached), the main.loop function is not even present >>> in the dump. >> >> Digging deeper into the source, here is the problem: > > [...] > >> So, those $LBB labels are at the wrong place. During runtime, we get >> (oh, why can't we display PC in hex?!): > > Following testcase: > > --cut here-- > package main > > func loop(i int, c chan bool) { > close(c) > for { > for j := 0; j < 0x7fffffff; j++ { > } > } > } > --cut here-- > > can be compiled with a crosscompiler to alpha-linux-gnu: > > ~/gcc-build-alpha/gcc/go1 -O2 -fkeep-static-functions l.go > > to generate assembly, where $LBB2 is placed after "lda" insn. [...] > I will open a PR: PR 83992 [1]. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83992 Uros.
Index: go/runtime/crash_unix_test.go =================================================================== --- go/runtime/crash_unix_test.go (revision 256858) +++ go/runtime/crash_unix_test.go (working copy) @@ -63,7 +63,7 @@ func TestCrashDumpsAllThreads(t *testing.T) { t.Fatalf("failed to create Go file: %v", err) } - cmd := exec.Command(testenv.GoToolPath(t), "build", "-o", "a.exe") + cmd := exec.Command(testenv.GoToolPath(t), "build", "-gccgoflags", "-O0", "-o", "a.exe") cmd.Dir = dir out, err := testenv.CleanCmdEnv(cmd).CombinedOutput() if err != nil {