Re: [Mingw-w64-public] [PATCH 3/3] headers: Use register variable for NtCurrentTeb implementation on aarch64.
On 29.12.2024 18:57, Jeremy Drake wrote: On Sun, 29 Dec 2024, Jacek Caban wrote: We may need to revert use of register keyword for TEB access or make it conditional if that's indeed problematic for C++. I couldn't reproduce the warning with Clang. I did some playing with godbolt.org. It seems OK with `register` with `__asm__`: https://godbolt.org/z/xrdn3rG45 but gives an error without: error: ISO C++17 does not allow 'register' storage class specifier [-Wregister] https://godbolt.org/z/cMbhhffhs (of course, it also gives an error for file-scoped register variable too). It's good to know that our use of the register keyword is fine and that the only issue lies with the -Dregister= directive in Doxygen. It seems Doxygen enables this behavior only for Flex versions older than 2.6.0, which is now 9 years old. I'm not very familiar with MSYS2, but it appears the Doxygen package might be missing a Flex build dependency. Could this explain why this outdated code path is being used? Thanks, Jacek ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 1/3] headers: Treat __MSVCRT_VERSION__=0x600 as compiling for msvcrt.dll ABI
On Monday 30 December 2024 15:32:16 Martin Storsjö wrote: > On Thu, 12 Dec 2024, Martin Storsjö wrote: > > > On Thu, 12 Dec 2024, LIU Hao wrote: > > > > > 在 2024-12-12 21:17, Martin Storsjö 写道: > > > > On Thu, 28 Nov 2024, Pali Rohár wrote: > > > > > msvcrt.dll preinstalled as part of the Windows system provides more > > > > > functions than the original Visual C++ 6.0, but still it is > > > > > compatible with > > > > > the original version. So setting __MSVCRT_VERSION__ to 0x600 > > > > > for msvcrt.dll > > > > > build is relatively sane option. > > > > > > > > > > I was considering to use 0x6FF value for this case, but this > > > > > would make it > > > > > more complicated for applications if they need to know if they are > > > > > being > > > > > compiled for msvcrt.dll library ABI. They would need to > > > > > check if the macro > > > > > __MSVCRT_VERSION__ is set to 0x600 (targetting the original > > > > > VC++ msvcrt.dll > > > > > library) or to value 0x6FFF (targettting the system msvcrt.dll > > > > > library). > > > > > This looks to be additional complication as it would be > > > > > needed to track all > > > > > possible values for __MSVCRT_VERSION__. So using just one value 0x600 > > > > > for > > > > > both cases should be enough because for system version > > > > > specific version is > > > > > always needed to check also _WIN32_WINNT value. > > > > > > > > I think this change looks reasonable to me, but I'd like Liu > > > > Hao, who suggested the 0x6FF version, to follow up here. > > > > > > I'm fine with this change, too. > > > > Thanks, I pushed this one now then. > > It turns out that this change did cause some breakage in user projects - see > https://github.com/msys2/MINGW-packages/pull/22907. > > Also see > https://codesearch.debian.net/search?q=if.*__MSVCRT_VERSION__.*0x0*%5B76%5D&literal=0&page=2 > - it seems that there are a number of other projects that also have > conditionals relating to this, which also may run into trouble. > > (E.g. with old mingw.org headers, functions like _aligned_malloc used to be > guarded within "#if __MSVCRT_VERSION__ >= 0x0700", so some projects either > check __MSVCRT_VERSION__ to know whether it is available, or even try to set > __MSVCRT_VERSION__ themselves in order to opt in to using such functions, > that were available in msvcrt.dll since XP.) > > Not saying that this requires revisiting our decision, but it may cause some > amount of confusion and breakage in existing user code out there. > > // Martin That is not good. But I do not know what can be done better. If the application is using #if __MSVCRT_VERSION__ >= ... and one of the preprocessor branch result in broken compiled application then it is obviously bug in application. But I do not know what to do with it. Either we need to say that the whole __MSVCRT_VERSION__ macro is now broken because it is wrongly used by applications and we should rather deprecate it (by providing some fixed value to let application compile) and create a new macro e.g. __MSVCRT_VERSION_REAL__ which would contain the real version. Or we need to say that it is application bug and do not care about it. Or maybe completely remove __MSVCRT_VERSION__ macro and always provides all function declarations in header files. But I think that any of those options just cause new problems. This does not have a sane solution at all. Whatever solution is chosen, I would propose to put big warning into release notes what is happening and how application code could be adjusted. ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] headers: undef register in case projects define it.
On 30.12.2024 20:09, Jeremy Drake via Mingw-w64-public wrote:
For example, doxygen defines `register` to shut up an error due to it being
deprecated/removed in ISO C++ 17, but that causes issues with this
construct, which is an extension and still supported.
Fixes: f0044963d7ba ("headers: Use register variable for NtCurrentTeb implementation
on aarch64.")
---
Is this OK, or does it need any other preprocessor checks before using
push_macro/pop_macro? I grepped for other usages and it seems like this
is how it's done...
Assuming this is indeed a concern for mingw-w64, the patch seems fine to
me. However, I’m not fully convinced it’s necessary, -Dregister= is a
rather awful hack in general.
As I mentioned in the other thread, doxygen appears to tamper with the
register definition only for outdated flex versions. If that’s the case,
this might be an MSYS2 packaging issue. Are there other instances of
similar problems?
Thanks,
Jacek
___
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [RFC PATCH 2/2] crt: Merge lib32/ws2_32.def into lib-common/ws2_32.def.in
On Sunday 29 December 2024 17:46:22 Martin Storsjö wrote: > On Sat, 14 Dec 2024, Pali Rohár wrote: > > > Now all I386 symbols in lib-common/ws2_32.def.in file are defined with > > stdcall @ suffixes. These suffixes are automatically removed for > > non-I386 builds by Makefile.am rule during processing of > > lib-common/*.def.in files. > > > > During merging of lib32/ws2_32.def and lib-common/ws2_32.def.in files, > > all symbols were sorted in the final file. > > --- > > mingw-w64-crt/lib-common/ws2_32.def.in | 362 - > > mingw-w64-crt/lib32/ws2_32.def | 188 - > > 2 files changed, 181 insertions(+), 369 deletions(-) > > delete mode 100644 mingw-w64-crt/lib32/ws2_32.def > > A change like this looks acceptable to me. It's very hard to keep track of > what happens here though, but we would need to trust you to verify that the > output set of symbols stays the same (or ends up as a supserset of the > previous set of symbols) for each architecture - or set up procedures for > doublechecking it. (In some cases, adding symbols that didn't exist before > can be problematic, e.g. in case of some symbols in kernel32.dll or similar, > but for most cases, making the def files include more symbols than before is > fine.) I understand. I did my own checks that the list of symbols for individual arch after running those preprocessing, is same as before this change. No symbol added or removed. > Here, it's a bit tricky/awkward when we already have some symbols within > F64(), like F64(WSCEnableNSProvider32) - as those don't get any stdcall > suffix, as there is no reference for what the suffix would be in that case. > > // Martin That is cost of the fact that "no new symbol for i386 was added" by this change. I have not even tried to check if that symbol is in some new version of ws2_32 for I386 or not. Do you think that this is a direction which can follow? Biswapriyo, what is your opinion? Would it be easier or harder to do new changes in def files? ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 1/3] headers: Treat __MSVCRT_VERSION__=0x600 as compiling for msvcrt.dll ABI
On Thu, 12 Dec 2024, Martin Storsjö wrote: On Thu, 12 Dec 2024, LIU Hao wrote: 在 2024-12-12 21:17, Martin Storsjö 写道: On Thu, 28 Nov 2024, Pali Rohár wrote: msvcrt.dll preinstalled as part of the Windows system provides more functions than the original Visual C++ 6.0, but still it is compatible with the original version. So setting __MSVCRT_VERSION__ to 0x600 for msvcrt.dll build is relatively sane option. I was considering to use 0x6FF value for this case, but this would make it more complicated for applications if they need to know if they are being compiled for msvcrt.dll library ABI. They would need to check if the macro __MSVCRT_VERSION__ is set to 0x600 (targetting the original VC++ msvcrt.dll library) or to value 0x6FFF (targettting the system msvcrt.dll library). This looks to be additional complication as it would be needed to track all possible values for __MSVCRT_VERSION__. So using just one value 0x600 for both cases should be enough because for system version specific version is always needed to check also _WIN32_WINNT value. I think this change looks reasonable to me, but I'd like Liu Hao, who suggested the 0x6FF version, to follow up here. I'm fine with this change, too. Thanks, I pushed this one now then. It turns out that this change did cause some breakage in user projects - see https://github.com/msys2/MINGW-packages/pull/22907. Also see https://codesearch.debian.net/search?q=if.*__MSVCRT_VERSION__.*0x0*%5B76%5D&literal=0&page=2 - it seems that there are a number of other projects that also have conditionals relating to this, which also may run into trouble. (E.g. with old mingw.org headers, functions like _aligned_malloc used to be guarded within "#if __MSVCRT_VERSION__ >= 0x0700", so some projects either check __MSVCRT_VERSION__ to know whether it is available, or even try to set __MSVCRT_VERSION__ themselves in order to opt in to using such functions, that were available in msvcrt.dll since XP.) Not saying that this requires revisiting our decision, but it may cause some amount of confusion and breakage in existing user code out there. // Martin ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
[Mingw-w64-public] [PATCH] headers: undef register in case projects define it.
For example, doxygen defines `register` to shut up an error due to it being
deprecated/removed in ISO C++ 17, but that causes issues with this
construct, which is an extension and still supported.
Fixes: f0044963d7ba ("headers: Use register variable for NtCurrentTeb
implementation on aarch64.")
---
Is this OK, or does it need any other preprocessor checks before using
push_macro/pop_macro? I grepped for other usages and it seems like this
is how it's done...
mingw-w64-headers/include/winnt.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mingw-w64-headers/include/winnt.h
b/mingw-w64-headers/include/winnt.h
index b6d1f58ed..e6fdfac51 100644
--- a/mingw-w64-headers/include/winnt.h
+++ b/mingw-w64-headers/include/winnt.h
@@ -10211,7 +10211,10 @@ typedef DWORD (WINAPI
*PRTL_RUN_ONCE_INIT_FN)(PRTL_RUN_ONCE, PVOID, PVOID *);
PVOID GetFiberData(VOID);
#if defined (__aarch64__) || defined(__arm64ec__)
+#pragma push_macro ("register")
+#undef register
register struct _TEB *__mingw_current_teb __asm__("x18");
+#pragma pop_macro ("register")
FORCEINLINE struct _TEB *NtCurrentTeb(VOID)
{
return __mingw_current_teb;
--
2.47.1.windows.1
___
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] headers: undef register in case projects define it.
在 2024-12-31 04:46, Jacek Caban 写道: Assuming this is indeed a concern for mingw-w64, the patch seems fine to me. However, I’m not fully convinced it’s necessary, -Dregister= is a rather awful hack in general. As I mentioned in the other thread, doxygen appears to tamper with the register definition only for outdated flex versions. If that’s the case, this might be an MSYS2 packaging issue. Are there other instances of similar problems? I kinda agree with this. Defining a macro which matches a keyword is undefined behavior, which nobody should do. -- Best regards, LIU Hao OpenPGP_signature.asc Description: OpenPGP digital signature ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH 1/3] headers: Treat __MSVCRT_VERSION__=0x600 as compiling for msvcrt.dll ABI
On Monday 30 December 2024 18:50:57 Pali Rohár wrote: > On Monday 30 December 2024 15:32:16 Martin Storsjö wrote: > > On Thu, 12 Dec 2024, Martin Storsjö wrote: > > > > > On Thu, 12 Dec 2024, LIU Hao wrote: > > > > > > > 在 2024-12-12 21:17, Martin Storsjö 写道: > > > > > On Thu, 28 Nov 2024, Pali Rohár wrote: > > > > > > msvcrt.dll preinstalled as part of the Windows system provides more > > > > > > functions than the original Visual C++ 6.0, but still it is > > > > > > compatible with > > > > > > the original version. So setting __MSVCRT_VERSION__ to 0x600 > > > > > > for msvcrt.dll > > > > > > build is relatively sane option. > > > > > > > > > > > > I was considering to use 0x6FF value for this case, but this > > > > > > would make it > > > > > > more complicated for applications if they need to know if they are > > > > > > being > > > > > > compiled for msvcrt.dll library ABI. They would need to > > > > > > check if the macro > > > > > > __MSVCRT_VERSION__ is set to 0x600 (targetting the original > > > > > > VC++ msvcrt.dll > > > > > > library) or to value 0x6FFF (targettting the system msvcrt.dll > > > > > > library). > > > > > > This looks to be additional complication as it would be > > > > > > needed to track all > > > > > > possible values for __MSVCRT_VERSION__. So using just one value > > > > > > 0x600 for > > > > > > both cases should be enough because for system version > > > > > > specific version is > > > > > > always needed to check also _WIN32_WINNT value. > > > > > > > > > > I think this change looks reasonable to me, but I'd like Liu > > > > > Hao, who suggested the 0x6FF version, to follow up here. > > > > > > > > I'm fine with this change, too. > > > > > > Thanks, I pushed this one now then. > > > > It turns out that this change did cause some breakage in user projects - see > > https://github.com/msys2/MINGW-packages/pull/22907. > > > > Also see > > https://codesearch.debian.net/search?q=if.*__MSVCRT_VERSION__.*0x0*%5B76%5D&literal=0&page=2 > > - it seems that there are a number of other projects that also have > > conditionals relating to this, which also may run into trouble. > > > > (E.g. with old mingw.org headers, functions like _aligned_malloc used to be > > guarded within "#if __MSVCRT_VERSION__ >= 0x0700", so some projects either > > check __MSVCRT_VERSION__ to know whether it is available, or even try to set > > __MSVCRT_VERSION__ themselves in order to opt in to using such functions, > > that were available in msvcrt.dll since XP.) > > > > Not saying that this requires revisiting our decision, but it may cause some > > amount of confusion and breakage in existing user code out there. > > > > // Martin > > That is not good. But I do not know what can be done better. If the > application is using #if __MSVCRT_VERSION__ >= ... and one of the > preprocessor branch result in broken compiled application then it is > obviously bug in application. > > But I do not know what to do with it. Either we need to say that the > whole __MSVCRT_VERSION__ macro is now broken because it is wrongly used > by applications and we should rather deprecate it (by providing some > fixed value to let application compile) and create a new macro e.g. > __MSVCRT_VERSION_REAL__ which would contain the real version. > > Or we need to say that it is application bug and do not care about it. > > Or maybe completely remove __MSVCRT_VERSION__ macro and always provides > all function declarations in header files. > > But I think that any of those options just cause new problems. This does > not have a sane solution at all. > > Whatever solution is chosen, I would propose to put big warning into > release notes what is happening and how application code could be > adjusted. I was thinking more about this problem... It looks like that most "problematic" SW either check or set __MSVCRT_VERSION__ macro against value 0x0601. What about setting __MSVCRT_VERSION__ to value 0x0601 in configure phase and then in header files just do relaxed check that upper word is 0x06? #if (__MSVCRT_VERSION__ >> 8) == 0x06 ... compiling for system msvcrt.dll ... #endif So whatever 0x06?? value the application code choose, mingw-w64 header files would interpret it as using system msvcrt.dll. If I'm looking correctly at this, it should also fix the problem with OSGeo/gdal project. And maybe instead of 0x0601 set it in configure phase to 0x06FF what Liu Hao suggested? PS: Nice tool for regex searching across public git repositories is sourcegraph.com: https://sourcegraph.com/search?q=context:global+if.*__MSVCRT_VERSION__.*0x&patternType=regexp&sm=0 ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
Re: [Mingw-w64-public] [PATCH] headers: undef register in case projects define it.
On Mon, Dec 30, 2024 at 9:47 PM Jacek Caban wrote: > As I mentioned in the other thread, doxygen appears to tamper with the > register definition only for outdated flex versions. If that’s the case, > this might be an MSYS2 packaging issue. Are there other instances of > similar problems? Good catch, this is a bug in the doxygen build script. I've created https://github.com/doxygen/doxygen/pull/11302 ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
