it appears i broke some tests, pls hold off reviewing this for a bit. On ...
14 years, 10 months ago
(2010-09-07 21:35:54 UTC)
#2
it appears i broke some tests, pls hold off reviewing this for a bit.
On Tue, Sep 7, 2010 at 23:03, <lvd@google.com> wrote:
> Reviewers: rsc, ken2, r,
>
> Message:
> Hello rsc, ken2, r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> 6l/8l: emit DWARF frame info.
>
> Please review this at http://codereview.appspot.com/2151044/
>
> Affected files:
> M src/cmd/6l/l.h
> M src/cmd/6l/pass.c
> M src/cmd/6l/span.c
> M src/cmd/8l/l.h
> M src/cmd/8l/pass.c
> M src/cmd/8l/span.c
> M src/cmd/ld/dwarf.c
> M src/cmd/ld/dwarf_defs.h
>
>
>
Braindump: The frame info works quite reasonable, (although gdb still loses track sometimes deep in ...
14 years, 10 months ago
(2010-09-09 10:48:27 UTC)
#3
Braindump:
The frame info works quite reasonable, (although gdb still loses track
sometimes deep in some runtime routines, i suspect the '#pragma textflag 7
may be involed'). But to get to this, i set the line numbers of the
prologue instructions to -1, so i can find the end of it later on. i then
want to skip these instructions in the gopcln and the dwarf _line tables,
and that seems to trigger a subtle bug somewhere, because in log_test.go,
the reported number should be 58 or 60, but it's 181 more, the exact
linecount of log.go, meaning some 'z' symbol didnt make it to the runtime's
ln/pc decoding.
studying the output of 6l -O i realized it's because the ATEXT symbol doesnt
lead to an emitted history entry with the prologue gone, so any instructions
in the prologue are now attributed to the last line of the previous
function. so i fix that, by having the ATEXT instruction generate a pc/ln.
but the pkg/log test still fails, because the ATEXT symbol has the
linenumber of the last instruction, although from looking at the pcln table,
that shouldn't matter, but ok. i looked around in 6g for a bit, but i don't
know what the obvious place to fix this is, so for now i just fix it up in
dostkoff just before inserting the prologue with linenumbers -1.
and now it works again, except i have to clean it up from all the print
statements, and i seem to have broken the frameinfo in 6l again, but not in
8l. i'll hg upload it when thats all done.
i am not sure yet this is all related to the reported runtime errors, i'll
look into those later. i did manage to reproduce stephen's one.
/L
On Tue, Sep 7, 2010 at 23:35, Luuk van Dijk <lvd@google.com> wrote:
> it appears i broke some tests, pls hold off reviewing this for a bit.
>
>
> On Tue, Sep 7, 2010 at 23:03, <lvd@google.com> wrote:
>
>> Reviewers: rsc, ken2, r,
>>
>> Message:
>> Hello rsc, ken2, r (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change.
>>
>>
>> Description:
>> 6l/8l: emit DWARF frame info.
>>
>> Please review this at http://codereview.appspot.com/2151044/
>>
>> Affected files:
>> M src/cmd/6l/l.h
>> M src/cmd/6l/pass.c
>> M src/cmd/6l/span.c
>> M src/cmd/8l/l.h
>> M src/cmd/8l/pass.c
>> M src/cmd/8l/span.c
>> M src/cmd/ld/dwarf.c
>> M src/cmd/ld/dwarf_defs.h
>>
>>
>>
>
If it's Stephen's bug with stack traces from a few days ago, I checked in ...
14 years, 10 months ago
(2010-09-09 11:57:15 UTC)
#4
If it's Stephen's bug with stack traces from a few days ago, I checked
in a fix today. If you sync it should be gone. The traceback code
wasn't compensating for deferproc frames.
-rob
> The frame info works quite reasonable, (although gdb still loses track > sometimes deep ...
14 years, 10 months ago
(2010-09-09 14:04:13 UTC)
#5
> The frame info works quite reasonable, (although gdb still loses track
> sometimes deep in some runtime routines, i suspect the '#pragma textflag 7
> may be involed'). But to get to this, i set the line numbers of the
> prologue instructions to -1, so i can find the end of it later on. i then
> want to skip these instructions in the gopcln and the dwarf _line tables,
> and that seems to trigger a subtle bug somewhere,
#pragma textflag 7 functions don't have prologue instructions.
Is the code assuming that they are always there?
Russ
On Thu, Sep 9, 2010 at 10:04, Russ Cox <rsc@golang.org> wrote: >> The frame info ...
14 years, 10 months ago
(2010-09-09 14:05:15 UTC)
#6
On Thu, Sep 9, 2010 at 10:04, Russ Cox <rsc@golang.org> wrote:
>> The frame info works quite reasonable, (although gdb still loses track
>> sometimes deep in some runtime routines, i suspect the '#pragma textflag 7
>> may be involed'). But to get to this, i set the line numbers of the
>> prologue instructions to -1, so i can find the end of it later on. i then
>> want to skip these instructions in the gopcln and the dwarf _line tables,
>> and that seems to trigger a subtle bug somewhere,
>
> #pragma textflag 7 functions don't have prologue instructions.
> Is the code assuming that they are always there?
Actually I guess that's not true, but they have fewer (just 1,
the subtract from SP).
Russ
no the code currently 1 - gets the frame offset as the ATEXT Prog's to.offset ...
14 years, 10 months ago
(2010-09-09 14:19:02 UTC)
#7
no the code currently
1 - gets the frame offset as the ATEXT Prog's to.offset
2 - scans for the first instruction following the ATEXT that has a line
number != -1
3 - the frame info starts off in the state 'cfa == $rsp + 8'
4 - then after that first non-prologue instruction the cfa += the offset
from step 1
so it Should Just Work. the only case it messes up is code that modifies
the SP outside, currently only the morestack routines.
anyway, it's totally usable at the moment. i checked goroutines and defer
statements with some programs in test/, and they look fine too.
sample session:
lvd@hpgntai-ubiq89:~/g3/bin$ gdb godoc
GNU gdb (GDB) 7.1-gg7
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html
>
Type "show copying" and "show warranty" for licensing/warranty details.
This GDB was configured as "x86_64-linux".
<http://wiki/Main/GnuDebugger FAQ: http://go/gdb Email: gdb-team>
Hey, I'm GDB 7.x. Check me out! http://wiki/Main/Gdb7x
Reading symbols from /home/lvd/g3/bin/godoc...done.
(gdb) break print.go:137
Breakpoint 1 at 0x806ef0e: file /home/lvd/g3/src/pkg/fmt/print.go, line 137.
(gdb) run
Starting program: /home/lvd/g3/bin/godoc
warning: the debug information found in "/lib/ld-2.7.so" does not match
"/lib/ld-linux.so.2" (CRC mismatch).
warning: the debug information found in "/usr/lib/debug//lib/ld-2.7.so" does
not match "/lib/ld-linux.so.2" (CRC mismatch).
warning: the debug information found in "/usr/lib/debug/lib/ld-2.7.so" does
not match "/lib/ld-linux.so.2" (CRC mismatch).
Breakpoint 1, fmt.Fprintf () at /home/lvd/g3/src/pkg/fmt/print.go:137
137 p := newPrinter()
(gdb) bt
#0 fmt.Fprintf () at /home/lvd/g3/src/pkg/fmt/print.go:137
#1 0x08055b80 in main.usage () at /home/lvd/g3/src/cmd/godoc/main.go:149
#2 0x0805612d in main.main () at /home/lvd/g3/src/cmd/godoc/main.go:229
#3 0x08064251 in mainstart () at /home/lvd/g3/src/pkg/runtime/386/asm.s:83
#4 0x0805ce2e in initdone () at /home/lvd/g3/src/pkg/runtime/proc.c:141
#5 0x00000000 in ?? ()
(gdb) cont
Continuing.
usage: godoc package [name ...]
godoc -http=:6060
Breakpoint 1, fmt.Fprintf () at /home/lvd/g3/src/pkg/fmt/print.go:137
137 p := newPrinter()
(gdb) quit
/L
On Thu, Sep 9, 2010 at 16:05, Russ Cox <rsc@golang.org> wrote:
> On Thu, Sep 9, 2010 at 10:04, Russ Cox <rsc@golang.org> wrote:
> >> The frame info works quite reasonable, (although gdb still loses track
> >> sometimes deep in some runtime routines, i suspect the '#pragma textflag
> 7
> >> may be involed'). But to get to this, i set the line numbers of the
> >> prologue instructions to -1, so i can find the end of it later on. i
> then
> >> want to skip these instructions in the gopcln and the dwarf _line
> tables,
> >> and that seems to trigger a subtle bug somewhere,
> >
> > #pragma textflag 7 functions don't have prologue instructions.
> > Is the code assuming that they are always there?
>
> Actually I guess that's not true, but they have fewer (just 1,
> the subtract from SP).
>
> Russ
>
On Thu, Sep 9, 2010 at 21:57, <rsc@google.com> wrote: > Looks pretty good. I would ...
14 years, 10 months ago
(2010-09-09 20:32:22 UTC)
#9
On Thu, Sep 9, 2010 at 21:57, <rsc@google.com> wrote:
> Looks pretty good. I would suggest going the
> deltasp route (see below) and then we never have
> to revisit this code and it will work correctly even
> for functions that play with the stack pointer
>
I had that first but its a lot klunkier and it requires me to introduce
architecture specific code in {6,8l}/, which in itself is not a disaster of
course. but i figured the need to port this to 5l would probably come
before the need to debug the handcrafted stack manipulating routines in the
basement, so this is cleaner for now. I propose i revisit this after the
data is done (other pending CL, i expect done before the weekend is over).
> themselves.
>
> Russ
>
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/6l/pass.c
> File src/cmd/6l/pass.c (right):
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/6l/pass.c#newcode667
> src/cmd/6l/pass.c:667: // The ATEXT symbol comes with the line number
> Are you sure? I don't believe this.
>
*lvd@hpgntai-ubiq89:~/play$ nl -ba hello.go *
* 1 package main*
* 2*
* 3 import "fmt"*
* 4 import "lala"*
* 5*
* 6 func main() {*
* 7*
* 8 fmt.Print("hello there\n")*
* 9*
* 10*
* 11 lala.Lala()*
* 12 // lala.Pipo()*
* 13*
* 14 x := lala.Hiha(41)*
* 15*
* 16 // panic("aargh")*
* 17 fmt.Print("bye there %d\n", x)*
* 18 }*
*lvd@hpgntai-ubiq89:~/play$ * ~/g2/bin/6l -L. -O hello.6 | head -20
400800 (17) TEXT main.main+0(SB),$96
0 lc+17(17)
400800 (17) MOVQ -16(FS),CX
400809 (17) CMPQ SP,(CX)
40080c (17) JHI ,400813
40080e (17) CALL ,405d52+runtime.morestack00
400813 (17) SUBQ $96,SP
1 pc+22*1(150) lc4294967287(73)
400817 (8) MOVL $16,(SP)
40081e (8) CALL ,4041d8+runtime.mal
400823 (8) MOVQ 8(SP),BX
400828 (8) MOVL $1,main.autotmp_0000+88(SP)
400830 (8) MOVL $1,main.autotmp_0000+92(SP)
400838 (8) MOVQ BX,main.autotmp_0000+80(SP)
40083d (8) MOVQ $type.string+0(SB),(SP)
400845 (8) LEAQ go.string."hello there\n"+0(SB),SI
40084d (8) LEAQ 8(SP),DI
400852 (8) MOVSQ ,
400854 (8) MOVSQ ,
400856 (8) CALL ,404bc6+runtime.convT2E
> I can't reproduce it in the output of
> 6g -S, 8g -S, 6c -S, or 8c -S, and the
> output of 6l -a looks right too.
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/6l/pass.c#newcode680
> src/cmd/6l/pass.c:680: p->line = -1; // mark as prologue
> Can we do this with a different field, one
> that wouldn't require changing asmlc?
> I'd suggest p->width = -1, changing the
> declaration in l.h to say
>
> /* fake for DATA, prologue/epilogue for TEXT */
>
>
ok. ill have a stab at that
> Also, it probably makes sense to put the
> assignment in appendp, so that it applies
> to all the new instructions here, not just the
> few that have been tagged.
>
appendp copies the linenumber from the previous Prog
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/6l/span.c
> File src/cmd/6l/span.c (right):
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/6l/span.c#newcode377
> src/cmd/6l/span.c:377: if(p->line == oldlc || p->line == -1 || p->as ==
> ANOP) {
> Excluding ATEXT here (really including it later)
> is going to break pragma 7 functions because this
> loop is going to emit twice for the pc at the
> beginning of the function, shared by both ATEXT
> and the real first instruction.
>
no it only emits if pc has changed. i inspected the -L output and it looks
sane. but ill double check.
>
> The ATEXT || ANOP that used to be here was
> skipping over 0-length instructions, and it still should.
> Instead of using -1 here I'd suggest getting the line
> numbers correct when emitting the prologue.
> Then asmlc can stay the same.
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c
> File src/cmd/ld/dwarf.c (right):
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode29
> src/cmd/ld/dwarf.c:29: static void addrput(vlong addr)
> newline before addrput
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode51
> src/cmd/ld/dwarf.c:51: if (v) c |= 0x80;
> newline after )
> next line too
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode71
> src/cmd/ld/dwarf.c:71: if (dst) *dst++ = c;
> newline after )
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode155
> src/cmd/ld/dwarf.c:155: n = strlen((char *) abbrevs[i].attr) / 2;
> too many spaces
> (char*)abbrevs
> and on next line too
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode695
> src/cmd/ld/dwarf.c:695: flushunit(q?q->pc:pc, unitstart);
> spaces. q ? q->pc : pc
> looks like maybe you're indenting with spaces too.
> please check here and elsewhere
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode784
> src/cmd/ld/dwarf.c:784: newattr(dwinfo->child, DW_AT_high_pc,
> DW_CLS_ADDRESS, q?q->pc:pc, 0);
> spaces around ? :
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode788
> src/cmd/ld/dwarf.c:788: flushunit(q?q->pc:pc, unitstart);
> spaces
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode820
> src/cmd/ld/dwarf.c:820: // First instruction past the prologue (i.e.
> line != 0)
> fix or delete parenthetical
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode880
> src/cmd/ld/dwarf.c:880: // we'll have to walk the instruction chain and
> describe what happens
> pass.c already does this. The variable is called deltasp.
> Another option is instead of reusing the width field
> in the Prog and thinking about prologue instructions,
> record deltasp during pass.c and use it here.
>
> That will have the added benefit of handling calls to deferproc
> and newproc correctly (there are two pushes and pops around
> each call), which is important. We just fixed the stack traces
> for those in the other code last night.
>
is there a program in test/ i can check for this? if youre right then the
deltasp route may be better after all.
>
>
> http://codereview.appspot.com/2151044/diff/24001/src/cmd/ld/dwarf.c#newcode945
> src/cmd/ld/dwarf.c:945: addrput((p->pcond ? p->pcond : q)->pc - p->pc);
> // address range
> too complicated for ? : - use a real variable
we are obviously from different planets wrt to the meaning of 'too
complicated'. but will do.
>
>
> http://codereview.appspot.com/2151044/
>
On Thu, Sep 9, 2010 at 16:32, Luuk van Dijk <lvd@google.com> wrote: > lvd@hpgntai-ubiq89:~/play$ ~/g2/bin/6l ...
14 years, 10 months ago
(2010-09-09 20:39:42 UTC)
#10
On Thu, Sep 9, 2010 at 16:32, Luuk van Dijk <lvd@google.com> wrote:
> lvd@hpgntai-ubiq89:~/play$ ~/g2/bin/6l -L. -O hello.6 | head -20
> 400800 (17) TEXT main.main+0(SB),$96
Thanks for the test case. Reproduced without
package lala using both 6g and 8g. Will fix them.
Russ
all comments addressed, where still applicable. lvd@hpgntai-ubiq89:~/play$ cd ../g/test lvd@hpgntai-ubiq89:~/g/test$ 6g defer.go lvd@hpgntai-ubiq89:~/g/test$ 6l defer.6 ...
14 years, 9 months ago
(2010-09-15 11:55:24 UTC)
#11
all comments addressed, where still applicable.
lvd@hpgntai-ubiq89:~/play$ cd ../g/test
lvd@hpgntai-ubiq89:~/g/test$ 6g defer.go
lvd@hpgntai-ubiq89:~/g/test$ 6l defer.6
glvd@hpgntai-ubiq89:~/g/test$ gdb 6.out
GNU gdb (GDB) 7.1-gg7
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html
>
Type "show copying" and "show warranty" for licensing/warranty details.
This GDB was configured as "x86_64-linux".
<http://wiki/Main/GnuDebugger FAQ: http://go/gdb Email: gdb-team>
Hey, I'm GDB 7.x. Check me out! http://wiki/Main/Gdb7x
Reading symbols from /home/lvd/g/test/6.out...done.
(gdb) break proc.c:900
Breakpoint 1 at 0x40380b: file /home/lvd/g/src/pkg/runtime/proc.c, line 900.
(gdb) run
Starting program: /home/lvd/g/test/6.out
warning: no loadable sections found in added symbol-file /usr/lib/debug/lib/
ld-2.7.so
Breakpoint 1, runtime.deferproc () at /home/lvd/g/src/pkg/runtime/proc.c:906
906 return 0;
(gdb) bt
#0 runtime.deferproc () at /home/lvd/g/src/pkg/runtime/proc.c:906
#1 0x0000000000400924 in main.test1helper () at
/home/lvd/g/test/defer.go:17
#2 0x0000000000400979 in main.test1 () at /home/lvd/g/test/defer.go:23
#3 0x0000000000400cf5 in main.main () at /home/lvd/g/test/defer.go:46
#4 0x0000000000406a8e in mainstart () at
/home/lvd/g/src/pkg/runtime/amd64/asm.s:78
#5 0x0000000000000000 in ?? ()
On Thu, Sep 9, 2010 at 22:39, Russ Cox <rsc@golang.org> wrote:
> On Thu, Sep 9, 2010 at 16:32, Luuk van Dijk <lvd@google.com> wrote:
> > lvd@hpgntai-ubiq89:~/play$ ~/g2/bin/6l -L. -O hello.6 | head -20
> > 400800 (17) TEXT main.main+0(SB),$96
>
> Thanks for the test case. Reproduced without
> package lala using both 6g and 8g. Will fix them.
>
> Russ
>
Almost there. In addition to the comment below, please use tabs for indentation. http://codereview.appspot.com/2151044/diff/36001/src/cmd/6l/l.h File ...
14 years, 9 months ago
(2010-09-17 18:50:00 UTC)
#12
Almost there. In addition to the comment below,
please use tabs for indentation.
http://codereview.appspot.com/2151044/diff/36001/src/cmd/6l/l.h
File src/cmd/6l/l.h (right):
http://codereview.appspot.com/2151044/diff/36001/src/cmd/6l/l.h#newcode98
src/cmd/6l/l.h:98: int32 deltasp;
I would find it clearer to call this something different, like adjsp,
because it is the delta of the previously existing variable deltasp.
(The delta in deltasp is the delta from the frame pointer, but
this deltasp is the change in deltasp made by one instruction.)
Issue 2151044: code review 2151044: 6l/8l: emit DWARF frame info.
(Closed)
Created 14 years, 10 months ago by lvd
Modified 14 years, 9 months ago
Reviewers:
Base URL:
Comments: 25
