|
From: Adam G. <ar...@cy...> - 2003-04-01 11:10:55
|
I have just uploaded a slightly modified version of the patch which enables valgrind to work with WINE. This fixes "signal handler frame" uninitialized errors. http://sourceforge.net/tracker/index.php?func=detail&aid=710006&group_id=46268&atid=445588 is there any chance of this patch getting into the CVS repository? most of the changes are actually bug fixes to valgrind (mostly its threading support and signal handling, plus a translation for the XLAT instruction). the only "extra" code is the support for the PE/PDB file formats, which is pretty much restricted to vg_symtab.c comments? bug reports? Seeya, Adam -- Real Programmers don't comment their code. If it was hard to write, it should be hard to read, and even harder to modify. These are all my own opinions. |
|
From: Julian S. <js...@ac...> - 2003-04-01 23:01:24
|
Hi, Just to let you know your patch is under consideration. Thanks for it, looks like a lot of work, and useful. I'll try and get back to you on it in a few days, but we're (me, at least) are just overloaded and haven't had time to look at it properly yet. J I had a look over the patch just now. > is there any chance of this patch getting into the CVS repository? most of > the changes are actually bug fixes to valgrind (mostly its threading > support and signal handling, plus a translation for the XLAT instruction). > > the only "extra" code is the support for the PE/PDB file formats, which is > pretty much restricted to vg_symtab.c > > comments? bug reports? > > > Seeya, > Adam |
|
From: Jeremy F. <je...@go...> - 2003-04-02 00:54:01
|
On Tue, 2003-04-01 at 15:09, Julian Seward wrote: > Just to let you know your patch is under consideration. Thanks for > it, > looks like a lot of work, and useful. I'll try and get back to you > on it in a few days, but we're (me, at least) are just overloaded and > haven't had time to look at it properly yet. Hm. It looks like this patch will clash a lot with the work I've been doing on making UML work well, and the work on extracting symtab info from various file formats. There's a lot of different things in there: some are clearly generically OK (XLAT), others need a good hard looking at to see whether they're Wine-specific or are generally OK (signal and clone handing) and others are purely Wine-specific (PE handling). With the Wine-specific things, I wonder if we need to look at refactoring the object file handing a bit to make it easier to plug in different object file handlers. Adam: Can you look at separating your patch into logically distinct pieces (ie, signal handling, clone handling, PE parsing, XLAT, etc) rather than one big lump patch? Then could you look at http://www.goop.org/~jeremy/valgrind/ and see where there's overlap. It seem to me that 77-sigcontext is a dup of a lot of your signal handling stuff. I also think that splitting the DOS/PE parsing stuff into a separate file would be a good idea (like 44-symbolic-addr does with stabs vs DWARF). I definitely think you should have completely separate code for handing PE vs ELF, even if there are similarities in the file formats. You might also want to look at 70-exe-dataseg regarding the discrimination of code-bearing mappings from non-code-bearing mappings. I'm also curious about this change to mmap_segment: static -void mmap_segment ( Addr a, UInt len, UInt prot, Int fd ) +void mmap_segment ( Addr a, UInt len, UInt prot, UInt flags, Int fd, Int Offset ) { Bool rr, ww, xx; /* Records segment, reads debug symbols if necessary */ - if ((prot & PROT_EXEC) && fd != -1) + if ((((prot & (PROT_EXEC|PROT_WRITE)) == PROT_EXEC) || /* native */ + (((prot & (PROT_READ|PROT_WRITE)) == PROT_READ) && + (flags & MAP_PRIVATE) && + (Offset == 0))) /* Windows PE? */ + && fd != -1) VG_(new_exe_segment) ( a, len ); This seems to suggest that any private read-only (no X) mapping from offset 0 of a file could be a PE file. Is that right? Certainly a lot of other mmapings (perhaps even most) would pass the same test. I wonder if checking for a magic number or something would be useful. In fact, there seems to be a lot of reliance on looking at shared vs. private mappings to try and distinguish between ELF and PE segments: that doesn't seem like a generally safe thing to do. Thanks, J |
|
From: Adam G. <ar...@cy...> - 2003-04-02 13:34:21
|
At 16:51 01/04/03 -0800, you wrote: >On Tue, 2003-04-01 at 15:09, Julian Seward wrote: >>Just to let you know your patch is under consideration. Thanks for it, >>looks like a lot of work, and useful. I'll try and get back to you >>on it in a few days, but we're (me, at least) are just overloaded and >>haven't had time to look at it properly yet. >Hm. It looks like this patch will clash a lot with the work I've been doing on making UML work well, and the work on extracting symtab info from various file formats. There's a lot of different things in there: some are clearly generically OK (XLAT), others need a good hard looking at to see whether they're Wine-specific or are generally OK (signal and clone handing) and others are purely Wine-specific (PE handling). With the Wine-specific things, I wonder if we need to look at refactoring the object file handing a bit to make it easier to plug in different object file handlers. the only *really* WINE specific stuff is the PE/PDB handling. The rest is generic - clone(), signals, better handling of threads which switch stacks, and XLAT. >Adam: > >Can you look at separating your patch into logically distinct pieces (ie, signal handling, clone handling, PE parsing, XLAT, etc) rather than one big lump patch? Then could you look at <http://www.goop.org/~jeremy/valgrind/>http://www.goop.org/~jeremy/valgrind/ and see where there's overlap. It seem to me that <http://www.goop.org/%7Ejeremy/valgrind/77-sigcontext.patch>77-sigcontext is a dup of a lot of your signal handling stuff<http://www.goop.org/%7Ejeremy/valgrind/44-symbolic-addr.patch>. looks like 77-sigcontext is providing the siginfo_t and ucontext_t arguments to signal handlers which ask for them - unfortunately, WINE uses the older deprecated (but still documented & valid) method of declaring a signal handler as "int handler( int signo, sigcontext_t context )" so that it can access the sigcontext_t directly off the stack. It looks like both patches need combining. > I also think that splitting the DOS/PE parsing stuff into a separate file would be a good idea (like <http://www.goop.org/%7Ejeremy/valgrind/44-symbolic-addr.patch>44-symbolic-addr does with stabs vs DWARF). I definitely think you should have completely separate code for handing PE vs ELF, good idea - but I'd like to have a patch that WINE developers can easily apply to a valgrind CVS checkout - giving a list of "get valgrind CVS, then apply this patch, then this patch, then..." isn't going to get many users. Is your patch likely to hit CVS soon? > even if there are similarities in the file formats. You might also want to look at <http://www.goop.org/%7Ejeremy/valgrind/70-exe-dataseg.patch>70-exe-dataseg regarding the discrimination of code-bearing mappings from non-code-bearing mappings. not sure why this is needed - the code in vg_symtab.c already checks the signatures of the sections it examines (ELF, PE etc etc). >I'm also curious about this change to mmap_segment: > static >-void mmap_segment ( Addr a, UInt len, UInt prot, Int fd ) >+void mmap_segment ( Addr a, UInt len, UInt prot, UInt flags, Int fd, Int Offset ) > { > Bool rr, ww, xx; > > /* Records segment, reads debug symbols if necessary */ >- if ((prot & PROT_EXEC) && fd != -1) >+ if ((((prot & (PROT_EXEC|PROT_WRITE)) == PROT_EXEC) || /* native */ >+ (((prot & (PROT_READ|PROT_WRITE)) == PROT_READ) && >+ (flags & MAP_PRIVATE) && >+ (Offset == 0))) /* Windows PE? */ >+ && fd != -1) > VG_(new_exe_segment) ( a, len ); > >This seems to suggest that any private read-only (no X) mapping from offset 0 of a file could be a PE file. Is that right? Certainly a lot of other mmapings (perhaps even most) would pass the same test. I wonder if checking for a magic number or something would be useful. In fact, there seems to be a lot of reliance on looking at shared vs. private mappings to try and distinguish between ELF and PE segments: that doesn't seem like a generally safe thing to do. shared segments are always ignored by PE simply to reduce the number of potential hits. ELF & PE are distinguished by looking for magic numbers. The only way to identify the PE file as it is mapped is to find the header which is mapped RO, then figure out where the rest of the code will be mapped to. This is mainly down to Window's ability to map files from non-page aligned offsets - multiple sections of the PE file have to be copied by WINE into the correct bit of memory rather than mmapped (and they can't be identified at this point because there is no identifiable header in the sections). Seeya, Adam -- Real Programmers don't comment their code. If it was hard to write, it should be hard to read, and even harder to modify. These are all my own opinions. |
|
From: Jeremy F. <je...@go...> - 2003-04-02 22:28:46
|
On Wed, 2003-04-02 at 05:33, Adam Gundy wrote: > >Can you look at separating your patch into logically distinct pieces > (ie, signal handling, clone handling, PE parsing, XLAT, etc) rather > than one big lump patch? Then could you look at > <http://www.goop.org/~jeremy/valgrind/>http://www.goop.org/~jeremy/valgrind/ and see where there's overlap. It seem to me that <http://www.goop.org/%7Ejeremy/valgrind/77-sigcontext.patch>77-sigcontext is a dup of a lot of your signal handling stuff<http://www.goop.org/%7Ejeremy/valgrind/44-symbolic-addr.patch>. > > > looks like 77-sigcontext is providing the siginfo_t and ucontext_t arguments to signal handlers > which ask for them - unfortunately, WINE uses the older deprecated (but still documented & valid) > method of declaring a signal handler as "int handler( int signo, sigcontext_t context )" so that > it can access the sigcontext_t directly off the stack. It looks like both patches need combining. Looks like 77-sigcontext implements the sigcontext argument for the non-siginfo case as well (the chunk at +1078, vg_push_signal_frame). Is the other part of your signal patch is making SIGCONT and SIGSTOP work on threads? Anything else signal related? > > I also think that splitting the DOS/PE parsing stuff into a separate > file would be a good idea (like > <http://www.goop.org/%7Ejeremy/valgrind/44-symbolic-addr.patch>44-symbolic-addr does with stabs vs DWARF). I definitely think you should have completely separate code for handing PE vs ELF, > > > good idea - but I'd like to have a patch that WINE developers can easily apply to a valgrind CVS > checkout - giving a list of "get valgrind CVS, then apply this patch, then this patch, then..." isn't > going to get many users. Is your patch likely to hit CVS soon? Well, I need to finish the DWARF support before symbolic-addr is really ready to go in, but the object file handling refactoring could be brought out into a separate patch. > The only way to identify the PE file as it is mapped is to find the header which is mapped RO, then figure > out where the rest of the code will be mapped to. This is mainly down to Window's ability to map > files from non-page aligned offsets - multiple sections of the PE file have to be copied by WINE into > the correct bit of memory rather than mmapped (and they can't be identified at this point because there > is no identifiable header in the sections). Urg. I wonder if a better solution might be to add some more VALGRIND_* client callbacks so that the client app can tell Valgrind what's doing on in cases like this, rather than trying to teach Valgrind about every obscure dynamic loading technique. Are you reading symbolic information out of PE files? J |