Re: [Valgrind-developers] new version of valgrind for WINE patch uploaded

[Jeremy F. <je...@go...>]

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