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

Martin Storsjö martin at martin.st
Tue Apr 16 12:17:01 CEST 2019


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


More information about the libav-devel mailing list