Menu

#871 line-sequential rewrite broken on win32

unclassified
accepted
nobody
4
2023-02-06
2023-02-06
No

This can be both seen with GCC from MSYS2 (likely from MinGW, too) and with MSVC.
I'd guess the "problematic" ftell is using the same underlying system function.

We do need the correct "current position" and use ftell to get it, but sometimes (after READ?) its position is wrong by 2 (possibly by 3 if UNIX_LF is not active).
This was seen during debugging in lineseq_read() in the two currently failing test cases:

900: LINE SEQUENTIAL REWRITE                         FAILED (run_file.at:12043)
902: Concatenated Files                              FAILED (run_file.at:12401)

MSDN says:

The value returned by ftell [...] may not reflect the physical byte offset for streams opened in text mode [...] use ftell with fseek [...] to return to file locations correctly"

I'm not really sure what that means.... ad if there are possible similar problems on other environments.

As an alternative we could increment the f->record_off field on each read/write on our own.

Discussion

  • Simon Sobisch

    Simon Sobisch - 2023-02-06
     
  • Simon Sobisch

    Simon Sobisch - 2023-02-06

    More debugging and reading (and testing) later:

    For ftell to work properly we need binary mode for both fopen via b mode and for open via O_BINARY.
    I've adjusted fileio.c to always use O_BINARY (that was done in nearly all places already) and use b in the few places where it was missing, depending on cobsetpr->unix_lf.
    With these changes the lsq rewrite tested work fine now with MSYS2 (GCC mingw) and MSVC.

    To really fix this the following code change will likely be needed (too many code paths that weren't taken before to enable this before the 3.2 release):

    • always set "b"inary mode for fmode - no matter the compiler/OS (I think that doesn't harm anywhere, but I'm not sure)
    • activate the never settable cob_ls_uses_cr make the default "true" on Win32, otherwise false
    • drop COB_UNIX_LF as "real" variable, instead: On Win32, if COB_LS_USES_CR is not explicit set additional check for COB_UNIX_LF before enabling it
    • recheck and manually validate cob_ls_uses_cr code paths, possibly add this to the testsuite

    For MSVC to not break with debug builds I also had prevent sync (actually _commit in the MSVC case) after fclose/close and directly after open:

    -       if (cobsetptr->cob_do_sync) {
    +       if (cobsetptr->cob_do_sync
    +        && !last_operation_open
    +        && f->open_mode != COB_OPEN_CLOSED) {
                cob_sync (f);
            }
    

    There were also issues with "double-close", as flcose after close.
    I'd tend to check for f->file first, and if it is set flcose, otherwise close (I think the former will always call the second after flushing the buffers, but I could be wrong). @rjnorman74 Do you have any different experiences here?

     

Log in to post a comment.