weird warning when building against ARM64EC UCRT

What about building with --enable-portable? You only need decoding, not file I/O?

Im kinda sick of this political game of Microsoft, suggesting the POSIX names are in
conflict with ISO C.
--
sent from mobile device, trustworthy or not

Well, messing up use of plain lseek counts as part of the same game to me. But let's get down to the technical. We're only talking about this code, right?

static int64_t internal_lseek64(void *handle, int64_t offset, int whence)
{
    struct wrap_data* ioh = handle;
#ifdef LFS_LARGEFILE_64
    return lseek64(ioh->fd, offset, whence);
#else
    if(offset < OFF_MIN || offset > OFF_MAX)
    {
        errno = EOVERFLOW;
        return -1;
    }
    return lseek(ioh->fd, (off_t)offset, whence);
#endif
}

With the corollary of off64_t being a thing or not. Since we contained all specific API already so far. I'd be fine with some path there for both _lseeki64 and _lseek with explicit cast to long.

Are you up to making (and crucially, testing) a patch of the configure.ac and ports/cmake/src/CMakeLists.txt parts to detect the MSVC variant of LFS_LARGEFILE_64, probably typedef of off64_t to int64_t while at that, and then have the lfs_wrap.c code use the MSVC way for non-largefile and 64 bit access? The configure test could just skimp on the actual testing and check the platform string. Even better if it doesn't run needless tests on a POSIX system.

With all the specific stuff contained in the LFS wrapper code, it indeed shouldn't be that hard. I expect minimal changes there. Remaining plain POSIX stuff in the mpg123 applications shouldn't bother MSVC library users.

I think we should not just do a hack to use _lseek instead of lseek, but support proper 64 bit offsets while at that.

I honestly do not see how this would work.
The old _64-suffixed API functions assume aliasing of off_t and off64_t iff off_t is 64bit, or a distinct 64bit off64_t otherwise. off_t will always be 32bit with MSVC, and libmpg123 should never invent/define any off64_t in a public API header itself. This would break immediately if any other library does the same thing. So, if the public API can neither change nor be extended (with the existing _64-suffixed functions), is there any point in calling _lseeki64 at all (I may well be missing some call paths here, that code is not exactly easy to follow :))? If there is, would just calling _lseeki64 instead of _lseek in internal_lseek64 do the trick, or is more of the LFS_LARGEFILE_64 machinery needed? I also fail to see any benefit in inventing new _64-suffixed declarations just for MSVC using int64_t instead of off64_t. The portable API already covers this case in a way better way.

Neither calling _lseeki64 nor _lseek in internal_lseek64 would require any change in CMake or Autoconf, instead it would even consolidate some incomplete checks already done for _open.

Draft patch attached.

Without yet having looked at the patch (just read your message in the train), I want to make more clear what I meant: I was talking about _lseeki64() and some kind of off64_t usage in lfs_wrap.c only. This is the one place that implements seeks for any public API.

Looking again … indeed, there should be no need to define any off64_t on MSVC. There is public 32 bit ABI with off_t, mapping to the _32 functions in lfs_wrap.c.

All further code paths use int64_t internally to pass things around.

We just need int64_t internal_lseek64() implemented to trigger a 64 bit seek if it is available and check that int INT123_wrap_open() does the right thing through INT123_compat_open() to enable largefile support.

This will only be usable through the int64_t API, not a off_t. This is the nature of things on a platform that doesn't do largefile off_t. We won't change that. But if there is explicit 64 bit API and we adapt to MSVC/CRT specifics, it would be strange not to enable 64 bit offsets internally.

Now looking at what you proposed … looks a lot like what I described above;-)
I think I'd rather

#define read _read
#define close _close

for the trivial ones up top. Less noise in the code. But for the plain _lseek(), I rather see the explicit call because it strictly is not the same prototype than lseek().

+#if defined(MPG123_COMPAT_MSVCRT_IO)
+   return _lseek(ioh->fd, (long)offset, whence);
+#else
    return lseek(ioh->fd, (off_t)offset, whence);
 #endif

We know that offset < OFF_MIN || offset > OFF_MAX will also be correct for long here … but maybe that bit should also be duplicated for clarity and cover LONG_MIN and LONG_MAX instead. It is a bit silly, but compilers are complaining about types that are identical but called differently nowadays. Speaking of which …

+#if defined(MPG123_COMPAT_MSVCRT_IO)
+       ioh->r_lseek = r_lseek != NULL ? r_lseek : _lseek;
+#else
        ioh->r_lseek = r_lseek != NULL ? r_lseek : lseek;
+#endif

The r_lseek pointer is using off_t in the prototype, not matching _lseek(). I guess we need a pointless fallback_lseek() wrapper just like fallback_read() to ensure the pointer type matches. And yes, I guess I should add an explicit cast and possibly even an overflow check to fallback_read() byte count argument.

But all in all … I guess this should be it to fix the build and by the way enable a possible 64 bit offset code path in the iternal I/O that used to only do 32 bits. Counts as a bug fix, even, in my book.

Is there no extra flag needed to make _open() open the file in 64 bit mode?

I really do not like

#define read _read
#define close _close

at all. That would be redefining names that do not belong to mpg123.

But for the plain _lseek(), I rather see the explicit call because it strictly is not the same prototype than lseek().
The r_lseek pointer is using off_t in the prototype, not matching _lseek(). I guess we need a pointless fallback_lseek() wrapper just like fallback_read() to ensure the pointer type matches.

Yeah, that was already wrong before for MSVCRT lseek(), which also uses long. Fixed.

Is there no extra flag needed to make _open() open the file in 64 bit mode?

No. MSVCRT does not need that. O_LARGEFILE (and friends) qualify as one of the most stupid API design decisions in the history of computing for me. It's solely a POSIX-ism, and frankly makes no sense.

We know that offset < OFF_MIN || offset > OFF_MAX will also be correct for long here … but maybe that bit should also be duplicated for clarity and cover LONG_MIN and LONG_MAX instead.

Fixed.

And yes, I guess I should add an explicit cast and possibly even an overflow check to fallback_read() byte count argument.

And I guess also internal_read64().

OK. Committed … with a bit of tweaking on top (attached). Does this work fine for you?

Yeah, works for me.

Looks like I missing switching to #if defined(MPG123_COMPAT_MSVCRT_IO) in INT123_compat_close. Can you do that as well?

OK, like that in trunk? We didn't discuss much the switch from WIN32 to the more specific compiler identities. You think that's safer and some other runtime that doesn't define UCRT will also not share its peculiarities?

I think the "demanding to use the _-variants" is Microsoft specific, I am not aware of any situation where the POSIX names do not exist while the _-variants do exist.

I really do not know if "_WIN32 implies _-variants" holds true in all situations, so defaulting to the POSIX names by default is probably a more conservative approach. Assuming all Windows C libraries are implementing _-variants sounds somewhat fragile to me.

On the other hand, with the current state there is a risk of missing some C libraries that mimic Microsoft's implementation (not a regression compared to what was there before, though).

Looking at documentation only, it looks like Watcom also provides the Microsoft-style names and would still be missing 64bit support. I do not have any deep experience with Watcom though.
Patch attached, based on https://open-watcom.github.io/open-watcom-v2-wikidocs/clib.pdf, https://en.wikipedia.org/wiki/Watcom_C/C%2B%2B#Release_history, https://open-watcom.github.io/open-watcom-v2-wikidocs/clr.html#Open_Watcom_CD16_and_CD32_Predefined_Macros, and https://github.com/cpredef/predef/blob/master/Compilers.md#watcom-c.
I do not know if Watcom's C/C99 support is even sufficient to build libmpg123.
Also, Watcom is using off_t like in POSIX instead of Microsoft's long. Making every situation hyper-correct is probably not worth the effort here, though.

I guess you could add Autoconf and CMake checks for the i64 variants if you really want to catch all situations.

What I do know is: the 64bit variant is not available in all situations I care about. In particular not with MinGW targeting CRTDLL (the C library shipped with the first release of Windows 95).

OK, applied. But we indeed should not spend too much work on platforms we don't really test.