[1/2] configure: Try adding -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600 for mingw as well

Message ID 20190410084825.14496-1-martin@martin.st
State Committed
Commit c0bd865ad60da31282c5d8e1000c98366249c31e
Headers show
Series
  • [1/2] configure: Try adding -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600 for mingw as well
Related show

Commit Message

Martin Storsjö April 10, 2019, 8:48 a.m.
Mingw headers have got header inline implementations of localtime_r
and gmtime_r, but only visible if certain posix thread safe functions
have been requested.

This is a preparatory step for improving the detection of those
functions.
---
An alternative fix is also provided in a different patch series,
by adjusting libavutil/time_internal.h.
---
 configure | 2 ++
 1 file changed, 2 insertions(+)

Comments

Luca Barbato April 10, 2019, 3:13 p.m. | #1
On 10/04/2019 10:48, Martin Storsjö wrote:
> Mingw headers have got header inline implementations of localtime_r
> and gmtime_r, but only visible if certain posix thread safe functions
> have been requested.
> 
> This is a preparatory step for improving the detection of those
> functions.
> ---
> An alternative fix is also provided in a different patch series,
> by adjusting libavutil/time_internal.h.
> ---
>   configure | 2 ++
>   1 file changed, 2 insertions(+)
> 

Seems fine to me.
Martin Storsjö April 11, 2019, 1:35 p.m. | #2
On Wed, 10 Apr 2019, Luca Barbato wrote:

> On 10/04/2019 10:48, Martin Storsjö wrote:
>> Mingw headers have got header inline implementations of localtime_r
>> and gmtime_r, but only visible if certain posix thread safe functions
>> have been requested.
>> 
>> This is a preparatory step for improving the detection of those
>> functions.
>> ---
>> An alternative fix is also provided in a different patch series,
>> by adjusting libavutil/time_internal.h.
>> ---
>>   configure | 2 ++
>>   1 file changed, 2 insertions(+)
>> 
>
> Seems fine to me.

Which ones do you mean - this series of 2 patches, the other one, or both?

// Martin
Luca Barbato April 12, 2019, 1:36 p.m. | #3
On 11/04/2019 15:35, Martin Storsjö wrote:
> On Wed, 10 Apr 2019, Luca Barbato wrote:
> 
>> On 10/04/2019 10:48, Martin Storsjö wrote:
>>> Mingw headers have got header inline implementations of localtime_r
>>> and gmtime_r, but only visible if certain posix thread safe functions
>>> have been requested.
>>>
>>> This is a preparatory step for improving the detection of those
>>> functions.
>>> ---
>>> An alternative fix is also provided in a different patch series,
>>> by adjusting libavutil/time_internal.h.
>>> ---
>>>   configure | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>
>> Seems fine to me.
> 
> Which ones do you mean - this series of 2 patches, the other one, or both?
> 

This series seems fine to me.
Martin Storsjö April 12, 2019, 9:58 p.m. | #4
On Fri, 12 Apr 2019, Luca Barbato wrote:

> On 11/04/2019 15:35, Martin Storsjö wrote:
>> On Wed, 10 Apr 2019, Luca Barbato wrote:
>> 
>>> On 10/04/2019 10:48, Martin Storsjö wrote:
>>>> Mingw headers have got header inline implementations of localtime_r
>>>> and gmtime_r, but only visible if certain posix thread safe functions
>>>> have been requested.
>>>>
>>>> This is a preparatory step for improving the detection of those
>>>> functions.
>>>> ---
>>>> An alternative fix is also provided in a different patch series,
>>>> by adjusting libavutil/time_internal.h.
>>>> ---
>>>>   configure | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>
>>> Seems fine to me.
>> 
>> Which ones do you mean - this series of 2 patches, the other one, or both?
>> 
>
> This series seems fine to me.

Ok. FWIW, the change in mingw-w64 that broke it was reverted (there was a 
similar issue within gcc as well), but I guess this change probably is 
good to make anyway.

// Martin
Diego Biurrun April 14, 2019, 4:34 p.m. | #5
On Sat, Apr 13, 2019 at 12:58:40AM +0300, Martin Storsjö wrote:
> On Fri, 12 Apr 2019, Luca Barbato wrote:
> > On 11/04/2019 15:35, Martin Storsjö wrote:
> > > On Wed, 10 Apr 2019, Luca Barbato wrote:
> > > > On 10/04/2019 10:48, Martin Storsjö wrote:
> > > > > Mingw headers have got header inline implementations of localtime_r
> > > > > and gmtime_r, but only visible if certain posix thread safe functions
> > > > > have been requested.
> > > > > 
> > > > > This is a preparatory step for improving the detection of those
> > > > > functions.
> > > > > ---
> > > > > An alternative fix is also provided in a different patch series,
> > > > > by adjusting libavutil/time_internal.h.
> > > > 
> > > > Seems fine to me.
> > > 
> > > Which ones do you mean - this series of 2 patches, the other one, or both?
> > > 
> > 
> > This series seems fine to me.
> 
> Ok. FWIW, the change in mingw-w64 that broke it was reverted (there was a
> similar issue within gcc as well), but I guess this change probably is good
> to make anyway.

I generally don't think that adding workarounds for foreign bugs is a
sustainable strategy, but I clearly prefer the configure change.

Also, s/Try adding/Add/ in the log message, you're not just trying to add
those flags :-)

Diego
Martin Storsjö April 14, 2019, 6:33 p.m. | #6
On Sun, 14 Apr 2019, Diego Biurrun wrote:

> On Sat, Apr 13, 2019 at 12:58:40AM +0300, Martin Storsjö wrote:
>> On Fri, 12 Apr 2019, Luca Barbato wrote:
>> > On 11/04/2019 15:35, Martin Storsjö wrote:
>> > > On Wed, 10 Apr 2019, Luca Barbato wrote:
>> > > > On 10/04/2019 10:48, Martin Storsjö wrote:
>> > > > > Mingw headers have got header inline implementations of localtime_r
>> > > > > and gmtime_r, but only visible if certain posix thread safe functions
>> > > > > have been requested.
>> > > > > 
>> > > > > This is a preparatory step for improving the detection of those
>> > > > > functions.
>> > > > > ---
>> > > > > An alternative fix is also provided in a different patch series,
>> > > > > by adjusting libavutil/time_internal.h.
>> > > > 
>> > > > Seems fine to me.
>> > > 
>> > > Which ones do you mean - this series of 2 patches, the other one, or both?
>> > > 
>> > 
>> > This series seems fine to me.
>> 
>> Ok. FWIW, the change in mingw-w64 that broke it was reverted (there was a
>> similar issue within gcc as well), but I guess this change probably is good
>> to make anyway.
>
> I generally don't think that adding workarounds for foreign bugs is a
> sustainable strategy,

Well, the idea of prefixing local system function fallbacks/replacements 
isn't so much of a "workaround" as a sensible idea in general IMO. This is 
a pattern that already is used e.g. for ff_getaddrinfo, ff_poll etc.

That is, regardless of what the reason for using a fallback is (the real 
function does not exist, the real function is declared in headers but 
missing in libs, the real function exists but we want to avoid it because 
it's buggy, etc), the pattern of

     #include <systemheader.h>
     static inline ff_systemfunc() {
         ...
     }
     #define systemfunc ff_systemfunc

should always be safe. So I think that should be a generally beneficial 
change in any case as well.

> but I clearly prefer the configure change.

Well, the check_func_headers change obviously is for the better, yes. 
Adding the _POSIX_C_SOURCE define when building for mingw most probably 
also is sensible, but the fact that we add it manually to most OSes, while 
we don't add it automatically for all, makes it a little less clear cut.

> Also, s/Try adding/Add/ in the log message, you're not just trying to add
> those flags :-)

Right, it wasn't a check_cflags but straightforward add_cflags. Yeah, I'll 
change that.

// Martin
Diego Biurrun April 16, 2019, 8:36 a.m. | #7
On Sun, Apr 14, 2019 at 09:33:40PM +0300, Martin Storsjö wrote:
> On Sun, 14 Apr 2019, Diego Biurrun wrote:
> > On Sat, Apr 13, 2019 at 12:58:40AM +0300, Martin Storsjö wrote:
> > > On Fri, 12 Apr 2019, Luca Barbato wrote:
> > > > On 11/04/2019 15:35, Martin Storsjö wrote:
> > > > > On Wed, 10 Apr 2019, Luca Barbato wrote:
> > > > > > On 10/04/2019 10:48, Martin Storsjö wrote:
> > > > > > > Mingw headers have got header inline implementations of localtime_r
> > > > > > > and gmtime_r, but only visible if certain posix thread safe functions
> > > > > > > have been requested.
> > > > > > > > > > > this is a preparatory step for improving the detection of those
> > > > > > > functions.
> > > > > > > ---
> > > > > > > An alternative fix is also provided in a different patch series,
> > > > > > > by adjusting libavutil/time_internal.h.
> > > > > > > > > Seems fine to me.
> > > > > > > Which ones do you mean - this series of 2 patches, the other one, or both?
> > > > > > > This series seems fine to me.
> > > 
> > > Ok. FWIW, the change in mingw-w64 that broke it was reverted (there was a
> > > similar issue within gcc as well), but I guess this change probably is good
> > > to make anyway.
> > 
> > I generally don't think that adding workarounds for foreign bugs is a
> > sustainable strategy,
> 
> Well, the idea of prefixing local system function fallbacks/replacements
> isn't so much of a "workaround" as a sensible idea in general IMO. This is a
> pattern that already is used e.g. for ff_getaddrinfo, ff_poll etc.
> 
> That is, regardless of what the reason for using a fallback is (the real
> function does not exist, the real function is declared in headers but
> missing in libs, the real function exists but we want to avoid it because
> it's buggy, etc), the pattern of
> 
>     #include <systemheader.h>
>     static inline ff_systemfunc() {
>         ...
>     }
>     #define systemfunc ff_systemfunc
> 
> should always be safe. So I think that should be a generally beneficial
> change in any case as well.

IIRC we only do that within libavformat and use a different pattern within
libavutil. Then again, my code knowledge might be getting a bit rusty.

> > but I clearly prefer the configure change.
> 
> Well, the check_func_headers change obviously is for the better, yes. Adding
> the _POSIX_C_SOURCE define when building for mingw most probably also is
> sensible, but the fact that we add it manually to most OSes, while we don't
> add it automatically for all, makes it a little less clear cut.

Switching from trying to set some flags globally for all platforms, inevitably
hitting a snag on some fringe system, then adding an exception for that system,
to setting flags by platform and strictly only when necessary on that platform,
is - oddly enough - one of the single biggest improvements to the whole
configure machinery.

So I'm very weary of changes in that area due to having been burned so often
in the past. If the change was motivated by a bug (since fixed) in mingw,
then we should not add workarounds for it.

Anyway, I've presented my arguments. I trust you to make a good decision.
Push at your discretion.

Diego
Martin Storsjö April 16, 2019, 8:47 a.m. | #8
On Tue, 16 Apr 2019, Diego Biurrun wrote:

> On Sun, Apr 14, 2019 at 09:33:40PM +0300, Martin Storsjö wrote:
>> On Sun, 14 Apr 2019, Diego Biurrun wrote:
>> > On Sat, Apr 13, 2019 at 12:58:40AM +0300, Martin Storsjö wrote:
>> > > On Fri, 12 Apr 2019, Luca Barbato wrote:
>> > > > On 11/04/2019 15:35, Martin Storsjö wrote:
>> > > > > On Wed, 10 Apr 2019, Luca Barbato wrote:
>> > > > > > On 10/04/2019 10:48, Martin Storsjö wrote:
>> > > > > > > Mingw headers have got header inline implementations of localtime_r
>> > > > > > > and gmtime_r, but only visible if certain posix thread safe functions
>> > > > > > > have been requested.
>> > > > > > > > > > > this is a preparatory step for improving the detection of those
>> > > > > > > functions.
>> > > > > > > ---
>> > > > > > > An alternative fix is also provided in a different patch series,
>> > > > > > > by adjusting libavutil/time_internal.h.
>> > > > > > > > > Seems fine to me.
>> > > > > > > Which ones do you mean - this series of 2 patches, the other one, or both?
>> > > > > > > This series seems fine to me.
>> > > 
>> > > Ok. FWIW, the change in mingw-w64 that broke it was reverted (there was a
>> > > similar issue within gcc as well), but I guess this change probably is good
>> > > to make anyway.
>> > 
>> > I generally don't think that adding workarounds for foreign bugs is a
>> > sustainable strategy,
>> 
>> Well, the idea of prefixing local system function fallbacks/replacements
>> isn't so much of a "workaround" as a sensible idea in general IMO. This is a
>> pattern that already is used e.g. for ff_getaddrinfo, ff_poll etc.
>> 
>> That is, regardless of what the reason for using a fallback is (the real
>> function does not exist, the real function is declared in headers but
>> missing in libs, the real function exists but we want to avoid it because
>> it's buggy, etc), the pattern of
>>
>>     #include <systemheader.h>
>>     static inline ff_systemfunc() {
>>         ...
>>     }
>>     #define systemfunc ff_systemfunc
>> 
>> should always be safe. So I think that should be a generally beneficial
>> change in any case as well.
>
> IIRC we only do that within libavformat and use a different pattern within
> libavutil. Then again, my code knowledge might be getting a bit rusty.

True, e.g. libavutil/libm.h does define some static inline functions 
unprefixed as well.

Nevertheless, using prefixes for fallback functions is not a 
workaround/hack in my book, but a sane and healthy development practice.

>> > but I clearly prefer the configure change.
>> 
>> Well, the check_func_headers change obviously is for the better, yes. Adding
>> the _POSIX_C_SOURCE define when building for mingw most probably also is
>> sensible, but the fact that we add it manually to most OSes, while we don't
>> add it automatically for all, makes it a little less clear cut.
>
> Switching from trying to set some flags globally for all platforms, inevitably
> hitting a snag on some fringe system, then adding an exception for that system,
> to setting flags by platform and strictly only when necessary on that platform,
> is - oddly enough - one of the single biggest improvements to the whole
> configure machinery.

Sure, I generally agree with that. I was generally a bit weary of forcing 
the posix defines on other systems, but I generally think it should be 
good for this case, as it reduces inconsistencies between 
available/visible functions.

> So I'm very weary of changes in that area due to having been burned so often
> in the past. If the change was motivated by a bug (since fixed) in mingw,
> then we should not add workarounds for it.

Well it's not quite as simple. The immediate issue is gone again, but the 
general underlying issue remains.

The TL;DR version is:

- mingw-w64 contains localtime_r/gmtime_r, but only visible if posix 
thread safe functions have been requested by some means. We currently 
don't detect these in configure. In practice, the posix thread safe 
functions define could be enabled transitively by some other included 
header (which has also been somewhat mitigated within mingw-w64). To 
safeguard against this inconsistency, defining it in configure would be 
helpful IMO.

- Even if localtime_r was visible from mingw-w64 headers, it used to not 
conflict with ours, because the mingw-w64 was defined as extern inline, 
while ours was static inline. The mingw-w64 headers were changed to define 
this as static inline, and later reverted again.

> Anyway, I've presented my arguments. I trust you to make a good decision.
> Push at your discretion.

Well in that case, I'd push all four paches.

// Martin
Martin Storsjö April 16, 2019, 10:17 a.m. | #9
On Tue, 16 Apr 2019, Martin Storsjö wrote:

> On Tue, 16 Apr 2019, Diego Biurrun wrote:
>
>> On Sun, Apr 14, 2019 at 09:33:40PM +0300, Martin Storsjö wrote:
>>> On Sun, 14 Apr 2019, Diego Biurrun wrote:
>>> > On Sat, Apr 13, 2019 at 12:58:40AM +0300, Martin Storsjö wrote:
>>> > > On Fri, 12 Apr 2019, Luca Barbato wrote:
>>> > > > On 11/04/2019 15:35, Martin Storsjö wrote:
>>> > > > > On Wed, 10 Apr 2019, Luca Barbato wrote:
>>> > > > > > On 10/04/2019 10:48, Martin Storsjö wrote:
>>> > > > > > > Mingw headers have got header inline implementations of 
> localtime_r
>>> > > > > > > and gmtime_r, but only visible if certain posix thread safe 
> functions
>>> > > > > > > have been requested.
>>> > > > > > > > > > > this is a preparatory step for improving the detection 
> of those
>>> > > > > > > functions.
>>> > > > > > > ---
>>> > > > > > > An alternative fix is also provided in a different patch 
> series,
>>> > > > > > > by adjusting libavutil/time_internal.h.
>>> > > > > > > > > Seems fine to me.
>>> > > > > > > Which ones do you mean - this series of 2 patches, the other 
> one, or both?
>>> > > > > > > This series seems fine to me.
>>> > > 
>>> > > Ok. FWIW, the change in mingw-w64 that broke it was reverted (there 
> was a
>>> > > similar issue within gcc as well), but I guess this change probably is 
> good
>>> > > to make anyway.
>>> > 
>>> > I generally don't think that adding workarounds for foreign bugs is a
>>> > sustainable strategy,
>>> 
>>> Well, the idea of prefixing local system function fallbacks/replacements
>>> isn't so much of a "workaround" as a sensible idea in general IMO. This is 
> a
>>> pattern that already is used e.g. for ff_getaddrinfo, ff_poll etc.
>>> 
>>> That is, regardless of what the reason for using a fallback is (the real
>>> function does not exist, the real function is declared in headers but
>>> missing in libs, the real function exists but we want to avoid it because
>>> it's buggy, etc), the pattern of
>>>
>>>     #include <systemheader.h>
>>>     static inline ff_systemfunc() {
>>>         ...
>>>     }
>>>     #define systemfunc ff_systemfunc
>>> 
>>> should always be safe. So I think that should be a generally beneficial
>>> change in any case as well.
>>
>> IIRC we only do that within libavformat and use a different pattern within
>> libavutil. Then again, my code knowledge might be getting a bit rusty.
>
> True, e.g. libavutil/libm.h does define some static inline functions 
> unprefixed as well.
>
> Nevertheless, using prefixes for fallback functions is not a 
> workaround/hack in my book, but a sane and healthy development practice.
>
>>> > but I clearly prefer the configure change.
>>> 
>>> Well, the check_func_headers change obviously is for the better, yes. 
> Adding
>>> the _POSIX_C_SOURCE define when building for mingw most probably also is
>>> sensible, but the fact that we add it manually to most OSes, while we 
> don't
>>> add it automatically for all, makes it a little less clear cut.
>>
>> Switching from trying to set some flags globally for all platforms, 
> inevitably
>> hitting a snag on some fringe system, then adding an exception for that 
> system,
>> to setting flags by platform and strictly only when necessary on that 
> platform,
>> is - oddly enough - one of the single biggest improvements to the whole
>> configure machinery.
>
> Sure, I generally agree with that. I was generally a bit weary of forcing 
> the posix defines on other systems, but I generally think it should be 
> good for this case, as it reduces inconsistencies between 
> available/visible functions.
>
>> So I'm very weary of changes in that area due to having been burned so 
> often
>> in the past. If the change was motivated by a bug (since fixed) in mingw,
>> then we should not add workarounds for it.
>
> Well it's not quite as simple. The immediate issue is gone again, but the 
> general underlying issue remains.
>
> The TL;DR version is:
>
> - mingw-w64 contains localtime_r/gmtime_r, but only visible if posix 
> thread safe functions have been requested by some means. We currently 
> don't detect these in configure. In practice, the posix thread safe 
> functions define could be enabled transitively by some other included 
> header (which has also been somewhat mitigated within mingw-w64). To 
> safeguard against this inconsistency, defining it in configure would be 
> helpful IMO.
>
> - Even if localtime_r was visible from mingw-w64 headers, it used to not 
> conflict with ours, because the mingw-w64 was defined as extern inline, 
> while ours was static inline. The mingw-w64 headers were changed to define 
> this as static inline, and later reverted again.
>
>> Anyway, I've presented my arguments. I trust you to make a good decision.
>> Push at your discretion.
>
> Well in that case, I'd push all four paches.

Pushed all four - thanks for the discussion!

// Martin

Patch

diff --git a/configure b/configure
index 26455054ba..3e8f2dcde1 100755
--- a/configure
+++ b/configure
@@ -4124,6 +4124,7 @@  probe_libc(){
             add_${pfx}cppflags -D__printf__=__gnu_printf__
         test_${pfx}cpp_condition windows.h "!defined(_WIN32_WINNT) || _WIN32_WINNT < 0x0600" &&
             add_${pfx}cppflags -D_WIN32_WINNT=0x0600
+        add_${pfx}cppflags -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600
     elif test_${pfx}cpp_condition _mingw.h "defined __MINGW_VERSION"  ||
          test_${pfx}cpp_condition _mingw.h "defined __MINGW32_VERSION"; then
         eval ${pfx}libc_type=mingw32
@@ -4137,6 +4138,7 @@  probe_libc(){
             add_${pfx}cppflags -D_WIN32_WINNT=0x0600
         eval test \$${pfx_no_}cc_type = "gcc" &&
             add_${pfx}cppflags -D__printf__=__gnu_printf__
+        add_${pfx}cppflags -D_POSIX_C_SOURCE=200112 -D_XOPEN_SOURCE=600
     elif test_${pfx}cpp_condition crtversion.h "defined _VC_CRT_MAJOR_VERSION"; then
         eval ${pfx}libc_type=msvcrt
         if test_${pfx}cpp_condition crtversion.h "_VC_CRT_MAJOR_VERSION < 14"; then