Closed
Bug 1119980
Opened 10 years ago
Closed 10 years ago
Fix Werrors in Lollipop-based b2g build
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(blocking-b2g:2.5+, tracking-b2g:backlog, firefox41 fixed, b2g-v2.2 fixed, b2g-master fixed)
RESOLVED
FIXED
2.2 S13 (29may)
People
(Reporter: botond, Assigned: botond)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 8 obsolete files)
822 bytes,
patch
|
glandium
:
review+
bajaj
:
approval-mozilla-b2g37+
|
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 |
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•10 years ago
|
Assignee: nobody → botond
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8546914 -
Flags: review?(waychen)
Assignee | ||
Comment 2•10 years ago
|
||
This wasn't actually a *warning*, but I needed it to get a successful build.
Attachment #8546915 -
Flags: review?(kinetik)
Assignee | ||
Comment 3•10 years ago
|
||
Ehsan, I officially owe you a beer (or several)!
Attachment #8546916 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•10 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•10 years ago
|
Attachment #8546914 -
Flags: review?(waychen) → review+
Assignee | ||
Comment 5•10 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•10 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•10 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 8•10 years ago
|
||
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•10 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•10 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)
Updated•10 years ago
|
blocking-b2g: --- → 2.2?
Updated•10 years ago
|
blocking-b2g: 2.2? → 2.2+
Comment 13•10 years ago
|
||
(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•10 years ago
|
Blocks: CAF-v2.2-metabug
Assignee | ||
Comment 14•10 years ago
|
||
Flags: needinfo?(botond)
Attachment #8550615 -
Flags: review?(mh+mozilla)
Comment 15•10 years ago
|
||
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+
Assignee | ||
Comment 16•10 years ago
|
||
Rebased.
Attachment #8550615 -
Attachment is obsolete: true
Attachment #8551644 -
Flags: review?(mh+mozilla)
Updated•10 years ago
|
Attachment #8551644 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 17•10 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
Comment 18•10 years ago
|
||
Comment 19•10 years ago
|
||
(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•10 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.
Comment 21•10 years ago
|
||
Uplifting the temporary fix to v2.2 would be very nice, as we're working on v2.2 not master.
Assignee | ||
Comment 22•10 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•10 years ago
|
Attachment #8551644 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•10 years ago
|
Whiteboard: [CR 786383]
Updated•10 years ago
|
Whiteboard: [CR 786383] → [caf priority: p2][CR 786383]
Comment 23•10 years ago
|
||
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•10 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•10 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)
Comment 26•10 years ago
|
||
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•10 years ago
|
||
Ok, thanks. Do let me know if you encounter Werror-related bustage in the future!
Assignee | ||
Comment 28•10 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
Comment 29•10 years ago
|
||
(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)
Updated•10 years ago
|
Flags: needinfo?(dflanagan)
Assignee | ||
Comment 31•10 years ago
|
||
(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 32•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 years ago
|
||
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 36•10 years ago
|
||
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+
Comment 37•10 years ago
|
||
(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•10 years ago
|
Attachment #8562896 -
Flags: review?(sotaro.ikeda.g) → review+
Comment 38•10 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•10 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•10 years ago
|
||
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)
Comment 41•10 years ago
|
||
(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: CAF-v2.2-metabug
blocking-b2g: 2.2+ → 2.2?
Whiteboard: [caf priority: p2][CR 786383]
Comment 42•10 years ago
|
||
triage: per comment 41, it's working fine so no longer blocking.
blocking-b2g: 2.2? → backlog
Comment 43•10 years ago
|
||
Calling v2.2 as fixed as it's likely every going to be.
status-b2g-v2.2:
--- → fixed
status-b2g-master:
--- → affected
![]() |
||
Comment 44•10 years ago
|
||
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•10 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 | ||
Comment 46•10 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)
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
Assignee | ||
Comment 48•10 years ago
|
||
(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 49•10 years ago
|
||
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•10 years ago
|
||
It is time to back out the temporary solution and land the right solution.
Thanks
--
Keven
Assignee | ||
Comment 51•10 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•10 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
Updated•10 years ago
|
Keywords: leave-open
Comment 52•10 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: gonk-L
Comment 53•10 years ago
|
||
[Blocking Requested - why for this release]:with Lollipop-based b2g build, 3.0 affected
blocking-b2g: 2.2+ → 3.0?
Assignee | ||
Comment 55•10 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•10 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•10 years ago
|
||
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•10 years ago
|
||
Attachment #8609510 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 58•10 years ago
|
||
Updated•10 years ago
|
blocking-b2g: 3.0? → 3.0+
Updated•10 years ago
|
Attachment #8609510 -
Flags: review?(mh+mozilla) → review+
Comment 59•10 years ago
|
||
Comment 60•10 years ago
|
||
Comment 61•10 years ago
|
||
(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•10 years ago
|
||
Indeed. Fixed locally, and verified that a local Lollipop build is successful.
Comment 63•10 years ago
|
||
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
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S13 (29may)
Updated•10 years ago
|
Updated•10 years ago
|
status-b2g-v2.5:
--- → fixed
Updated•10 years ago
|
status-b2g-v2.5:
fixed → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•