code review 1987042: Dwarf output for 6l.

Dwarf output for 6l.
Part 1, general scaffolding and pc/lc sections.

[lvd, 2010-08-16 21:20:59]

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

I'd like you to review this change.

[lvd, 2010-08-16 21:25:39]

sorry i expected hg mail to let me write some lines to comment.

this is functional: i can open any file from test/... compile it by hand and
single step and next through it.  with
http://codereview.appspot.com/1954049/ it can also find the .goc lines.   i
tested it by forcing -e on and ./all.bash in src.

On Mon, Aug 16, 2010 at 23:20, <lvd@google.com> wrote:

> Reviewers: rsc, r2, ken2,
>
> Message:
> Hello rsc, r2, ken2 (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change.
>
>
> Description:
> Dwarf output for 6l.
> Part 1, general scaffolding and pc/lc sections.
>
> Please review this at http://codereview.appspot.com/1987042/
>
> Affected files:
>  M src/cmd/6l/Makefile
>  M src/cmd/6l/asm.c
>  M src/cmd/6l/l.h
>  M src/cmd/6l/obj.c
>  A src/cmd/ld/dwarf.c
>  A src/cmd/ld/dwarf.h
>  A src/cmd/ld/dwarf_defs.h
>
>
>

[rsc1, 2010-08-16 21:29:00]

syntactic stuff

http://codereview.appspot.com/1987042/diff/13001/3008
File src/cmd/6l/Makefile (right):

http://codereview.appspot.com/1987042/diff/13001/3008#newcode22
src/cmd/6l/Makefile:22: dwarf.$O\
sort list
delete extra blank line (next line)

http://codereview.appspot.com/1987042/diff/13001/3009
File src/cmd/6l/asm.c (right):

http://codereview.appspot.com/1987042/diff/13001/3009#newcode991
src/cmd/6l/asm.c:991: off_t
this code typically avoids _t typedefs.
the file position is a vlong

http://codereview.appspot.com/1987042/diff/13001/3010
File src/cmd/6l/l.h (right):

http://codereview.appspot.com/1987042/diff/13001/3010#newcode397
src/cmd/6l/l.h:397: off_t   cpos(void);
vlong

http://codereview.appspot.com/1987042/diff/13001/3011
File src/cmd/6l/obj.c (right):

http://codereview.appspot.com/1987042/diff/13001/3011#newcode71
src/cmd/6l/obj.c:71: debug['e']++;// TEST(lvd)
delete
and blank line

http://codereview.appspot.com/1987042/diff/13001/3012
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/1987042/diff/13001/3012#newcode76
src/cmd/ld/dwarf.c:76: struct DWAttr *link;
use tabs, here and elsewhere

http://codereview.appspot.com/1987042/diff/13001/3012#newcode146
src/cmd/ld/dwarf.c:146: putattr(int form, int cls, vlong value, char* data)
char *data

http://codereview.appspot.com/1987042/diff/13001/3012#newcode148
src/cmd/ld/dwarf.c:148: switch(form) {
switch indentation is

switch(asdf) { 
case on_same_column_as_switch:
    code indented one tab

see other switches for examples

http://codereview.appspot.com/1987042/diff/13001/3012#newcode242
src/cmd/ld/dwarf.c:242: static void putdies(struct DWDie* die)
newline before function name

http://codereview.appspot.com/1987042/diff/13001/3012#newcode269
src/cmd/ld/dwarf.c:269:
/******************************************************************************/
drop ascii art

http://codereview.appspot.com/1987042/diff/13001/3013
File src/cmd/ld/dwarf.h (right):

http://codereview.appspot.com/1987042/diff/13001/3013#newcode6
src/cmd/ld/dwarf.h:6: /* Register 'f' symbol file fragments.  Doing this while
parsing the
multiline comments are 
/*
 * text
 * text
 */

or just use // comments

http://codereview.appspot.com/1987042/diff/13001/3013#newcode8
src/cmd/ld/dwarf.h:8: void dw_addfrag(int n, char* frag);
I'd use dwarfaddfrag here.
It matches the elf names (spelled out, no _)
and is not noticeably longer.

http://codereview.appspot.com/1987042/diff/13001/3014
File src/cmd/ld/dwarf_defs.h (right):

http://codereview.appspot.com/1987042/diff/13001/3014#newcode5
src/cmd/ld/dwarf_defs.h:5: // Semi-automatically generated from tables in
how?

[rsc, 2010-08-16 21:30:12]

this is very cool, by the way.
i will digest the semantic stuff in a second pass later.

[lvd, 2010-08-16 21:49:56]

mostly done.

http://codereview.appspot.com/1987042/diff/13001/3008
File src/cmd/6l/Makefile (right):

http://codereview.appspot.com/1987042/diff/13001/3008#newcode22
src/cmd/6l/Makefile:22: dwarf.$O\
On 2010/08/16 21:29:00, rsc1 wrote:
> sort list
> delete extra blank line (next line)
> 

Done.

http://codereview.appspot.com/1987042/diff/13001/3009
File src/cmd/6l/asm.c (right):

http://codereview.appspot.com/1987042/diff/13001/3009#newcode991
src/cmd/6l/asm.c:991: off_t
On 2010/08/16 21:29:00, rsc1 wrote:
> this code typically avoids _t typedefs.
> the file position is a vlong
> 

Done.

http://codereview.appspot.com/1987042/diff/13001/3010
File src/cmd/6l/l.h (right):

http://codereview.appspot.com/1987042/diff/13001/3010#newcode397
src/cmd/6l/l.h:397: off_t   cpos(void);
On 2010/08/16 21:29:00, rsc1 wrote:
> vlong
> 

Done.

http://codereview.appspot.com/1987042/diff/13001/3011
File src/cmd/6l/obj.c (right):

http://codereview.appspot.com/1987042/diff/13001/3011#newcode71
src/cmd/6l/obj.c:71: debug['e']++;// TEST(lvd)
On 2010/08/16 21:29:00, rsc1 wrote:
> delete
> and blank line
> 

I'll delete everyting marked //TEST before submitting.
 this is the easiest way to run all tests with -e for now.

http://codereview.appspot.com/1987042/diff/13001/3012
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/1987042/diff/13001/3012#newcode76
src/cmd/ld/dwarf.c:76: struct DWAttr *link;
On 2010/08/16 21:29:00, rsc1 wrote:
> use tabs, here and elsewhere
> 

i'll run through indent later.

http://codereview.appspot.com/1987042/diff/13001/3012#newcode146
src/cmd/ld/dwarf.c:146: putattr(int form, int cls, vlong value, char* data)
On 2010/08/16 21:29:00, rsc1 wrote:
> char *data
> 

Done.

http://codereview.appspot.com/1987042/diff/13001/3012#newcode242
src/cmd/ld/dwarf.c:242: static void putdies(struct DWDie* die)
On 2010/08/16 21:29:00, rsc1 wrote:
> newline before function name
> 

Done.

http://codereview.appspot.com/1987042/diff/13001/3012#newcode269
src/cmd/ld/dwarf.c:269:
/******************************************************************************/
On 2010/08/16 21:29:00, rsc1 wrote:
> drop ascii art
> 

Done.

http://codereview.appspot.com/1987042/diff/13001/3013
File src/cmd/ld/dwarf.h (right):

http://codereview.appspot.com/1987042/diff/13001/3013#newcode6
src/cmd/ld/dwarf.h:6: /* Register 'f' symbol file fragments.  Doing this while
parsing the
On 2010/08/16 21:29:00, rsc1 wrote:
> multiline comments are 
> /*
>  * text
>  * text
>  */
> 
> or just use // comments
> 

Done.

http://codereview.appspot.com/1987042/diff/13001/3013#newcode8
src/cmd/ld/dwarf.h:8: void dw_addfrag(int n, char* frag);
On 2010/08/16 21:29:00, rsc1 wrote:
> I'd use dwarfaddfrag here.
> It matches the elf names (spelled out, no _)
> and is not noticeably longer.
> 

Done.

http://codereview.appspot.com/1987042/diff/13001/3014
File src/cmd/ld/dwarf_defs.h (right):

http://codereview.appspot.com/1987042/diff/13001/3014#newcode5
src/cmd/ld/dwarf_defs.h:5: // Semi-automatically generated from tables in
On 2010/08/16 21:29:00, rsc1 wrote:
> how?
> 
 by cutting from the pdf, pasting into a shell with cat > and running some
tr/awk massaging on it that looked like  tr -d '?' | awk -vORS='' '{n=$1; v=$2;
$1=""; $2=""; print "\t"n" = "v","} NR > 3 {print "\t//"$0} {print "\n"}'
not sure if that's worth adding to the comment here

[rsc, 2010-08-16 21:59:27]

> I'll delete everyting marked //TEST before submitting.
>  this is the easiest way to run all tests with -e for now.

-e was a mistake.  just run the code all the time
and delete the flag entirely.  (should still omit if debug['s'] != 0.)

>  by cutting from the pdf, pasting into a shell with cat > and running
> some tr/awk massaging on it that looked like  tr -d '?' | awk -vORS=''
> '{n=$1; v=$2; $1=""; $2=""; print "\t"n" = "v","} NR > 3 {print
> "\t//"$0} {print "\n"}'
> not sure if that's worth adding to the comment here

worth saying

// Generated by cut and paste and processing of
// http://url.

[rsc1, 2010-08-17 05:56:42]

Another volley of comments.
I apologize for all the C style stuff but it will help
not to be distracted by inconsistent formatting
when we come back to this in a few months, and once
you internalize the style there will be many fewer of these.
The bulk of the code looks great.

http://codereview.appspot.com/1987042/diff/23001/17003
File src/cmd/6l/asm.c (right):

http://codereview.appspot.com/1987042/diff/23001/17003#newcode304
src/cmd/6l/asm.c:304: if(debug['e']) {
delete debug['e'] as discussed via mail.
search for others

http://codereview.appspot.com/1987042/diff/23001/17003#newcode994
src/cmd/6l/asm.c:994: return seek(cout, 0, 1) + sizeof(buf.cbuf) - cbc;
please fix indent

http://codereview.appspot.com/1987042/diff/23001/17005
File src/cmd/6l/obj.c (right):

http://codereview.appspot.com/1987042/diff/23001/17005#newcode71
src/cmd/6l/obj.c:71: debug['e']++;// TEST(lvd)
delete

http://codereview.appspot.com/1987042/diff/23001/17006
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/1987042/diff/23001/17006#newcode24
src/cmd/ld/dwarf.c:24: * Defining Abbrevs.  This is done at compile time, and
there will be
s/This is done at compile time/These are hard-coded/

(compile time is ambiguous in a compiler tool chain)

http://codereview.appspot.com/1987042/diff/23001/17006#newcode31
src/cmd/ld/dwarf.c:31: struct DWAttrForm {
give your structs typedef names 

typedef struct DWAttrForm DWAttrForm;
struct DWAttrForm {

etc.

then you can drop all the "struct " when referring to them.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode36
src/cmd/ld/dwarf.c:36: enum { DW_NATTR = 30 };    // maximum number of
attributes in an abbrev.
can drop this and say 30 below.
if it ever comes up (it doesn't in this code)
you can refer to nelem(a.attr)

http://codereview.appspot.com/1987042/diff/23001/17006#newcode50
src/cmd/ld/dwarf.c:50: /* DW_ABRV_NULL */
s/DW_ABRV_//
to match next comment

http://codereview.appspot.com/1987042/diff/23001/17006#newcode51
src/cmd/ld/dwarf.c:51: { 0, 0, { { 0, 0 } } },
{ 0 },

seems like it would suffice

http://codereview.appspot.com/1987042/diff/23001/17006#newcode54
src/cmd/ld/dwarf.c:54: DW_TAG_compile_unit, DW_CHILDREN_yes,
The structure here is beautiful but unnecessary.
Feel free to use

{ 
    DW_TAG_compile_unit,
    DW_CHILDREN_yes,
    DW_AT_name, DW_FORM_string,
    DW_AT_language, DW_FORM_data1,
    ...
}

and save some indentation

http://codereview.appspot.com/1987042/diff/23001/17006#newcode64
src/cmd/ld/dwarf.c:64: /* ... */
delete

http://codereview.appspot.com/1987042/diff/23001/17006#newcode68
src/cmd/ld/dwarf.c:68: * DebuggingInformationEntries and their attributes
unless that's a program name, insert spaces

http://codereview.appspot.com/1987042/diff/23001/17006#newcode93
src/cmd/ld/dwarf.c:93: static struct DWDie *
an exception to the usual rule: can drop the space before * here
(look at other files for examples).

static DWDie*

http://codereview.appspot.com/1987042/diff/23001/17006#newcode96
src/cmd/ld/dwarf.c:96: struct DWDie *die = mal(sizeof *die);
please do not declare and initialize,
and please separate declarations and code
by a blank line:

DWDie *die;

die = mal(sizeof *die);

http://codereview.appspot.com/1987042/diff/23001/17006#newcode105
src/cmd/ld/dwarf.c:105: struct DWAttr *a = mal(sizeof *a);
ditto

http://codereview.appspot.com/1987042/diff/23001/17006#newcode118
src/cmd/ld/dwarf.c:118: uint8 c;
blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode131
src/cmd/ld/dwarf.c:131: int neg = (v<0);
init+blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode135
src/cmd/ld/dwarf.c:135: #if 1   // TODO check if >> is arithmethic
delete.
since v is signed, >> sign extends

http://codereview.appspot.com/1987042/diff/23001/17006#newcode138
src/cmd/ld/dwarf.c:138: if (((v != -1) || !(c & 0x40)) &&
over parenthesized => hard to read.

if((v != -1 || !(c & 0x40)) && (v != 0 || (c & 0x40)))
    c |= 0x80;

don't bother trying to line things up with spaces.
some of us use variable-width fonts and it just 
looks funny.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode142
src/cmd/ld/dwarf.c:142: } while (c & 0x80);
the style in this code is to omit the space
between a keyword and (:  if( while( switch(.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode148
src/cmd/ld/dwarf.c:148: switch(form) {
fix spacing of switch cases

http://codereview.appspot.com/1987042/diff/23001/17006#newcode156
src/cmd/ld/dwarf.c:156: while(value--) cput(*data++);
don't omit newlines

while(value--)
    cput(*data++);

etc

http://codereview.appspot.com/1987042/diff/23001/17006#newcode205
src/cmd/ld/dwarf.c:205: cput(value?1:0);
avoid ternary operator.
cput(value) is fine - just make sure value is 1 or 0.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode225
src/cmd/ld/dwarf.c:225: struct DWAttr *attrs[DW_AT_recursive + 1];
blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode229
src/cmd/ld/dwarf.c:229: struct DWAttrForm* af;
please put all declaration at top of function.
c99 allows mixing them but it's not the local style.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode230
src/cmd/ld/dwarf.c:230: for(af = abbrevs[abbrev].attr; af->attr; ++af)
af++, etc. throughout file.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode245
src/cmd/ld/dwarf.c:245: for(; die; die = die->link) putdie(die);
newline

http://codereview.appspot.com/1987042/diff/23001/17006#newcode261
src/cmd/ld/dwarf.c:261: struct DWDie* curr = *list;
init+blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode283
src/cmd/ld/dwarf.c:283: int s = ftabsize;
init+blank line.  conventional to put int s at top of function too.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode285
src/cmd/ld/dwarf.c:285: ftab = realloc(ftab, ftabsize * sizeof(char *));
sizeof ftab[0]
here and next line.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode294
src/cmd/ld/dwarf.c:294: // Returns a malloc'ed string, piecewise copied frm the
ftab.
s/frm/from/

http://codereview.appspot.com/1987042/diff/23001/17006#newcode298
src/cmd/ld/dwarf.c:298: int len = 0;
init+blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode300
src/cmd/ld/dwarf.c:300: while (ss[1]) {
I think this should be while(ss[0] || ss[1]), no?
Really it's while the o is not zero, so this could be
for(;;) {
and then below 
if(o == 0)
    break;

http://codereview.appspot.com/1987042/diff/23001/17006#newcode301
src/cmd/ld/dwarf.c:301: uint8 h = ss[0];
more vars to declare at top of function. 
or just use

o = ((uint8)ss[0] << 8) | (uint8)ss[1];

http://codereview.appspot.com/1987042/diff/23001/17006#newcode305
src/cmd/ld/dwarf.c:305: diag("corrupt z entry.");
drop trailing dot (match rest of ld)

http://codereview.appspot.com/1987042/diff/23001/17006#newcode308
src/cmd/ld/dwarf.c:308: char *f = ftab[o];
var

http://codereview.appspot.com/1987042/diff/23001/17006#newcode309
src/cmd/ld/dwarf.c:309: if (!f) {
please use f == nil and reserve ! for booleans.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode310
src/cmd/ld/dwarf.c:310: diag("corrupt z entry.");
drop trailing dot

http://codereview.appspot.com/1987042/diff/23001/17006#newcode320
src/cmd/ld/dwarf.c:320: char *r = malloc(len + 1);
vars

http://codereview.appspot.com/1987042/diff/23001/17006#newcode325
src/cmd/ld/dwarf.c:325: while (s[1]) {
same comment

http://codereview.appspot.com/1987042/diff/23001/17006#newcode348
src/cmd/ld/dwarf.c:348: clearhistfile(void) {
brace on next line, here and below

http://codereview.appspot.com/1987042/diff/23001/17006#newcode349
src/cmd/ld/dwarf.c:349: int i;
blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode350
src/cmd/ld/dwarf.c:350: for (i = 1; i < histfilesize; ++i)
say why loop begins at i = 1.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode350
src/cmd/ld/dwarf.c:350: for (i = 1; i < histfilesize; ++i)
i++

http://codereview.appspot.com/1987042/diff/23001/17006#newcode361
src/cmd/ld/dwarf.c:361: if (histfilesize == 0) {
can omit braces around single line body

http://codereview.appspot.com/1987042/diff/23001/17006#newcode365
src/cmd/ld/dwarf.c:365: char* fname = decodez(zentry);
var

http://codereview.appspot.com/1987042/diff/23001/17006#newcode366
src/cmd/ld/dwarf.c:366: if (fname == 0) return -1;
newline

http://codereview.appspot.com/1987042/diff/23001/17006#newcode368
src/cmd/ld/dwarf.c:368: if (strcmp(fname, histfile[histfilesize - 1]) == 0) {
can drop spaces in nested expression to make overall expression easier to grok
at a glance:

if(strcmp(fname, histfile[histfilesize-1]) == 0) {

http://codereview.appspot.com/1987042/diff/23001/17006#newcode378
src/cmd/ld/dwarf.c:378: enum { NMAXNEST = 16 };
same comment: just use 16 below and nelem(includestack)
if you need the size.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode399
src/cmd/ld/dwarf.c:399: int i;
blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode406
src/cmd/ld/dwarf.c:406: for (i = 0; i < NMAXNEST; ++i)
s/NMAXNEST/includetop/

http://codereview.appspot.com/1987042/diff/23001/17006#newcode407
src/cmd/ld/dwarf.c:407: diag("\tfile:%s", histfile[includestack[i].file]);
s/file://

http://codereview.appspot.com/1987042/diff/23001/17006#newcode419
src/cmd/ld/dwarf.c:419: if (a==0) return 0;
newline.
use nil instead of 0 for pointer.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode424
src/cmd/ld/dwarf.c:424: diag("Stray 'z' with offset %d", a->aoffset);
diag messages start lowercase (they have a prefix already)
s/Stray/stray/

http://codereview.appspot.com/1987042/diff/23001/17006#newcode428
src/cmd/ld/dwarf.c:428: char* unitname = decodez(a->asym->name);
var, star

http://codereview.appspot.com/1987042/diff/23001/17006#newcode436
src/cmd/ld/dwarf.c:436: while (linehist) {
linehist != nil

http://codereview.appspot.com/1987042/diff/23001/17006#newcode444
src/cmd/ld/dwarf.c:444: 
stray blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode446
src/cmd/ld/dwarf.c:446: 
more stray blank lines.
no need for a blank line after {

http://codereview.appspot.com/1987042/diff/23001/17006#newcode450
src/cmd/ld/dwarf.c:450: 
another; lots in this function

http://codereview.appspot.com/1987042/diff/23001/17006#newcode478
src/cmd/ld/dwarf.c:478: } else continue;
newline

http://codereview.appspot.com/1987042/diff/23001/17006#newcode496
src/cmd/ld/dwarf.c:496: searchhist(vlong absline) {
brace on next line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode497
src/cmd/ld/dwarf.c:497: struct Linehist *lh;
newline

http://codereview.appspot.com/1987042/diff/23001/17006#newcode529
src/cmd/ld/dwarf.c:529: int l = strlen(s);
This should be a one-line function:

return strlen(s) >= 3 && strcmp(s+strlen(s)-3, ".go") == 0;

The current version is more efficient but harder to read
and the function itself is not worth optimizing.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode547
src/cmd/ld/dwarf.c:547: LINE_BASE = -1,
tabs

http://codereview.appspot.com/1987042/diff/23001/17006#newcode575
src/cmd/ld/dwarf.c:575: * Walk prog table, emit line program and build DIE trie.
s/trie/tree/

http://codereview.appspot.com/1987042/diff/23001/17006#newcode580
src/cmd/ld/dwarf.c:580: flushunit(vlong pc, vlong unitstart) {
newline before brace

http://codereview.appspot.com/1987042/diff/23001/17006#newcode591
src/cmd/ld/dwarf.c:591: vlong here = cpos();
var

http://codereview.appspot.com/1987042/diff/23001/17006#newcode603
src/cmd/ld/dwarf.c:603: vlong unitstart = -1;
inits

http://codereview.appspot.com/1987042/diff/23001/17006#newcode612
src/cmd/ld/dwarf.c:612: 
stray blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode613
src/cmd/ld/dwarf.c:613: Sym *s = p->from.sym;
init, var

http://codereview.appspot.com/1987042/diff/23001/17006#newcode614
src/cmd/ld/dwarf.c:614: if (!s || s->type != STEXT) {
s == nil

http://codereview.appspot.com/1987042/diff/23001/17006#newcode622
src/cmd/ld/dwarf.c:622: char* unitname;
var.  s/* / */

http://codereview.appspot.com/1987042/diff/23001/17006#newcode624
src/cmd/ld/dwarf.c:624: 
many stray blank lines

http://codereview.appspot.com/1987042/diff/23001/17006#newcode629
src/cmd/ld/dwarf.c:629: if(debug['v']) {
debug['v'] > 1 or delete.
this is going to be too noisy.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode630
src/cmd/ld/dwarf.c:630: print("dwarf::writelines found %s\n", unitname);
s/::/ /

http://codereview.appspot.com/1987042/diff/23001/17006#newcode640
src/cmd/ld/dwarf.c:640: isgosrc(unitname) ? DW_LANG_Go : DW_LANG_C, 0);
avoid ternary.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode662
src/cmd/ld/dwarf.c:662: int i;
var

http://codereview.appspot.com/1987042/diff/23001/17006#newcode665
src/cmd/ld/dwarf.c:665: uleb128put(1+ strlen(histfile[i]) + 4);
consistent spacing around +, one way or the other

http://codereview.appspot.com/1987042/diff/23001/17006#newcode680
src/cmd/ld/dwarf.c:680: 
stray blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode683
src/cmd/ld/dwarf.c:683: if (!p->from.sym->reachable) continue;
newline

http://codereview.appspot.com/1987042/diff/23001/17006#newcode691
src/cmd/ld/dwarf.c:691: struct Linehist* lh = searchhist(q->line);
var

http://codereview.appspot.com/1987042/diff/23001/17006#newcode698
src/cmd/ld/dwarf.c:698: vlong lline = lh->line + q->line - lh->absline;
var

http://codereview.appspot.com/1987042/diff/23001/17006#newcode704
src/cmd/ld/dwarf.c:704: if (q->line == lc) continue;
newline

http://codereview.appspot.com/1987042/diff/23001/17006#newcode719
src/cmd/ld/dwarf.c:719: }  // function
drop comment.
i thought it meant the function was over,
which was surprising given that the } wasn't
at the beginning of the line.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode722
src/cmd/ld/dwarf.c:722: 
delete blank line

http://codereview.appspot.com/1987042/diff/23001/17006#newcode789
src/cmd/ld/dwarf.c:789: elfstrdbg[ElfStrDebugAbbrev]   = addstring(shstrtab,
".debug_abbrev");
,s/ +/ /g
drop funny spacing.
it's okay (and easier) for things not to line up.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode801
src/cmd/ld/dwarf.c:801: 
one blank line only between functions

http://codereview.appspot.com/1987042/diff/23001/17006#newcode810
src/cmd/ld/dwarf.c:810: 
same

http://codereview.appspot.com/1987042/diff/23001/17006#newcode815
src/cmd/ld/dwarf.c:815: 
move blank line below var decls

http://codereview.appspot.com/1987042/diff/23001/17008
File src/cmd/ld/dwarf_defs.h (right):

http://codereview.appspot.com/1987042/diff/23001/17008#newcode372
src/cmd/ld/dwarf_defs.h:372: DW_LANG_Go = 0x8015,	// TODO(lvd) Temporary
why 0x8015 and not 0x8000?
ask iant what he is using.

[lvd, 2010-08-17 11:05:34]

i didn't buy the 'distraction by indentation' argument when i joined google and
i don't buy it from you, and i think you just made me waste 2 hours i could have
spent on getting the stackframes working, but i'm a good boy and i have done
your bidding, as i will in the future.

http://codereview.appspot.com/1987042/diff/23001/17003
File src/cmd/6l/asm.c (right):

http://codereview.appspot.com/1987042/diff/23001/17003#newcode304
src/cmd/6l/asm.c:304: if(debug['e']) {
On 2010/08/17 05:56:42, rsc1 wrote:
> delete debug['e'] as discussed via mail.
> search for others
> 
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17005
File src/cmd/6l/obj.c (right):

http://codereview.appspot.com/1987042/diff/23001/17005#newcode71
src/cmd/6l/obj.c:71: debug['e']++;// TEST(lvd)
On 2010/08/17 05:56:42, rsc1 wrote:
> delete
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/1987042/diff/23001/17006#newcode24
src/cmd/ld/dwarf.c:24: * Defining Abbrevs.  This is done at compile time, and
there will be
On 2010/08/17 05:56:42, rsc1 wrote:
> s/This is done at compile time/These are hard-coded/
> 
> (compile time is ambiguous in a compiler tool chain)
> 

You're not gonna believe this but i woke up around 2am this morning, sat upright
in bed, thinking about this very line and saying to myself "that is ambiguous
here" and went back to sleep.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode31
src/cmd/ld/dwarf.c:31: struct DWAttrForm {
On 2010/08/17 05:56:42, rsc1 wrote:
> give your structs typedef names 
> 
> typedef struct DWAttrForm DWAttrForm;
> struct DWAttrForm {
> 
> etc.
> 
> then you can drop all the "struct " when referring to them.
> 

i know i can, but i don't see much benefit. but done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode36
src/cmd/ld/dwarf.c:36: enum { DW_NATTR = 30 };    // maximum number of
attributes in an abbrev.
On 2010/08/17 05:56:42, rsc1 wrote:
> can drop this and say 30 below.
> if it ever comes up (it doesn't in this code)
> you can refer to nelem(a.attr)
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode50
src/cmd/ld/dwarf.c:50: /* DW_ABRV_NULL */
On 2010/08/17 05:56:42, rsc1 wrote:
> s/DW_ABRV_//
> to match next comment
> 
yes but NULL is confusingly ambiguous

http://codereview.appspot.com/1987042/diff/23001/17006#newcode51
src/cmd/ld/dwarf.c:51: { 0, 0, { { 0, 0 } } },
On 2010/08/17 05:56:42, rsc1 wrote:
> { 0 },
> 
> seems like it would suffice
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode54
src/cmd/ld/dwarf.c:54: DW_TAG_compile_unit, DW_CHILDREN_yes,
On 2010/08/17 05:56:42, rsc1 wrote:
> The structure here is beautiful but unnecessary.
> Feel free to use
> 
> { 
>     DW_TAG_compile_unit,
>     DW_CHILDREN_yes,
>     DW_AT_name, DW_FORM_string,
>     DW_AT_language, DW_FORM_data1,
>     ...
> }
> 
> and save some indentation

i had no idea the compiler was _that_ smart. thx

http://codereview.appspot.com/1987042/diff/23001/17006#newcode64
src/cmd/ld/dwarf.c:64: /* ... */
On 2010/08/17 05:56:42, rsc1 wrote:
> delete
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode68
src/cmd/ld/dwarf.c:68: * DebuggingInformationEntries and their attributes
On 2010/08/17 05:56:42, rsc1 wrote:
> unless that's a program name, insert spaces
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode93
src/cmd/ld/dwarf.c:93: static struct DWDie *
On 2010/08/17 05:56:42, rsc1 wrote:
> an exception to the usual rule: can drop the space before * here
> (look at other files for examples).
> 
> static DWDie*
> 
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode96
src/cmd/ld/dwarf.c:96: struct DWDie *die = mal(sizeof *die);
On 2010/08/17 05:56:42, rsc1 wrote:
> please do not declare and initialize,

why not?

> and please separate declarations and code
> by a blank line:
> 
> DWDie *die;
> 
> die = mal(sizeof *die);

http://codereview.appspot.com/1987042/diff/23001/17006#newcode105
src/cmd/ld/dwarf.c:105: struct DWAttr *a = mal(sizeof *a);
On 2010/08/17 05:56:42, rsc1 wrote:
> ditto
> 

craziness, but done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode118
src/cmd/ld/dwarf.c:118: uint8 c;
On 2010/08/17 05:56:42, rsc1 wrote:
> blank line
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode131
src/cmd/ld/dwarf.c:131: int neg = (v<0);
On 2010/08/17 05:56:42, rsc1 wrote:
> init+blank line
> 
gone

http://codereview.appspot.com/1987042/diff/23001/17006#newcode135
src/cmd/ld/dwarf.c:135: #if 1   // TODO check if >> is arithmethic
On 2010/08/17 05:56:42, rsc1 wrote:
> delete.
> since v is signed, >> sign extends
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode142
src/cmd/ld/dwarf.c:142: } while (c & 0x80);
On 2010/08/17 05:56:42, rsc1 wrote:
> the style in this code is to omit the space
> between a keyword and (:  if( while( switch(.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode142
src/cmd/ld/dwarf.c:142: } while (c & 0x80);
On 2010/08/17 05:56:42, rsc1 wrote:
> the style in this code is to omit the space
> between a keyword and (:  if( while( switch(.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode148
src/cmd/ld/dwarf.c:148: switch(form) {
On 2010/08/17 05:56:42, rsc1 wrote:
> fix spacing of switch cases
> 
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode156
src/cmd/ld/dwarf.c:156: while(value--) cput(*data++);
On 2010/08/17 05:56:42, rsc1 wrote:
> don't omit newlines
> 
> while(value--)
>     cput(*data++);
> 
> etc

yuk. but done.

>

http://codereview.appspot.com/1987042/diff/23001/17006#newcode205
src/cmd/ld/dwarf.c:205: cput(value?1:0);
On 2010/08/17 05:56:42, rsc1 wrote:
> avoid ternary operator.

why?

> cput(value) is fine - just make sure value is 1 or 0.
> 

i'd rather not. this causes the kind of bug you search for for hours.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode229
src/cmd/ld/dwarf.c:229: struct DWAttrForm* af;
On 2010/08/17 05:56:42, rsc1 wrote:
> please put all declaration at top of function.
> c99 allows mixing them but it's not the local style.
> 
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode230
src/cmd/ld/dwarf.c:230: for(af = abbrevs[abbrev].attr; af->attr; ++af)
On 2010/08/17 05:56:42, rsc1 wrote:
> af++, etc. throughout file.
> 

seriously,  who cares.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode245
src/cmd/ld/dwarf.c:245: for(; die; die = die->link) putdie(die);
On 2010/08/17 05:56:42, rsc1 wrote:
> newline
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode283
src/cmd/ld/dwarf.c:283: int s = ftabsize;
On 2010/08/17 05:56:42, rsc1 wrote:
> init+blank line.  conventional to put int s at top of function too.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode285
src/cmd/ld/dwarf.c:285: ftab = realloc(ftab, ftabsize * sizeof(char *));
On 2010/08/17 05:56:42, rsc1 wrote:
> sizeof ftab[0]
> here and next line.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode294
src/cmd/ld/dwarf.c:294: // Returns a malloc'ed string, piecewise copied frm the
ftab.
On 2010/08/17 05:56:42, rsc1 wrote:
> s/frm/from/
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode300
src/cmd/ld/dwarf.c:300: while (ss[1]) {
On 2010/08/17 05:56:42, rsc1 wrote:
> I think this should be while(ss[0] || ss[1]), no?
> Really it's while the o is not zero, so this could be
> for(;;) {
> and then below 
> if(o == 0)
>     break;
> 

this is actually the first bug you spot in this review.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode301
src/cmd/ld/dwarf.c:301: uint8 h = ss[0];
On 2010/08/17 05:56:42, rsc1 wrote:
> more vars to declare at top of function. 
> or just use
> 
> o = ((uint8)ss[0] << 8) | (uint8)ss[1];
> 
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode305
src/cmd/ld/dwarf.c:305: diag("corrupt z entry.");
On 2010/08/17 05:56:42, rsc1 wrote:
> drop trailing dot (match rest of ld)
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode308
src/cmd/ld/dwarf.c:308: char *f = ftab[o];
On 2010/08/17 05:56:42, rsc1 wrote:
> var
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode309
src/cmd/ld/dwarf.c:309: if (!f) {
On 2010/08/17 05:56:42, rsc1 wrote:
> please use f == nil and reserve ! for booleans.
> 
since you ask politely, i will.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode310
src/cmd/ld/dwarf.c:310: diag("corrupt z entry.");
On 2010/08/17 05:56:42, rsc1 wrote:
> drop trailing dot

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode320
src/cmd/ld/dwarf.c:320: char *r = malloc(len + 1);
On 2010/08/17 05:56:42, rsc1 wrote:
> vars
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode325
src/cmd/ld/dwarf.c:325: while (s[1]) {
On 2010/08/17 05:56:42, rsc1 wrote:
> same comment
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode348
src/cmd/ld/dwarf.c:348: clearhistfile(void) {
On 2010/08/17 05:56:42, rsc1 wrote:
> brace on next line, here and below
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode349
src/cmd/ld/dwarf.c:349: int i;
On 2010/08/17 05:56:42, rsc1 wrote:
> blank line
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode350
src/cmd/ld/dwarf.c:350: for (i = 1; i < histfilesize; ++i)
On 2010/08/17 05:56:42, rsc1 wrote:
> say why loop begins at i = 1.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode361
src/cmd/ld/dwarf.c:361: if (histfilesize == 0) {
On 2010/08/17 05:56:42, rsc1 wrote:
> can omit braces around single line body
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode365
src/cmd/ld/dwarf.c:365: char* fname = decodez(zentry);
On 2010/08/17 05:56:42, rsc1 wrote:
> var

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode366
src/cmd/ld/dwarf.c:366: if (fname == 0) return -1;
On 2010/08/17 05:56:42, rsc1 wrote:
> newline
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode378
src/cmd/ld/dwarf.c:378: enum { NMAXNEST = 16 };
On 2010/08/17 05:56:42, rsc1 wrote:
> same comment: just use 16 below and nelem(includestack)
> if you need the size.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode399
src/cmd/ld/dwarf.c:399: int i;
On 2010/08/17 05:56:42, rsc1 wrote:
> blank line
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode406
src/cmd/ld/dwarf.c:406: for (i = 0; i < NMAXNEST; ++i)
On 2010/08/17 05:56:42, rsc1 wrote:
> s/NMAXNEST/includetop/
> 

no, if includetop > NMAXNEST, it got corrupted somehow, but there won't be more
than NMAXNEST (or nelem(includestack) now) printable entries.  don't want a segv
in my diagnostic message.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode419
src/cmd/ld/dwarf.c:419: if (a==0) return 0;
On 2010/08/17 05:56:42, rsc1 wrote:
> newline.
> use nil instead of 0 for pointer.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode424
src/cmd/ld/dwarf.c:424: diag("Stray 'z' with offset %d", a->aoffset);
On 2010/08/17 05:56:42, rsc1 wrote:
> diag messages start lowercase (they have a prefix already)
> s/Stray/stray/
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode428
src/cmd/ld/dwarf.c:428: char* unitname = decodez(a->asym->name);
On 2010/08/17 05:56:42, rsc1 wrote:
> var, star
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode450
src/cmd/ld/dwarf.c:450: 
On 2010/08/17 05:56:42, rsc1 wrote:
> another; lots in this function
> 
> 

thats so i could _easily_ _read_ the structure of this function.  but obfuscated
to your taste

http://codereview.appspot.com/1987042/diff/23001/17006#newcode478
src/cmd/ld/dwarf.c:478: } else continue;
On 2010/08/17 05:56:42, rsc1 wrote:
> newline
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode496
src/cmd/ld/dwarf.c:496: searchhist(vlong absline) {
On 2010/08/17 05:56:42, rsc1 wrote:
> brace on next line
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode497
src/cmd/ld/dwarf.c:497: struct Linehist *lh;
On 2010/08/17 05:56:42, rsc1 wrote:
> newline
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode529
src/cmd/ld/dwarf.c:529: int l = strlen(s);
On 2010/08/17 05:56:42, rsc1 wrote:
> This should be a one-line function:
> 
> return strlen(s) >= 3 && strcmp(s+strlen(s)-3, ".go") == 0;
> 
> The current version is more efficient but harder to read
> and the function itself is not worth optimizing.
gone altogether

http://codereview.appspot.com/1987042/diff/23001/17006#newcode547
src/cmd/ld/dwarf.c:547: LINE_BASE = -1,
On 2010/08/17 05:56:42, rsc1 wrote:
> tabs
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode575
src/cmd/ld/dwarf.c:575: * Walk prog table, emit line program and build DIE trie.
On 2010/08/17 05:56:42, rsc1 wrote:
> s/trie/tree/
> 
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode580
src/cmd/ld/dwarf.c:580: flushunit(vlong pc, vlong unitstart) {
On 2010/08/17 05:56:42, rsc1 wrote:
> newline before brace
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode612
src/cmd/ld/dwarf.c:612: 
On 2010/08/17 05:56:42, rsc1 wrote:
> stray blank line
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode613
src/cmd/ld/dwarf.c:613: Sym *s = p->from.sym;
On 2010/08/17 05:56:42, rsc1 wrote:
> init, var
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode614
src/cmd/ld/dwarf.c:614: if (!s || s->type != STEXT) {
On 2010/08/17 05:56:42, rsc1 wrote:
> s == nil
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode622
src/cmd/ld/dwarf.c:622: char* unitname;
On 2010/08/17 05:56:42, rsc1 wrote:
> var.  s/* / */
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode624
src/cmd/ld/dwarf.c:624: 
On 2010/08/17 05:56:42, rsc1 wrote:
> many stray blank lines
> 

they're not stray.  they say what's going on. said.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode629
src/cmd/ld/dwarf.c:629: if(debug['v']) {
On 2010/08/17 05:56:42, rsc1 wrote:
> debug['v'] > 1 or delete.
> this is going to be too noisy.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode630
src/cmd/ld/dwarf.c:630: print("dwarf::writelines found %s\n", unitname);
On 2010/08/17 05:56:42, rsc1 wrote:
> s/::/ /
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode640
src/cmd/ld/dwarf.c:640: isgosrc(unitname) ? DW_LANG_Go : DW_LANG_C, 0);
On 2010/08/17 05:56:42, rsc1 wrote:
> avoid ternary.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode662
src/cmd/ld/dwarf.c:662: int i;
On 2010/08/17 05:56:42, rsc1 wrote:
> var
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode665
src/cmd/ld/dwarf.c:665: uleb128put(1+ strlen(histfile[i]) + 4);
On 2010/08/17 05:56:42, rsc1 wrote:
> consistent spacing around +, one way or the other
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode680
src/cmd/ld/dwarf.c:680: 
On 2010/08/17 05:56:42, rsc1 wrote:
> stray blank line
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode683
src/cmd/ld/dwarf.c:683: if (!p->from.sym->reachable) continue;
On 2010/08/17 05:56:42, rsc1 wrote:
> newline
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode691
src/cmd/ld/dwarf.c:691: struct Linehist* lh = searchhist(q->line);
On 2010/08/17 05:56:42, rsc1 wrote:
> var
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode698
src/cmd/ld/dwarf.c:698: vlong lline = lh->line + q->line - lh->absline;
On 2010/08/17 05:56:42, rsc1 wrote:
> var
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode704
src/cmd/ld/dwarf.c:704: if (q->line == lc) continue;
On 2010/08/17 05:56:42, rsc1 wrote:
> newline
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode719
src/cmd/ld/dwarf.c:719: }  // function
On 2010/08/17 05:56:42, rsc1 wrote:
> drop comment.
> i thought it meant the function was over,
> which was surprising given that the } wasn't
> at the beginning of the line.
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode722
src/cmd/ld/dwarf.c:722: 
On 2010/08/17 05:56:42, rsc1 wrote:
> delete blank line
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode789
src/cmd/ld/dwarf.c:789: elfstrdbg[ElfStrDebugAbbrev]   = addstring(shstrtab,
".debug_abbrev");
On 2010/08/17 05:56:42, rsc1 wrote:
> ,s/ +/ /g
> drop funny spacing.
> it's okay (and easier) for things not to line up.
> 

no, this is the place where a future edit inserts ElfStrDebugFoo, and makes a
copy and paste error. the Anal Alignment helps me catch those and saves easily 2
hrs of frustration looking for it.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode801
src/cmd/ld/dwarf.c:801: 
On 2010/08/17 05:56:42, rsc1 wrote:
> one blank line only between functions
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode810
src/cmd/ld/dwarf.c:810: 
On 2010/08/17 05:56:42, rsc1 wrote:
> same
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17006#newcode815
src/cmd/ld/dwarf.c:815: 
On 2010/08/17 05:56:42, rsc1 wrote:
> move blank line below var decls
> 

Done.

http://codereview.appspot.com/1987042/diff/23001/17008
File src/cmd/ld/dwarf_defs.h (right):

http://codereview.appspot.com/1987042/diff/23001/17008#newcode372
src/cmd/ld/dwarf_defs.h:372: DW_LANG_Go = 0x8015,	// TODO(lvd) Temporary
On 2010/08/17 05:56:42, rsc1 wrote:
> why 0x8015 and not 0x8000?
> ask iant what he is using.
> 

8000 is a common number, and 15 is the future intended value.  but i'll ask
iant.  i should probably look at what gccgo outputs anyway.

[rsc, 2010-08-17 16:05:48]

On Tue, Aug 17, 2010 at 04:05,  <lvd@google.com> wrote:
> i didn't buy the 'distraction by indentation' argument when i joined
> google and i don't buy it from you, and i think you just made me waste 2
> hours i could have spent on getting the stackframes working, but i'm a
> good boy and i have done your bidding, as i will in the future.

we believe in this so much that robert even took the time to
automate the process for go.  i shouldn't be able to tell which
lines you wrote and which lines other people wrote.  again, sorry.
i know it's a pain but it pays off in the long run.  (i've been working
with this code for fifteen years.)

russ

[lvd, 2010-08-17 16:18:23]

On Tue, Aug 17, 2010 at 18:05, Russ Cox <rsc@golang.org> wrote:

> On Tue, Aug 17, 2010 at 04:05,  <lvd@google.com> wrote:
> > i didn't buy the 'distraction by indentation' argument when i joined
> > google and i don't buy it from you, and i think you just made me waste 2
> > hours i could have spent on getting the stackframes working, but i'm a
> > good boy and i have done your bidding, as i will in the future.
>
> we believe in this so much that robert even took the time to
> automate the process for go.  i shouldn't be able to tell which
> lines you wrote and which lines other people wrote.  again, sorry.
> i know it's a pain but it pays off in the long run.  (i've been working
> with this code for fifteen years.)
>

cool.  I've been not distracted by whitespace for at least 15 years by not
being distracted by whitespace for 15 years, and i happily internalize
whatever anyone insists.

i thought gri wrote gofmt to avoid the discussions about it, not because you
believe in 1 style.  which reminds me: why not just specify the indent
settings?

happy to adopt whatever style you wish, be it the one of cmd/ld/*.c,
pkg/runtime/*.c or gccgo/lib/go/runtme/go-*.c.  i find it totally easy to
not care that i can see they were written by different people.

enough said.   on with the review.


>
> russ
>

[rsc1, 2010-08-17 23:44:06]

LGTM

few more style things; fix and submit

http://codereview.appspot.com/1987042/diff/28001/29005
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/1987042/diff/28001/29005#newcode134
src/cmd/ld/dwarf.c:134: uint8 c, s;
blank line after this

http://codereview.appspot.com/1987042/diff/28001/29005#newcode301
src/cmd/ld/dwarf.c:301: ++frag;
frag++

http://codereview.appspot.com/1987042/diff/28001/29005#newcode336
src/cmd/ld/dwarf.c:336: ++s;
s++

http://codereview.appspot.com/1987042/diff/28001/29005#newcode368
src/cmd/ld/dwarf.c:368: addhistfile(char* zentry)
char *zentry

and char *fname two lines later

http://codereview.appspot.com/1987042/diff/28001/29005#newcode394
src/cmd/ld/dwarf.c:394: int file;
tabs for the next ten lines or so

http://codereview.appspot.com/1987042/diff/28001/29005#newcode408
src/cmd/ld/dwarf.c:408: static Linehist* linehist;
Linehist *linehist

http://codereview.appspot.com/1987042/diff/28001/29005#newcode416
src/cmd/ld/dwarf.c:416: diag("Corrupt z stack.");
no capital; no period

corrupt z stack

http://codereview.appspot.com/1987042/diff/28001/29005#newcode420
src/cmd/ld/dwarf.c:420: diag("Nesting too deep to generate debug info, try
linking with -s.");
diag("nesting too deep")
is fine.  shouldn't happen.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode466
src/cmd/ld/dwarf.c:466: --includetop;
includetop--

http://codereview.appspot.com/1987042/diff/28001/29005#newcode470
src/cmd/ld/dwarf.c:470: ++includetop;
includetop++

http://codereview.appspot.com/1987042/diff/28001/29005#newcode686
src/cmd/ld/dwarf.c:686: diag("Corrupt history or bad absolute line: %P", q);
corrupt

[lvd, 2010-08-18 07:40:59]

all done, but before i submit, i'll check if this doesn't slow down src/all.bash
noticably, and if the size of hello.go's 6.out isn't through the roof.   if so,
a flag might be prudent anyway.

the dwarf line/pc program is not as compact as the plan9 one.

http://codereview.appspot.com/1987042/diff/28001/29005
File src/cmd/ld/dwarf.c (right):

http://codereview.appspot.com/1987042/diff/28001/29005#newcode134
src/cmd/ld/dwarf.c:134: uint8 c, s;
On 2010/08/17 23:44:06, rsc1 wrote:
> blank line after this
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode301
src/cmd/ld/dwarf.c:301: ++frag;
On 2010/08/17 23:44:06, rsc1 wrote:
> frag++
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode336
src/cmd/ld/dwarf.c:336: ++s;
On 2010/08/17 23:44:06, rsc1 wrote:
> s++
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode368
src/cmd/ld/dwarf.c:368: addhistfile(char* zentry)
On 2010/08/17 23:44:06, rsc1 wrote:
> char *zentry
> 
> and char *fname two lines later
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode394
src/cmd/ld/dwarf.c:394: int file;
On 2010/08/17 23:44:06, rsc1 wrote:
> tabs for the next ten lines or so
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode408
src/cmd/ld/dwarf.c:408: static Linehist* linehist;
On 2010/08/17 23:44:06, rsc1 wrote:
> Linehist *linehist
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode416
src/cmd/ld/dwarf.c:416: diag("Corrupt z stack.");
On 2010/08/17 23:44:06, rsc1 wrote:
> no capital; no period
> 
> corrupt z stack
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode420
src/cmd/ld/dwarf.c:420: diag("Nesting too deep to generate debug info, try
linking with -s.");
On 2010/08/17 23:44:06, rsc1 wrote:
> diag("nesting too deep")
> is fine.  shouldn't happen.
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode466
src/cmd/ld/dwarf.c:466: --includetop;
On 2010/08/17 23:44:06, rsc1 wrote:
> includetop--
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode470
src/cmd/ld/dwarf.c:470: ++includetop;
On 2010/08/17 23:44:06, rsc1 wrote:
> includetop++
> 

Done.

http://codereview.appspot.com/1987042/diff/28001/29005#newcode686
src/cmd/ld/dwarf.c:686: diag("Corrupt history or bad absolute line: %P", q);
On 2010/08/17 23:44:06, rsc1 wrote:
> corrupt
> 

Done.

[lvd, 2010-08-18 10:00:33]

no noticable tijming differences on make.bash (but i guess 6l's part of that
isnt dominating).

compared the -v timing output for some small programs: no noticable
difference, but the resolution is only 10ms on a total time of 330ms.

the dwarf line table is roughly 2x the size of the gopclntab.  i may look
into tweaking some of the parameters in its generation.  i expect my dwarf
linecount generation and the asmlc to have roughly the same runtime.

so i guess a flag isn't neccesary indeed.

i'd like to try gdb-ing a larger program for sanity checking before
submitting.  any good candidates?

/Luuk

On Wed, Aug 18, 2010 at 09:40, <lvd@google.com> wrote:

> all done, but before i submit, i'll check if this doesn't slow down
> src/all.bash noticably, and if the size of hello.go's 6.out isn't
> through the roof.   if so, a flag might be prudent anyway.
>
> the dwarf line/pc program is not as compact as the plan9 one.
>
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005
> File src/cmd/ld/dwarf.c (right):
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode134
> src/cmd/ld/dwarf.c:134: uint8 c, s;
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> blank line after this
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode301
> src/cmd/ld/dwarf.c:301: ++frag;
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> frag++
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode336
> src/cmd/ld/dwarf.c:336: ++s;
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> s++
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode368
> src/cmd/ld/dwarf.c:368: addhistfile(char* zentry)
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> char *zentry
>>
>
>  and char *fname two lines later
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode394
> src/cmd/ld/dwarf.c:394: int file;
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> tabs for the next ten lines or so
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode408
> src/cmd/ld/dwarf.c:408: static Linehist* linehist;
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> Linehist *linehist
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode416
> src/cmd/ld/dwarf.c:416: diag("Corrupt z stack.");
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> no capital; no period
>>
>
>  corrupt z stack
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode420
> src/cmd/ld/dwarf.c:420: diag("Nesting too deep to generate debug info,
> try linking with -s.");
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> diag("nesting too deep")
>> is fine.  shouldn't happen.
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode466
> src/cmd/ld/dwarf.c:466: --includetop;
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> includetop--
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode470
> src/cmd/ld/dwarf.c:470: ++includetop;
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> includetop++
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/diff/28001/29005#newcode686
> src/cmd/ld/dwarf.c:686: diag("Corrupt history or bad absolute line: %P",
> q);
> On 2010/08/17 23:44:06, rsc1 wrote:
>
>> corrupt
>>
>
>
> Done.
>
>
> http://codereview.appspot.com/1987042/
>

[lvd, 2010-08-18 10:11:20]

On Wed, Aug 18, 2010 at 12:00, Luuk van Dijk <lvd@google.com> wrote:

> no noticable tijming differences on make.bash (but i guess 6l's part of
> that isnt dominating).


same for gomake after clean in src/pkg, which isn't dominated by gcc.


>
> compared the -v timing output for some small programs: no noticable
> difference, but the resolution is only 10ms on a total time of 330ms.
>
> the dwarf line table is roughly 2x the size of the gopclntab.  i may look
> into tweaking some of the parameters in its generation.  i expect my dwarf
> linecount generation and the asmlc to have roughly the same runtime.
>
> so i guess a flag isn't neccesary indeed.
>
> i'd like to try gdb-ing a larger program for sanity checking before
> submitting.  any good candidates?
>
> /Luuk
>
>
> On Wed, Aug 18, 2010 at 09:40, <lvd@google.com> wrote:
>
>> all done, but before i submit, i'll check if this doesn't slow down
>> src/all.bash noticably, and if the size of hello.go's 6.out isn't
>> through the roof.   if so, a flag might be prudent anyway.
>>
>> the dwarf line/pc program is not as compact as the plan9 one.
>>
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005
>> File src/cmd/ld/dwarf.c (right):
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode134
>> src/cmd/ld/dwarf.c:134: uint8 c, s;
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> blank line after this
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode301
>> src/cmd/ld/dwarf.c:301: ++frag;
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> frag++
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode336
>> src/cmd/ld/dwarf.c:336: ++s;
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> s++
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode368
>> src/cmd/ld/dwarf.c:368: addhistfile(char* zentry)
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> char *zentry
>>>
>>
>>  and char *fname two lines later
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode394
>> src/cmd/ld/dwarf.c:394: int file;
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> tabs for the next ten lines or so
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode408
>> src/cmd/ld/dwarf.c:408: static Linehist* linehist;
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> Linehist *linehist
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode416
>> src/cmd/ld/dwarf.c:416: diag("Corrupt z stack.");
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> no capital; no period
>>>
>>
>>  corrupt z stack
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode420
>> src/cmd/ld/dwarf.c:420: diag("Nesting too deep to generate debug info,
>> try linking with -s.");
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> diag("nesting too deep")
>>> is fine.  shouldn't happen.
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode466
>> src/cmd/ld/dwarf.c:466: --includetop;
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> includetop--
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode470
>> src/cmd/ld/dwarf.c:470: ++includetop;
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> includetop++
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/diff/28001/29005#newcode686
>> src/cmd/ld/dwarf.c:686: diag("Corrupt history or bad absolute line: %P",
>> q);
>> On 2010/08/17 23:44:06, rsc1 wrote:
>>
>>> corrupt
>>>
>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/1987042/
>>
>
>

[rsc, 2010-08-18 12:55:57]

> i'd like to try gdb-ing a larger program for sanity checking before
> submitting.  any good candidates?

godoc is the large program.

russ

[lvd, 2010-08-18 14:03:22]

works not unreasonable.  stepping into a go statement jumps into closure.c
and it would be good if i got bt working asap, but we're getting somewhere.

On Wed, Aug 18, 2010 at 14:55, Russ Cox <rsc@golang.org> wrote:

> > i'd like to try gdb-ing a larger program for sanity checking before
> > submitting.  any good candidates?
>
> godoc is the large program.
>
> russ
>

[rsc, 2010-08-18 14:20:23]

you should submit what you have and continue
work in a different CL.

On Wed, Aug 18, 2010 at 10:03, Luuk van Dijk <lvd@google.com> wrote:
> works not unreasonable.  stepping into a go statement jumps into closure.c
> and it would be good if i got bt working asap, but we're getting somewhere.
>
> On Wed, Aug 18, 2010 at 14:55, Russ Cox <rsc@golang.org> wrote:
>>
>> > i'd like to try gdb-ing a larger program for sanity checking before
>> > submitting.  any good candidates?
>>
>> godoc is the large program.
>>
>> russ
>
>

[lvd, 2010-08-18 14:53:33]

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

Dwarf output for 6l.
Part 1, general scaffolding and pc/lc sections.

R=rsc, r, ken2
CC=golang-dev
http://codereview.appspot.com/1987042