[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 10:47:10 CEST 2019


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


More information about the libav-devel mailing list