Loading ...
Sorry, an error occurred while loading the content.
 

RE: Win64-related patches

Expand Messages
  • Michael Wookey
    ... -register , ... This seems to fix the bug... Index: src/message.c ... +++ src/message.c (working copy) @@ -2987,7 +2987,7 @@ * If verbosefile is set
    Message 1 of 11 , Feb 13, 2007
      > One bug that I didn't fix. Build gvim.exe with OLE=no, run 'gvim
      -register',
      > and watch it crash while trying to display an error message.

      This seems to fix the bug...

      Index: src/message.c
      ===================================================================
      --- src/message.c (revision 212)
      +++ src/message.c (working copy)
      @@ -2987,7 +2987,7 @@
      * If 'verbosefile' is set write message in that file.
      * Must come before the rest because of updating "msg_col".
      */
      - if (*p_vfile != NUL)
      + if (p_vfile && *p_vfile != NUL)
      verbose_write(s, maxlen);

      if (redir_fd != NULL
      Index: src/misc2.c
      ===================================================================
      --- src/misc2.c (revision 212)
      +++ src/misc2.c (working copy)
      @@ -1748,7 +1748,7 @@
      return NULL;
      }
      #endif
      - while ((b = *p) != NUL)
      + while (p && (b = *p) != NUL)
      {
      if (b == c)
      return p;
    • Alexei Alexandrov
      ... I ve checked the patch - works fine for me as well. A couple of other notes: * It seems stuff like # path of the compiler and linker; name of include
      Message 2 of 11 , Feb 13, 2007
        Hi George V. Reilly, you wrote:

        > * Win64 changes to make code compile cleanly: eval.c, misc2.c, if_ole.*
        > ...

        I've checked the patch - works fine for me as well. A couple of other notes:

        * It seems stuff like

        #>>>>> path of the compiler and linker; name of include and lib directories
        # PATH = c:\msvc20\bin;$(PATH)
        # INCLUDE = c:\msvc20\include
        # LIB = c:\msvc20\lib

        should be cleaned out as well.

        * In statements

        CFLAGS = $(CFLAGS) /MD
        LIBC = msvcrt.lib

        and other 3 variants of this, setting LIBC is redundant. The /MXx options already add information about the library to be linked in into object files. I still think that /nodefaultlib should be removed completely. This option is really dangerous and 99% of cases when you have a warning from linker like

        LINK : warning LNK4098: defaultlib "LIBCMT" conflicts with use of other libs; use /NODEFAULTLIB:library

        you're going to have problems due to runtime conflicts.

        * BTW, I've got a nuclear mix of VS + Platform SDK on my home machine which gives the following warning during the compilation:

        c:\program files\microsoft sdk\include\winnt.h(768) : warning C4163: '_rotl64' : not available as an intrinsic function

        I get a lot of them. It's not about this particular patch, though - just a complain. :-)

        --
        Alexei Alexandrov
      • Bram Moolenaar
        ... Well, that may fix it, but the problem is that the order of initializations is violated. Normally all option pointers are not NULL. What is the message
        Message 3 of 11 , Feb 14, 2007
          Michael Wookey wrote:

          > > One bug that I didn't fix. Build gvim.exe with OLE=no, run 'gvim -register',
          > > and watch it crash while trying to display an error message.
          >
          > This seems to fix the bug...
          >
          > Index: src/message.c
          > ===================================================================
          > --- src/message.c (revision 212)
          > +++ src/message.c (working copy)
          > @@ -2987,7 +2987,7 @@
          > * If 'verbosefile' is set write message in that file.
          > * Must come before the rest because of updating "msg_col".
          > */
          > - if (*p_vfile != NUL)
          > + if (p_vfile && *p_vfile != NUL)
          > verbose_write(s, maxlen);
          >
          > if (redir_fd != NULL
          > Index: src/misc2.c
          > ===================================================================
          > --- src/misc2.c (revision 212)
          > +++ src/misc2.c (working copy)
          > @@ -1748,7 +1748,7 @@
          > return NULL;
          > }
          > #endif
          > - while ((b = *p) != NUL)
          > + while (p && (b = *p) != NUL)
          > {
          > if (b == c)
          > return p;
          >

          Well, that may fix it, but the problem is that the order of
          initializations is violated. Normally all option pointers are not NULL.

          What is the message that triggers this problem? That message should
          probably be changed to mch_errmsg().

          --
          The difference between theory and practice, is that in theory, there
          is no difference between theory and practice.

          /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
          /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
          \\\ download, build and distribute -- http://www.A-A-P.org ///
          \\\ help me help AIDS victims -- http://ICCF-Holland.org ///
        • Michael Wookey
          ... The actual error message wrapper is src/gui_w32:ole_error() which just wraps EMSG2(). When calling gvim -register , the error propagates from
          Message 4 of 11 , Feb 14, 2007
            > Michael Wookey wrote:
            >
            > > > One bug that I didn't fix. Build gvim.exe with OLE=no, run 'gvim -
            > register',
            > > > and watch it crash while trying to display an error message.
            > >
            > > This seems to fix the bug...
            > >
            > > Index: src/message.c
            > > ===================================================================
            > > --- src/message.c (revision 212)
            > > +++ src/message.c (working copy)
            > > @@ -2987,7 +2987,7 @@
            > > * If 'verbosefile' is set write message in that file.
            > > * Must come before the rest because of updating "msg_col".
            > > */
            > > - if (*p_vfile != NUL)
            > > + if (p_vfile && *p_vfile != NUL)
            > > verbose_write(s, maxlen);
            > >
            > > if (redir_fd != NULL
            > > Index: src/misc2.c
            > > ===================================================================
            > > --- src/misc2.c (revision 212)
            > > +++ src/misc2.c (working copy)
            > > @@ -1748,7 +1748,7 @@
            > > return NULL;
            > > }
            > > #endif
            > > - while ((b = *p) != NUL)
            > > + while (p && (b = *p) != NUL)
            > > {
            > > if (b == c)
            > > return p;
            > >
            >
            > Well, that may fix it, but the problem is that the order of
            > initializations is violated. Normally all option pointers are not
            > NULL.
            >
            > What is the message that triggers this problem? That message should
            > probably be changed to mch_errmsg().

            The actual error message "wrapper" is src/gui_w32:ole_error() which just
            wraps EMSG2(). When calling "gvim -register", the error propagates from
            src/gui_w32.c:gui_mch_prepare().

            I agree that the above patch fixes the symptom of the bug, but not the
            true cause.

            cheers
          • Mike Williams
            ... Last week s service pack for VS2005 has changed the nmake version number. It just needs the following three lines adding after the ones for the previous
            Message 5 of 11 , Feb 20, 2007
              On 12/02/2007 07:48, George V. Reilly wrote:
              > * Win64 changes to make code compile cleanly: eval.c, misc2.c, if_ole.*
              > * Fixed install.exe bug
              > * Fixed annoying warning from Explorer about gvimext.dll
              > * Fixed gvim.exe.mnf to be cross-platform. No longer needs to be generated
              > from Make_mvc.mak
              > * Re-fixed spell.c so that it works with VC6. Unit tests go into an infinite
              > loop otherwise.
              > * Updated INSTALLpc.txt to reflect that Visual C++ 2005 Express Edition is
              > now free forever, recommending it over the VC 2003 Toolkit.
              > * Cleaned up Make_mvc.mak, incorporating (and fixing) recent patches from
              > Alexei Alexandrov and Mike Williams

              Last week's service pack for VS2005 has changed the nmake version
              number. It just needs the following three lines adding after the ones
              for the previous version for VS2005.

              !if "$(_NMAKE_VER)" == "8.00.50727.762"
              MSVCVER = "8.0"
              !endif

              I also had to edit make_mvc.mak for tests of MSVCVER as nmake did not
              like it enclosed in quotes. The expressions had to be of the form

              !if $(MSVCVER) == ...

              Since MSVCVER is set to a string the extra quotes end up being double
              double quotes which is invalid syntax. Should MSVCVER be set to string
              in the makefile? What do they come through as from the platform SDK?

              Apart from that all seems to work as expected.

              > * Added mkdist.bat to copy all of the installable files to vim70 directory,
              > where they are zipped up, for later installation on Win64 or Win32.
              > * Made a futile attempt to get gvim.nsi building. Just building.
              > Never mind running on Win64.
              > * Fixed a bug in test60: test60.ok must have Unix line endings
              >
              >
              > I have tested this code with the VS 98 (VC6), VS .NET 2003 (VC 7.1),
              > VS 2003 Toolkit (VC 7.1), Visual Studio 2005 (VC8), Visual Studio 2005
              > Express Edition (VC 8), and the VS 2005 x64 cross-compiler.
              >
              > I'll re-test the Win64 binaries on a borrowed AMD64 machine at work
              > tomorrow.
              > As of yesterday, I was able to use install.exe to successfully install
              > gvim and register gvimext.dll, giving the Edit with Vim entry in
              > the Explorer context menu. Once everything is retested, I'll make fresh
              > Win64 binaries available.
              >
              > One bug that I didn't fix. Build gvim.exe with OLE=no, run 'gvim -register',
              > and watch it crash while trying to display an error message.
              >

              Mike
              --
              Sometimes a majority means that all the fools are on one side.
            • Michael Wookey
              Hi Bram, ... I think I ve found it. In src/main.c:main(), the call to gui_prepare() needs to occur after set_init_1() for two reasons: 1. set_init_1() ends up
              Message 6 of 11 , Feb 24, 2007
                Hi Bram,

                > Michael Wookey wrote:
                >
                > > > One bug that I didn't fix. Build gvim.exe with OLE=no, run 'gvim -
                > register',
                > > > and watch it crash while trying to display an error message.
                > >
                > > This seems to fix the bug...
                > >
                > > Index: src/message.c
                > > ===================================================================
                > > --- src/message.c (revision 212)
                > > +++ src/message.c (working copy)
                > > @@ -2987,7 +2987,7 @@
                > > * If 'verbosefile' is set write message in that file.
                > > * Must come before the rest because of updating "msg_col".
                > > */
                > > - if (*p_vfile != NUL)
                > > + if (p_vfile && *p_vfile != NUL)
                > > verbose_write(s, maxlen);
                > >
                > > if (redir_fd != NULL
                > > Index: src/misc2.c
                > > ===================================================================
                > > --- src/misc2.c (revision 212)
                > > +++ src/misc2.c (working copy)
                > > @@ -1748,7 +1748,7 @@
                > > return NULL;
                > > }
                > > #endif
                > > - while ((b = *p) != NUL)
                > > + while (p && (b = *p) != NUL)
                > > {
                > > if (b == c)
                > > return p;
                > >
                >
                > Well, that may fix it, but the problem is that the order of
                > initializations is violated. Normally all option pointers are not
                > NULL.

                I think I've found it. In src/main.c:main(), the call to gui_prepare()
                needs to occur after set_init_1() for two reasons:

                1. set_init_1() ends up initialising the options table - and therefore
                p_vfile.

                2. The win32 implementation of gui_mch_prepare() as called from
                gui_prepare() calls ole_error() in an error path. ole_error() calls
                through to EMSG2() which eventually uses p_vfile. However since
                gui_prepare() is called before set_init_1(), p_vfile has not yet been
                initialised.

                The attached patch demonstrates the solution by moving gui_prepare()
                after set_init_1().

                cheers
              • Michael Wookey
                ... gui_prepare() ... I was over anxious! - attached is the correct patch. The gui_prepare() must also appear before command_line_scan() so that the
                Message 7 of 11 , Feb 24, 2007
                  > Hi Bram,
                  >
                  > > Michael Wookey wrote:
                  > >
                  > > > > One bug that I didn't fix. Build gvim.exe with OLE=no, run 'gvim
                  > -
                  > > register',
                  > > > > and watch it crash while trying to display an error message.
                  > > >
                  > > > This seems to fix the bug...
                  > > >
                  > > > Index: src/message.c
                  > > >
                  ===================================================================
                  > > > --- src/message.c (revision 212)
                  > > > +++ src/message.c (working copy)
                  > > > @@ -2987,7 +2987,7 @@
                  > > > * If 'verbosefile' is set write message in that file.
                  > > > * Must come before the rest because of updating "msg_col".
                  > > > */
                  > > > - if (*p_vfile != NUL)
                  > > > + if (p_vfile && *p_vfile != NUL)
                  > > > verbose_write(s, maxlen);
                  > > >
                  > > > if (redir_fd != NULL
                  > > > Index: src/misc2.c
                  > > >
                  ===================================================================
                  > > > --- src/misc2.c (revision 212)
                  > > > +++ src/misc2.c (working copy)
                  > > > @@ -1748,7 +1748,7 @@
                  > > > return NULL;
                  > > > }
                  > > > #endif
                  > > > - while ((b = *p) != NUL)
                  > > > + while (p && (b = *p) != NUL)
                  > > > {
                  > > > if (b == c)
                  > > > return p;
                  > > >
                  > >
                  > > Well, that may fix it, but the problem is that the order of
                  > > initializations is violated. Normally all option pointers are not
                  > > NULL.
                  >
                  > I think I've found it. In src/main.c:main(), the call to
                  gui_prepare()
                  > needs to occur after set_init_1() for two reasons:
                  >
                  > 1. set_init_1() ends up initialising the options table - and therefore
                  > p_vfile.
                  >
                  > 2. The win32 implementation of gui_mch_prepare() as called from
                  > gui_prepare() calls ole_error() in an error path. ole_error() calls
                  > through to EMSG2() which eventually uses p_vfile. However since
                  > gui_prepare() is called before set_init_1(), p_vfile has not yet been
                  > initialised.
                  >
                  > The attached patch demonstrates the solution by moving gui_prepare()
                  > after set_init_1().

                  I was over anxious! - attached is the correct patch. The gui_prepare()
                  must also appear before command_line_scan() so that the "-register" is
                  recognised.

                  cheers
                • Bram Moolenaar
                  ... Changing the order of initializations is tricky. I would rather not do this without extensive testing. Isn t it much simpler to change ole_error() to
                  Message 8 of 11 , Feb 25, 2007
                    Michael Wookey wrote:

                    > > Michael Wookey wrote:
                    > >
                    > > > > One bug that I didn't fix. Build gvim.exe with OLE=no, run 'gvim
                    > > > > - register', and watch it crash while trying to display an error
                    > > > > message.
                    > > >
                    > > > This seems to fix the bug...
                    > > >
                    > > > Index: src/message.c
                    > > > ===================================================================
                    > > > --- src/message.c (revision 212)
                    > > > +++ src/message.c (working copy)
                    > > > @@ -2987,7 +2987,7 @@
                    > > > * If 'verbosefile' is set write message in that file.
                    > > > * Must come before the rest because of updating "msg_col".
                    > > > */
                    > > > - if (*p_vfile != NUL)
                    > > > + if (p_vfile && *p_vfile != NUL)
                    > > > verbose_write(s, maxlen);
                    > > >
                    > > > if (redir_fd != NULL
                    > > > Index: src/misc2.c
                    > > > ===================================================================
                    > > > --- src/misc2.c (revision 212)
                    > > > +++ src/misc2.c (working copy)
                    > > > @@ -1748,7 +1748,7 @@
                    > > > return NULL;
                    > > > }
                    > > > #endif
                    > > > - while ((b = *p) != NUL)
                    > > > + while (p && (b = *p) != NUL)
                    > > > {
                    > > > if (b == c)
                    > > > return p;
                    > > >
                    > >
                    > > Well, that may fix it, but the problem is that the order of
                    > > initializations is violated. Normally all option pointers are not
                    > > NULL.
                    >
                    > I think I've found it. In src/main.c:main(), the call to gui_prepare()
                    > needs to occur after set_init_1() for two reasons:
                    >
                    > 1. set_init_1() ends up initialising the options table - and therefore
                    > p_vfile.
                    >
                    > 2. The win32 implementation of gui_mch_prepare() as called from
                    > gui_prepare() calls ole_error() in an error path. ole_error() calls
                    > through to EMSG2() which eventually uses p_vfile. However since
                    > gui_prepare() is called before set_init_1(), p_vfile has not yet been
                    > initialised.
                    >
                    > The attached patch demonstrates the solution by moving gui_prepare()
                    > after set_init_1().

                    Changing the order of initializations is tricky. I would rather not do
                    this without extensive testing.

                    Isn't it much simpler to change ole_error() to invoke mch_errmsg()
                    instead of EMSG2()?

                    --
                    hundred-and-one symptoms of being an internet addict:
                    194. Your business cards contain your e-mail and home page address.

                    /// Bram Moolenaar -- Bram@... -- http://www.Moolenaar.net \\\
                    /// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
                    \\\ download, build and distribute -- http://www.A-A-P.org ///
                    \\\ help me help AIDS victims -- http://ICCF-Holland.org ///
                  • Michael Wookey
                    ... been ... do ... You are right - changing the initialisation order is quite a dramatic change. A patch to use mch_errmsg() is attached. There is one bug
                    Message 9 of 11 , Feb 25, 2007
                      > Michael Wookey wrote:
                      >
                      > > > Michael Wookey wrote:
                      > > >
                      > > > > > One bug that I didn't fix. Build gvim.exe with OLE=no, run
                      > 'gvim
                      > > > > > - register', and watch it crash while trying to display an
                      > error
                      > > > > > message.
                      > > > >
                      > > > > This seems to fix the bug...
                      > > > >
                      > > > > Index: src/message.c
                      > > > >
                      > ===================================================================
                      > > > > --- src/message.c (revision 212)
                      > > > > +++ src/message.c (working copy)
                      > > > > @@ -2987,7 +2987,7 @@
                      > > > > * If 'verbosefile' is set write message in that file.
                      > > > > * Must come before the rest because of updating "msg_col".
                      > > > > */
                      > > > > - if (*p_vfile != NUL)
                      > > > > + if (p_vfile && *p_vfile != NUL)
                      > > > > verbose_write(s, maxlen);
                      > > > >
                      > > > > if (redir_fd != NULL
                      > > > > Index: src/misc2.c
                      > > > >
                      > ===================================================================
                      > > > > --- src/misc2.c (revision 212)
                      > > > > +++ src/misc2.c (working copy)
                      > > > > @@ -1748,7 +1748,7 @@
                      > > > > return NULL;
                      > > > > }
                      > > > > #endif
                      > > > > - while ((b = *p) != NUL)
                      > > > > + while (p && (b = *p) != NUL)
                      > > > > {
                      > > > > if (b == c)
                      > > > > return p;
                      > > > >
                      > > >
                      > > > Well, that may fix it, but the problem is that the order of
                      > > > initializations is violated. Normally all option pointers are not
                      > > > NULL.
                      > >
                      > > I think I've found it. In src/main.c:main(), the call to
                      > gui_prepare()
                      > > needs to occur after set_init_1() for two reasons:
                      > >
                      > > 1. set_init_1() ends up initialising the options table - and
                      > therefore
                      > > p_vfile.
                      > >
                      > > 2. The win32 implementation of gui_mch_prepare() as called from
                      > > gui_prepare() calls ole_error() in an error path. ole_error() calls
                      > > through to EMSG2() which eventually uses p_vfile. However since
                      > > gui_prepare() is called before set_init_1(), p_vfile has not yet
                      been
                      > > initialised.
                      > >
                      > > The attached patch demonstrates the solution by moving gui_prepare()
                      > > after set_init_1().
                      >
                      > Changing the order of initializations is tricky. I would rather not
                      do
                      > this without extensive testing.
                      >
                      > Isn't it much simpler to change ole_error() to invoke mch_errmsg()
                      > instead of EMSG2()?

                      You are right - changing the initialisation order is quite a dramatic
                      change. A patch to use mch_errmsg() is attached. There is one bug that
                      needed to be fixed in vim_strchr() to handle a NULL input parameter for
                      this to work correctly (otherwise dereferencing a NULL pointer occurs).

                      cheers
                    Your message has been successfully submitted and would be delivered to recipients shortly.