Re: CFA: pseudo-reloc v2
On 5/5/2010 4:30 PM, Christopher Faylor wrote: > Chuck? Do you have anything I could use to test what I did? Yes, most recent version attached. (embedded READMEs describe expected output). -- Chuck pseudo-reloc-tests-v3.tar.bz2 Description: Binary data
Re: CFA: pseudo-reloc v2
On 5/5/2010 3:13 PM, Christopher Faylor wrote: > That's basically it and I have it more-or-less coded but I haven't > finished thinking about DLLs. Maybe that's more complication than is > warranted. I have to do more research there. We could, and I think > should, put most of the code in pseudo_reloc.c in cygwin1.dll, though, > rather than duplicate it in every source file. I disagree with this statement. I spent a lot of effort trying to synchronize our version of pseudo_reloc.c with the mingw and mingw64 versions -- specifically so that we could leverage Kai's v2 efforts. If we -- meaning cygwin -- move most of the guts into the cygwin DLL, then ... we either (1) fork our version from the mingw[32|64] version permanently, and lose the possibility of "easy" code sharing between the three projects, or (2) this portion of the code lives in both places (pseudo_reloc.c and some-other-cygwin-dll-source-file), but is #ifdef'ed in pseudo_reloc.c when compiled on cygwin, because there's this other identical copy over in some-other-cygwin-dll-source-file. Yuck. (I don't mind "losing" the effort I put in, because whatever happens we now have v2 support. But...why make it harder if somebody in mingw-land invents v3? Or make it harder on them, if WE do?) -- Chuck
Re: CFA: pseudo-reloc v2
On 05/05/2010 21:30, Christopher Faylor wrote: > I have something written now. I'll dig through the cygwin archives to > see if I can find the original message which started this but are there > other test cases that I could use to verify that I caught all of the > code paths in the DLL? http://cygwin.com/ml/cygwin/2010-04/msg00957.html comes with a couple of testcases attached, although you can only be sure they've worked by running them and seeing that no .stackdump file was generated in your $CWD. > Chuck? Do you have anything I could use to test what I did? There were some fork-related testcases in the original thread, but I didn't refer back to them when I was revising this, so they're probably worth verifying: http://www.cygwin.com/ml/cygwin-developers/2009-10/msg00052.html > What I did: > > 1) Move pseudo-reloc.c out of lib and into the dll (making > it a c++ file in the process). > > 2) Record the three values needed by _pei386_runtime_relocator in the > per_process structure. That bit worries me - even adding a single pointer in a place where there would never have been a field before caused us enough trouble! But, it's probably the right thing to do; it's the defined mechanism for conveying image-specific information from the module to the cygwin dll. > 3) Modify _pei386_runtime_relocator() to take a per_process * argument > and to check that the api of the per_process structure supports the > additional three values. Changing per_process was not as easy as I had hoped when I did it! > 4) For fork call _pei386_runtime_relocator() before the copy of the program's > data/bss in child_info_fork::handle_fork(). > > 5) For non-fork, call _pei386_runtime_relocator() in dll_crt0_1(). > > 6) For dll's, call _pei386_runtime_relocator() in dll_list::alloc(). Re-ordering startup is the thing I didn't want to approach. The separation, serialisation and ordering between process attach and first thread attach is probably as reliable as anything we could hope for though. > I haven't added any optimizations to make this implementation avoid > copying the data/bss but that is doable using Dave's technique. It > just isn't needed now since the fork data copy should always trump > _pei386_runtime_relocator(). Well, as long as it works it must make sense; it's just a matter of which we figure is more long-term reliable and maintainable: your approach depends on inferences about which things happen in which order during startup, mine depends on inferences about which sections of the EI get copied from the parent during a fork. So, this post is a commentary, rather than an objection. cheers, DaveK
Re: CFA: pseudo-reloc v2
On Wed, May 05, 2010 at 08:48:28PM +0100, Dave Korn wrote: >On 05/05/2010 20:13, Christopher Faylor wrote: > >> Yeah, I realized that two seconds after sending the message. However, >> is this particular problem really an issue for DLLs? DLLs should get >> their data/bss updated after _pei386_runtime_relocator() is called. So >> it seems like you'd get the same thing being written twice. It's not >> optimal but it shouldn't be fatal. >> >> The program's data/bss is different since that gets copied during DLL >> initialization and before _pei386_runtime_relocator() is (was) called. So >> I could see how it could be screwed up. > > Ah, right; I wasn't looking at how much later the dll sections got copied, I >just figured the safest and consistent solution was just to treat everything >the same. > >> That's basically it and I have it more-or-less coded but I haven't >> finished thinking about DLLs. Maybe that's more complication than is >> warranted. I have to do more research there. We could, and I think >> should, put most of the code in pseudo_reloc.c in cygwin1.dll, though, >> rather than duplicate it in every source file. > > Yeh, the only thing we need in the source file is to capture the module's >idea of its section start/end pointers, as we already do in the per_process; >we could consider passing pointers to the pseudo-relocs in that as well, but >horrible backward-compatibility problems could arise. It would make sense to >inline the remnants of _pei386_runtime_relocator into _cygwin_crt0_common and >do away with the pseudo-reloc.c file altogether. > >> This information is all recorded for fork() so it should be doable. It is >> more complicated to do it outside of the program but, like I said, it allows >> us to fix problems by a new release of the DLL rather than telling people >> "You must relink your program". > > Yeh. Unfortunately it's too late to help with this time, but it would help >any future problem (so long as it didn't require us to capture additional data >in the lib/ part of the executable but could be fixed with what we were >already passing to the Cygwin DLL). I have something written now. I'll dig through the cygwin archives to see if I can find the original message which started this but are there other test cases that I could use to verify that I caught all of the code paths in the DLL? Chuck? Do you have anything I could use to test what I did? What I did: 1) Move pseudo-reloc.c out of lib and into the dll (making it a c++ file in the process). 2) Record the three values needed by _pei386_runtime_relocator in the per_process structure. 3) Modify _pei386_runtime_relocator() to take a per_process * argument and to check that the api of the per_process structure supports the additional three values. 4) For fork call _pei386_runtime_relocator() before the copy of the program's data/bss in child_info_fork::handle_fork(). 5) For non-fork, call _pei386_runtime_relocator() in dll_crt0_1(). 6) For dll's, call _pei386_runtime_relocator() in dll_list::alloc(). I haven't added any optimizations to make this implementation avoid copying the data/bss but that is doable using Dave's technique. It just isn't needed now since the fork data copy should always trump _pei386_runtime_relocator(). cgf
Re: CFA: pseudo-reloc v2
On 05/05/2010 20:13, Christopher Faylor wrote: > Yeah, I realized that two seconds after sending the message. However, > is this particular problem really an issue for DLLs? DLLs should get > their data/bss updated after _pei386_runtime_relocator() is called. So > it seems like you'd get the same thing being written twice. It's not > optimal but it shouldn't be fatal. > > The program's data/bss is different since that gets copied during DLL > initialization and before _pei386_runtime_relocator() is (was) called. So > I could see how it could be screwed up. Ah, right; I wasn't looking at how much later the dll sections got copied, I just figured the safest and consistent solution was just to treat everything the same. > That's basically it and I have it more-or-less coded but I haven't > finished thinking about DLLs. Maybe that's more complication than is > warranted. I have to do more research there. We could, and I think > should, put most of the code in pseudo_reloc.c in cygwin1.dll, though, > rather than duplicate it in every source file. Yeh, the only thing we need in the source file is to capture the module's idea of its section start/end pointers, as we already do in the per_process; we could consider passing pointers to the pseudo-relocs in that as well, but horrible backward-compatibility problems could arise. It would make sense to inline the remnants of _pei386_runtime_relocator into _cygwin_crt0_common and do away with the pseudo-reloc.c file altogether. > This information is all recorded for fork() so it should be doable. It is > more complicated to do it outside of the program but, like I said, it allows > us to fix problems by a new release of the DLL rather than telling people > "You must relink your program". Yeh. Unfortunately it's too late to help with this time, but it would help any future problem (so long as it didn't require us to capture additional data in the lib/ part of the executable but could be fixed with what we were already passing to the Cygwin DLL). cheers, DaveK
Re: CFA: pseudo-reloc v2
On Wed, May 05, 2010 at 07:58:20PM +0100, Dave Korn wrote: >On 05/05/2010 18:56, Christopher Faylor wrote: > >> I like the idea but I have a few problems with this, some stylistic and >> some implementation. >> >> Stylistic: > > Those all make sense to me, but I won't rework it yet until we've seen your >PoC/discussed further. > >> Implementation: >> >> I don't like keeping a list of "places we know we need to ignore" separate >> from >> the Cygwin DLL. That means that if there is something new added besides >> data/bss >> this function becomes obsolete. >> >> I think this argues for most of this functionality being moved to the >> Cygwin DLL itself so that an application gets improvements for free. I >> should have suggested that when this function first made its way into >> the libcygwin.a (or maybe I did and someone will enlighten me about why that >> won't work). >> >> I'll see I can come up with a proof-of-concept of what I'm talking about >> soon. >> >> Btw, don't we have to worry about data/bss for DLLs too? Is that >> handled by this change or is that not an issue? > > We do have to worry and it is handled. This code gets linked statically >into each DLL and EXE, each instance referring just to its own pseudo-reloc >tables. The Cygwin DLL copies all the data and bss for both DLLs and EXEs (at >process attach time), then they all run their own pseudo-relocs (at first >thread attach time). Yeah, I realized that two seconds after sending the message. However, is this particular problem really an issue for DLLs? DLLs should get their data/bss updated after _pei386_runtime_relocator() is called. So it seems like you'd get the same thing being written twice. It's not optimal but it shouldn't be fatal. The program's data/bss is different since that gets copied during DLL initialization and before _pei386_runtime_relocator() is (was) called. So I could see how it could be screwed up. > So we could move the core __write_memory and do_pseudo_relocs routines into >the DLL, and adjust the code in _cygwin_crt0_common to pass the per_process >struct to _pei386_runtime_relocator which could pass it and the reloc list >start/end pointers through to the code in the DLL, and it could then be code >in the DLL that knows which memory ranges it copied and should avoid >re-relocating. That's basically it and I have it more-or-less coded but I haven't finished thinking about DLLs. Maybe that's more complication than is warranted. I have to do more research there. We could, and I think should, put most of the code in pseudo_reloc.c in cygwin1.dll, though, rather than duplicate it in every source file. >Is that the kind of structure you were thinking of? The problem I saw >with any kind of approach based on actually knowing which ranges were >actually copied (as opposed to simply inferring that it was the data >and bss sections between their start and end labels) is that that all >takes place in the parent rather than the child, so how to communicate >it to the child where the relocating is taking place would be pretty >tricky, I thought. This information is all recorded for fork() so it should be doable. It is more complicated to do it outside of the program but, like I said, it allows us to fix problems by a new release of the DLL rather than telling people "You must relink your program". cgf
Re: CFA: pseudo-reloc v2
On 05/05/2010 18:56, Christopher Faylor wrote: > I like the idea but I have a few problems with this, some stylistic and > some implementation. > > Stylistic: Those all make sense to me, but I won't rework it yet until we've seen your PoC/discussed further. > Implementation: > > I don't like keeping a list of "places we know we need to ignore" separate > from > the Cygwin DLL. That means that if there is something new added besides > data/bss > this function becomes obsolete. > > I think this argues for most of this functionality being moved to the > Cygwin DLL itself so that an application gets improvements for free. I > should have suggested that when this function first made its way into > the libcygwin.a (or maybe I did and someone will enlighten me about why that > won't work). > > I'll see I can come up with a proof-of-concept of what I'm talking about soon. > > Btw, don't we have to worry about data/bss for DLLs too? Is that > handled by this change or is that not an issue? We do have to worry and it is handled. This code gets linked statically into each DLL and EXE, each instance referring just to its own pseudo-reloc tables. The Cygwin DLL copies all the data and bss for both DLLs and EXEs (at process attach time), then they all run their own pseudo-relocs (at first thread attach time). This only works /because/ the module is linked into each executable image (i.e., DLL or EXE, and henceforth 'EI') separately. While we could move the core code into the Cygwin DLL, we'd still have to have a statically linked object in each EI to capture the module's definitions of __RUNTIME_PSEUDO_RELOC_LIST__ and __RUNTIME_PSEUDO_RELOC_LIST_END__, and so it's also a valid place to capture the local module's __data/bss_start/end_ definitions (as indeed is already done in _cygwin_crt0_common where it sets up the per_process userdata structure). So we could move the core __write_memory and do_pseudo_relocs routines into the DLL, and adjust the code in _cygwin_crt0_common to pass the per_process struct to _pei386_runtime_relocator which could pass it and the reloc list start/end pointers through to the code in the DLL, and it could then be code in the DLL that knows which memory ranges it copied and should avoid re-relocating. Is that the kind of structure you were thinking of? The problem I saw with any kind of approach based on actually knowing which ranges were actually copied (as opposed to simply inferring that it was the data and bss sections between their start and end labels) is that that all takes place in the parent rather than the child, so how to communicate it to the child where the relocating is taking place would be pretty tricky, I thought. cheers, DaveK
Re: CFA: pseudo-reloc v2
On Wed, May 05, 2010 at 05:54:29PM +0100, Dave Korn wrote:
>[ redirected from cygwin-developers. ]
>On 04/10/2009 05:11, Charles Wilson wrote:
>[ thread seriously necro'd! ]
>> Dave Korn wrote:
>>> Charles Wilson wrote:
120 void
121 _pei386_runtime_relocator ()
122 {
123 static int was_init = 0;
124 if (was_init)
125 return;
126 ++was_init;
127 do_pseudo_reloc
(&__RUNTIME_PSEUDO_RELOC_LIST__,&__RUNTIME_PSEUDO_RELOC_LIST_END__,&_image_base__);
128 }
>>> Maybe that static should be NO_COPY? If everything gets remapped in the
>>> forkee, do the relocs need rerunning? (I'm not sure about the behaviour of
>>> NtCreateProcess w.r.t modified .text section pages.)
>>
>> Good guess! With the following patch, all of these fork tests perform
>> as expected.
>
> Aha, not so good as all that after all! We need to re-apply relocs in the
>forkee - but only if they *don't* point to regions covered by the .data/.bss
>section copying at startup. Argh!
>
>> One oddity; it turns out that __INSIDE_CYGWIN__ is not
>> defined inside pseudo-reloc.c, so I used __CYGWIN__ as a guard.
>
> Dunno if we ever went into that, but it's right; pseudo-reloc.o is part of
>the CRT in winsup/cygwin/lib/, and is linked statically into every exe and
>(user) dll, but is not part of the cygwin1 dll. Hence not "inside Cygwin".
>
> So, the attached is my proposed fix. It resolves the problem reported on
>the main list the other day(*) and the supplied testcases all work once it's
>applied. There are two things that people might want to change: the minor one
>is that I let a couple of the lines get a bit long, but no longer than we
>already have in the definition of NO_COPY at the top of the file, so I didn't
>wrap them for the sake of one trailing word. The slightly bigger one is that,
>if I remember, the reason for having non-#if-CYGWIN code in the file at all is
>to make any potential future merges from upstream MinGW sources theoretically
>easier, but now that I've had to diverge the internal interfaces anyway, is
>there any reason not to just delete the whole lot?
>
>winsup/cygwin/ChangeLog:
>
> lib/pseudo-reloc.c (memskip_t): New struct and typedef.
> (__write_memory): Accept an optional memskip_t argument and avoid
> writing to any memory ranges mentioned in the linked list.
> (do_pseudo_reloc): Accept an optional memskip_t argument and pass
> it through in all calls to __write_memory.
> (_pei386_runtime_relocator): When reapplying relocs in a forked
> child process, avoid doubly-relocating the .data and .bss sections
> that were copied from the parent.
>
>cheers,
> DaveK
>--
>(*) - http://cygwin.com/ml/cygwin/2010-04/msg00957.html
>
>Index: winsup/cygwin/lib/pseudo-reloc.c
>===
>RCS file: /cvs/src/src/winsup/cygwin/lib/pseudo-reloc.c,v
>retrieving revision 1.4
>diff -p -u -r1.4 pseudo-reloc.c
>--- winsup/cygwin/lib/pseudo-reloc.c 26 Oct 2009 14:50:09 - 1.4
>+++ winsup/cygwin/lib/pseudo-reloc.c 5 May 2010 16:04:46 -
>@@ -78,6 +78,20 @@ typedef struct {
> DWORD version;
> } runtime_pseudo_reloc_v2;
>
>+/* This trivial struct is passed right down through do_pseudo_reloc
>+ to __write_memory where it is used to avoid re-relocating those
>+ memory areas that we know will have been pre-relocated by copying
>+ from the parent of a forked child process. Since there will only
>+ ever be two ranges it's not worth worrying hugely about making it
>+ efficient so a simple singly-linked list will do; if we ever start
>+ encountering user applications with more than a few hundred or so
>+ pseudo-relocs, there might come a time to rethink this. */
>+typedef struct memskip {
>+ DWORD start;
>+ DWORD end;
>+ const struct memskip *next;
>+} memskip_t;
>+
> static void ATTRIBUTE_NORETURN
> __report_error (const char *msg, ...)
> {
>@@ -169,7 +183,7 @@ __report_error (const char *msg, ...)
> * is folded into the (writable) .data when --enable-auto-import.
> */
> static void
>-__write_memory (void *addr, const void *src, size_t len)
>+__write_memory (void *addr, const void *src, size_t len, const memskip_t
>*skipranges)
> {
> MEMORY_BASIC_INFORMATION b;
> DWORD oldprot;
>@@ -177,6 +191,13 @@ __write_memory (void *addr, const void *
> if (!len)
> return;
>
>+ while (skipranges)
>+{
>+ if ((skipranges->start <= (DWORD)addr) && (skipranges->end >
>(DWORD)addr))
>+ return;
>+ skipranges = skipranges->next;
>+}
>+
> if (!VirtualQuery (addr, &b, sizeof(b)))
> {
> __report_error (" VirtualQuery failed for %d bytes at address %p",
>@@ -198,7 +219,7 @@ __write_memory (void *addr, const void *
> #define RP_VERSION_V2 1
>
> static void
>-do_pseudo_reloc (void * start, void * end, void * base)
>+do_pseudo_reloc (void * start, void * end, void * base, const memskip_t
>*
Re: CFA: pseudo-reloc v2
[ redirected from cygwin-developers. ]
On 04/10/2009 05:11, Charles Wilson wrote:
[ thread seriously necro'd! ]
> Dave Korn wrote:
>> Charles Wilson wrote:
>>> 120 void
>>> 121 _pei386_runtime_relocator ()
>>> 122 {
>>> 123 static int was_init = 0;
>>> 124 if (was_init)
>>> 125 return;
>>> 126 ++was_init;
>>> 127 do_pseudo_reloc
>>> (&__RUNTIME_PSEUDO_RELOC_LIST__,&__RUNTIME_PSEUDO_RELOC_LIST_END__,&_image_base__);
>>> 128 }
>> Maybe that static should be NO_COPY? If everything gets remapped in the
>> forkee, do the relocs need rerunning? (I'm not sure about the behaviour of
>> NtCreateProcess w.r.t modified .text section pages.)
>
> Good guess! With the following patch, all of these fork tests perform
> as expected.
Aha, not so good as all that after all! We need to re-apply relocs in the
forkee - but only if they *don't* point to regions covered by the .data/.bss
section copying at startup. Argh!
> One oddity; it turns out that __INSIDE_CYGWIN__ is not
> defined inside pseudo-reloc.c, so I used __CYGWIN__ as a guard.
Dunno if we ever went into that, but it's right; pseudo-reloc.o is part of
the CRT in winsup/cygwin/lib/, and is linked statically into every exe and
(user) dll, but is not part of the cygwin1 dll. Hence not "inside Cygwin".
So, the attached is my proposed fix. It resolves the problem reported on
the main list the other day(*) and the supplied testcases all work once it's
applied. There are two things that people might want to change: the minor one
is that I let a couple of the lines get a bit long, but no longer than we
already have in the definition of NO_COPY at the top of the file, so I didn't
wrap them for the sake of one trailing word. The slightly bigger one is that,
if I remember, the reason for having non-#if-CYGWIN code in the file at all is
to make any potential future merges from upstream MinGW sources theoretically
easier, but now that I've had to diverge the internal interfaces anyway, is
there any reason not to just delete the whole lot?
winsup/cygwin/ChangeLog:
lib/pseudo-reloc.c (memskip_t): New struct and typedef.
(__write_memory): Accept an optional memskip_t argument and avoid
writing to any memory ranges mentioned in the linked list.
(do_pseudo_reloc): Accept an optional memskip_t argument and pass
it through in all calls to __write_memory.
(_pei386_runtime_relocator): When reapplying relocs in a forked
child process, avoid doubly-relocating the .data and .bss sections
that were copied from the parent.
cheers,
DaveK
--
(*) - http://cygwin.com/ml/cygwin/2010-04/msg00957.html
Index: winsup/cygwin/lib/pseudo-reloc.c
===
RCS file: /cvs/src/src/winsup/cygwin/lib/pseudo-reloc.c,v
retrieving revision 1.4
diff -p -u -r1.4 pseudo-reloc.c
--- winsup/cygwin/lib/pseudo-reloc.c 26 Oct 2009 14:50:09 - 1.4
+++ winsup/cygwin/lib/pseudo-reloc.c 5 May 2010 16:04:46 -
@@ -78,6 +78,20 @@ typedef struct {
DWORD version;
} runtime_pseudo_reloc_v2;
+/* This trivial struct is passed right down through do_pseudo_reloc
+ to __write_memory where it is used to avoid re-relocating those
+ memory areas that we know will have been pre-relocated by copying
+ from the parent of a forked child process. Since there will only
+ ever be two ranges it's not worth worrying hugely about making it
+ efficient so a simple singly-linked list will do; if we ever start
+ encountering user applications with more than a few hundred or so
+ pseudo-relocs, there might come a time to rethink this. */
+typedef struct memskip {
+ DWORD start;
+ DWORD end;
+ const struct memskip *next;
+} memskip_t;
+
static void ATTRIBUTE_NORETURN
__report_error (const char *msg, ...)
{
@@ -169,7 +183,7 @@ __report_error (const char *msg, ...)
* is folded into the (writable) .data when --enable-auto-import.
*/
static void
-__write_memory (void *addr, const void *src, size_t len)
+__write_memory (void *addr, const void *src, size_t len, const memskip_t *skipranges)
{
MEMORY_BASIC_INFORMATION b;
DWORD oldprot;
@@ -177,6 +191,13 @@ __write_memory (void *addr, const void *
if (!len)
return;
+ while (skipranges)
+{
+ if ((skipranges->start <= (DWORD)addr) && (skipranges->end > (DWORD)addr))
+ return;
+ skipranges = skipranges->next;
+}
+
if (!VirtualQuery (addr, &b, sizeof(b)))
{
__report_error (" VirtualQuery failed for %d bytes at address %p",
@@ -198,7 +219,7 @@ __write_memory (void *addr, const void *
#define RP_VERSION_V2 1
static void
-do_pseudo_reloc (void * start, void * end, void * base)
+do_pseudo_reloc (void * start, void * end, void * base, const memskip_t *skipranges)
{
ptrdiff_t addr_imp, reldata;
ptrdiff_t reloc_target = (ptrdiff_t) ((char *)end - (char*)start);
@@ -259,7 +280,7 @@ do_pseudo_reloc (void * start, void * en
DWORD n
