Re: [Mingw-w64-public] [PATCH] crt: Use importlibs for more time functions.
在 2019/4/8 下午7:18, Jacek Caban 写道: > > Sure, but what does it have to do with __forceinline? __forceinline is > for functions that are really meant to be inlined and thus I think it > should not try to preserve single pointer value. With removed extern, we > leave the choice for user, one could still use extern __forceinline. > There is no guarantee that a `__forceinline` function will be inlined; in case of inline failure a user would get an undefined reference. But in reality, all SSE intrinsic functions provided by GCC are marked `extern __inline __attribute__((__gnu_inline__, __always_inline__))`. So I think the `extern` specifier is unnecessary for 'functions that are really meant to be inlined'. As for our current implementation (`__gnu_inline__` for C) there will be no external definitions, no matter whether the function is inlined or not, and `extern __forceinline` will be exactly the same as `__forceinline`. So what is the use of `extern` there? > > And in this particular context of time functions, we really don't want > to allow compiler/linker using the available symbol. It may be a > different variant of the function than was used by the header. > > Yeah I agree. The `extern` can be removed from `__forceinline` provided that such functions must be inlined. -- Best regards, LH_Mouse 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] crt: Use importlibs for more time functions.
On 4/5/19 3:43 PM, Martin Storsjö wrote: How about doing things in msvc ucrt compatible way? Like the patch I attached as an example (not yet tested)? This looks good to me (but didn't test it yet). This approach of modernizing small areas at a time seems good also, wrt finding out what regressions it might cause, compared to larger refactorings of everything at once. In general I think we should review all use of __forceinline/__CRT_INLINE wrt to what symbols actually are available in UCRT. Testing looks good, I will send the patch for review. 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] crt: Use importlibs for more time functions.
On 4/8/19 4:33 AM, Liu Hao wrote: 在 2019/4/5 下午8:07, Jacek Caban 写道: On 4/5/19 9:15 AM, Martin Storsjö wrote: It would be great to have __forceinline fixed. I've seen a problems in code using static __forceinline multiple times. I also recall that attempts to change that had their problems, so it's more tricky than it seems (if possible). The `extern` specifier is required in the case of C (but is not in C++) because plain `inline` is not the same thing with `extern inline` [1]. When compiled as C++, `extern` has no effect on an `inline` function. If the entire function body is inlined then no external definition is emitted; otherwise a weak definition is emitted, so taking the address of an inline function in all translate units returns the same value, and no multiple definition errors may occur. I haven't heard of such a thing in C89 mode; it could be achieved by using the `weak` or `selectany` attribute with C99 `extern inline` semantics, anyway. Sure, but what does it have to do with __forceinline? __forceinline is for functions that are really meant to be inlined and thus I think it should not try to preserve single pointer value. With removed extern, we leave the choice for user, one could still use extern __forceinline. And in this particular context of time functions, we really don't want to allow compiler/linker using the available symbol. It may be a different variant of the function than was used by the header. 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] crt: Use importlibs for more time functions.
On Fri, 5 Apr 2019, Jacek Caban wrote: On 4/5/19 9:15 AM, Martin Storsjö wrote: On Thu, 4 Apr 2019, Jacek Caban wrote: On 4/4/19 3:07 PM, Martin Storsjö wrote: On Thu, 4 Apr 2019, Jacek Caban wrote: Signed-off-by: Jacek Caban --- mingw-w64-crt/Makefile.am | 2 +- mingw-w64-crt/lib-common/msvcrt.def.in | 4 +- mingw-w64-crt/lib32/msvcr100.def.in | 4 +- mingw-w64-crt/lib32/msvcr80.def.in | 2 + mingw-w64-crt/lib32/msvcr90.def.in | 4 +- mingw-w64-crt/lib64/msvcr100.def.in | 4 +- mingw-w64-crt/lib64/msvcr80.def.in | 2 + mingw-w64-crt/lib64/msvcr90.def.in | 4 +- mingw-w64-crt/misc/difftime.c | 21 -- mingw-w64-crt/misc/difftime32.c | 9 - mingw-w64-crt/misc/difftime64.c | 9 - mingw-w64-headers/crt/time.h | 55 +++--- 12 files changed, 37 insertions(+), 83 deletions(-) delete mode 100644 mingw-w64-crt/misc/difftime.c delete mode 100644 mingw-w64-crt/misc/difftime32.c delete mode 100644 mingw-w64-crt/misc/difftime64.c Ok with me, if you extend the commit message to mention all the changes. The changes IMO now include the following three change categories: - Use importlibs for more time functions - Unify inline attribute strategies for time functions in headers - Use _CRTIMP on more time functions Multiple line in commit message usually is a good sign that the patch should be split ;) And I agree, I could do better job at that. I split the patch and pushed. This turned out to break compilation in certain cases for me. The culprit is that __forceinline is a bit deceptive (and so is __CRT_INLINE). For good reasons I was using __mingw_static_ovr here before, for the UCRT case. These three macros expand to slightly different things depending on compiler and context, but for the C, modern clang/gcc case, they are as follows: #define __CRT_INLINE extern inline __attribute__((__gnu_inline__)) #define __forceinline extern __inline__ __attribute__((__always_inline__,__gnu_inline__)) #define __mingw_static_ovr static __attribute__ ((__unused__)) __inline__ __cdecl Both __CRT_INLINE and __forceinline declare the inline functions as extern, in combination with the gnu_inline attribute. The gnu_inline attribute, in combination with extern, means that when you take the address of the function, it shouldn't generate and reference the inline version of the function, but reference the extern copy of the function available in another translation unit. This obviously is desireable for getting the same function pointer to a function in all translation units that refer to it, but doesn't work for functions that don't exist elsewhere in that form. I'm not sure about how many of the existing cases of __forceinline and __CRT_INLINE that really intentionally want this behaviour, so I've proceeded cautiously and used __mingw_static_ovr in a few places for UCRT where I otherwise ran into issues. But in any case, we either need to add import library aliases for these cases (which doesn't work for things that vary depending on _USE_32BIT_TIME_T), remove extern from __forceinline and __CRT_INLINE, or use a different inline attribute (e.g. __mingw_static_ovr) for these particular inlines. It would be great to have __forceinline fixed. I've seen a problems in code using static __forceinline multiple times. Understandable, as static extern is a tautology. I also recall that attempts to change that had their problems, so it's more tricky than it seems (if possible). Yes, that's what I've feared, I haven't tried changing it for this very reason. How about doing things in msvc ucrt compatible way? Like the patch I attached as an example (not yet tested)? This looks good to me (but didn't test it yet). This approach of modernizing small areas at a time seems good also, wrt finding out what regressions it might cause, compared to larger refactorings of everything at once. In general I think we should review all use of __forceinline/__CRT_INLINE wrt to what symbols actually are available in UCRT. // Martin ___ 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] crt: Use importlibs for more time functions.
On 4/5/19 9:15 AM, Martin Storsjö wrote:
On Thu, 4 Apr 2019, Jacek Caban wrote:
On 4/4/19 3:07 PM, Martin Storsjö wrote:
On Thu, 4 Apr 2019, Jacek Caban wrote:
Signed-off-by: Jacek Caban
---
mingw-w64-crt/Makefile.am | 2 +-
mingw-w64-crt/lib-common/msvcrt.def.in | 4 +-
mingw-w64-crt/lib32/msvcr100.def.in | 4 +-
mingw-w64-crt/lib32/msvcr80.def.in | 2 +
mingw-w64-crt/lib32/msvcr90.def.in | 4 +-
mingw-w64-crt/lib64/msvcr100.def.in | 4 +-
mingw-w64-crt/lib64/msvcr80.def.in | 2 +
mingw-w64-crt/lib64/msvcr90.def.in | 4 +-
mingw-w64-crt/misc/difftime.c | 21 --
mingw-w64-crt/misc/difftime32.c | 9 -
mingw-w64-crt/misc/difftime64.c | 9 -
mingw-w64-headers/crt/time.h | 55 +++---
12 files changed, 37 insertions(+), 83 deletions(-)
delete mode 100644 mingw-w64-crt/misc/difftime.c
delete mode 100644 mingw-w64-crt/misc/difftime32.c
delete mode 100644 mingw-w64-crt/misc/difftime64.c
Ok with me, if you extend the commit message to mention all the
changes. The changes IMO now include the following three change
categories:
- Use importlibs for more time functions
- Unify inline attribute strategies for time functions in headers
- Use _CRTIMP on more time functions
Multiple line in commit message usually is a good sign that the patch
should be split ;) And I agree, I could do better job at that. I
split the patch and pushed.
This turned out to break compilation in certain cases for me.
The culprit is that __forceinline is a bit deceptive (and so is
__CRT_INLINE). For good reasons I was using __mingw_static_ovr here
before, for the UCRT case.
These three macros expand to slightly different things depending on
compiler and context, but for the C, modern clang/gcc case, they are
as follows:
#define __CRT_INLINE extern inline __attribute__((__gnu_inline__))
#define __forceinline extern __inline__
__attribute__((__always_inline__,__gnu_inline__))
#define __mingw_static_ovr static __attribute__ ((__unused__))
__inline__ __cdecl
Both __CRT_INLINE and __forceinline declare the inline functions as
extern, in combination with the gnu_inline attribute. The gnu_inline
attribute, in combination with extern, means that when you take the
address of the function, it shouldn't generate and reference the
inline version of the function, but reference the extern copy of the
function available in another translation unit.
This obviously is desireable for getting the same function pointer to
a function in all translation units that refer to it, but doesn't work
for functions that don't exist elsewhere in that form.
I'm not sure about how many of the existing cases of __forceinline and
__CRT_INLINE that really intentionally want this behaviour, so I've
proceeded cautiously and used __mingw_static_ovr in a few places for
UCRT where I otherwise ran into issues.
But in any case, we either need to add import library aliases for
these cases (which doesn't work for things that vary depending on
_USE_32BIT_TIME_T), remove extern from __forceinline and __CRT_INLINE,
or use a different inline attribute (e.g. __mingw_static_ovr) for
these particular inlines.
It would be great to have __forceinline fixed. I've seen a problems in
code using static __forceinline multiple times. I also recall that
attempts to change that had their problems, so it's more tricky than it
seems (if possible).
How about doing things in msvc ucrt compatible way? Like the patch I
attached as an example (not yet tested)?
Jacek
diff --git a/mingw-w64-headers/crt/corecrt.h b/mingw-w64-headers/crt/corecrt.h
index d171a013..44d32d62 100644
--- a/mingw-w64-headers/crt/corecrt.h
+++ b/mingw-w64-headers/crt/corecrt.h
@@ -143,6 +143,14 @@ typedef __time64_t time_t;
#define _CRT_SECURE_CPP_NOTHROW throw()
#endif
+#ifndef __CRTDECL
+#if !defined(__cplusplus) && defined(__GNUC__)
+#define __CRTDECL __cdecl __attribute__ ((__unused__))
+#else
+#define __CRTDECL __cdecl
+#endif
+#endif
+
#if defined(__cplusplus) && _CRT_SECURE_CPP_OVERLOAD_SECURE_NAMES
#define __DEFINE_CPP_OVERLOAD_SECURE_FUNC_0_0(__ret,__func,__dsttype,__dst) \
diff --git a/mingw-w64-headers/crt/time.h b/mingw-w64-headers/crt/time.h
index 02abd7b6..be2398d4 100644
--- a/mingw-w64-headers/crt/time.h
+++ b/mingw-w64-headers/crt/time.h
@@ -216,27 +216,27 @@ extern "C" {
#ifndef RC_INVOKED
#ifdef _USE_32BIT_TIME_T
-__forceinline time_t __cdecl time(time_t *_Time) { return _time32(_Time); }
-__forceinline double __cdecl difftime(time_t _Time1,time_t _Time2) { return _difftime32(_Time1,_Time2); }
-__forceinline struct tm *__cdecl localtime(const time_t *_Time) { return _localtime32(_Time); }
-__forceinline errno_t __cdecl localtime_s(struct tm *_Tm,const time_t *_Time) { return _localtime32_s(_Tm,_Time); }
-__forceinline struct tm *__cdecl gmtime(const time_t *_Time) { return _gmtime32(_Time); }
-__forceinline errno_t __cdecl gmtime_s(struct
Re: [Mingw-w64-public] [PATCH] crt: Use importlibs for more time functions.
On Thu, 4 Apr 2019, Jacek Caban wrote: On 4/4/19 3:07 PM, Martin Storsjö wrote: On Thu, 4 Apr 2019, Jacek Caban wrote: Signed-off-by: Jacek Caban --- mingw-w64-crt/Makefile.am | 2 +- mingw-w64-crt/lib-common/msvcrt.def.in | 4 +- mingw-w64-crt/lib32/msvcr100.def.in | 4 +- mingw-w64-crt/lib32/msvcr80.def.in | 2 + mingw-w64-crt/lib32/msvcr90.def.in | 4 +- mingw-w64-crt/lib64/msvcr100.def.in | 4 +- mingw-w64-crt/lib64/msvcr80.def.in | 2 + mingw-w64-crt/lib64/msvcr90.def.in | 4 +- mingw-w64-crt/misc/difftime.c | 21 -- mingw-w64-crt/misc/difftime32.c | 9 - mingw-w64-crt/misc/difftime64.c | 9 - mingw-w64-headers/crt/time.h | 55 +++--- 12 files changed, 37 insertions(+), 83 deletions(-) delete mode 100644 mingw-w64-crt/misc/difftime.c delete mode 100644 mingw-w64-crt/misc/difftime32.c delete mode 100644 mingw-w64-crt/misc/difftime64.c Ok with me, if you extend the commit message to mention all the changes. The changes IMO now include the following three change categories: - Use importlibs for more time functions - Unify inline attribute strategies for time functions in headers - Use _CRTIMP on more time functions Multiple line in commit message usually is a good sign that the patch should be split ;) And I agree, I could do better job at that. I split the patch and pushed. This turned out to break compilation in certain cases for me. The culprit is that __forceinline is a bit deceptive (and so is __CRT_INLINE). For good reasons I was using __mingw_static_ovr here before, for the UCRT case. These three macros expand to slightly different things depending on compiler and context, but for the C, modern clang/gcc case, they are as follows: #define __CRT_INLINE extern inline __attribute__((__gnu_inline__)) #define __forceinline extern __inline__ __attribute__((__always_inline__,__gnu_inline__)) #define __mingw_static_ovr static __attribute__ ((__unused__)) __inline__ __cdecl Both __CRT_INLINE and __forceinline declare the inline functions as extern, in combination with the gnu_inline attribute. The gnu_inline attribute, in combination with extern, means that when you take the address of the function, it shouldn't generate and reference the inline version of the function, but reference the extern copy of the function available in another translation unit. This obviously is desireable for getting the same function pointer to a function in all translation units that refer to it, but doesn't work for functions that don't exist elsewhere in that form. I'm not sure about how many of the existing cases of __forceinline and __CRT_INLINE that really intentionally want this behaviour, so I've proceeded cautiously and used __mingw_static_ovr in a few places for UCRT where I otherwise ran into issues. But in any case, we either need to add import library aliases for these cases (which doesn't work for things that vary depending on _USE_32BIT_TIME_T), remove extern from __forceinline and __CRT_INLINE, or use a different inline attribute (e.g. __mingw_static_ovr) for these particular inlines. // Martin ___ 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] crt: Use importlibs for more time functions.
On 4/4/19 3:07 PM, Martin Storsjö wrote: On Thu, 4 Apr 2019, Jacek Caban wrote: Signed-off-by: Jacek Caban --- mingw-w64-crt/Makefile.am | 2 +- mingw-w64-crt/lib-common/msvcrt.def.in | 4 +- mingw-w64-crt/lib32/msvcr100.def.in | 4 +- mingw-w64-crt/lib32/msvcr80.def.in | 2 + mingw-w64-crt/lib32/msvcr90.def.in | 4 +- mingw-w64-crt/lib64/msvcr100.def.in | 4 +- mingw-w64-crt/lib64/msvcr80.def.in | 2 + mingw-w64-crt/lib64/msvcr90.def.in | 4 +- mingw-w64-crt/misc/difftime.c | 21 -- mingw-w64-crt/misc/difftime32.c | 9 - mingw-w64-crt/misc/difftime64.c | 9 - mingw-w64-headers/crt/time.h | 55 +++--- 12 files changed, 37 insertions(+), 83 deletions(-) delete mode 100644 mingw-w64-crt/misc/difftime.c delete mode 100644 mingw-w64-crt/misc/difftime32.c delete mode 100644 mingw-w64-crt/misc/difftime64.c Ok with me, if you extend the commit message to mention all the changes. The changes IMO now include the following three change categories: - Use importlibs for more time functions - Unify inline attribute strategies for time functions in headers - Use _CRTIMP on more time functions Multiple line in commit message usually is a good sign that the patch should be split ;) And I agree, I could do better job at that. I split the patch and pushed. Thanks or the review, 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] crt: Use importlibs for more time functions.
On Thu, 4 Apr 2019, Jacek Caban wrote: Signed-off-by: Jacek Caban --- mingw-w64-crt/Makefile.am | 2 +- mingw-w64-crt/lib-common/msvcrt.def.in | 4 +- mingw-w64-crt/lib32/msvcr100.def.in| 4 +- mingw-w64-crt/lib32/msvcr80.def.in | 2 + mingw-w64-crt/lib32/msvcr90.def.in | 4 +- mingw-w64-crt/lib64/msvcr100.def.in| 4 +- mingw-w64-crt/lib64/msvcr80.def.in | 2 + mingw-w64-crt/lib64/msvcr90.def.in | 4 +- mingw-w64-crt/misc/difftime.c | 21 -- mingw-w64-crt/misc/difftime32.c| 9 - mingw-w64-crt/misc/difftime64.c| 9 - mingw-w64-headers/crt/time.h | 55 +++--- 12 files changed, 37 insertions(+), 83 deletions(-) delete mode 100644 mingw-w64-crt/misc/difftime.c delete mode 100644 mingw-w64-crt/misc/difftime32.c delete mode 100644 mingw-w64-crt/misc/difftime64.c Ok with me, if you extend the commit message to mention all the changes. The changes IMO now include the following three change categories: - Use importlibs for more time functions - Unify inline attribute strategies for time functions in headers - Use _CRTIMP on more time functions // Martin ___ Mingw-w64-public mailing list Mingw-w64-public@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
