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

Re: Regression introduced in patch 7.2.50: fwrite() failures incorrectly checked

Expand Messages
  • Elimar Riesebieter
    * Dominique Pelle [081129 09:10 +0100] ... Same here: Test results: test58 FAILED test59 FAILED TEST FAILURE make[2]: *** [report] Error 1 make[2]: Leaving
    Message 1 of 7 , Nov 29, 2008
    • 0 Attachment
      * Dominique Pelle [081129 09:10 +0100]
      >
      > Tests 58 and 59 fail when doing 'make test' with latest Vim-7.2.55
      > (huge version, tested on Linux).
      >
      > I tried older versions:
      >
      > - Vim-7.2.49 .... All tests pass
      > - Vim-7.2.50 .... Tests 58 & 59 fail.

      Same here:

      Test results:
      test58 FAILED
      test59 FAILED
      TEST FAILURE
      make[2]: *** [report] Error 1
      make[2]: Leaving directory
      `/source/vim/vim-7.2.055/src/vim-basic/testdir'
      make[1]: *** [test] Error 2
      make[1]: Leaving directory `/source/vim/vim-7.2.055/src/vim-basic'
      make: *** [install-stamp-vim-basic] Error 2

      Elimar


      --
      "Talking much about oneself can also
      be a means to conceal oneself."
      -Friedrich Nietzsche

      --~--~---------~--~----~------------~-------~--~----~
      You received this message from the "vim_dev" maillist.
      For more information, visit http://www.vim.org/maillist.php
      -~----------~----~----~----~------~----~------~--~---
    • Dominique Pelle
      ... [...snip...] ... Yes, you re right indeed, all the fwrite() here write only 1 element, so the patch looks OK at first sight. Yet, the patch does break
      Message 2 of 7 , Nov 29, 2008
      • 0 Attachment
        2008/11/29 Bram Moolenaar wrote:

        > Dominique Pelle wrote:
        >
        >> Tests 58 and 59 fail when doing 'make test' with latest Vim-7.2.55
        >> (huge version, tested on Linux).
        >>
        >> I tried older versions:
        >>
        >> - Vim-7.2.49 .... All tests pass
        >> - Vim-7.2.50 .... Tests 58 & 59 fail.
        >>
        >> Patch 7.2.50 introduces the regression:

        [...snip...]

        > All the fwrite() calls have a "number of elements" argument of 1. When
        > fwrite succeeds it returns 1, when it fails it returns 0. So and-ing
        > all the 1 values together gives 1. If there is one zero the and-ed
        > value is zero.
        >
        > Perhaps frwrite() actually returns something different for you? Or
        > there is a problem with the conversion of size_t to int? You can change
        > the type of "fwv" to size_t and see if that makes a difference.


        Yes, you're right indeed, all the fwrite() here write only 1 element, so
        the patch looks OK at first sight. Yet, the patch does break tests 58
        and 59. And reverting it definitely makes the tests pass.

        I added some printf(...) to debug, and I saw that at least in this
        line in spell.c...

        8115 fwv &= fwrite(p, l, (size_t)1, fd);

        ... fwrite(...) returns 0 for me in some cases. It happens because
        second argument l is 0, meaning that it's trying to write 1 element
        of length 0, which is either useless or is a bug (I don't understand
        the code well enough to say). In any case, it's a bit of an odd case.
        Man page of fwrite(...) does not say what fwrite() should return when
        writing 1 element of size 0. I suspect it's not portable (like malloc(0)),
        and for me it returns 0 (interpreted as an error) and for you it returns 1.

        The attached patch makes the tests 58 and 59 succeed, by doing
        the frwrite(...) only if l > 0. But I'm not sure whether it should be
        considered as a workaround or whether it's the actual fix. Also,
        my patch adds only one "if (l > 0)" which is enough to make all test
        pass, but there are a few other places where the same may be
        necessary.

        -- Dominique

        --~--~---------~--~----~------------~-------~--~----~
        You received this message from the "vim_dev" maillist.
        For more information, visit http://www.vim.org/maillist.php
        -~----------~----~----~----~------~----~------~--~---
      • Elimar Riesebieter
        * Dominique Pelle [081129 16:40 +0100] [...] ... Confirmed Elimar -- On the keyboard of life you have always to keep a finger at the escape key;-)
        Message 3 of 7 , Nov 29, 2008
        • 0 Attachment
          * Dominique Pelle [081129 16:40 +0100]
          [...]
          > The attached patch makes the tests 58 and 59 succeed, by doing

          Confirmed

          Elimar


          --
          On the keyboard of life you have always
          to keep a finger at the escape key;-)

          --~--~---------~--~----~------------~-------~--~----~
          You received this message from the "vim_dev" maillist.
          For more information, visit http://www.vim.org/maillist.php
          -~----------~----~----~----~------~----~------~--~---
        • Bram Moolenaar
          ... Thanks for pinning this down. It does make sense, I ll include the patch. I don t think any of the other fwrite() can have a zero size. -- GUARD #1:
          Message 4 of 7 , Nov 29, 2008
          • 0 Attachment
            Dominique Pelle wrote:

            > >> Tests 58 and 59 fail when doing 'make test' with latest Vim-7.2.55
            > >> (huge version, tested on Linux).
            > >>
            > >> I tried older versions:
            > >>
            > >> - Vim-7.2.49 .... All tests pass
            > >> - Vim-7.2.50 .... Tests 58 & 59 fail.
            > >>
            > >> Patch 7.2.50 introduces the regression:
            >
            > [...snip...]
            >
            > > All the fwrite() calls have a "number of elements" argument of 1. When
            > > fwrite succeeds it returns 1, when it fails it returns 0. So and-ing
            > > all the 1 values together gives 1. If there is one zero the and-ed
            > > value is zero.
            > >
            > > Perhaps frwrite() actually returns something different for you? Or
            > > there is a problem with the conversion of size_t to int? You can change
            > > the type of "fwv" to size_t and see if that makes a difference.
            >
            >
            > Yes, you're right indeed, all the fwrite() here write only 1 element, so
            > the patch looks OK at first sight. Yet, the patch does break tests 58
            > and 59. And reverting it definitely makes the tests pass.
            >
            > I added some printf(...) to debug, and I saw that at least in this
            > line in spell.c...
            >
            > 8115 fwv &= fwrite(p, l, (size_t)1, fd);
            >
            > ... fwrite(...) returns 0 for me in some cases. It happens because
            > second argument l is 0, meaning that it's trying to write 1 element
            > of length 0, which is either useless or is a bug (I don't understand
            > the code well enough to say). In any case, it's a bit of an odd case.
            > Man page of fwrite(...) does not say what fwrite() should return when
            > writing 1 element of size 0. I suspect it's not portable (like malloc(0)),
            > and for me it returns 0 (interpreted as an error) and for you it returns 1.
            >
            > The attached patch makes the tests 58 and 59 succeed, by doing
            > the frwrite(...) only if l > 0. But I'm not sure whether it should be
            > considered as a workaround or whether it's the actual fix. Also,
            > my patch adds only one "if (l > 0)" which is enough to make all test
            > pass, but there are a few other places where the same may be
            > necessary.

            Thanks for pinning this down. It does make sense, I'll include the
            patch. I don't think any of the other fwrite() can have a zero size.

            --
            GUARD #1: What, ridden on a horse?
            ARTHUR: Yes!
            GUARD #1: You're using coconuts!
            ARTHUR: What?
            GUARD #1: You've got two empty halves of coconut and you're bangin' 'em
            together.
            The Quest for the Holy Grail (Monty Python)

            /// 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 ///

            --~--~---------~--~----~------------~-------~--~----~
            You received this message from the "vim_dev" maillist.
            For more information, visit http://www.vim.org/maillist.php
            -~----------~----~----~----~------~----~------~--~---
          • Bill McCarthy
            ... I tried Make_ming.mak from Windows - test58 failed with patch level 55. With patch level 57 all went fine! I noted that for the testdir directory, all
            Message 5 of 7 , Nov 29, 2008
            • 0 Attachment
              On Sat 29-Nov-08 1:12pm -0600, Bram Moolenaar wrote:
              > Dominique Pelle wrote:

              > Thanks for pinning this down. It does make sense, I'll include the
              > patch. I don't think any of the other fwrite() can have a zero size.

              I tried Make_ming.mak from Windows - test58 failed with
              patch level 55.

              With patch level 57 all went fine!

              I noted that for the testdir\ directory, all *.in, *.ok,
              Make_ming.mak, Make_vms.mms and main.aap are unix files. I
              changed these to ff=dos and windiff found no differences
              (except the .svn stuff).

              --
              Best regards,
              Bill


              --~--~---------~--~----~------------~-------~--~----~
              You received this message from the "vim_dev" maillist.
              For more information, visit http://www.vim.org/maillist.php
              -~----------~----~----~----~------~----~------~--~---
            Your message has been successfully submitted and would be delivered to recipients shortly.