Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#18 Sync cygwin, mingw32, and mingw64 pseudo-reloc.c

closed
nobody
None
5
2009-10-28
2009-10-24
Charles Wilson
No

It appears that there has been some drift between the versions of pseudo-reloc.c in mingw32, cygwin, and mingw64. Last month, I managed to integrate support for v2 pseudo-relocs into cygwin's runtime, using mingw32's version of pseudo-reloc.c as the basis for that effort. Naturally, changes were required (mostly to ensure that v2 relocs worked across a "fork()" call -- which obviously wasn't an issue for mingw[32|64]. Also, the error handling needed some work, in order to mesh properly with cygwin at this early stage of the application life cycle). All of these changes were guarded by #ifdef CYGWIN, so the effect on mingw32 was practically nil. These changes were merged back in to the mingw32 version.

I had planned on also attempting to harmonize both of those versions with the mingw64 version.

I now notice that some attempt was made at this already, just a few days ago:

2009-10-20 Ozkan Sezer <...>

* crt/pseudo-reloc.c: Merged documentation updates and most of the
whitespace changes from cygwin. Additional whitespace changes.
Shortened some of the error messages. Removed assert.h #include
which isn't necessary any more.

However, the result of Mr. Sezer's effort is still very different from the cygwin (and, as of 10/23/2009, the mingw32) version. Should we make an effort to ensure that these three files are all identical, or should we just continue as we have, with ad-hoc partial synchronization efforts, such as mine and Mr. Sezer's?

Discussion

  • Kai Tietz
    Kai Tietz
    2009-10-24

    Well, my vote goes for a synchronization between all three ventures here. We as author of this piece of code, merged changes done by mingw.org to it already, and improved weaknesses we saw in the mingw.org copy of it by ensuring error messages are shown, and an abort is called. Some of the issues you found are serious IOHO, too. About the fork issue we didn't thought about, as you mention, there is no forking by default for mingw target.
    Sezero merged the comments (and some whitespace trims) of your porting, but we spared out for the moment the cygwin related parts, as I expected that here some further discussions could be helpful.

    Kai

     
  • Charles Wilson
    Charles Wilson
    2009-10-24

    Understood. I figured there would be a few rounds necessary to get this synchronized between all three versions. I've added a patch against the mingw64 version, which includes the necessary CYGWIN changes but uses the framework created when Sezero imported and modified the original cygwin stuff.

    The version created (in mingw64) by applying this patch, when copied verbatim to cygwin and mingw32, works "over there" but I can't test it on mingw64 (but it should have no actual effect for you guys, since everything is inside #ifdefs).

    IMO it's mingw64's "turn": let's try to update mingw-w64-crt/crt/pseudo-reloc.c as needed, and get that committed to your SVN (I'll keep testing each revision on cygwin and mingw). Then, once you've got it committed here, I'll go back to cygwin and mingw and do the "final" synchronization. After that, they should all be the same -- until another round of changes is needed. OK?

    So: what are your thoughts on the attached patch?

     
  • Ozkan Sezer
    Ozkan Sezer
    2009-10-25

    The patch looks OK for me, personally. The only change I suggest is keeping the prototype of _pei386_runtime_relocator() to avoid -Wstrict-prototypes. It seems like I am not allowed to attach a file here, so an interdiff of my local patch and yours look like:

    @@ -48,6 +48,8 @@
    extern char __RUNTIME_PSEUDO_RELOC_LIST_END__;
    extern char __MINGW_LSYMBOL(_image_base__);

    +void _pei386_runtime_relocator (void);
    +
    /* v1 relocation is basically:
    * *(base + .target) += .addend
    * where (base + .target) is always assumed to point

    .. where I also moved th prototype to the top.

     
  • Charles Wilson
    Charles Wilson
    2009-10-25

    That's fine with me. Can you or ktietz check it in -- after you do, I'll take the result back to mingw(32) and cygwin.

     
  • Ozkan Sezer
    Ozkan Sezer
    2009-10-25

    Kai must OK it, first. Then he or I or any other of us can apply it.

     
  • Ozkan,

    modification is ok.

    Thanks
    Kai

     
  • Kai Tietz
    Kai Tietz
    2009-10-25

    Sorry, one comment I have. In your patch you used '# ifdef __MINGW64__', this macro isn't doing that what you expects. __MINGW64__ is just defined for x64 by gcc, but it is no way to differ between runtime header-set. Instead we should check here instead for '__MINGW64_VERSION_MAJOR'.

    Cheers,
    Kai

     
  • Charles Wilson
    Charles Wilson
    2009-10-25

    OK as revised? The new version includes both sezero's and kteitz's suggested changes.

    2009-10-25 Charles Wilson <...>

    * crt/pseudo-reloc.c: Merged additional cygwin changes.

     
  • Ozkan Sezer
    Ozkan Sezer
    2009-10-25

    Applied to the trunk as r1498 and to the 1.0 branch as r1499. I also used __MINGW64_VERSION_MAJOR in place of __MINGW64__. If all are OK, Mr. Wilson can copy over the changes to mingw32 and cygwin and then we can close this entry.

     
  • Charles Wilson
    Charles Wilson
    2009-10-25

    Never mind -- looks like Sezer got it; our updates crossed in the wires. I'll take the result back to mingw(32) and cygwin now. Thanks, guys.

     
  • Kai Tietz
    Kai Tietz
    2009-10-25

    Thanks for your work.

    Cheers,
    Kai

     
  • Charles Wilson
    Charles Wilson
    2009-10-26

    When I took the new version of pseudo-reloc.c back to cygwin, it occurred to me that although I had tested the "working" code path, I had not yet tested the "error path" in that version. And, sure enough, there was a problem. So, the version committed to cygwin and mingw32 is still just slightly different than the current mingw64 version.

    To make matters worse, I *just* noticed a *second* error path that I hadn't explicitly tested -- and it had the same problem that I had fixed, over at cygwin, in the first error path. Sigh.

    So, the attached patch corrects both of those problems (and will result in me having to take another tiny patch back to cygwin/mingw32). But we're slowly converging on a common, stable implementation. I /hope/ this is the last iteration.

    OK?
    2009-10-26 Charles Wilson <...>

    * crt/pseudo-reloc.c (__report_error) [CYGWIN]: Correct size bug
    regarding error messages.

     
  • Ozkan Sezer
    Ozkan Sezer
    2009-10-26

    Applied as obvious to the v1.0 release branch as r1502 and to the trunk as r1503.

     
  • Charles Wilson
    Charles Wilson
    2009-10-26

    When I did an svn update, I got a conflict. Somehow the patch I posted, or which was committed to svn, doesn't match my file -- and in fact, has a bug. It should be fixed by changing line 97 as follows:

    - static const size_t UNKNOWN_MODULE_LEN = sizeof (CYGWIN_FAILURE_MSG) - 1;
    + static const size_t UNKNOWN_MODULE_LEN = sizeof (UNKNOWN_MODULE) - 1;

     
  • Ozkan Sezer
    Ozkan Sezer
    2009-10-26

    Fixed in trunk (r1504) and the v1.0 (r1505).

     
  • Kai Tietz
    Kai Tietz
    2009-10-28

    I hope we are through by this. Any more changes necessary here?

     
  • Ozkan Sezer
    Ozkan Sezer
    2009-10-28

    The cygwin and mingw-w64 versions are in sync. The mingw32 version seems one version behind, yet, but it's up to them to update it. My vote is for closing this one as fixed.

     
  • Charles Wilson
    Charles Wilson
    2009-10-28

    Sure, go ahead and close it. I'm just waiting for the official "OK" from the mingw folks to check in the final version on the mingw32 side. I don't expect any issues.

     
  • Kai Tietz
    Kai Tietz
    2009-10-28

    Ok, so I close it. Thank you all for you work.

     
  • Kai Tietz
    Kai Tietz
    2009-10-28

    • status: open --> closed