media/ffvpx/libavutil/mem.c:36:0: error: #error "<malloc.h> has been replaced by <stdlib.h>

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jbeich, Unassigned)

Tracking

({regression})

Trunk
mozilla48
Unspecified
FreeBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46+ wontfix, firefox47+ fixed, firefox48+ fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 years ago
Same as bug 1228230. Hardcoding stuff detected by top-level is fragile:
- arc4random() is available on all BSDs (but unused in media/ffvpx)
- malloc.h is not available on DragonFly, #error on FreeBSD and #warning on OpenBSD

In file included from media/ffvpx/libavutil/mem.c:29:
In file included from media/ffvpx/config.h:23:
media/ffvpx/config_unix64.h:272:9: warning: 'HAVE_ARC4RANDOM' macro redefined
      [-Wmacro-redefined]
#define HAVE_ARC4RANDOM 0
        ^
mozilla-config.h:42:9: note: previous definition is here
#define HAVE_ARC4RANDOM 1
        ^
In file included from media/ffvpx/libavutil/mem.c:36:
/usr/include/malloc.h:3:2: error: "<malloc.h> has been replaced by <stdlib.h>"
#error "<malloc.h> has been replaced by <stdlib.h>"
 ^
1 warning and 1 error generated.
Reporter

Comment 2

3 years ago
Comment on attachment 8707739 [details]
MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya

The diff is generated after running:

  $ sed -Ei '' '/HAVE_MALLOC_H|HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMALIGN/d' media/libav/config*

Also, the second reviewer was supposed to be assignee of bug 1214462. Not sure how to change via reviewboard UI...
Attachment #8707739 - Flags: review?(ajones) → review?(jyavenard)
Comment on attachment 8707739 [details]
MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya

Those files are self generated, so the next time you do an update the changes will be lost.

I would much prefer that either:
1-you set the value to 0 (we don't need them anyway)
2-have a dedicated config_freebsd.h/asm

And in any case update the README_MOZILLA to document what was done, bonus point for making a script that automate the steps described in README_MOZILLA

just so that when I next update media/ffvpx (very soon and often: the code is receiving a lot of fixes as we're currently fuzzing ffmpeg)
Attachment #8707739 - Flags: review?(jyavenard) → review-
Attachment #8707739 - Flags: review?(mh+mozilla)
Fwiw, OpenBSD has removed malloc.h.
(In reply to Jean-Yves Avenard [:jya] from comment #3)

> I would much prefer that either:
> 1-you set the value to 0 (we don't need them anyway)

But if you set the value to 0, wouldnt the warning about #define being redefined be triggered ?
Reporter

Comment 6

3 years ago
Is MOZ_FFVPX limited to MSE or any VP8/9 content?

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> I would much prefer that either:
> 1-you set the value to 0 (we don't need them anyway)

It wouldn't address bug 1214462 comment 19 or bug 1228230 comment 6. For one, arc4random() warning affects Android x86 but not Linux x86.

> 2-have a dedicated config_freebsd.h/asm

For a Tier3 platform a separate config often means bitrot due to ENOTIME e.g., forget to regen after AVX2 support was added half a year ago. And what about other BSDs? Each has enough difference in the config to make sharing slightly risky without testing or if more files are added in the future.

config_unix* is also a misnomer, maybe rename to config_linux*. About the only not Unix-like system Mozilla still supports is Windows. Of those only OS X is allowed to use UNIX trademark. POSIX would also be wrong after replacing posix_memalign() with memalign().

Other options worth considering:

  3-run ffmpeg configure as sub-configure during build similar to ICU, jemalloc, freetype
  4-don't build media/ffvpx on Tier3 but always rely on system ffmpeg providing vp8/vp9 decoders

> And in any case update the README_MOZILLA to document what was done,

yasm 1.1 issue doesn't affect Tier 3 because of NPOTB. However, FreeBSD needs '--cc=cc' due to lack of 'gcc' hardlink to 'clang'.

> bonus point for making a script that automate the steps described in README_MOZILLA

How that would help if a Mozilla contributor doesn't have a FreeBSD box around?
Reporter

Comment 7

3 years ago
Comment on attachment 8707739 [details]
MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30857/diff/1-2/
Attachment #8707739 - Attachment description: MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?k17e → MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya
Attachment #8707739 - Flags: review- → review?(jyavenard)
Reporter

Comment 9

3 years ago
Note, those are unmaintainable examples.
Attachment #8707739 - Flags: review?(jyavenard) → review+
Comment on attachment 8707739 [details]
MozReview Request: Bug 1239550 - Rename Linux configs under media/ffvpx/ to avoid confusion. r?jya

https://reviewboard.mozilla.org/r/30857/#review36445
Fwiw, because of this i cant build 46.0b5 on OpenBSD when malloc.h is missing. Will have to figure out what to put in those damn os-specific config files...
I'm wondering if it wouldn't be simpler to have a special header added to CFLAGS doing something like
CFLAGS -include bsd.h

that would do something like:
#undef HAVE_MALLOC_H
#define HAVE_MALLOC_H 0

would be much easier and also much easier to maintain
Actually, much more easily, in config.h

Add at the end:
#if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) // or whaever is required for openbsd)
#undef HAVE_MALLOC_H
#define HAVE_MALLOC_H 0
#endif

:gaston could you give this a try? (can wrap a patch for you if needed)
Flags: needinfo?(landry)
I had a slightly different hack (cf https://cgit.rhaalovely.net/mozilla-firefox/commit/?h=beta&id=0bbd84875372e45aad60a3a2d9371a0af6e00884) but i'll try this.

While here, shouldnt we handle HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMALIGN the same ? jan ?
Flags: needinfo?(landry) → needinfo?(jbeich)
i don't want to modify the original ffmpeg code. makes resync too cumbersome.
Reporter

Comment 16

3 years ago
(In reply to Landry Breuil (:gaston) from comment #14)
> I had a slightly different hack (cf
> https://cgit.rhaalovely.net/mozilla-firefox/commit/
> ?h=beta&id=0bbd84875372e45aad60a3a2d9371a0af6e00884) but i'll try this.

I plan to use comment 1 fix for our firefox46 update. comment 7 is too cumbersome to backport. media/ffvpx can be disabled as well as it's a fallback for when system ffmpeg is not available.

> While here, shouldnt we handle
> HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMALIGN the same
> ? jan ?

Are you referring to comment 13? HAVE_LOCALTIME_R value is the same as on Linux. While ugly it'd be acceptable. Maybe also check defined(__Bitrig__) in new diffs[1].

[1] if using Rust bootstraps as an indicator of how alive the community is ;)
    https://github.com/bitrig/bitrig/commit/e040863a6a32f6e676e046aac31b823bac563a84
Flags: needinfo?(jbeich)
(In reply to Jean-Yves Avenard [:jya] from comment #13)
> Actually, much more easily, in config.h
> 
> Add at the end:
> #if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) ||
> defined(__OpenBSD__) // or whaever is required for openbsd)
> #undef HAVE_MALLOC_H
> #define HAVE_MALLOC_H 0
> #endif
> 
> :gaston could you give this a try? (can wrap a patch for you if needed)

This works here.

(In reply to Jan Beich from comment #16)

> Are you referring to comment 13? HAVE_LOCALTIME_R value is the same as on
> Linux. While ugly it'd be acceptable. Maybe also check defined(__Bitrig__)
> in new diffs[1].

Yeah, i was mostly thinking about HAVE_ARC4_RANDOM since it was yielding warnings..

Bitrig is more or less dead.
(In reply to Jan Beich from comment #16)
> I plan to use comment 1 fix for our firefox46 update. comment 7 is too
> cumbersome to backport. media/ffvpx can be disabled as well as it's a
> fallback for when system ffmpeg is not available.

not in 47 and later.

media/ffvpx is always used to decode vp8 and vp9. We prefer it over the system ffmpeg as we control it (and it's the latest version)

if :gaston is reporting that it works, that I may as well just do that. Easy to write, and easy to maintain.
Attachment #8729951 - Flags: review?(mh+mozilla)
Attachment #8729951 - Flags: review?(jyavenard)
Attachment #8729951 - Flags: review-
Any hope of getting this commited 'soonish' so that it reaches beta/46 ?
Flags: needinfo?(jyavenard)
Reporter

Comment 20

3 years ago
comment 13 approach wouldn't fix HAVE_ARC4RANDOM warning unless you do #undef before *and* after #include "config_*.h".

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> Those files are self generated, so the next time you do an update the changes will be lost.

For comment 1 the following in README_MOZILLA would be enough

  config*:
  replace: /HAVE_MALLOC_H|HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMALIGN/d

maybe next to

  config_unix32.h:
  add to configure command: --disable-asm --disable-yasm --cc='clang -m32'
  replace: s/HAVE_SYSCTL 1/HAVE_SYSCTL 0/ and s/HAVE_MEMALIGN 1/HAVE_MEMALIGN 0/ and s/HAVE_POSIX_MEMALIGN 1/HAVE_POSIX_MEMALIGN 0/
(In reply to Jan Beich from comment #20)
> comment 13 approach wouldn't fix HAVE_ARC4RANDOM warning unless you do
> #undef before *and* after #include "config_*.h".

HAVE_ARC4RANDOM is unused in the ffvpx code, and only defined in the config headers. So why would its value matter ?
Flags: needinfo?(jyavenard)
sorry, re-reading the original comment. maybe easiest is to remove the value completely seeing that it doesn't matter.
(In reply to Jan Beich from comment #20)
> comment 13 approach wouldn't fix HAVE_ARC4RANDOM warning unless you do
> #undef before *and* after #include "config_*.h".
> 
> (In reply to Jean-Yves Avenard [:jya] from comment #3)
> > Those files are self generated, so the next time you do an update the changes will be lost.
> 
> For comment 1 the following in README_MOZILLA would be enough
> 
>   config*:
>   replace:
> /
> HAVE_MALLOC_H|HAVE_ARC4RANDOM|HAVE_LOCALTIME_R|HAVE_MEMALIGN|HAVE_POSIX_MEMAL
> IGN/d

I'm a bit confused. Are you saying HAVE_MALLOC_H should be removed completely on all config files ?

otherwise, if so yes, this may be easier for all systems (And also easier to create the files)
Reporter

Comment 24

3 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #23)
> I'm a bit confused. Are you saying HAVE_MALLOC_H should be removed
> completely on all config files ?

Correct. old-configure.in via mozilla-config.h already defines HAVE_MALLOC_H if <malloc.h> exists. mozilla-config.h is forced upon everything within Gecko even if it only causes namespace pollution.

HAVE_ARC4RANDOM and a few others are also defined by old-configure.in.
Reporter

Comment 25

3 years ago
Comment on attachment 8729951 [details]
MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39613/diff/1-2/
Attachment #8729951 - Attachment description: MozReview Request: Bug 1239550 - Add media/ffvpx/ configs for DragonFly, FreeBSD, NetBSD. r?jya r?glandium → MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya
Attachment #8729951 - Flags: review?(mh+mozilla)
Attachment #8729951 - Flags: review?(jyavenard)
Attachment #8729951 - Flags: review-
Reporter

Updated

3 years ago
Attachment #8707739 - Attachment is obsolete: true
Reporter

Comment 26

3 years ago
MozReview didn't notice but this is a rebased version from comment 1 with comment 3 addressed by a comment in README_MOZILLA. The replace command requires passing -r or -E flag to sed(1) to enable extended regular expressions:

  /HAVE_(MALLOC_H|ARC4RANDOM|LOCALTIME_R|MEMALIGN|POSIX_MEMALIGN)/d

unless you prefer longer version

  /HAVE_MALLOC_H/d; /HAVE_ARC4RANDOM/d; /HAVE_LOCALTIME_R/d; /HAVE_MEMALIGN/d; /HAVE_POSIX_MEMALIGN/d
Comment on attachment 8729951 [details]
MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya

https://reviewboard.mozilla.org/r/39613/#review41863
Attachment #8729951 - Flags: review?(jyavenard) → review+
Comment on attachment 8729951 [details]
MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya

https://reviewboard.mozilla.org/r/39613/#review42165

::: media/ffvpx/README_MOZILLA:18
(Diff revision 2)
>  
>  configuration files were generated as follow using the configure script:
>  ./configure --disable-everything --disable-protocols --disable-demuxers --disable-muxers --disable-filters --disable-programs --disable-doc --disable-parsers --enable-parser=vp8 --enable-parser=vp9 --enable-decoder=vp8 --enable-decoder=vp9 --disable-static --enable-shared --disable-debug --disable-sdl --disable-libxcb --disable-securetransport --disable-iconv --disable-swresample --disable-swscale --disable-avdevice --disable-avfilter --disable-avformat --disable-d3d11va --disable-dxva2 --disable-vaapi --disable-vda --disable-vdpau --disable-videotoolbox --enable-asm --enable-yasm
>  
> +config*:
> +replace: /HAVE_(MALLOC_H|ARC4RANDOM|LOCALTIME_R|MEMALIGN|POSIX_MEMALIGN)/d

That's not replace, that's delete
Attachment #8729951 - Flags: review?(mh+mozilla) → review+
Reporter

Comment 29

3 years ago
https://reviewboard.mozilla.org/r/39613/#review42165

> That's not replace, that's delete

Syntax of various fields in README_MOZILLA is underdocumented. `replace` can be either sed(1), perl(1) or ad hoc expression. Technically, any modification is a "replacement" of the existing content.
Reporter

Comment 30

3 years ago
Jean, how do you invoke `replace` lines after ffmpeg update?
Flags: needinfo?(jyavenard)
(In reply to Jan Beich from comment #30)
> Jean, how do you invoke `replace` lines after ffmpeg update?

not sure what you mean...

I do it all manually. so here I'll be using sed :)
Flags: needinfo?(jyavenard)
https://reviewboard.mozilla.org/r/39613/#review42165

> Syntax of various fields in README_MOZILLA is underdocumented. `replace` can be either sed(1), perl(1) or ad hoc expression. Technically, any modification is a "replacement" of the existing content.

I think it's okay as is, or maybe just update the comment giving the GNU sed syntax (as the command will be run from linux). I can't push while this issue is opened. Once marked as fixed, I can autoland it.
Reporter

Comment 33

3 years ago
https://reviewboard.mozilla.org/r/39613/#review42165

> I think it's okay as is, or maybe just update the comment giving the GNU sed syntax (as the command will be run from linux). I can't push while this issue is opened. Once marked as fixed, I can autoland it.

The syntax used here is the same as POSIX except for extended (i.e. not basic) regular expressions. Both GNU sed and BSD sed support it using `-r` or `-E` flag. I'm not sure how to prepend command-line flags for `replace` commands.
The just either drop the issue or make it as fixed. until then it can't be committed.
Reporter

Comment 35

3 years ago
But I did drop already via MozReview. Do you mean something else?

Comment 37

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/037893f93079
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
[Tracking Requested - why for this release]:

Do we want to backport this to beta/aurora to fix build on BSDs ?

Guess we'll let the rev bake on m-c for some time first..
Too late for 46, tracking for 47+ since it sounds possible we could uplift a fix so that BSD builds will work.
Reporter

Comment 40

3 years ago
Comment on attachment 8729951 [details]
MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya

This could have made into to 46.0.1. ;( Now is really too late but we're not even halfway to 47.0, so 46.0.2 may still happen (like with 40.x).

Approval Request Comment
[Feature/regressing bug #]: bug 1214462 regression
[User impact if declined]: broken build Bitrig, DragonFly, FreeBSD, OpenBSD
[Describe test coverage new/current, TreeHerder]: m-c
[Risks and why]: Low, broken build if too out of sync with autoconf changes
[String/UUID change made/needed]: None
Attachment #8729951 - Flags: approval-mozilla-release?
Attachment #8729951 - Flags: approval-mozilla-beta?
It's great that we have a fix now!

Some ways to avoid this situation: add the keyword tag "regression" to the bug. Request tracking earlier than the last week of beta cycle. And explain the impact clearly. 

Do we have test coverage now so we don't break BSD builds like this without noticing?
Flags: needinfo?(jyavenard)
Flags: needinfo?(jbeich)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #41)

> Do we have test coverage now so we don't break BSD builds like this without
> noticing?

I'm not sure there's particular test coverage for this issue, but we have an out-of-mozilla BSD buildbot...
http://buildbot.rhaalovely.net/one_line_per_build i dunno why freebsd is red those days, but openbsd went definitely green after this issue was fixed in central (and aurora turned green at the last central->aurora merge)
Comment on attachment 8729951 [details]
MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya

Fix broken BSD builds, Aurora48+, Beta47+
Attachment #8729951 - Flags: approval-mozilla-release?
Attachment #8729951 - Flags: approval-mozilla-release+
Attachment #8729951 - Flags: approval-mozilla-beta?
Attachment #8729951 - Flags: approval-mozilla-beta+
Comment on attachment 8729951 [details]
MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya

Oops, I thought I was approving for Aurora. Did *not* mean to approve for release. It's Liz's decision.
Attachment #8729951 - Flags: approval-mozilla-release+ → approval-mozilla-release?
Reporter

Comment 45

3 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #41)
> add the keyword tag "regression" to the bug

Thanks for clearing the keyword usage for Tier3 build issues.

> Do we have test coverage now so we don't break BSD builds like this without
> noticing?

All BSDs are Tier3 platforms i.e., no Treeherder support. Landry's buildbot doesn't attempt to run tests.

For FFmpeg upstream - yes, see http://fate.ffmpeg.org/

(In reply to Landry Breuil (:gaston) from comment #42)
> i dunno why freebsd is red those days

libc++ issues. bug 1260208, bug 1246743 should improve test coverage. Aurora is currently broken by bug 1259537 and Nightly also by bug 1269171.
Flags: needinfo?(jbeich)
Keywords: regression
Doesn't the BSD port system have a set of patched to apply to the current tree anyway?

why can't this simply be added to the BSD port instead? at least you're guaranteed the outcome.

I haven't used FreeBSD since version 9, but that's how it used to work at least
Flags: needinfo?(jyavenard)
Reporter

Comment 47

3 years ago
Less patches makes it easier for upstream to review how downstream ships Firefox to the end users. When such users file a bug that's believed to be an upstream issue being able to build unpatched source without jumping through hoops to fix it also helps.

> why can't this simply be added to the BSD port instead?

PkgSrc lacks the fix. While mainly used on NetBSD (unaffected) it strives to support everything.
(In reply to Jean-Yves Avenard [:jya] from comment #46)
> Doesn't the BSD port system have a set of patched to apply to the current
> tree anyway?

There are some 'integration patches' in FreeBSD/OpenBSD/NetBSD ports-trees but we strive to be able to build mozilla-central *unpatched* on our buildbot.
> 
> why can't this simply be added to the BSD port instead? at least you're
> guaranteed the outcome.

That's already the case since 46 didnt build.. cf http://cvsweb.openbsd.org/cgi-bin/cvsweb/ports/www/mozilla-firefox/patches/patch-media_ffvpx_config_h?rev=1.1&content-type=text/x-cvsweb-markup - i went the easy route instead of a jumbo patch
Wontfix for 46 at this point. 47 is planned to release June 7.
Comment on attachment 8729951 [details]
MozReview Request: Bug 1239550 - Apply bug 1228230 against media/ffvpx. r?glandium r?jya

Clearing the m-r flag as 46 will EOL in a week and this fix is already in Beta47.
Attachment #8729951 - Flags: approval-mozilla-release?
You need to log in before you can comment on or make changes to this bug.