code review 2124041: 8l: emit DWARF in Windows PE.

8l: emit DWARF in Windows PE.

[vcc, 2010-09-01 16:31:28]

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

I'd like you to review this change.

[lvd, 2010-09-01 16:35:15]

as a general comment, i'd prefer it if you could add a function to emit the pe
headers to dwarf.c

for macho on mac os, i'm doing exactly that: call dwarfemitdebugsections() from
asm.c in the right place, and call dwarfaddmachoheaders() from macho.c.  i will
rename dwarfaddheaders() to dwarfaddelfheaders().

that way all the size and offsets of the sections can remain static in dwarf.c

[lvd, 2010-09-01 16:36:41]

see http://codereview.appspot.com/2123041/ for how the macho stuff is taking
shape, except that it doesnt work yet.  ignore the if(iself) modifications,
those will go.

/L

On Wed, Sep 1, 2010 at 18:35, <lvd@google.com> wrote:

> as a general comment, i'd prefer it if you could add a function to emit
> the pe headers to dwarf.c
>
> for macho on mac os, i'm doing exactly that: call
> dwarfemitdebugsections() from asm.c in the right place, and call
> dwarfaddmachoheaders() from macho.c.  i will rename dwarfaddheaders() to
> dwarfaddelfheaders().
>
> that way all the size and offsets of the sections can remain static in
> dwarf.c
>
>
> http://codereview.appspot.com/2124041/
>

[rsc1, 2010-09-01 16:41:37]

I'd like to hold off on this CL.
The DWARF support is still very much in flux.
Let's wait until it is done before we complicate
it with another architecture.

Also, why DWARF on Windows?
Wouldn't it make more sense to use
whatever the native Windows format is?

[vcc, 2010-09-01 16:53:42]

2010/9/2 <lvd@google.com>

> as a general comment, i'd prefer it if you could add a function to emit
> the pe headers to dwarf.c
>
> for macho on mac os, i'm doing exactly that: call
> dwarfemitdebugsections() from asm.c in the right place, and call
> dwarfaddmachoheaders() from macho.c.  i will rename dwarfaddheaders() to
> dwarfaddelfheaders().
>
> that way all the size and offsets of the sections can remain static in
> dwarf.c
>
>
OK, thanks.


>
> http://codereview.appspot.com/2124041/
>

[vcc, 2010-09-01 16:54:10]

2010/9/2 <lvd@google.com>

> as a general comment, i'd prefer it if you could add a function to emit
> the pe headers to dwarf.c
>
> for macho on mac os, i'm doing exactly that: call
> dwarfemitdebugsections() from asm.c in the right place, and call
> dwarfaddmachoheaders() from macho.c.  i will rename dwarfaddheaders() to
> dwarfaddelfheaders().
>
> that way all the size and offsets of the sections can remain static in
> dwarf.c
>
>
OK, thanks.


>
> http://codereview.appspot.com/2124041/
>

[vcc, 2010-09-01 16:59:03]

2010/9/2 <rsc@google.com>

> Also, why DWARF on Windows?
> Wouldn't it make more sense to use
> whatever the native Windows format is?
>
>
Since we use mingw for windows port, so I think use gdb for debug is useful.


>
>
> http://codereview.appspot.com/2124041/
>

[vcc, 2010-09-01 17:20:30]

2010/9/2 <rsc@google.com>

> Also, why DWARF on Windows?
> Wouldn't it make more sense to use
> whatever the native Windows format is?
>
>
Since we use mingw for windows port, so I think use gdb for debug is useful.


>
>
> http://codereview.appspot.com/2124041/
>

[vcc, 2010-09-01 17:21:27]

2010/9/2 <rsc@google.com>

> Also, why DWARF on Windows?
> Wouldn't it make more sense to use
> whatever the native Windows format is?
>
>
Since we use mingw for windows port, so I think use gdb for debug is useful.


>
>
> http://codereview.appspot.com/2124041/
>

[vcc, 2010-09-01 17:21:48]

2010/9/2 <rsc@google.com>

> Also, why DWARF on Windows?
> Wouldn't it make more sense to use
> whatever the native Windows format is?
>
>
Since we use mingw for windows port, so I think use gdb for debug is useful.


>
>
> http://codereview.appspot.com/2124041/
>

[Joe Poirier, 2010-09-02 00:27:39]

On Wed, Sep 1, 2010 at 11:49 AM, Wei guangjing <vcc.163@gmail.com> wrote:
> 2010/9/2 <rsc@google.com>
>>
>> Also, why DWARF on Windows?
>> Wouldn't it make more sense to use
>> whatever the native Windows format is?
>
> Since we use mingw for windows port, so I think use gdb for debug is useful.
>

FWIW -

mingw provides a crt and a gcc distribution. gcc supports several debugging
formats along with an option for (extra) gdb specific debug information. mingw
and its crt can be recompiled to include any of the debugging formats that gcc
supports.

The only thing tying the windows port to mingw are the __MINGWXX__ compile
switches, which at some point should be changed to __WINDOWS__.

gcc is required to build Go on windows but it doesn't necessarily have
to be via
mingw.

-joe

[brainman, 2010-09-02 00:46:28]

On 2010/09/01 16:31:28, vcc wrote:
> I'd like you to review this change.

I think before we fight this fight, we need to understand what we're trying to
achieve here. When someone proposes a "solution", I always ask myself, what are
we solving here.

I understand, that everyone wants to have ability to "debug" go programs. But
what does it means? Does it means "use gdb", or does it mean "use Microsoft
debugging tools", or whatever?

I don't think we're clear on that for more mature platforms, let along Windows.
Perhaps, we could see how story unfolds there, before we make any decision here.


Alex

[rsc, 2010-09-02 01:09:45]

> I don't think we're clear on that for more mature platforms, let along
> Windows. Perhaps, we could see how story unfolds there, before we make
> any decision here.

Just to be fair, I think the situation is much clearer
on the Unix platforms: all the tools expect debugging
information in ELF and DWARF format, so that's what
we must provide.  The situation on Windows is definitely
more murky.

Russ

[vcc, 2010-09-02 03:34:33]

2010/9/2 Russ Cox <rsc@golang.org>

>
> Just to be fair, I think the situation is much clearer
> on the Unix platforms: all the tools expect debugging
> information in ELF and DWARF format, so that's what
> we must provide.  The situation on Windows is definitely
> more murky.
>

We can support DWARF and Native windows format both use command line
options, for example use -g for DWARF, -d for native windows format.

Wei guangjing

[Joe Poirier, 2010-09-02 03:42:02]

On Wed, Sep 1, 2010 at 8:09 PM, Russ Cox <rsc@golang.org> wrote:
>> I don't think we're clear on that for more mature platforms, let along
>> Windows. Perhaps, we could see how story unfolds there, before we make
>> any decision here.
>
> Just to be fair, I think the situation is much clearer
> on the Unix platforms: all the tools expect debugging
> information in ELF and DWARF format, so that's what
> we must provide.  The situation on Windows is definitely
> more murky.
>

My previous comment was just to clarify how mingw fit in to
the windows port, but I guess I'll add my $.02.

As far as I know, there aren't that many (good) source level
debuggers for windows, so the format question gets narrowed down
considerably. And IMHO, it's more a question of, will the windows
port support a debug format that can be used with a MS windows
debugger or not?

The two MS windows debuggers are VC++ and WinDB. The two
non-propriety debug formats they support are DBG and COFF,
which are pretty old formats. I know COFF has some limitations,
eg data representation, not sure about DBG. Oh, and I believe
the Pelles IDE also supports DBG and COFF. What about other
debuggers that support either of these two formats?

Then there are PGDBG (Portland Group) and GDB, which both
support the DWARF format. GDB also supports, correct me if I'm
wrong, COFF, and a few others formats as well. Are there any
other windows debuggers that support DWARF on windows?

WinDB, VC++, PGDBG, and GDB are all great debuggers; I've
never used the Pelles IDE. GDB, and most front-ends for it, are
free as is WinDB. There's a free version of VC++ that includes
the debugger (I think remote debugging is disabled) but
PGDBG cost money.

Can anyone bullet the deltas between DWARF and COFF?

-joe

Visual C++ and WinDB (supports all 3 formats?)
----------------------------------------------------------------------
COFF
DBG (CodeView)
PDB (Program Database, proprietary as far as I know)

Pelles
--------
COFF and DBG

The Portland Group PGDBG
-----------------------------------------
DWARF

GDB
-------
DWARF, COFF, others

[vcc, 2010-09-06 14:57:31]

move code to dwarf.c, please take another look.

[lvd, 2010-09-06 15:01:53]

I like that much better, thanks.

do you mind if we keep this on hold for a bit until i have the frame info
and the data inspection working?

also, i'm not sure what the conclusion was on the discussion around dwarf vs
coff vs other.

regards/Luuk van Dijk

On Mon, Sep 6, 2010 at 16:57, <vcc.163@gmail.com> wrote:

> move code to dwarf.c, please take another look.
>
>
> http://codereview.appspot.com/2124041/
>

[vcc, 2010-09-06 15:20:31]

2010/9/6 Luuk van Dijk <lvd@google.com>

> I like that much better, thanks.
>
> do you mind if we keep this on hold for a bit until i have the frame info
> and the data inspection working?
>
>
OK, no problem.

Thanks.

Wei guangjing



> also, i'm not sure what the conclusion was on the discussion around dwarf
> vs coff vs other.
>
> regards/Luuk van Dijk
>
>
> On Mon, Sep 6, 2010 at 16:57, <vcc.163@gmail.com> wrote:
>
>> move code to dwarf.c, please take another look.
>>
>>
>> http://codereview.appspot.com/2124041/
>>
>
>

[vcc, 2010-10-25 15:15:45]

Update code for global and local variables and type info, please take another
look.

Debug is very useful, I can't finish windows x64 port without this support,
thanks Luuk van Dijk!

[lvd, 2010-10-25 15:24:52]

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

http://codereview.appspot.com/2124041/diff/36001/src/cmd/ld/dwarf.c#newcode2028
src/cmd/ld/dwarf.c:2028: debug_abbrev = newPESection("/4", abbrevsize, 0);
Can't you add a newPESection() variant that takes the abbrevo and the other
static ...o variables in this file and constructs the right headers?  after you
call  dwarfemitdebugsections() like everybody else?  This looks like unfortunate
replication of most of that function.

[vcc, 2010-10-25 16:42:52]

Thanks for review, please take another look.

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

http://codereview.appspot.com/2124041/diff/36001/src/cmd/ld/dwarf.c#newcode2028
src/cmd/ld/dwarf.c:2028: debug_abbrev = newPESection("/4", abbrevsize, 0);
On 2010/10/25 15:24:52, lvd wrote:
> Can't you add a newPESection() variant that takes the abbrevo and the other
> static ...o variables in this file and constructs the right headers?  after
you
> call  dwarfemitdebugsections() like everybody else?  This looks like
unfortunate
> replication of most of that function.

Done.

[lvd, 2010-10-25 19:17:03]

i don't see much difference. did you hg upload?

On Mon, Oct 25, 2010 at 18:42, <vcc.163@gmail.com> wrote:

> Thanks for review, please take another look.
>
>
>
> http://codereview.appspot.com/2124041/diff/36001/src/cmd/ld/dwarf.c
> File src/cmd/ld/dwarf.c (right):
>
>
>
http://codereview.appspot.com/2124041/diff/36001/src/cmd/ld/dwarf.c#newcode2028
> src/cmd/ld/dwarf.c:2028: debug_abbrev = newPESection("/4", abbrevsize,
> 0);
> On 2010/10/25 15:24:52, lvd wrote:
>
>> Can't you add a newPESection() variant that takes the abbrevo and the
>>
> other
>
>> static ...o variables in this file and constructs the right headers?
>>
> after you
>
>> call  dwarfemitdebugsections() like everybody else?  This looks like
>>
> unfortunate
>
>> replication of most of that function.
>>
>
> Done.
>
>
> http://codereview.appspot.com/2124041/
>

[vcc, 2010-10-26 04:25:10]

2010/10/26 Luuk van Dijk <lvd@google.com>

> i don't see much difference. did you hg upload?
>
>
Yes, I do hg upload, and I can see the new changeset.

change as follow:
diff --git a/src/cmd/ld/dwarf.c b/src/cmd/ld/dwarf.c
--- a/src/cmd/ld/dwarf.c
+++ b/src/cmd/ld/dwarf.c
@@ -17,6 +17,7 @@
 #include    "../ld/dwarf_defs.h"
 #include    "../ld/elf.h"
 #include    "../ld/macho.h"
+#include    "../ld/pe.h"

 /*
  * Offsets and sizes of the debug_* sections in the cout file.
@@ -1983,3 +1984,32 @@
         ms->filesize += msect->size;
     }
 }
+
+/*
+ * Windows PE
+ */
+void
+dwarfaddpeheaders(void)
+{
+    vlong off;
+    off = seek(cout, 0, 1);
+    seek(cout, 0, 2);
+
+    dwarfemitdebugsections();
+    newPEDWARFSection("/4", abbrevo, abbrevsize);
+    newPEDWARFSection("/30", lineo, linesize);
+    newPEDWARFSection("/42", frameo, framesize);
+    newPEDWARFSection("/18", infoo, infosize);
+    if (pubnamessize > 0) {
+        newPEDWARFSection("/55", pubnameso, pubnamessize);
+    }
+    if (pubtypessize > 0) {
+        newPEDWARFSection("/71", pubtypeso, pubtypessize);
+    }
+    if (arangessize > 0) {
+        newPEDWARFSection("/87", arangeso, arangessize);
+    }
+    newPESymbolStringTable();
+
+    seek(cout, off, 0);
+}
diff --git a/src/cmd/ld/dwarf.h b/src/cmd/ld/dwarf.h
--- a/src/cmd/ld/dwarf.h
+++ b/src/cmd/ld/dwarf.h
@@ -27,3 +27,4 @@
  */
 void dwarfaddelfheaders(void);
 void dwarfaddmachoheaders(void);
+void dwarfaddpeheaders(void);
diff --git a/src/cmd/ld/pe.c b/src/cmd/ld/pe.c
--- a/src/cmd/ld/pe.c
+++ b/src/cmd/ld/pe.c
@@ -10,6 +10,7 @@
 #include "l.h"
 #include "../ld/lib.h"
 #include "../ld/pe.h"
+#include "../ld/dwarf.h"

 // DOS stub that prints out
 // "This program cannot be run in DOS mode."
@@ -44,7 +45,7 @@
 static IMAGE_SECTION_HEADER *textsect, *datsect, *bsssect;

 static IMAGE_SECTION_HEADER*
-new_section(char *name, int size, int noraw)
+newPESection(char *name, int size, int noraw)
 {
     IMAGE_SECTION_HEADER *h;

@@ -103,8 +104,8 @@
 void
 dope(void)
 {
-    textsect = new_section(".text", segtext.len, 0);
+    textsect = newPESection(".text", segtext.len, 0);
     textsect->Characteristics = IMAGE_SCN_CNT_CODE|
         IMAGE_SCN_CNT_INITIALIZED_DATA|
         IMAGE_SCN_MEM_EXECUTE|IMAGE_SCN_MEM_READ;

@@ -107,10 +108,10 @@
     textsect->Characteristics = IMAGE_SCN_CNT_CODE|
         IMAGE_SCN_CNT_INITIALIZED_DATA|
         IMAGE_SCN_MEM_EXECUTE|IMAGE_SCN_MEM_READ;

-    datsect = new_section(".data", segdata.filelen, 0);
+    datsect = newPESection(".data", segdata.filelen, 0);
     datsect->Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA|
         IMAGE_SCN_MEM_READ|IMAGE_SCN_MEM_WRITE;
     if(segdata.vaddr != PEBASE+datsect->VirtualAddress)
         diag("segdata.vaddr = %#llux, want %#llux", (vlong)segdata.vaddr,
(vlong)(PEBASE+datsect->VirtualAddress));

@@ -112,9 +113,9 @@
     datsect->Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA|
         IMAGE_SCN_MEM_READ|IMAGE_SCN_MEM_WRITE;
     if(segdata.vaddr != PEBASE+datsect->VirtualAddress)
         diag("segdata.vaddr = %#llux, want %#llux", (vlong)segdata.vaddr,
(vlong)(PEBASE+datsect->VirtualAddress));

-    bsssect = new_section(".bss", segdata.len - segdata.filelen, 1);
+    bsssect = newPESection(".bss", segdata.len - segdata.filelen, 1);
     bsssect->Characteristics = IMAGE_SCN_CNT_UNINITIALIZED_DATA|
         IMAGE_SCN_MEM_READ|IMAGE_SCN_MEM_WRITE;
 }
@@ -155,7 +156,7 @@
         size += sizeof(fs[0].thunk);

     IMAGE_SECTION_HEADER *isect;
-    isect = new_section(".idata", size, 0);
+    isect = newPESection(".idata", size, 0);
     isect->Characteristics = IMAGE_SCN_CNT_INITIALIZED_DATA|
         IMAGE_SCN_MEM_READ|IMAGE_SCN_MEM_WRITE;

@@ -211,6 +212,10 @@
     strnput("", rnd(eof, INITRND) - eof);

     add_import_table();
+
+    if (!debug['s']) {
+        dwarfaddpeheaders();
+    }

     fh.NumberOfSections = nsect;
     fh.TimeDateStamp = time(0);
@@ -250,3 +255,45 @@

     pewrite();
 }
+
+IMAGE_SECTION_HEADER*
+newPEDWARFSection(char *name, vlong off, vlong size)
+{
+    IMAGE_SECTION_HEADER *h;
+
+    if(nsect == 16) {
+        diag("too many sections");
+        errorexit();
+    }
+    h = &sh[nsect++];
+    strncpy((char*)h->Name, name, sizeof(h->Name));
+    h->VirtualSize = size;
+    if(!sect_virt_begin)
+        sect_virt_begin = 0x1000;
+    h->VirtualAddress = sect_virt_begin;
+    sect_virt_begin = rnd(sect_virt_begin+size, 0x1000);
+    h->SizeOfRawData = size;
+    h->PointerToRawData = off;
+    h->Characteristics = IMAGE_SCN_MEM_READ|
+        IMAGE_SCN_ALIGN_1BYTES|
+        IMAGE_SCN_MEM_DISCARDABLE;
+    sect_raw_begin += h->SizeOfRawData;
+    return h;
+}
+
+void
+newPESymbolStringTable(void)
+{
+    fh.PointerToSymbolTable = cpos();
+    fh.NumberOfSymbols = 0;
+    // put symbol string table
+    lputl(102);
+    strput(".debug_abbrev");
+    strput(".debug_info");
+    strput(".debug_line");
+    strput(".debug_frame");
+    strput(".debug_pubnames");
+    strput(".debug_pubtypes");
+    strput(".debug_aranges");
+    cflush();
+}
diff --git a/src/cmd/ld/pe.h b/src/cmd/ld/pe.h
--- a/src/cmd/ld/pe.h
+++ b/src/cmd/ld/pe.h
@@ -92,6 +92,9 @@
     IMAGE_SCN_MEM_EXECUTE = 0x20000000,
     IMAGE_SCN_MEM_READ = 0x40000000,
     IMAGE_SCN_MEM_WRITE = 0x80000000,
+    IMAGE_SCN_ALIGN_1BYTES = 0x100000,
+    IMAGE_SCN_ALIGN_4BYTES = 0x300000,
+    IMAGE_SCN_MEM_DISCARDABLE = 0x2000000,

     IMAGE_DIRECTORY_ENTRY_EXPORT = 0,
     IMAGE_DIRECTORY_ENTRY_IMPORT = 1,
@@ -114,3 +117,6 @@
 void peinit(void);
 void dope(void);
 void asmbpe(void);
+
+IMAGE_SECTION_HEADER* newPEDWARFSection(char *name, vlong off, vlong size);
+void newPESymbolStringTable(void);


> On Mon, Oct 25, 2010 at 18:42, <vcc.163@gmail.com> wrote:
>
>> Thanks for review, please take another look.
>>
>>
>>
>> http://codereview.appspot.com/2124041/diff/36001/src/cmd/ld/dwarf.c
>> File src/cmd/ld/dwarf.c (right):
>>
>>
>>
http://codereview.appspot.com/2124041/diff/36001/src/cmd/ld/dwarf.c#newcode2028
>> src/cmd/ld/dwarf.c:2028: debug_abbrev = newPESection("/4", abbrevsize,
>> 0);
>> On 2010/10/25 15:24:52, lvd wrote:
>>
>>> Can't you add a newPESection() variant that takes the abbrevo and the
>>>
>> other
>>
>>> static ...o variables in this file and constructs the right headers?
>>>
>> after you
>>
>>> call  dwarfemitdebugsections() like everybody else?  This looks like
>>>
>> unfortunate
>>
>>> replication of most of that function.
>>>
>>
>> Done.
>>
>>
>> http://codereview.appspot.com/2124041/
>>
>
>

[lvd, 2010-10-28 15:37:27]

from my point of view this is nearly ok, see comments below.  i defer to rsc to
decide about whether dwarf for windows/PE is desirable at this point.

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

http://codereview.appspot.com/2124041/diff/43001/src/cmd/ld/dwarf.c#newcode1994
src/cmd/ld/dwarf.c:1994: vlong off;
a blank line here

http://codereview.appspot.com/2124041/diff/43001/src/cmd/ld/dwarf.c#newcode1998
src/cmd/ld/dwarf.c:1998: dwarfemitdebugsections();
the idea is to call dwarfemitdebugsections from the appropriate place in asm.c
and then call dwarfadd*headers() separately.  if you could make that so, it
would be nicely parallel for all output formats.

[rsc1, 2010-11-01 20:30:41]

please add the blank line that lvd pointed out
and i will submit.  i don't mind the section setup
being different on windows, since more of the
output is in pe.c here.

[vcc, 2010-11-02 02:54:03]

Please take another look.

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

http://codereview.appspot.com/2124041/diff/43001/src/cmd/ld/dwarf.c#newcode1994
src/cmd/ld/dwarf.c:1994: vlong off;
On 2010/10/28 15:37:27, lvd wrote:
> a blank line here

Done, remove these unnecessary var and seek operations.

[brainman, 2010-11-02 22:05:47]

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

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
Why do you need to have so many separate pe sections? Who uses them and how? Why
do your pe sections have those funny names "/4", "/30" and so on?

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode259
src/cmd/ld/pe.c:259: newPEDWARFSection(char *name, vlong off, vlong size)
Since you're adding pe section here, you should use addpesection() function
instead of repeating everything it does.

[vcc, 2010-11-03 03:34:50]

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

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
On 2010/11/02 22:05:48, brainman wrote:
> Why do you need to have so many separate pe sections? Who uses them and how?
Why
> do your pe sections have those funny names "/4", "/30" and so on?

These are .debug_abbrev, .debug_line, ... For more than 8 characters section
names, will contains a slash (/) followed by ASCII representation of a decimal
number: this number is an offset into the string table.

please seek PE format Specification for details.

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode259
src/cmd/ld/pe.c:259: newPEDWARFSection(char *name, vlong off, vlong size)
On 2010/11/02 22:05:48, brainman wrote:
> Since you're adding pe section here, you should use addpesection() function
> instead of repeating everything it does.

Dwarf sections use 1 byte align, so don't need align raw data, I don't want
change exists addpection, so write a new one.

[brainman, 2010-11-03 03:47:27]

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

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
Why do you need to have so many separate pe sections? Who uses them and how?

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode259
src/cmd/ld/pe.c:259: newPEDWARFSection(char *name, vlong off, vlong size)
All PE sections must have same alignment (one for in file, and the other for in
memory), you can't have some sections aligned on 4096 and others on 1. Also 1
byte is not a valid alignment value for either file or memory alignment.

I don't believe you need to have many separate pe sections ("/4", "/30" and so
on) for different parts of dwarf info. I'm not even sure they can't be part of
.text or .data sections we have at this moment.

[vcc, 2010-11-04 04:44:32]

2010/11/3 <alex.brainman@gmail.com>

>
> http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c
> File src/cmd/ld/dwarf.c (right):
>
>
>
http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
> src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
> Why do you need to have so many separate pe sections? Who uses them and
> how?
>

GDB use them.


> http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
> File src/cmd/ld/pe.c (right):
>
> http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode259
> src/cmd/ld/pe.c:259: newPEDWARFSection(char *name, vlong off, vlong
> size)
> All PE sections must have same alignment (one for in file, and the other
> for in memory), you can't have some sections aligned on 4096 and others
> on 1. Also 1 byte is not a valid alignment value for either file or
> memory alignment.
>
>
Virtual address of section align of course  is same as 4096, but raw data
can be 1. It's works, at least works for gdb on dwarf.

I don't believe you need to have many separate pe sections ("/4", "/30"
> and so on) for different parts of dwarf info. I'm not even sure they
> can't be part of .text or .data sections we have at this moment.


GDB need all these sections.


>
>
> http://codereview.appspot.com/2124041/
>

[brainman, 2010-11-09 00:45:48]

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

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
Please, provide some references for what you're doing here. I can't find any
spec anywhere about what you're doing here.

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode275
src/cmd/ld/pe.c:275: IMAGE_SCN_ALIGN_1BYTES|
You can't use this flag in executable. From
http://kishorekumar.net/pecoff_v8.1.htm:

"IMAGE_SCN_ALIGN_1BYTES - Align data on a 1-byte boundary. Valid only for object
files."

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode286
src/cmd/ld/pe.c:286: // put symbol string table
You must put that data in a pe section.

[vcc, 2010-11-15 13:15:57]

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

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
On 2010/11/09 00:45:48, brainman wrote:
> Please, provide some references for what you're doing here. I can't find any
> spec anywhere about what you're doing here.

http://www.microsoft.com/whdc/system/platform/firmware/PECOFFdwn.mspx

Page 24.

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode275
src/cmd/ld/pe.c:275: IMAGE_SCN_ALIGN_1BYTES|
On 2010/11/09 00:45:48, brainman wrote:
> You can't use this flag in executable. From
> http://kishorekumar.net/pecoff_v8.1.htm:
> 
> "IMAGE_SCN_ALIGN_1BYTES - Align data on a 1-byte boundary. Valid only for
object
> files."

Yes, this only valia for object files. I just try it, and seem works.

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode286
src/cmd/ld/pe.c:286: // put symbol string table
On 2010/11/09 00:45:48, brainman wrote:
> You must put that data in a pe section.

put in a pe section would be nice, but just put in the last of file also ok. 

Maybe put these in .data section?

[rsc, 2010-12-07 18:41:53]

seems fine to me.
when alex is happy, LGTM

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

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
On 2010/11/15 13:15:58, vcc wrote:
> On 2010/11/09 00:45:48, brainman wrote:
> > Please, provide some references for what you're doing here. I can't find any
> > spec anywhere about what you're doing here.
> 
> http://www.microsoft.com/whdc/system/platform/firmware/PECOFFdwn.mspx
> 
> Page 24.

Please add this reference to the source code

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1999
src/cmd/ld/dwarf.c:1999: if (pubnamessize > 0) {
can drop the { } for one-line bodies

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode215
src/cmd/ld/pe.c:215: if (!debug['s']) {
drop {}

[brainman, 2010-12-20 00:50:30]

I didn't find any references on how to incorporate dwarf info in pe file, so, I
suppose, we have to just follow an example. I looked at what gcc does, and it is
pretty similar to what you do, with one difference. All their pe sections (dwarf
and others) are PEFILEALIGN aligned. Please, see if you could change your code
to do the same. Then you won't need newPEDWARFSection(), but will be able to use
addpesection() instead, like everything else.

Thank you.

Alex

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

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
There is too much "black magic" for me <g>! Please, replace all these const
("/4", "/30" and others) with code that produce the values.

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode287
src/cmd/ld/pe.c:287: lputl(102);
There is too much "black magic" for me <g>! Please, replace all these const with
code that produce the values.

[vcc, 2010-12-20 13:22:56]

On 2010/12/20 00:50:30, brainman wrote:
> I didn't find any references on how to incorporate dwarf info in pe file, so,
I
> suppose, we have to just follow an example. I looked at what gcc does, and it
is
> pretty similar to what you do, with one difference. All their pe sections
(dwarf
> and others) are PEFILEALIGN aligned. Please, see if you could change your code
> to do the same. Then you won't need newPEDWARFSection(), but will be able to
use
> addpesection() instead, like everything else.

Done.

> 
> Thank you.
> 
> Alex
> 
> http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c
> File src/cmd/ld/dwarf.c (right):
> 
>
http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
> src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
> There is too much "black magic" for me <g>! Please, replace all these const
> ("/4", "/30" and others) with code that produce the values.
> 
> http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
> File src/cmd/ld/pe.c (right):
> 
> http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode287
> src/cmd/ld/pe.c:287: lputl(102);
> There is too much "black magic" for me <g>! Please, replace all these const
with
> code that produce the values.

[vcc, 2010-12-20 13:23:19]

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

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1995
src/cmd/ld/dwarf.c:1995: newPEDWARFSection("/4", abbrevo, abbrevsize);
On 2010/12/07 18:41:53, rsc wrote:
> On 2010/11/15 13:15:58, vcc wrote:
> > On 2010/11/09 00:45:48, brainman wrote:
> > > Please, provide some references for what you're doing here. I can't find
any
> > > spec anywhere about what you're doing here.
> > 
> > http://www.microsoft.com/whdc/system/platform/firmware/PECOFFdwn.mspx
> > 
> > Page 24.
> 
> Please add this reference to the source code
> 

Done.

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/dwarf.c#newcode1999
src/cmd/ld/dwarf.c:1999: if (pubnamessize > 0) {
On 2010/12/07 18:41:53, rsc wrote:
> can drop the { } for one-line bodies

Done.

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/61001/src/cmd/ld/pe.c#newcode215
src/cmd/ld/pe.c:215: if (!debug['s']) {
On 2010/12/07 18:41:53, rsc wrote:
> drop {}

Done.

[vcc, 2010-12-20 13:23:43]

Please take another look.

[brainman, 2010-12-20 23:39:01]

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

http://codereview.appspot.com/2124041/diff/79001/src/cmd/ld/dwarf.c#newcode2574
src/cmd/ld/dwarf.c:2574: newPEDWARFSection("/4", abbrevsize); // "/4" as
".debug_abbrev", For more than 8 characters section names, name contains a slash
(/) that is followed by an ASCII representation of a decimal number that is an
offset into the string table.
I would rather you do

newPEDWARFSection(".debug_abbrev", abbrevsize);

instead, and then do all the magic of converting ".debug_abbrev" into "/4"
inside of newPEDWARFSection(). So people who read your code understand what
happens.

http://codereview.appspot.com/2124041/diff/79001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/79001/src/cmd/ld/pe.c#newcode386
src/cmd/ld/pe.c:386: vlong size = 121;
Where did 121 come from?

I think, you should accumulate all section names (".debug_abbrev",
".debug_info", ...) as newPEDWARFSection() gets called, and then write them all
out here. Then you won't need to hard code any of those names or numbers.

[vcc, 2010-12-21 05:33:07]

On 2010/12/20 23:39:01, brainman wrote:
> http://codereview.appspot.com/2124041/diff/79001/src/cmd/ld/dwarf.c
> File src/cmd/ld/dwarf.c (right):
> 
>
http://codereview.appspot.com/2124041/diff/79001/src/cmd/ld/dwarf.c#newcode2574
> src/cmd/ld/dwarf.c:2574: newPEDWARFSection("/4", abbrevsize); // "/4" as
> ".debug_abbrev", For more than 8 characters section names, name contains a
slash
> (/) that is followed by an ASCII representation of a decimal number that is an
> offset into the string table.
> I would rather you do
> 
> newPEDWARFSection(".debug_abbrev", abbrevsize);
> 
> instead, and then do all the magic of converting ".debug_abbrev" into "/4"
> inside of newPEDWARFSection(). So people who read your code understand what
> happens.
> 
> http://codereview.appspot.com/2124041/diff/79001/src/cmd/ld/pe.c
> File src/cmd/ld/pe.c (right):
> 
> http://codereview.appspot.com/2124041/diff/79001/src/cmd/ld/pe.c#newcode386
> src/cmd/ld/pe.c:386: vlong size = 121;
> Where did 121 come from?
> 
> I think, you should accumulate all section names (".debug_abbrev",
> ".debug_info", ...) as newPEDWARFSection() gets called, and then write them
all
> out here. Then you won't need to hard code any of those names or numbers.

Done.

[vcc, 2010-12-21 05:33:25]

Please take another look.

[brainman, 2010-12-21 05:52:24]

Getting there <g>. Thank you.

Alex

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

http://codereview.appspot.com/2124041/diff/75002/src/cmd/ld/dwarf.c#newcode2586
src/cmd/ld/dwarf.c:2586: newPESymbolStringTable();
You could call newPESymbolStringTable() from asmbpe(), so it is less visible
from outside. Maybe even make its name shorter (addsymtable() or something).

http://codereview.appspot.com/2124041/diff/75002/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/75002/src/cmd/ld/pe.c#newcode391
src/cmd/ld/pe.c:391: 
You should start with empty symnames table (I would make it a linked list
instead). And add new name to it for every call into pesymname(). Then you
should be able to work out integer position of a name as you add it to the
table.

[vcc, 2010-12-21 07:06:58]

Please take another look.

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

http://codereview.appspot.com/2124041/diff/75002/src/cmd/ld/dwarf.c#newcode2586
src/cmd/ld/dwarf.c:2586: newPESymbolStringTable();
On 2010/12/21 05:52:24, brainman wrote:
> You could call newPESymbolStringTable() from asmbpe(), so it is less visible
> from outside. Maybe even make its name shorter (addsymtable() or something).

Done.

http://codereview.appspot.com/2124041/diff/75002/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/75002/src/cmd/ld/pe.c#newcode391
src/cmd/ld/pe.c:391: 
On 2010/12/21 05:52:24, brainman wrote:
> You should start with empty symnames table (I would make it a linked list
> instead). And add new name to it for every call into pesymname(). Then you
> should be able to work out integer position of a name as you add it to the
> table.

Done.

[brainman, 2010-12-22 00:16:08]

It doesn't build:

%%%% making 8l %%%%

quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/cmd/8l/asm.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/data.c
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/dwarf.c
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/elf.c
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/go.c
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/ldelf.c
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/ldmacho.c
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/lib.c
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/cmd/8l/list.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/macho.c
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/cmd/8l/obj.c"
../ld/macho.c:110: warning: 'nexpsym' defined but not used
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/cmd/8l/optab.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/cmd/8l/pass.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/pe.c
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/cmd/8l/prof.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/cmd/8l/span.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c -I. ../ld/symtab.c
../ld/pe.c:355: warning: implicit declaration of function 'dwarfaddpeheaders'
../ld/pe.c:417: warning: implicit declaration of function 'itoa'
quietgcc -o 8l -L"/root/hg/go"/lib asm.o data.o dwarf.o elf.o enam.o go.o
ldelf.o ldmacho.o lib.o list.o macho.o obj.o optab.o pass.o pe.o prof.o span.o
symtab.o   -lbio -l9 -lm
../ld/pe.c:417: undefined reference to `itoa'
collect2: ld returned 1 exit status
make: *** [8l] Error 1

Also, please see my inline comments. Thank you.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode36
src/cmd/ld/pe.c:36: static char *symnames;
I would do linked list myself. But plain large buffer will do the job here too.

The only problem I have with your code is allocation/reallocation - it is
overkill here. Your data (section names) is not going to change often. Just
declare buffer big enough, and fail if adding a section name overflow it. 256
bytes should cover us more then plenty:

static char symname[256];

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode37
src/cmd/ld/pe.c:37: static int symnamessize = 1024;
Won't need that if size of buffer is fixed.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode120
src/cmd/ld/pe.c:120: symnames    = mal(symnamessize);
Won't need that if size of buffer is fixed.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode311
src/cmd/ld/pe.c:311: 
Please, do not create symtable if it is empty - check nextsymoff.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode356
src/cmd/ld/pe.c:356: addsymtable();
I think you should call addsymtable() outside of this if. What if one day you'll
have non-debug related data in symtable?

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode400
src/cmd/ld/pe.c:400: * reference: pecoff_v8.docx
<http://www.microsoft.com/whdc/system/platform/firmware/PECOFFdwn.mspx>, Page
24.
Please, wrap this message as much as you can. My screen is wide, but not that
wide <g>.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode404
src/cmd/ld/pe.c:404: {
This function is too small, please inline it into newPEDWARFSection(). There is
too much happening in this source file as is.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode426
src/cmd/ld/pe.c:426: newPEDWARFSection(char *name, vlong size)
Please, move this function into between dope() and addsymtable(). This way it is
close to addsymtable() function it is related to and in the order of calls.

[vcc, 2010-12-22 02:59:40]

Please take another look.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode36
src/cmd/ld/pe.c:36: static char *symnames;
On 2010/12/22 00:16:08, brainman wrote:
> I would do linked list myself. But plain large buffer will do the job here
too.
> 
> The only problem I have with your code is allocation/reallocation - it is
> overkill here. Your data (section names) is not going to change often. Just
> declare buffer big enough, and fail if adding a section name overflow it. 256
> bytes should cover us more then plenty:
> 
> static char symname[256];

Done.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode37
src/cmd/ld/pe.c:37: static int symnamessize = 1024;
On 2010/12/22 00:16:08, brainman wrote:
> Won't need that if size of buffer is fixed.

Done.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode120
src/cmd/ld/pe.c:120: symnames    = mal(symnamessize);
On 2010/12/22 00:16:08, brainman wrote:
> Won't need that if size of buffer is fixed.

Done.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode311
src/cmd/ld/pe.c:311: 
On 2010/12/22 00:16:08, brainman wrote:
> Please, do not create symtable if it is empty - check nextsymoff.

Done.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode356
src/cmd/ld/pe.c:356: addsymtable();
On 2010/12/22 00:16:08, brainman wrote:
> I think you should call addsymtable() outside of this if. What if one day
you'll
> have non-debug related data in symtable?

Done.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode400
src/cmd/ld/pe.c:400: * reference: pecoff_v8.docx
<http://www.microsoft.com/whdc/system/platform/firmware/PECOFFdwn.mspx>, Page
24.
On 2010/12/22 00:16:08, brainman wrote:
> Please, wrap this message as much as you can. My screen is wide, but not that
> wide <g>.

Done.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode404
src/cmd/ld/pe.c:404: {
On 2010/12/22 00:16:08, brainman wrote:
> This function is too small, please inline it into newPEDWARFSection(). There
is
> too much happening in this source file as is.

Done.

http://codereview.appspot.com/2124041/diff/95001/src/cmd/ld/pe.c#newcode426
src/cmd/ld/pe.c:426: newPEDWARFSection(char *name, vlong size)
On 2010/12/22 00:16:08, brainman wrote:
> Please, move this function into between dope() and addsymtable(). This way it
is
> close to addsymtable() function it is related to and in the order of calls.

Done.

[brainman, 2010-12-22 03:42:25]

http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c#newcode9
src/cmd/ld/pe.c:9: #include <stdlib.h>
It doesn't have itoa() function. It doesn't help. Please, remove.

http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c#newcode317
src/cmd/ld/pe.c:317: 
Should check if you run out of space in symnames and abort here.

http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c#newcode319
src/cmd/ld/pe.c:319: itoa(nextsymoff+4, &s[1], 10);
Build still fails here! You should test before you send things for review.

Perhaps, you could do

sprint(s, "/%d\0", nextsymoff+4);

[vcc, 2010-12-22 04:49:16]

2010/12/22 <alex.brainman@gmail.com>

>
> http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c
>
> File src/cmd/ld/pe.c (right):
>
> http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c#newcode9
> src/cmd/ld/pe.c:9: #include <stdlib.h>
> It doesn't have itoa() function. It doesn't help. Please, remove.
>
>
> http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c#newcode317
> src/cmd/ld/pe.c:317:
> Should check if you run out of space in symnames and abort here.
>
>
> http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c#newcode319
> src/cmd/ld/pe.c:319: itoa(nextsymoff+4, &s[1], 10);
> Build still fails here! You should test before you send things for
> review.
>

I can build it on windows, /mingw/include/stdlib.h has itoa.

Perhaps, you could do
>
> sprint(s, "/%d\0", nextsymoff+4);


I will change to use sprint, thank you.


>
>
> http://codereview.appspot.com/2124041/
>

[vcc, 2010-12-22 05:10:30]

Please take another look.

http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c#newcode9
src/cmd/ld/pe.c:9: #include <stdlib.h>
On 2010/12/22 03:42:25, brainman wrote:
> It doesn't have itoa() function. It doesn't help. Please, remove.

Done.

http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c#newcode317
src/cmd/ld/pe.c:317: 
On 2010/12/22 03:42:25, brainman wrote:
> Should check if you run out of space in symnames and abort here.

Done.

http://codereview.appspot.com/2124041/diff/101001/src/cmd/ld/pe.c#newcode319
src/cmd/ld/pe.c:319: itoa(nextsymoff+4, &s[1], 10);
On 2010/12/22 03:42:25, brainman wrote:
> Build still fails here! You should test before you send things for review.
> 
> Perhaps, you could do
> 
> sprint(s, "/%d\0", nextsymoff+4);

Done.

[brainman, 2010-12-22 05:25:10]

One more.

http://codereview.appspot.com/2124041/diff/105001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/105001/src/cmd/ld/pe.c#newcode317
src/cmd/ld/pe.c:317: if(nextsymoff+strlen(name) > 255) {
I think it should be:

if(nextsymoff+strlen(name)+1 > sizeof(symnames))

[vcc, 2010-12-22 06:37:27]

Please take another look.

http://codereview.appspot.com/2124041/diff/105001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/105001/src/cmd/ld/pe.c#newcode317
src/cmd/ld/pe.c:317: if(nextsymoff+strlen(name) > 255) {
On 2010/12/22 05:25:11, brainman wrote:
> I think it should be:
> 
> if(nextsymoff+strlen(name)+1 > sizeof(symnames))

Done.

[brainman, 2010-12-22 23:29:13]

LGTM. Good job. Thank you.

Russ, would you look over it again, please. It's changed a lot. Thank you.

http://codereview.appspot.com/2124041/diff/110001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/110001/src/cmd/ld/pe.c#newcode318
src/cmd/ld/pe.c:318: diag("too many names");
Maybe better (more specific) error message:

diag("pe string table is full");

[lvd, 2010-12-23 08:51:18]

the dwarf.c and .h LGTM,  thanks. i'm curious if the .debug_gdb_scripts
mechanism will out of the box work for you.

/Luuk

On Thu, Dec 23, 2010 at 00:29, <alex.brainman@gmail.com> wrote:

> LGTM. Good job. Thank you.
>
> Russ, would you look over it again, please. It's changed a lot. Thank
> you.
>
>
> http://codereview.appspot.com/2124041/diff/110001/src/cmd/ld/pe.c
>
> File src/cmd/ld/pe.c (right):
>
>
> http://codereview.appspot.com/2124041/diff/110001/src/cmd/ld/pe.c#newcode318
> src/cmd/ld/pe.c:318: diag("too many names");
> Maybe better (more specific) error message:
>
> diag("pe string table is full");
>
>
> http://codereview.appspot.com/2124041/
>

[brainman, 2010-12-23 23:58:29]

On 2010/12/23 08:51:18, lvd wrote:
> ... i'm curious if the .debug_gdb_scripts
> mechanism will out of the box work for you.
> 

Perhaps it won't work. Looking at gcc compiled c program, it has only those
sections:

debug_abbrev
debug_line
debug_frame
debug_info
debug_pubnames
debug_aranges

Maybe we should not output .debug_gdb_scripts. How can I tell if it is any use?
I could do this on my win2k computer (under mingw):

brainman@ooo /c/TMP/a
$ cat test.go
package main

import (
        "fmt"
)

func main() {
        fmt.Printf("Hello\n")
}

brainman@ooo /c/TMP/a
$ make
8g -o _go_.8 test.go
8l -o test.exe _go_.8

brainman@ooo /c/TMP/a
$ gdb test.exe
GNU gdb (GDB) 7.2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i686-pc-mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
This binary was built by Equation Solution <http://www.Equation.com>...
Reading symbols from c:\TMP\a/test.exe...done.
(gdb) break "main.main"
Breakpoint 1 at 0x401000: file c:/TMP/a/test.go, line 7.
(gdb) run
Starting program: c:\TMP\a/test.exe
[New Thread 2156.0x878]

Breakpoint 1, main.main () at c:/TMP/a/test.go:7
7       func main() {
(gdb) n
8               fmt.Printf("Hello\n")
(gdb)
Hello
0x0040105a in main.main ()
(gdb)
Single stepping until exit from function main.main,
which has no line number information.
runtime.mainstart () at 386/asm.s:85
85      386/asm.s: No such file or directory.
        in 386/asm.s
(gdb)
86      in 386/asm.s
(gdb)

Program exited normally.
(gdb) quit

brainman@ooo /c/TMP/a
$

Alex

PS: I can see, there is a problem finding 386/asm.s at the end. Perhaps it is
related to http://code.google.com/p/go/issues/detail?id=1143.

[vcc, 2010-12-24 02:24:58]

2010/12/24 <alex.brainman@gmail.com>

> ...

PS: I can see, there is a problem finding 386/asm.s at the end. Perhaps
> it is related to http://code.google.com/p/go/issues/detail?id=1143.


need set source search dir, for example my goroot is /opensource/go-w32,
then run cmd "dir /opensource/go-w32/src/pkg/runtime" in gdb , or put it in
.gdbinit file.



> http://codereview.appspot.com/2124041/
>

[vcc, 2011-01-12 04:09:37]

2010/12/23 Luuk van Dijk <lvd@google.com>

> the dwarf.c and .h LGTM,  thanks. i'm curious if the .debug_gdb_scripts
> mechanism will out of the box work for you.
>

I tested with Qt SDK's gdb for mingw (with python support), it works.


>
> /Luuk
>
> On Thu, Dec 23, 2010 at 00:29, <alex.brainman@gmail.com> wrote:
>
>> LGTM. Good job. Thank you.
>>
>> Russ, would you look over it again, please. It's changed a lot. Thank
>> you.
>>
>>
>> http://codereview.appspot.com/2124041/diff/110001/src/cmd/ld/pe.c
>>
>> File src/cmd/ld/pe.c (right):
>>
>>
>> http://codereview.appspot.com/2124041/diff/110001/src/cmd/ld/pe.c#newcode318
>> src/cmd/ld/pe.c:318: diag("too many names");
>> Maybe better (more specific) error message:
>>
>> diag("pe string table is full");
>>
>>
>> http://codereview.appspot.com/2124041/
>>
>
>

[rsc, 2011-01-19 20:14:42]

LGTM

If you make the change Alex suggested about "too many names"
I will submit.

Thanks.

[vcc, 2011-01-20 00:46:52]

http://codereview.appspot.com/2124041/diff/110001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (right):

http://codereview.appspot.com/2124041/diff/110001/src/cmd/ld/pe.c#newcode318
src/cmd/ld/pe.c:318: diag("too many names");
On 2010/12/22 23:29:14, brainman wrote:
> Maybe better (more specific) error message:
> 
> diag("pe string table is full");

Done.

[vcc, 2011-01-20 00:47:39]

Please take another look.

On 2011/01/19 20:14:42, rsc wrote:
> LGTM
> 
> If you make the change Alex suggested about "too many names"
> I will submit.

 Done.

[vcc, 2011-01-20 15:50:53]

hg sync & hg upload, please take another look.

[rsc, 2011-01-20 16:28:35]

*** Submitted as 79b8f17beda8 ***

8l: emit DWARF in Windows PE.

R=rsc, lvd, brainman, Joe Poirier
CC=golang-dev
http://codereview.appspot.com/2124041

Committer: Russ Cox <rsc@golang.org>