code review 2201044: 6l/8l: global and local variables and type info.

6l/8l: global and local variables and type info.

[lvd, 2010-10-21 17:16:21]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc1, 2010-10-21 19:51:43]

looking forward to this

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode4
src/cmd/ld/dwarf.c:4: //
make this line blank
just to set the copyright comment off from the real one

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode11
src/cmd/ld/dwarf.c:11: //	 ptype struct '[]uint8'	and qualifiers need to be
quoted away
tab before 'and' should be a space

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode395
src/cmd/ld/dwarf.c:395: // Each DIE (except the root ones) has at least 1
attribute, it's name.	 getattr first moves
s/it's/its/

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode422
src/cmd/ld/dwarf.c:422: // written out if it is listed in the abbrev).	If it's
parent is
s/it's/its/

it's = it is

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode711
src/cmd/ld/dwarf.c:711: decode_reloc(Sym *s, int32 off) {
\n before {.  etc.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode713
src/cmd/ld/dwarf.c:713: for (i = 0; i < s->nr; ++i)
i++
just for consistency

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode720
src/cmd/ld/dwarf.c:720: static uint8 decodetype_kind(Sym *s) { return s->p[0x1f]
& ~KindNoPointers; }
need more newlines here, sorry.  etc.

static uint8
decodetype_kind(Sym *s)
{
    return s->p[0x1f] & ~KindNoPointers;
}

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode723
src/cmd/ld/dwarf.c:723: static vlong decodetype_size(Sym *s) { return
*(uintptr*)(s->p + 0x10); }  //TODO inuxi
please do

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode732
src/cmd/ld/dwarf.c:732: static int decodetype_structfieldcount(Sym *s) { return
*(uint32*)(s->p + 0x38); }  //TODO inuxi
please do

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode736
src/cmd/ld/dwarf.c:736: Sym *p	= decode_reloc(s, 0x40 + i*0x28);  //
go.string."foo"
declare variables in block at top of function, then blank line

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode737
src/cmd/ld/dwarf.c:737: if (p == nil) return nil;
newline after if condition too

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode740
src/cmd/ld/dwarf.c:740: char *n = mal(p->np);			// make a 0-terminated copy
it's already NUL-terminated
string."foo" = C string "foo"

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode884
src/cmd/ld/dwarf.c:884: break;
funny spacing; tab issue?

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode890
src/cmd/ld/dwarf.c:890: newrefattr(fld, DW_AT_type, find(&dwtypes, "<byte*>"));
not *byte?

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode903
src/cmd/ld/dwarf.c:903: s =	  decodetype_structfieldtype(gotype, i);
s/ +/ /

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode914
src/cmd/ld/dwarf.c:914: newrefattr(die, DW_AT_type, find(&dwtypes, "void"));  
// must be defined before
// void already defined

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode951
src/cmd/ld/dwarf.c:951: 
You asked about distinguishing globals from locals.
Locals have t == 'a' (local aka auto variable) or t == 'p' (parameter) here.
That's how to tell.  The Sym* doesn't have that information.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1115
src/cmd/ld/dwarf.c:1115: /* Return false if the a->link chain contains no
history, otherwise
// c99 comments are fine

/*
 * c89 comments should look
 * like this
 */

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1389
src/cmd/ld/dwarf.c:1389: // diag("instruction with linenumber past EOF in %s:
%P", histfile[1], q);
line number is still two words  :-)

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1630
src/cmd/ld/dwarf.c:1630: vlong here = cpos();
decl at top, please

[lvd, 2010-10-21 22:46:43]

after this is in i should maybe spend some time on a page on golang.org on
dealing with gdb & go, to both manage expectations and help people get
around some of the quirks that are there and wont be ironed out for a
while.

On Thu, Oct 21, 2010 at 21:51, <rsc@google.com> wrote:

> looking forward to this
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c
> File src/cmd/ld/dwarf.c (right):
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode4
> src/cmd/ld/dwarf.c:4: //
> make this line blank
> just to set the copyright comment off from the real one
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode11
> src/cmd/ld/dwarf.c:11: //        ptype struct '[]uint8' and qualifiers need
> to
> be quoted away
> tab before 'and' should be a space
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode395
> src/cmd/ld/dwarf.c:395: // Each DIE (except the root ones) has at least
> 1 attribute, it's name.  getattr first moves
> s/it's/its/
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode422
> src/cmd/ld/dwarf.c:422: // written out if it is listed in the
> abbrev).        If it's parent is
> s/it's/its/
>
> it's = it is
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode711
> src/cmd/ld/dwarf.c:711: decode_reloc(Sym *s, int32 off) {
> \n before {.  etc.
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode713
> src/cmd/ld/dwarf.c:713: for (i = 0; i < s->nr; ++i)
> i++
> just for consistency
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode720
> src/cmd/ld/dwarf.c:720: static uint8 decodetype_kind(Sym *s) { return
> s->p[0x1f] & ~KindNoPointers; }
> need more newlines here, sorry.  etc.
>
> static uint8
> decodetype_kind(Sym *s)
> {
>    return s->p[0x1f] & ~KindNoPointers;
> }
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode723
> src/cmd/ld/dwarf.c:723: static vlong decodetype_size(Sym *s) { return
> *(uintptr*)(s->p + 0x10); }  //TODO inuxi
> please do
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode732
> src/cmd/ld/dwarf.c:732: static int decodetype_structfieldcount(Sym *s) {
> return *(uint32*)(s->p + 0x38); }  //TODO inuxi
> please do
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode736
> src/cmd/ld/dwarf.c:736: Sym *p  = decode_reloc(s, 0x40 + i*0x28);  //
> go.string."foo"
> declare variables in block at top of function, then blank line
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode737
> src/cmd/ld/dwarf.c:737: if (p == nil) return nil;
> newline after if condition too
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode740
> src/cmd/ld/dwarf.c:740: char *n = mal(p->np);                   // make a
> 0-terminated
> copy
> it's already NUL-terminated
> string."foo" = C string "foo"
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode884
> src/cmd/ld/dwarf.c:884: break;
> funny spacing; tab issue?
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode890
> src/cmd/ld/dwarf.c:890: newrefattr(fld, DW_AT_type, find(&dwtypes,
> "<byte*>"));
> not *byte?
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode903
> src/cmd/ld/dwarf.c:903: s =       decodetype_structfieldtype(gotype, i);
> s/ +/ /
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode914
> src/cmd/ld/dwarf.c:914: newrefattr(die, DW_AT_type, find(&dwtypes,
> "void"));   // must be defined before
> // void already defined
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode951
> src/cmd/ld/dwarf.c:951:
> You asked about distinguishing globals from locals.
> Locals have t == 'a' (local aka auto variable) or t == 'p' (parameter)
> here.
> That's how to tell.  The Sym* doesn't have that information.
>
>
>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1115
> src/cmd/ld/dwarf.c:1115: /* Return false if the a->link chain contains
> no history, otherwise
> // c99 comments are fine
>
> /*
>  * c89 comments should look
>  * like this
>  */
>
>
>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1389
> src/cmd/ld/dwarf.c:1389: // diag("instruction with linenumber past EOF
> in %s: %P", histfile[1], q);
> line number is still two words  :-)
>
>
>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1630
> src/cmd/ld/dwarf.c:1630: vlong here = cpos();
> decl at top, please
>
>
> http://codereview.appspot.com/2201044/
>

[lvd, 2010-10-22 13:20:13]

all done.  still need to go over the decoding for 8l.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode4
src/cmd/ld/dwarf.c:4: //
On 2010/10/21 19:51:43, rsc1 wrote:
> make this line blank
> just to set the copyright comment off from the real one

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode11
src/cmd/ld/dwarf.c:11: //	 ptype struct '[]uint8'	and qualifiers need to be
quoted away
On 2010/10/21 19:51:43, rsc1 wrote:
> tab before 'and' should be a space

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode395
src/cmd/ld/dwarf.c:395: // Each DIE (except the root ones) has at least 1
attribute, it's name.	 getattr first moves
On 2010/10/21 19:51:43, rsc1 wrote:
> s/it's/its/

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode422
src/cmd/ld/dwarf.c:422: // written out if it is listed in the abbrev).	If it's
parent is
On 2010/10/21 19:51:43, rsc1 wrote:
> s/it's/its/
> 
> it's = it is

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode711
src/cmd/ld/dwarf.c:711: decode_reloc(Sym *s, int32 off) {
On 2010/10/21 19:51:43, rsc1 wrote:
> \n before {.  etc.
> 

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode713
src/cmd/ld/dwarf.c:713: for (i = 0; i < s->nr; ++i)
On 2010/10/21 19:51:43, rsc1 wrote:
> i++
> just for consistency

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode720
src/cmd/ld/dwarf.c:720: static uint8 decodetype_kind(Sym *s) { return s->p[0x1f]
& ~KindNoPointers; }
On 2010/10/21 19:51:43, rsc1 wrote:
> need more newlines here, sorry.  etc.
> 
> static uint8
> decodetype_kind(Sym *s)
> {
>     return s->p[0x1f] & ~KindNoPointers;
> }

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode736
src/cmd/ld/dwarf.c:736: Sym *p	= decode_reloc(s, 0x40 + i*0x28);  //
go.string."foo"
On 2010/10/21 19:51:43, rsc1 wrote:
> declare variables in block at top of function, then blank line

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode737
src/cmd/ld/dwarf.c:737: if (p == nil) return nil;
On 2010/10/21 19:51:43, rsc1 wrote:
> newline after if condition too

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode740
src/cmd/ld/dwarf.c:740: char *n = mal(p->np);			// make a 0-terminated copy
On 2010/10/21 19:51:43, rsc1 wrote:
> it's already NUL-terminated
> string."foo" = C string "foo"

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode884
src/cmd/ld/dwarf.c:884: break;
On 2010/10/21 19:51:43, rsc1 wrote:
> funny spacing; tab issue?

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode890
src/cmd/ld/dwarf.c:890: newrefattr(fld, DW_AT_type, find(&dwtypes, "<byte*>"));
On 2010/10/21 19:51:43, rsc1 wrote:
> not *byte?

a matter of taste.  i predefine some types to be able to describe the internal
c-structs for slices and strings etc. they are C, not go. i dont want to mix
them with the official types, so the <..> is just to offset them from the c and
go ones.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode903
src/cmd/ld/dwarf.c:903: s =	  decodetype_structfieldtype(gotype, i);
On 2010/10/21 19:51:43, rsc1 wrote:
> s/ +/ /

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode914
src/cmd/ld/dwarf.c:914: newrefattr(die, DW_AT_type, find(&dwtypes, "void"));  
// must be defined before
On 2010/10/21 19:51:43, rsc1 wrote:
> // void already defined
> 

Done.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode951
src/cmd/ld/dwarf.c:951: 
On 2010/10/21 19:51:43, rsc1 wrote:
> You asked about distinguishing globals from locals.
> Locals have t == 'a' (local aka auto variable) or t == 'p' (parameter) here.
> That's how to tell.  The Sym* doesn't have that information.

yes.  and this just goes over the globals anyway.  i was confused as to why gdb
was confused.  it knows given the address which variable you talk about, but not
given which name, which can be a problem if you have auto and params hiding
eachotehr.  that needs some thought still.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1389
src/cmd/ld/dwarf.c:1389: // diag("instruction with linenumber past EOF in %s:
%P", histfile[1], q);
On 2010/10/21 19:51:43, rsc1 wrote:
> line number is still two words  :-)

english is such a primitive language.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1630
src/cmd/ld/dwarf.c:1630: vlong here = cpos();
On 2010/10/21 19:51:43, rsc1 wrote:
> decl at top, please

Done.

[lvd, 2010-10-24 19:35:59]

On Fri, Oct 22, 2010 at 15:20, <lvd@google.com> wrote:

> all done.  still need to go over the decoding for 8l.
>
>
made all the decodetype_* functions compute the offsets of the fields they
dig out from PtrSize.  i also left in some hexdumping code that is really
useful if i ever need to update them, but i could take that out.


>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c
> File src/cmd/ld/dwarf.c (right):
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode4
> src/cmd/ld/dwarf.c:4: //
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> make this line blank
>> just to set the copyright comment off from the real one
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode11
> src/cmd/ld/dwarf.c:11: //        ptype struct '[]uint8' and qualifiers need
> to
> be quoted away
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> tab before 'and' should be a space
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode395
> src/cmd/ld/dwarf.c:395: // Each DIE (except the root ones) has at least
> 1 attribute, it's name.  getattr first moves
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> s/it's/its/
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode422
> src/cmd/ld/dwarf.c:422: // written out if it is listed in the
> abbrev).        If it's parent is
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> s/it's/its/
>>
>
>  it's = it is
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode711
> src/cmd/ld/dwarf.c:711: decode_reloc(Sym *s, int32 off) {
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> \n before {.  etc.
>>
>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode713
> src/cmd/ld/dwarf.c:713: for (i = 0; i < s->nr; ++i)
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> i++
>> just for consistency
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode720
> src/cmd/ld/dwarf.c:720: static uint8 decodetype_kind(Sym *s) { return
> s->p[0x1f] & ~KindNoPointers; }
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> need more newlines here, sorry.  etc.
>>
>
>  static uint8
>> decodetype_kind(Sym *s)
>> {
>>     return s->p[0x1f] & ~KindNoPointers;
>> }
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode736
> src/cmd/ld/dwarf.c:736: Sym *p  = decode_reloc(s, 0x40 + i*0x28);  //
> go.string."foo"
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> declare variables in block at top of function, then blank line
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode737
> src/cmd/ld/dwarf.c:737: if (p == nil) return nil;
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> newline after if condition too
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode740
> src/cmd/ld/dwarf.c:740: char *n = mal(p->np);                   // make a
> 0-terminated
> copy
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> it's already NUL-terminated
>> string."foo" = C string "foo"
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode884
> src/cmd/ld/dwarf.c:884: break;
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> funny spacing; tab issue?
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode890
> src/cmd/ld/dwarf.c:890: newrefattr(fld, DW_AT_type, find(&dwtypes,
> "<byte*>"));
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> not *byte?
>>
>
> a matter of taste.  i predefine some types to be able to describe the
> internal c-structs for slices and strings etc. they are C, not go. i
> dont want to mix them with the official types, so the <..> is just to
> offset them from the c and go ones.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode903
> src/cmd/ld/dwarf.c:903: s =       decodetype_structfieldtype(gotype, i);
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> s/ +/ /
>>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode914
> src/cmd/ld/dwarf.c:914: newrefattr(die, DW_AT_type, find(&dwtypes,
> "void"));   // must be defined before
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> // void already defined
>>
>
>
> Done.
>
>
>
> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode951
> src/cmd/ld/dwarf.c:951:
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> You asked about distinguishing globals from locals.
>> Locals have t == 'a' (local aka auto variable) or t == 'p' (parameter)
>>
> here.
>
>> That's how to tell.  The Sym* doesn't have that information.
>>
>
> yes.  and this just goes over the globals anyway.  i was confused as to
> why gdb was confused.  it knows given the address which variable you
> talk about, but not given which name, which can be a problem if you have
> auto and params hiding eachotehr.  that needs some thought still.
>
>
>
>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1389
> src/cmd/ld/dwarf.c:1389: // diag("instruction with linenumber past EOF
> in %s: %P", histfile[1], q);
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> line number is still two words  :-)
>>
>
> english is such a primitive language.
>
>
>
>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1630
> src/cmd/ld/dwarf.c:1630: vlong here = cpos();
> On 2010/10/21 19:51:43, rsc1 wrote:
>
>> decl at top, please
>>
>
> Done.
>
>
> http://codereview.appspot.com/2201044/
>

[lvd, 2010-10-24 19:39:26]

gdb now complains

warning: Loadable segment ".interp" outside of ELF segments

but everything seems to work fine.  i wonder if that has anything to do with
the fact that i had to grow the elf header reserve from 2048 to  3072 bytes.
 maybe that interferes with other code?

/L

On Sun, Oct 24, 2010 at 21:35, Luuk van Dijk <lvd@google.com> wrote:

>
>
> On Fri, Oct 22, 2010 at 15:20, <lvd@google.com> wrote:
>
>> all done.  still need to go over the decoding for 8l.
>>
>>
> made all the decodetype_* functions compute the offsets of the fields they
> dig out from PtrSize.  i also left in some hexdumping code that is really
> useful if i ever need to update them, but i could take that out.
>
>
>>
>>
>> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c
>> File src/cmd/ld/dwarf.c (right):
>>
>>
>> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode4
>> src/cmd/ld/dwarf.c:4: //
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> make this line blank
>>> just to set the copyright comment off from the real one
>>>
>>
>> Done.
>>
>>
>>
>> http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode11
>> src/cmd/ld/dwarf.c:11: //        ptype struct '[]uint8' and qualifiers
>> need to
>> be quoted away
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> tab before 'and' should be a space
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode395
>> src/cmd/ld/dwarf.c:395: // Each DIE (except the root ones) has at least
>> 1 attribute, it's name.  getattr first moves
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> s/it's/its/
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode422
>> src/cmd/ld/dwarf.c:422: // written out if it is listed in the
>> abbrev).        If it's parent is
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> s/it's/its/
>>>
>>
>>  it's = it is
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode711
>> src/cmd/ld/dwarf.c:711: decode_reloc(Sym *s, int32 off) {
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> \n before {.  etc.
>>>
>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode713
>> src/cmd/ld/dwarf.c:713: for (i = 0; i < s->nr; ++i)
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> i++
>>> just for consistency
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode720
>> src/cmd/ld/dwarf.c:720: static uint8 decodetype_kind(Sym *s) { return
>> s->p[0x1f] & ~KindNoPointers; }
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> need more newlines here, sorry.  etc.
>>>
>>
>>  static uint8
>>> decodetype_kind(Sym *s)
>>> {
>>>     return s->p[0x1f] & ~KindNoPointers;
>>> }
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode736
>> src/cmd/ld/dwarf.c:736: Sym *p  = decode_reloc(s, 0x40 + i*0x28);  //
>> go.string."foo"
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> declare variables in block at top of function, then blank line
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode737
>> src/cmd/ld/dwarf.c:737: if (p == nil) return nil;
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> newline after if condition too
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode740
>> src/cmd/ld/dwarf.c:740: char *n = mal(p->np);                   // make a
>> 0-terminated
>> copy
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> it's already NUL-terminated
>>> string."foo" = C string "foo"
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode884
>> src/cmd/ld/dwarf.c:884: break;
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> funny spacing; tab issue?
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode890
>> src/cmd/ld/dwarf.c:890: newrefattr(fld, DW_AT_type, find(&dwtypes,
>> "<byte*>"));
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> not *byte?
>>>
>>
>> a matter of taste.  i predefine some types to be able to describe the
>> internal c-structs for slices and strings etc. they are C, not go. i
>> dont want to mix them with the official types, so the <..> is just to
>> offset them from the c and go ones.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode903
>> src/cmd/ld/dwarf.c:903: s =       decodetype_structfieldtype(gotype, i);
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> s/ +/ /
>>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode914
>> src/cmd/ld/dwarf.c:914: newrefattr(die, DW_AT_type, find(&dwtypes,
>> "void"));   // must be defined before
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> // void already defined
>>>
>>
>>
>> Done.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode951
>> src/cmd/ld/dwarf.c:951:
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> You asked about distinguishing globals from locals.
>>> Locals have t == 'a' (local aka auto variable) or t == 'p' (parameter)
>>>
>> here.
>>
>>> That's how to tell.  The Sym* doesn't have that information.
>>>
>>
>> yes.  and this just goes over the globals anyway.  i was confused as to
>> why gdb was confused.  it knows given the address which variable you
>> talk about, but not given which name, which can be a problem if you have
>> auto and params hiding eachotehr.  that needs some thought still.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1389
>> src/cmd/ld/dwarf.c:1389: // diag("instruction with linenumber past EOF
>> in %s: %P", histfile[1], q);
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> line number is still two words  :-)
>>>
>>
>> english is such a primitive language.
>>
>>
>>
>>
http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1630
>> src/cmd/ld/dwarf.c:1630: vlong here = cpos();
>> On 2010/10/21 19:51:43, rsc1 wrote:
>>
>>> decl at top, please
>>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/2201044/
>>
>
>

[rsc1, 2010-10-24 19:45:44]

LGTM after fixes below

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1630
src/cmd/ld/dwarf.c:1630: vlong here = cpos();
On 2010/10/22 13:20:13, lvd wrote:
> On 2010/10/21 19:51:43, rsc1 wrote:
> > decl at top, please
> 
> Done.

Not done.

http://codereview.appspot.com/2201044/diff/29006/src/cmd/ld/dwarf.c#newcode713
src/cmd/ld/dwarf.c:713: for (i = 0; i < s->nr; ++i)
blank line between declarations and code

http://codereview.appspot.com/2201044/diff/29006/src/cmd/ld/dwarf.c#newcode724
src/cmd/ld/dwarf.c:724: 
uchar *inuxi

http://codereview.appspot.com/2201044/diff/29006/src/cmd/ld/dwarf.c#newcode740
src/cmd/ld/dwarf.c:740: char *n = mal(p->np);			// make a 0-terminated copy
diag("decode_inuxi %d", sz);

http://codereview.appspot.com/2201044/diff/29006/src/cmd/ld/dwarf.c#newcode834
src/cmd/ld/dwarf.c:834: die = newdie(&dwtypes, DW_ABRV_CHANTYPE, name);
delete #if 0 / #endif.
make if statement

if (0 && debug['v'] > 2) {

and then the code won't rot.

[lvd, 2010-10-24 21:05:29]

all done and submitting.

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/2201044/diff/21001/src/cmd/ld/dwarf.c#newcode1630
src/cmd/ld/dwarf.c:1630: vlong here = cpos();
On 2010/10/24 19:45:44, rsc1 wrote:
> On 2010/10/22 13:20:13, lvd wrote:
> > On 2010/10/21 19:51:43, rsc1 wrote:
> > > decl at top, please
> > 
> > Done.
> 
> Not done.

there were more.

[lvd, 2010-10-24 21:07:59]

*** Submitted as http://code.google.com/p/go/source/detail?r=a7b35b61a59e ***

6l/8l: global and local variables and type info.

R=rsc
CC=golang-dev
http://codereview.appspot.com/2201044