Fix Werrors in Lollipop-based b2g build

RESOLVED FIXED in Firefox 41, Firefox OS v2.2

Status

Firefox OS
General
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: botond, Assigned: botond)

Tracking

(Blocks: 1 bug)

unspecified
2.2 S13 (29may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.5+, tracking-b2g:backlog, firefox41 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(4 attachments, 8 obsolete attachments)

822 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
5.87 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
99.30 KB, patch
botond
: review+
Details | Diff | Splinter Review
818 bytes, patch
glandium
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Warnings-as-errors was enabled for b2g in bug 1073003, and it was clean on TBPL, but the Lollipop-based b2g build, which is not currently tested on TBPL, was broken.
(Assignee)

Updated

3 years ago
Assignee: nobody → botond
(Assignee)

Updated

3 years ago
Blocks: 1073003
(Assignee)

Comment 1

3 years ago
Created attachment 8546914 [details] [diff] [review]
Part 1 - Fix a -Wunused-function warning in AudioManager.cpp
Attachment #8546914 - Flags: review?(waychen)
(Assignee)

Comment 2

3 years ago
Created attachment 8546915 [details] [diff] [review]
Part 2 - Add a missing include to MediaSource.cpp

This wasn't actually a *warning*, but I needed it to get a successful build.
Attachment #8546915 - Flags: review?(kinetik)
(Assignee)

Comment 3

3 years ago
Created attachment 8546916 [details] [diff] [review]
Part 3 - Use 'snprintf' instead of 'sprintf'

Ehsan, I officially owe you a beer (or several)!
Attachment #8546916 - Flags: review?(ehsan)
(Assignee)

Comment 4

3 years ago
Try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=71e661a9b25d

This doesn't actually test Lollipop, since that doesn't run on TBPL; it's to help make sure I didn't break other things.

Updated

3 years ago
Attachment #8546914 - Flags: review?(waychen) → review+
(Assignee)

Comment 5

3 years ago
(In reply to Botond Ballo [:botond] from comment #4)
> Try run:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=71e661a9b25d

Seems like Windows doesn't have 'snprintf'...
(Assignee)

Comment 6

3 years ago
Comment on attachment 8546916 [details] [diff] [review]
Part 3 - Use 'snprintf' instead of 'sprintf'

Clearly this patch is no good as is.

Any suggestions for how to deal with not having snprintf on Windows? Do we have a suitable polyfill for it?
Flags: needinfo?(ehsan)
Attachment #8546916 - Flags: review?(ehsan) → review-
(Assignee)

Comment 7

3 years ago
(Meanwhile, if anyone working with Lollipop builds is blocked by this, please say so, and we can disable warnings-as-errors for Lollipop builds until the warnings are fixed.)
Comment on attachment 8546915 [details] [diff] [review]
Part 2 - Add a missing include to MediaSource.cpp

Review of attachment 8546915 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but note this was already fixed in bug 1119691.
Attachment #8546915 - Flags: review?(kinetik) → review+
(Assignee)

Comment 9

3 years ago
Comment on attachment 8546915 [details] [diff] [review]
Part 2 - Add a missing include to MediaSource.cpp

(In reply to Matthew Gregan [:kinetik] from comment #8)
> Looks good, but note this was already fixed in bug 1119691.

Ah, thanks - no need for it here then.
Attachment #8546915 - Attachment is obsolete: true

Comment 10

3 years ago
(In reply to Botond Ballo [:botond] from comment #6)
> Comment on attachment 8546916 [details] [diff] [review]
> Part 3 - Use 'snprintf' instead of 'sprintf'
> 
> Clearly this patch is no good as is.
> 
> Any suggestions for how to deal with not having snprintf on Windows? Do we
> have a suitable polyfill for it?

Use PR_snprintf with the pattern used in bug 1119776, since MSVC 2015 has added support for it (finally!)
Flags: needinfo?(ehsan)
Duplicate of this bug: 1120999
Blocks: 1094121
blocking-b2g: --- → 2.2?
Duplicate of this bug: 1121222

Updated

3 years ago
blocking-b2g: 2.2? → 2.2+
(In reply to Botond Ballo [:botond] from comment #7)
> (Meanwhile, if anyone working with Lollipop builds is blocked by this,
> please say so, and we can disable warnings-as-errors for Lollipop builds
> until the warnings are fixed.)

Botond, I think this is a good idea. AIK, some people working on gonk-l are blocked by this compile error, each of them need to work-around it locally. Could you help to disable warnings-as-errors for Lollipop builds until all warnings are fixed. Thanks!
Flags: needinfo?(botond)

Updated

3 years ago
Blocks: 1063044
(Assignee)

Comment 14

3 years ago
Created attachment 8550615 [details] [diff] [review]
[Temporary solution] disable warnings-as-errors in Lollipop-based builds
Flags: needinfo?(botond)
Attachment #8550615 - Flags: review?(mh+mozilla)
Comment on attachment 8550615 [details] [diff] [review]
[Temporary solution] disable warnings-as-errors in Lollipop-based builds

Review of attachment 8550615 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/confvars.sh
@@ +62,5 @@
>  # Warnings-as-errors cannot be enabled on gcc <= 4.4 due to bug 915555.
>  if test "$GCC_MAJOR_VERSION" -gt 4 -o \
>     "$GCC_MAJOR_VERSION" -eq 4 -a "$GCC_MINOR_VERSION" -gt 4; then
> +# Nor can they be enabled on Lollipop until bug 1119980 is fixed.
> +if test "$PLATFORM_SDK_VERSION" -lt 21; then

Your need to update your tree first. The gcc 4.4 check is gone.
Attachment #8550615 - Flags: review?(mh+mozilla) → feedback+
Blocks: 1111890
(Assignee)

Comment 16

3 years ago
Created attachment 8551644 [details] [diff] [review]
[2.2 solution] disable warnings-as-errors in Lollipop-based builds

Rebased.
Attachment #8550615 - Attachment is obsolete: true
Attachment #8551644 - Flags: review?(mh+mozilla)
Attachment #8551644 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 17

3 years ago
Landed patch to disable warnings-as-errors temporarily for Lollipop:

https://hg.mozilla.org/integration/mozilla-inbound/rev/311bc10282b2

Marking as 'leave-open' until the proper fix lands.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/311bc10282b2
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18)
> https://hg.mozilla.org/mozilla-central/rev/311bc10282b2

Is it going to land to v2.2 branch?
(Assignee)

Comment 20

3 years ago
(In reply to Hermes Cheng[:hermescheng] from comment #19)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #18)
> > https://hg.mozilla.org/mozilla-central/rev/311bc10282b2
> 
> Is it going to land to v2.2 branch?

I was going to request uplift of the proper fix once it's in place.

If you think the 2.2 branch needs a fix sooner, I can request uplift for the temporary fix.
Uplifting the temporary fix to v2.2 would be very nice, as we're working on v2.2 not master.
(Assignee)

Comment 22

3 years ago
Comment on attachment 8551644 [details] [diff] [review]
[2.2 solution] disable warnings-as-errors in Lollipop-based builds

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 1073003.

User impact if declined: 
  Lollipop-based b2g builds are broken by warnings-as-errors.

Testing completed: 
  Confirmed locally that patch fixes the build (by not treating
  the warnings as errors).
  Patch on m-c since 2015-01-20.

Risk to taking this patch (and alternatives if risky): 
  Very low. The patch just reverts, for Lollipop-based builds only,
  a very recent change to treat warnings as errors.

String or UUID changes made by this patch:
  None.
Attachment #8551644 - Flags: approval-mozilla-b2g37?

Updated

3 years ago
Attachment #8551644 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+

Updated

3 years ago
Whiteboard: [CR 786383]

Updated

3 years ago
Whiteboard: [CR 786383] → [caf priority: p2][CR 786383]
I just tried building B2G for Nexus 5 from scratch with:

  ./config.sh nexus-5-l
  ./build.sh

Botond's workaround patch is in B2G/gecko/b2g/confvars.sh but the build is still failing because warnings are treated as errors.
Flags: needinfo?(botond)
(Assignee)

Comment 24

3 years ago
(In reply to David Flanagan [:djf] from comment #23)
> I just tried building B2G for Nexus 5 from scratch with:
> 
>   ./config.sh nexus-5-l
>   ./build.sh
> 
> Botond's workaround patch is in B2G/gecko/b2g/confvars.sh but the build is
> still failing because warnings are treated as errors.

While doing a clean Lollipop build to try to repro this, I ran into a (non-Werror) build error. Filed bug 1126499 for it.
(Assignee)

Comment 25

3 years ago
(In reply to Botond Ballo [:botond] from comment #24)
> While doing a clean Lollipop build to try to repro this, I ran into a
> (non-Werror) build error. Filed bug 1126499 for it.

After reverting the commit causing this bustage (the proper solution is still being worked on), I was able to get a successful build.

David, is it possible you were seeing this bustage rather than a Werror bustage? If not, i.e. if you actually were seeing a Werror bustage, could you please post the compiler output? Thanks!
Flags: needinfo?(botond) → needinfo?(dflanagan)
Botond,

I did see that MP4Stream.h error on one of my build attempts. On prior attempts, I was also seeing a lot of warnings and I thought I saw something about those being treated as errors. Even when I ran with -j1 I was seeing a confusing mass of warnings.  Maybe the root cause of the build failure was always the missing MP4Stream.h and I just didn't understand the output.

I no longer have a copy of that output to post. Sorry.  Let's assume that it was the MP4Stream.h error plus possibly operator error on my part.
Flags: needinfo?(dflanagan)
(Assignee)

Comment 27

3 years ago
Ok, thanks. Do let me know if you encounter Werror-related bustage in the future!
(Assignee)

Comment 28

3 years ago
(In reply to [Away: 1/29-2/20] from comment #10)
> > Any suggestions for how to deal with not having snprintf on Windows? Do we
> > have a suitable polyfill for it?
> 
> Use PR_snprintf with the pattern used in bug 1119776, since MSVC 2015 has
> added support for it (finally!)

Implemented this. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13c550fa0a65
(In reply to Hermes Cheng[:hermescheng] from comment #19)
> Is it going to land to v2.2 branch?

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9210eb2c60f7
:djf, fyi : the errors/patch is in bug 1126499 which landed.
Flags: needinfo?(dflanagan)
No longer blocks: 1111890

Updated

3 years ago
Flags: needinfo?(dflanagan)
(Assignee)

Comment 31

3 years ago
Created attachment 8558311 [details] [diff] [review]
Part 2 - Use 'snprintf' instead of 'sprintf'

(In reply to Botond Ballo [:botond] from comment #28)
> (In reply to [Away: 1/29-2/20] from comment #10)
> > > Any suggestions for how to deal with not having snprintf on Windows? Do we
> > > have a suitable polyfill for it?
> > 
> > Use PR_snprintf with the pattern used in bug 1119776, since MSVC 2015 has
> > added support for it (finally!)
> 
> Implemented this. Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=13c550fa0a65

Had to make a few adjustments to get the patch to build successfully. Green build-only Try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c23c6ef8fd5

Daniel, would you be able to review this? I usually ask Ehsan for things like this, but he's on vacation.
Attachment #8546916 - Attachment is obsolete: true
Attachment #8558311 - Flags: review?(dholbert)
Comment on attachment 8558311 [details] [diff] [review]
Part 2 - Use 'snprintf' instead of 'sprintf'

Thanks for doing this! Hooray for switching to buffer-length-safe functions.

>diff --git a/dom/ipc/CrashReporterParent.cpp b/dom/ipc/CrashReporterParent.cpp
>--- a/dom/ipc/CrashReporterParent.cpp
>+++ b/dom/ipc/CrashReporterParent.cpp
>     char startTime[32];
>-    sprintf(startTime, "%lld", static_cast<long long>(mStartTime));
>+    // need to disambiguate between libc's snprintf and base::snprintf
>+    base::snprintf(startTime, 32, "%lld", static_cast<long long>(mStartTime));
[...]
>diff --git a/dom/media/AudioStream.cpp b/dom/media/AudioStream.cpp
>--- a/dom/media/AudioStream.cpp
>+++ b/dom/media/AudioStream.cpp
>   char buf[100];
>-  sprintf(buf, "dumped-audio-%d.wav", gDumpedAudioCount);
>+  snprintf(buf, 100, "dumped-audio-%d.wav", gDumpedAudioCount);

Hmm... repeating the hardcoded magic number in each case like this seems fragile.  I'd much prefer we use "ArrayLength(startTime)" & "ArrayLength(buf)" in the snprintf invocation.

ArrayLength is provided by mfbt/ArrayUtils.h:
 http://mxr.mozilla.org/mozilla-central/source/mfbt/ArrayUtils.h#47

Would you mind rewriting to use that? (I might punt on doing a thorough review until then, because it looks like this review-comment applies to most of the patch.)
(Assignee)

Comment 33

3 years ago
Comment on attachment 8558311 [details] [diff] [review]
Part 2 - Use 'snprintf' instead of 'sprintf'

(In reply to Daniel Holbert [:dholbert] from comment #32)
> Hmm... repeating the hardcoded magic number in each case like this seems
> fragile.  I'd much prefer we use "ArrayLength(startTime)" &
> "ArrayLength(buf)" in the snprintf invocation.
> 
> ArrayLength is provided by mfbt/ArrayUtils.h:
>  http://mxr.mozilla.org/mozilla-central/source/mfbt/ArrayUtils.h#47
> 
> Would you mind rewriting to use that? (I might punt on doing a thorough
> review until then, because it looks like this review-comment applies to most
> of the patch.)

Will do, thanks! Dropping review flag until I have an updated patch.
Attachment #8558311 - Flags: review?(dholbert)
(Assignee)

Comment 34

3 years ago
Created attachment 8562896 [details] [diff] [review]
Part 1 - Fix a -Woverloaded-virtual warning in gonk widget code

This fix accidentally snuck into my snprintf patch. Splitting out for separate review.
Attachment #8558311 - Attachment is obsolete: true
Attachment #8562896 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Comment 35

3 years ago
Created attachment 8562897 [details] [diff] [review]
Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds

And here's the updated snprintf patch, with much ArrayLength goodness.

Green build-only Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b57d85f00860
Attachment #8562897 - Flags: review?(dholbert)
Comment on attachment 8562897 [details] [diff] [review]
Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds

>+++ b/dom/ipc/CrashReporterParent.cpp
[...]
>-    sprintf(startTime, "%lld", static_cast<long long>(mStartTime));
>+    // need to disambiguate between libc's snprintf and base::snprintf
>+    base::snprintf(startTime, ArrayLength(startTime), "%lld", static_cast<long long>(mStartTime));

As discussed in IRC -- here, it looks like we're using the "base::" version simply because "snprintf" on its own causes a compile error in this spot, and there's no way (?) to resolve the ambiguity in favor of the libc version (as long as we have "using namespace base"). This feels like a bad reason to choose the "base::" version. (Maybe the base:: version is fine, but I'm not comfortable using it without auditing it more thoroughly -- and I'm not sure it's worth doing that, if we don't have to use it.)

Looks like you filed bug 1132153 to do away with the "using namespace base", which hopefully means we can use the standard version here and drop this "base::" -- nice.

>diff --git a/dom/media/AudioStream.cpp b/dom/media/AudioStream.cpp
>+#if defined(_MSC_VER) && _MSC_VER < 1900
>+#include <nspr/prprf.h>
>+#define snprintf PR_snprintf
>+#endif

Per tromey/froydnj's notes in IRC, I'm not sure it's a good idea to apply this #define across the board. As tromey says:
<tromey> like for nspr, "%l" means "take a 64 bit int"
<tromey> but for ordinary printf, "%l" means "take a long"
...and this could result in subtle bugs, where we'd be implicitly saying something different for win32-MSVC vs. win64-MSVC vs. other platforms.

Given that this is a mass-conversion, it seems possible/likely that this patch could introduce such a bug. So, to avoid that risk, we probably need to use a more standard-printf-flavored polyfill, if at all possible...

(Maybe base::snprintf could even be a solution :) though I'm not sure without having audited it.)

Also: given that I was initially completely unaware of the MSVC/nspr compat issues with snprintf, I don't really feel comfortable being the person to r+ this. I'd defer to froydnj or tromey (or someone else of your choice), if they're up for reviewing.
Attachment #8562897 - Flags: review?(dholbert) → feedback+
(In reply to Daniel Holbert [:dholbert] from comment #36)
> Comment on attachment 8562897 [details] [diff] [review]
> Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on
> Lollipop-based builds
> 
> >+++ b/dom/ipc/CrashReporterParent.cpp
> [...]
> >-    sprintf(startTime, "%lld", static_cast<long long>(mStartTime));
> >+    // need to disambiguate between libc's snprintf and base::snprintf
> >+    base::snprintf(startTime, ArrayLength(startTime), "%lld", static_cast<long long>(mStartTime));
> 
> As discussed in IRC -- here, it looks like we're using the "base::" version
> simply because "snprintf" on its own causes a compile error in this spot,
> and there's no way (?) to resolve the ambiguity in favor of the libc version
> (as long as we have "using namespace base"). This feels like a bad reason to
> choose the "base::" version. (Maybe the base:: version is fine, but I'm not
> comfortable using it without auditing it more thoroughly -- and I'm not sure
> it's worth doing that, if we don't have to use it.)

You should be able to use ::snprintf to favor libc's.

Updated

3 years ago
Attachment #8562896 - Flags: review?(sotaro.ikeda.g) → review+

Comment 38

3 years ago
Botond,

Now that this is r+'d what else is needed to land this for 2.2?

Thanks,
Mike
Flags: needinfo?(botond)
(Assignee)

Comment 39

3 years ago
(In reply to Mike Lee [:mlee] from comment #38)
> Now that this is r+'d what else is needed to land this for 2.2?

The temporary fix (disable -Werror on Lollipop) has already landed on 2.2 (see comment 29).

For the proper fix (re-enable -Werror and fix the warnings on Lollipop), I still need to address review comments for the Part 3 patch. I don't think I'll be uplifting that to 2.2 - I don't see much value in it. (I'm open to being convinced otherwise.)
Flags: needinfo?(botond)
(Assignee)

Comment 40

3 years ago
Created attachment 8571150 [details] [diff] [review]
Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds

All right, after a lot of fighting with MSVC, here's a version that, on MSVC versions that don't provide 'snprintf', delegates to 'vsnprintf', which they do provide.

The nice thing about this approach is that it doesn't require using a macro to do the wrapping.

Nathan, would you be able to review this?
Attachment #8562897 - Attachment is obsolete: true
Attachment #8571150 - Flags: review?(nfroyd)
(bump to v2.2? as the temp fix provided in comment 29 is sufficient over here, and comment 39 questions Moz's fortitude for an uplift)
No longer blocks: 1063044
blocking-b2g: 2.2+ → 2.2?
Whiteboard: [caf priority: p2][CR 786383]
triage: per comment 41, it's working fine so no longer blocking.
blocking-b2g: 2.2? → backlog
Calling v2.2 as fixed as it's likely every going to be.
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → affected
Comment on attachment 8571150 [details] [diff] [review]
Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds

Review of attachment 8571150 [details] [diff] [review]:
-----------------------------------------------------------------

If we're going to polyfill snprintf, we should add in a template overload that automagically computes the |const char[]| buffer length, so we can dispense with all the ArrayLength boilerplate.  WDYT?  (I guess that would have to be mozilla::snprintf, which could get confusing...)

Could you file a followup for adding |#pragma GCC poison sprintf| to mozilla-config.h.in?  (I'm not entirely sure we can get away with this, but it'd be excellent if we could.)

::: dom/media/AudioStream.cpp
@@ +21,5 @@
>  #include "nsPrintfCString.h"
>  #ifdef XP_MACOSX
>  #include <sys/sysctl.h>
>  #endif
> +#include "mozilla/Snprintf.h"

Nit: why not move this up with the rest of the mozilla/ includes?

::: gfx/thebes/gfxFontUtils.cpp
@@ +25,5 @@
>  
>  #include "plbase64.h"
>  #include "prlog.h"
>  
> +

Nit: added blank line?

::: gfx/thebes/gfxHarfBuzzShaper.cpp
@@ +19,5 @@
>  #include "harfbuzz/hb-ot.h"
>  
>  #include <algorithm>
>  
> +

Nit: added blank line?

@@ +817,5 @@
>  #if DEBUG
>                  {
>                      char buf[1024];
> +                    snprintf(buf, ArrayLength(buf), "unknown kern subtable in %s: "
> +                                        "ver 0 format %d\n",

Please fixup the indentation here.

@@ +879,5 @@
>  #if DEBUG
>                      {
>                          char buf[1024];
> +                        snprintf(buf, ArrayLength(buf), "unknown kern subtable in %s: "
> +                                            "ver 0 format %d\n",

...and here.

::: media/gmp-clearkey/0.1/openaes/oaes_lib.c
@@ +32,5 @@
>  #include <stddef.h>
>  #include <time.h> 
>  #include <string.h>
>  
> +#include "mozilla/Snprintf.h"

This looks like third-party code; please ask somebody on the media team to OK this.

::: tools/profiler/LulDwarf.cpp
@@ +54,2 @@
>  #include "mozilla/Assertions.h"
> +#include "mozilla/Snprintf.h"

This is all Android/Linux-only code, so we shouldn't need the polyfill here.

@@ +1893,5 @@
>    if (reg == return_address_)
>      return ustr__ZDra();
>  
>    char buf[30];
> +  snprintf(buf, 30, "dwarf_reg_%u", reg);

ArrayLength(buf)?

::: tools/profiler/LulMain.cpp
@@ +15,5 @@
>  
>  #include "mozilla/Assertions.h"
>  #include "mozilla/ArrayUtils.h"
>  #include "mozilla/MemoryChecking.h"
> +#include "mozilla/Snprintf.h"

This is all Android/Linux-only code, so we shouldn't need the polyfill here.
Attachment #8571150 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 45

3 years ago
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #44)
> If we're going to polyfill snprintf, we should add in a template overload
> that automagically computes the |const char[]| buffer length, so we can
> dispense with all the ArrayLength boilerplate.  WDYT?  (I guess that would
> have to be mozilla::snprintf, which could get confusing...)

I agree that this would be nice. I'll give it a try.

I addressed all other review comments locally.

> ::: media/gmp-clearkey/0.1/openaes/oaes_lib.c
> @@ +32,5 @@
> >  #include <stddef.h>
> >  #include <time.h> 
> >  #include <string.h>
> >  
> > +#include "mozilla/Snprintf.h"
> 
> This looks like third-party code; please ask somebody on the media team to
> OK this.

Edwin, does this change to oaes_lib.c look OK to you?
Flags: needinfo?(edwin)
(Assignee)

Updated

3 years ago
Blocks: 1141227
(Assignee)

Comment 46

3 years ago
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #44)
> Could you file a followup for adding |#pragma GCC poison sprintf| to
> mozilla-config.h.in?  (I'm not entirely sure we can get away with this, but
> it'd be excellent if we could.)

Filed bug 1141227.
(In reply to Botond Ballo [:botond] from comment #45)
> > ::: media/gmp-clearkey/0.1/openaes/oaes_lib.c
> > @@ +32,5 @@
> > >  #include <stddef.h>
> > >  #include <time.h> 
> > >  #include <string.h>
> > >  
> > > +#include "mozilla/Snprintf.h"
> > 
> > This looks like third-party code; please ask somebody on the media team to
> > OK this.
> 
> Edwin, does this change to oaes_lib.c look OK to you?

Sure.
Flags: needinfo?(edwin)
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
(Assignee)

Comment 48

3 years ago
Created attachment 8579403 [details] [diff] [review]
Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds

(In reply to Botond Ballo [:botond] from comment #45)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #44)
> > If we're going to polyfill snprintf, we should add in a template overload
> > that automagically computes the |const char[]| buffer length, so we can
> > dispense with all the ArrayLength boilerplate.  WDYT?  (I guess that would
> > have to be mozilla::snprintf, which could get confusing...)
> 
> I agree that this would be nice. I'll give it a try.

Updated patch to introduce the template overload, and to call it where possible.

Requesting review again as this is a significant change compared to the previous version.
Attachment #8571150 - Attachment is obsolete: true
Attachment #8579403 - Flags: review?(nfroyd)
Comment on attachment 8579403 [details] [diff] [review]
Part 3 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds

Review of attachment 8579403 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, nice, I didn't realize you could overload snprintf like that...seems kind of weird that one of the overloads has |extern "C"| linkage and the rest of them don't, but maybe that all just works out in the end.

I'm inclined to call this something else, though I'm not attached to a particular name.  snprintf_literal?  Kind of long.  lsnprintf ("literal snprintf")?  Suggestions?

::: intl/locale/nsScriptableDateFormat.cpp
@@ +11,5 @@
>  #include "nsCOMPtr.h"
>  #include "nsServiceManagerUtils.h"
>  
> +using mozilla::ArrayLength;
> +

No longer necessary?

::: ipc/netd/Netd.cpp
@@ +24,5 @@
>  #define INVALID_SOCKET -1
>  #define MAX_RECONNECT_TIMES 10
>  
> +using mozilla::ArrayLength;
> +

No longer necessary?

::: mfbt/Snprintf.h
@@ +30,5 @@
> +}
> +#endif
> +
> +// In addition, in C++ code, on all platforms, provide an overload of snprintf()
> +// which uses template argument deduction to deduces the size of the buffer,

Nit: "to deduce the size..."

::: mfbt/tests/TestIntegerPrintfMacros.cpp
@@ +3,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "mozilla/ArrayUtils.h"

No longer needed?

::: widget/nsBaseWidget.cpp
@@ +1947,5 @@
>    default:
>      {
>        char buf[32];
>  
> +      snprintf(buf,ArrayLength(buf),"UNKNOWN: %d",aGuiEvent->message);

No ArrayLength necessary?
Attachment #8579403 - Flags: review?(nfroyd) → review+

Comment 50

3 years ago
It is time to back out the temporary solution and land the right solution.
Thanks

--
Keven
blocking-b2g: --- → 2.2+
status-b2g-v2.2: fixed → affected
Flags: needinfo?(botond)
(Assignee)

Comment 51

3 years ago
(In reply to Keven Kuo [:kkuo] from comment #50)
> It is time to back out the temporary solution and land the right solution.

I still need to address the latest review comments (from comment 49).

I also think there's some confusion caused by the word "temporary". What I've described as the "temporary solution" is just disabling warning-as-errors. Warning-as-errors was previously disabled on all platforms; I enabled it on all platforms in the 2.2 timeframe, then realized that for Lollipop there are additional warnings to be fixed, so I disabled it for Lollipop only for 2.2, and will be enabling it for 3.0 when the patches in this bug land.

I don't see any reason to pursue enabling warnings-as-errors for Lollipop for 2.2 - it's not a regression fix, so there wouldn't be a great reason to uplift.
Flags: needinfo?(botond)
(Assignee)

Updated

3 years ago
Attachment #8551644 - Attachment description: [Temporary solution] disable warnings-as-errors in Lollipop-based builds → [2.2 solution] disable warnings-as-errors in Lollipop-based builds
status-b2g-v2.2: affected → fixed
Keywords: leave-open

Comment 52

3 years ago
(In reply to Botond Ballo [:botond] from comment #51)
> I don't see any reason to pursue enabling warnings-as-errors for Lollipop
> for 2.2 - it's not a regression fix, so there wouldn't be a great reason to
> uplift.

Hi! Botond,

Thanks for the clarification.

--
Keven
No longer blocks: 1094121

Comment 53

3 years ago
[Blocking Requested - why for this release]:with Lollipop-based b2g build, 3.0 affected
blocking-b2g: 2.2+ → 3.0?
Where do we stand here?
Flags: needinfo?(botond)
(Assignee)

Comment 55

2 years ago
Comment on attachment 8546914 [details] [diff] [review]
Part 1 - Fix a -Wunused-function warning in AudioManager.cpp

This was already fixed in bug 1134599.
Attachment #8546914 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8562896 - Attachment description: Part 2 - Fix a -Woverloaded-virtual warning in gonk widget code → Part 1 - Fix a -Woverloaded-virtual warning in gonk widget code
(Assignee)

Comment 56

2 years ago
Created attachment 8609509 [details] [diff] [review]
Part 2 - Use 'snprintf' instead of 'sprintf' to avoid a warning on Lollipop-based builds

Updated to address review comments from comment 49. Apologies for the delay in getting to this.

> I'm inclined to call this something else, though I'm not attached to a
> particular name.  snprintf_literal?  Kind of long.  lsnprintf ("literal
> snprintf")?  Suggestions?

I went with snprintf_literal, as we had discussed on IRC.

I also had to make a couple of new sprintf -> snprintf or sprintf -> snprintf_literal changes compared to the previous patch to keep up with changes to the code, but they were in the exact same vein as the existing changes in the patch, so I'll go ahead and carry r+.
Attachment #8579403 - Attachment is obsolete: true
Flags: needinfo?(botond)
Attachment #8609509 - Flags: review+
(Assignee)

Comment 57

2 years ago
Created attachment 8609510 [details] [diff] [review]
Part 3 - Enable warnings-as-errors for Lollipop builds
Attachment #8609510 - Flags: review?(mh+mozilla)
(Assignee)

Comment 58

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f169a5386980
blocking-b2g: 3.0? → 3.0+
Attachment #8609510 - Flags: review?(mh+mozilla) → review+

Comment 59

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a68a18840492
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdb8d05f8870
https://hg.mozilla.org/integration/mozilla-inbound/rev/12ce98475c6e
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c8053cb8ac32 for https://treeherder.mozilla.org/logviewer.html#?job_id=10135978&repo=mozilla-inbound
(Looks like the warning (treated as error) in comment 60 is for ssltunnel.cpp, which is only FAIL_ON_WARNINGS as of a few days ago, in bug 1167250.)
(Assignee)

Comment 62

2 years ago
Indeed. Fixed locally, and verified that a local Lollipop build is successful.

Comment 63

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/964c80bf087a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd4d2de214a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5adc9e191d7
https://hg.mozilla.org/mozilla-central/rev/964c80bf087a
https://hg.mozilla.org/mozilla-central/rev/3dd4d2de214a
https://hg.mozilla.org/mozilla-central/rev/d5adc9e191d7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S13 (29may)
status-b2g-master: affected → fixed
status-b2g-v2.5: --- → fixed
status-b2g-v2.5: fixed → ---
Depends on: 1197324
Depends on: 1277155
You need to log in before you can comment on or make changes to this bug.