code review 4894051: libmach: support reading symbols from Windows .exe for nm

libmach: support reading symbols from Windows .exe for nm

Fixes Issue 979.

[czaplinski, 2011-08-18 01:02:03]

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

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode460
src/cmd/ld/pe.c:460: size = nextsymoff + 4;
I believe this is more correct. DUMPBIN still seems to be working OK.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c
File src/libmach/executable.c (right):

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1245: /* TODO(czaplinski): structs are raw copy from
pe.h; need factoring out */
Shall these be moved to pe.h + #include "../ld/pe.h"? 
Or create $GOROOT/include/pecommon.h?

[rsc, 2011-08-18 17:38:30]

That was pretty easy!  Does 6nm work now?

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

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode35
src/cmd/ld/pe.c:35: static char *symlabels[] = {
either make this 1 line or put the closing } on its own line.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode36
src/cmd/ld/pe.c:36: "symtab", "esymtab", 0 };
You're going to want pclntab and epclntab too.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode460
src/cmd/ld/pe.c:460: size = nextsymoff + 4;
On 2011/08/18 01:02:04, czaplinski wrote:
> I believe this is more correct. DUMPBIN still seems to be working OK.

Why?  Do you have a document?  It seems strange that .symtab's
size would not include the bytes taken up by the symbols
themselves.  I would have expected 18*2 + nextsymoff + 4
for two symbols.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode469
src/cmd/ld/pe.c:469: // TODO[ematcza]: assert(strlen(s->name)<=8)
Handle or delete.
Don't assert.  If it's worth checking,
it's worth printing a useful error.
Since symlabels is defined above, just delete.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode471
src/cmd/ld/pe.c:471: lputl(datoff(s->value)); // TODO[ematcza]: assert(... <=
256**4)?
Delete the TODO.
Nothing here is going to work well with >4GB files.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode472
src/cmd/ld/pe.c:472: wputl(1); // TODO[ematcza]: is .text always section 1?
Yes.  But comment what these fields are.  For example.

wputl(1);  // .text is section 1

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c
File src/libmach/executable.c (right):

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:316: { 0x4d5a9000,    /* TODO(czaplinski): should be
4d5a only, but can we limit comparison to 2 bytes? */
No, it only lets you compare 4 bytes.
What's in the extra two bytes?  The MSDN docs I found
say that it is a 32-bit signature that contains "PE\0\0"
but obviously that's not this constant.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:318: nil, /* TODO */
Can drop the todo.  This is only for dynamic linking, 
which we're not going to have any time soon.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:321: nil, /* TODO */
&mi386,
or
&mamd64,

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1245: /* TODO(czaplinski): structs are raw copy from
pe.h; need factoring out */
On 2011/08/18 01:02:04, czaplinski wrote:
> Shall these be moved to pe.h + #include "../ld/pe.h"? 
> Or create $GOROOT/include/pecommon.h?

The duplication is not a problem.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1275: #pragma pack(1)
This isn't going to work with all compilers.  
Please just use a byte array and pull the data
out with explicit offsets.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1307: read(fd, (char *)&start, sizeof(start)); /* TODO:
check for underflow */
Do check the error (return 0).
You can drop the cast.  Also, please use readn.
The surrounding code is admittedly quite sloppy about this.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1309: seek(fd, start, 0);
Swap down below blank line, so it's next to the read
that depends on it.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1321: if (match8(sh.Name, ".text")) // TODO[ematcza]:
entry = ?
entry = AddressOfEntryPoint

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1323: if (match8(sh.Name, ".data")) // TODO[ematcza]:
bsssz = ?
datsz = SizeOfRawData (not VirtualSize)
bsssz = VirtualSize - SizeOfRawData

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1325: if (match8(sh.Name, ".symtab"))
Looks like this is dead code.

[czaplinski, 2011-08-19 00:30:36]

Hello rsc@golang.org (cc: alex.brainman@gmail.com, golang-dev@googlegroups.com,
vcc.163@gmail.com),

I'd like you to review this change to
http://go.googlecode.com/hg/

[brainman, 2011-08-19 00:31:31]

It is a good start. Thank you.

Please add "Fixes Issue 979" to CL description.

It fails to build:

%%%% making libmach %%%%

make: Entering directory `/root/hg/go/src/libmach'
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c
"/root/hg/go/src/libmach/executable.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c
"/root/hg/go/src/libmach/fakeobj.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/libmach/map.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/libmach/obj.c"
quietgcc -I"/root/hg/go/include" -ggdb -O2 -c "/root/hg/go/src/libmach/swap.c"
cc1: warnings being treated as errors
/root/hg/go/src/libmach/executable.c:1307: error: ignoring return value of
'read', declared with attribute warn_unused_result
/root/hg/go/src/libmach/executable.c:1311: error: ignoring return value of
'read', declared with attribute warn_unused_result
/root/hg/go/src/libmach/executable.c:1320: error: ignoring return value of
'read', declared with attribute warn_unused_result
/root/hg/go/src/libmach/executable.c:1332: error: ignoring return value of
'read', declared with attribute warn_unused_result
make: *** [executable.o] Error 1
make: *** Waiting for unfinished jobs
make: Leaving directory `/root/hg/go/src/libmach'

I think, you have to check read return value here.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c
File src/cmd/ld/pe.c (left):

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#oldcode457
src/cmd/ld/pe.c:457: h = addpesection(".symtab", size, size);
addpesection needs to be given total size of your data: size+18*i. So just move
it after you know what the i is equal to.

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

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode460
src/cmd/ld/pe.c:460: size = nextsymoff + 4;
On 2011/08/18 01:02:04, czaplinski wrote:
> I believe this is more correct. ...

You are correct, there is a bug here.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode460
src/cmd/ld/pe.c:460: size = nextsymoff + 4;
On 2011/08/18 17:38:30, rsc wrote:
> 
> Why?  Do you have a document?  

http://kishorekumar.net/pecoff_v8.1.htm

It is all starts at PointerToSymbolTable, but there are 2 different things here
"symbol table" followed by "string table". Our ".symtab" pe section will include
both, so pe section should be of size = "symbol table" size + "string table"
size.

As to the contents of each:

"symbol table" consists of NumberOfSymbols records 18 bytes each.

on the other hand

"string table" starts with 4 byte size of "string table" followed by 0
terminated strings one after the other.

That is my interpretation anyway.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode472
src/cmd/ld/pe.c:472: wputl(1); // TODO[ematcza]: is .text always section 1?
It is at the moment. But it would be wise not to hard code this. I think if you
grab value of nsect right after ".text" pe section is created with addpesection,
you should be good.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode475
src/cmd/ld/pe.c:475: cput(0);
Not must, but would be nice to know what these mean. Perhaps you could add
couple of these const to pe.h file and use them here.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c
File src/libmach/executable.c (right):

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:316: { 0x4d5a9000,    /* TODO(czaplinski): should be
4d5a only, but can we limit comparison to 2 bytes? */
I think you have no choice, but compare 2 bytes only.

typedef struct _IMAGE_DOS_HEADER {  // DOS .EXE header
    USHORT e_magic;         // Magic number
    USHORT e_cblp;          // Bytes on last page of file
...

You are checking for e_magic. There is nothing predefined about e_cblp. It will
be 0x9000 for every Go program, but only because we made it so. Not sure that
all other executables will match.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1245: /* TODO(czaplinski): structs are raw copy from
pe.h; need factoring out */
On 2011/08/18 01:02:04, czaplinski wrote:
> Shall these be moved to pe.h + #include "../ld/pe.h"? 
> Or create $GOROOT/include/pecommon.h?

I don't know the answer. I hope Russ will help. I agree we should share pe.h
between 8l and 6nm.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1273: } peheader;
I don't think you need to invent this struct. Just match 4 byte signature and
use IMAGE_FILE_HEADER straight after it.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1329: seek(fd, symtab, 0);
I would use IMAGE_FILE_HEADER.PointerToSymbolTable to find your symbols instead.
The fact that they are live in ".symtab" pe section is purely my choice. It is
not required by any documentation. While IMAGE_FILE_HEADER.PointerToSymbolTable
is described in the manual.

[czaplinski, 2011-08-19 00:34:10]

> That was pretty easy!  Does 6nm work now?

Sure easier than debugging Windows callbacks :)

6nm WorksForMe.

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

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode35
src/cmd/ld/pe.c:35: static char *symlabels[] = {
On 2011/08/18 17:38:30, rsc wrote:
> either make this 1 line or put the closing } on its own line.

Done.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode36
src/cmd/ld/pe.c:36: "symtab", "esymtab", 0 };
On 2011/08/18 17:38:30, rsc wrote:
> You're going to want pclntab and epclntab too.

Done.
Didn't want to introduce them if they're not used anywhere yet.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode460
src/cmd/ld/pe.c:460: size = nextsymoff + 4;
On 2011/08/18 17:38:30, rsc wrote:
> On 2011/08/18 01:02:04, czaplinski wrote:
> > I believe this is more correct. DUMPBIN still seems to be working OK.
> Why?  Do you have a document?  It seems strange that .symtab's
> size would not include the bytes taken up by the symbols
> themselves.  

Arrgh, I missed that it's also used in addpesection(). I had noticed only the
lputl(size) below, which is the one which should get the above value. Thanks.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode469
src/cmd/ld/pe.c:469: // TODO[ematcza]: assert(strlen(s->name)<=8)
On 2011/08/18 17:38:30, rsc wrote:
> Handle or delete.
> Don't assert.  If it's worth checking,
> it's worth printing a useful error.
> Since symlabels is defined above, just delete.

Done.
Thanks for the clarifications; I wasn't sure what's the policy on these things.
Maybe a warning comment near symlabels[] could still be helpful.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode471
src/cmd/ld/pe.c:471: lputl(datoff(s->value)); // TODO[ematcza]: assert(... <=
256**4)?
On 2011/08/18 17:38:30, rsc wrote:
> Delete the TODO.
> Nothing here is going to work well with >4GB files.

Done.

http://codereview.appspot.com/4894051/diff/2001/src/cmd/ld/pe.c#newcode472
src/cmd/ld/pe.c:472: wputl(1); // TODO[ematcza]: is .text always section 1?
On 2011/08/18 17:38:30, rsc wrote:
> Yes.  But comment what these fields are.  For example.
> 
> wputl(1);  // .text is section 1
> 

Done.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c
File src/libmach/executable.c (right):

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:316: { 0x4d5a9000,    /* TODO(czaplinski): should be
4d5a only, but can we limit comparison to 2 bytes? */
On 2011/08/18 17:38:30, rsc wrote:
> No, it only lets you compare 4 bytes.
> What's in the extra two bytes?  The MSDN docs I found
> say that it is a 32-bit signature that contains "PE\0\0"
> but obviously that's not this constant.

The "PE\0\0" appears only later in a file, in PE header. The very beginning of
the file is a MSDOS EXE header, for Go EXEs exactly dosstub[] from the beginning
of ld/pe.c. We can probably quite safely assume we'll never change it, and thus
check for it.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:318: nil, /* TODO */
On 2011/08/18 17:38:30, rsc wrote:
> Can drop the todo.  This is only for dynamic linking, 
> which we're not going to have any time soon.

Done.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:321: nil, /* TODO */
On 2011/08/18 17:38:30, rsc wrote:
> &mi386,
> or
> &mamd64,

Done.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1245: /* TODO(czaplinski): structs are raw copy from
pe.h; need factoring out */
On 2011/08/18 17:38:30, rsc wrote:
> The duplication is not a problem.

Ok, so to be clear, I understand I can leave all this stuff here.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1275: #pragma pack(1)
On 2011/08/18 17:38:30, rsc wrote:
> This isn't going to work with all compilers.  
> Please just use a byte array and pull the data
> out with explicit offsets.

Done.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1307: read(fd, (char *)&start, sizeof(start)); /* TODO:
check for underflow */
On 2011/08/18 17:38:30, rsc wrote:
> Do check the error (return 0).
> You can drop the cast.  Also, please use readn.
> The surrounding code is admittedly quite sloppy about this.
> 

Done.
BTW, shouldn't nm.c display the errstr to user?

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1309: seek(fd, start, 0);
On 2011/08/18 17:38:30, rsc wrote:
> Swap down below blank line, so it's next to the read
> that depends on it.

Done.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1321: if (match8(sh.Name, ".text")) // TODO[ematcza]:
entry = ?
On 2011/08/18 17:38:30, rsc wrote:
> entry = AddressOfEntryPoint

Done.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1323: if (match8(sh.Name, ".data")) // TODO[ematcza]:
bsssz = ?
On 2011/08/18 17:38:30, rsc wrote:
> datsz = SizeOfRawData (not VirtualSize)
> bsssz = VirtualSize - SizeOfRawData

Done.

http://codereview.appspot.com/4894051/diff/2001/src/libmach/executable.c#newc...
src/libmach/executable.c:1325: if (match8(sh.Name, ".symtab"))
On 2011/08/18 17:38:30, rsc wrote:
> Looks like this is dead code.

I believe it's not, but actually it doesn't matter anymore, as I've just
realized we can really skip this fragment by perusing a direct header field.

The variable name reuse I've attempted here was partially motivated by a
slightly unfortunate naming coincidence, that we have a Windows/COFF ".symtab",
which then points to a Go/Plan9 "symtab". But if we're at that, we shouldn't
also probably miss the potentially amusing similarity between "COFF symbols
table" and "COFF strings table". As well as ~3 official Magic Number fields
lurking around...

[brainman, 2011-08-19 01:14:00]

Please add "Fixes Issue 979" to CL description.

Other then that, it LGTM. Thank you.

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

http://codereview.appspot.com/4894051/diff/1002/src/cmd/ld/pe.c#newcode472
src/cmd/ld/pe.c:472: wputl(1);       // .text is section 1
I am concerned about this hard coding. If someone will decide to move .text
section about they would not have a clue about your assumption. I don't think it
is hard to implement it as I have described in my previous review. Thank you.

[czaplinski, 2011-08-19 07:35:42]

Just some comments, patchset will come later.

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

http://codereview.appspot.com/4894051/diff/1002/src/cmd/ld/pe.c#newcode472
src/cmd/ld/pe.c:472: wputl(1);       // .text is section 1
On 2011/08/19 01:14:00, brainman wrote:
> I am concerned about this hard coding. If someone will decide to move .text
> section about they would not have a clue about your assumption. I don't think
it
> is hard to implement it as I have described in my previous review. Thank you.

Ok, I'll try to do this, and also to update the CL desc.

I can also remove the peheader struct in executable.c if you think that'll be
cleaner.

To clarify, I sure didn't want to dismiss your comments, only coincidentally we
published nearly in the same moment, so I didn't have reflexes to react.

[czaplinski, 2011-08-19 12:31:31]

PTAL.

[brainman, 2011-08-20 03:35:48]

I don't have computer to build and check but it LGTM. Thank you for doing it.

Alex

[czaplinski, 2011-08-21 21:07:28]

Happy to help. Wish I had more time for having fun with these kinds of things.
/M.

[brainman, 2011-08-23 04:08:43]

I tried to use this changed 6nm with gopprof to view memory profile of a
program. And gopprof still does not work. The issue seems to be that gopprof
uses couple of different "extract names and addresses from executable" tools:

nm -n  --demangle test.exe 2>/dev/null
nm -D -n  --demangle test.exe 2>/dev/null
6nm test.exe 2>/dev/null | sort

in that order on off chance that one of them will work. Now, that we have added
4 symbols to our executable, nm will display them:

# nm test.exe
00000000004ca43c T epclntab
00000000004c36d4 T esymtab
00000000004c36d4 T pclntab
000000000048a814 T symtab
#

Unfortunetly, that is not what we want to see. We would like to see something
similar to ELF output:

# nm test | grep main
08048d42 T main.init
080f518c R main.initdoneÂ
08048d02 T main.main
08048c00 T main.stat
08049916 T runtime.mainstart
#

If I trim gopprof list of exe readers to 6nm only:

6nm test.exe 2>/dev/null | sort

then it works. This is quick and dirty solution.

Alternatively, our *windows symbol table* should contain all our real symbols.
Then both nm and 6nm (and perhaps some other tools) will be able to read them.
We could write them all out, but then we will have 3 copies of those: first
inside ".text" pe section that we have now, second inside dwarf, and third in
"windows symbol table".

Perhaps, we should decide what to do with gopprof before we submit this change.
Any comments?

Alex

[rsc, 2011-08-23 04:57:55]

In the long term the right thing to do is to write them all out
as Windows symbols.  It's okay that there are 3 copies:
only one gets loaded into memory.  Maybe if the nm copy
exists the DWARF copy can be omitted.  I don't think we
have separate ELF and DWARF symbol tables in ELF binaries.

Russ

[brainman, 2011-08-23 06:37:12]

On Tue, Aug 23, 2011 at 2:57 PM, Russ Cox <rsc@golang.org> wrote:
> In the long term the right thing to do is to write them all out
> as Windows symbols.  It's okay that there are 3 copies:
> only one gets loaded into memory. 

All our pe sections are loaded into the memory at this moment.

Memory map
Address   Size      Owner            Section    Contains                 Type 
Access   Initial >Mapped as
...
00400000  00001000 >test            >           PE header                Img  >R
       RWE Copy>
00401000  000CA000 >test            >.text      Code                     Img  >R
E      RWE Copy>
004CB000  00417000 >test            >.data      Data                     Img 
>RW  Copy>RWE Copy>
008E2000  00001000 >test            >/4                                  Img  >R
       RWE Copy>
008E3000  0000C000 >test            >/18                                 Img  >R
       RWE Copy>
008EF000  00008000 >test            >/30                                 Img  >R
       RWE Copy>
008F7000  00022000 >test            >/43                                 Img  >R
       RWE Copy>
00919000  0000D000 >test            >/55                                 Img  >R
       RWE Copy>
00926000  00003000 >test            >/71                                 Img  >R
       RWE Copy>
00929000  00001000 >test            >/87                                 Img  >R
       RWE Copy>
0092A000  00001000 >test            >/102                                Img  >R
       RWE Copy>
0092B000  00001000 >test            >.idata     Imports                  Img 
>RW  Copy>RWE Copy>
0092C000  00001000 >test            >.symtab                             Img  >R
       RWE Copy>
...

".text" section contains gosymbols and line numbers at this moment:

text = segtext.sect;
rodata = text->next;
symtab = rodata->next;
pclntab = symtab->next;

We can't leave any of them out, they are used for reflect and others.

Sections named '/4', '/18' and so on are dwarf sections. I don't know for sure,
but I suspect some of them repeat symbols and line number information too.

Section ".symtab" has 4 symbols introduced by that CL to point to "symtab" and
"pclntab" in ".text" section. It looks like nm will see symbols listed in this
section. Are you're saying we should leave dwarf and ".text" sections as they
are and just add all symbols to ".symtab" section as well?

> Maybe if the nm copy
> exists the DWARF copy can be omitted. 

Dwarf is used by gdb, we can't omit any of that. Perhaps, pe dwarf sections can
be changed, so they don't map into real memory, but I do not care about saving
address space just now.

> I don't think we
> have separate ELF and DWARF symbol tables in ELF binaries.

How does nm find symbols on linux?

Alex

[rsc, 2011-08-26 20:02:35]

On Tue, Aug 23, 2011 at 02:37,  <alex.brainman@gmail.com> wrote:
> On Tue, Aug 23, 2011 at 2:57 PM, Russ Cox <rsc@golang.org> wrote:
>>
>> In the long term the right thing to do is to write them all out
>> as Windows symbols.  It's okay that there are 3 copies:
>> only one gets loaded into memory.
>
> All our pe sections are loaded into the memory at this moment.
>
> Memory map
> Address   Size      Owner            Section    Contains
> Type  Access   Initial >Mapped as
> ...
> 00400000  00001000 >test            >           PE header
> Img  >R        RWE Copy>
> 00401000  000CA000 >test            >.text      Code
> Img  >R E      RWE Copy>
> 004CB000  00417000 >test            >.data      Data
> Img  >RW  Copy>RWE Copy>
> 008E2000  00001000 >test            >/4
> Img  >R        RWE Copy>
> 008E3000  0000C000 >test            >/18
> Img  >R        RWE Copy>
> 008EF000  00008000 >test            >/30
> Img  >R        RWE Copy>
> 008F7000  00022000 >test            >/43
> Img  >R        RWE Copy>
> 00919000  0000D000 >test            >/55
> Img  >R        RWE Copy>
> 00926000  00003000 >test            >/71
> Img  >R        RWE Copy>
> 00929000  00001000 >test            >/87
> Img  >R        RWE Copy>
> 0092A000  00001000 >test            >/102
> Img  >R        RWE Copy>
> 0092B000  00001000 >test            >.idata     Imports
> Img  >RW  Copy>RWE Copy>
> 0092C000  00001000 >test            >.symtab
> Img  >R        RWE Copy>
> ...
>
> ".text" section contains gosymbols and line numbers at this moment:
>
> text = segtext.sect;
> rodata = text->next;
> symtab = rodata->next;
> pclntab = symtab->next;
>
> We can't leave any of them out, they are used for reflect and others.
>
> Sections named '/4', '/18' and so on are dwarf sections. I don't know
> for sure, but I suspect some of them repeat symbols and line number
> information too.
>
> Section ".symtab" has 4 symbols introduced by that CL to point to
> "symtab" and "pclntab" in ".text" section. It looks like nm will see
> symbols listed in this section. Are you're saying we should leave dwarf
> and ".text" sections as they are and just add all symbols to ".symtab"
> section as well?

Are you saying that the dwarf and ".symtab" sections get
loaded into memory?  I don't see why that would be necessary
though I can believe it was easy.  I was saying that since neither
needs to be loaded into memory it is not a big deal if they
duplicate information that is also in memory.

So yes, I was suggesting to add all the symbols to '.symtab'
(the Windows table, which need not be in memory and is different
from the runtime's symtab data), so that Windows tools can
see Go's symbols.

Russ

[brainman, 2011-08-29 02:36:40]

> 
> Are you saying that the dwarf and ".symtab" sections get
> loaded into memory? 

I think they are, but will check:

http://code.google.com/p/go/issues/detail?id=2198

> So yes, I was suggesting to add all the symbols to '.symtab'
> (the Windows table, which need not be in memory and is different
> from the runtime's symtab data), so that Windows tools can
> see Go's symbols.

http://code.google.com/p/go/issues/detail?id=2199

I think we should go ahead with this CL. 6nm works fine now, and that is what
this CL was designed to achieve.

Alex

[rsc, 2011-08-29 13:30:14]

LGTM

[rsc, 2011-08-29 18:25:47]

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

libmach: support reading symbols from Windows .exe for nm

Fixes Issue 979.

R=rsc, alex.brainman
CC=golang-dev, vcc.163
http://codereview.appspot.com/4894051

Committer: Russ Cox <rsc@golang.org>