We blow up id_offset, comment_offset

RESOLVED FIXED in Firefox 26, Firefox OS v1.2

Status

()

Toolkit
Telemetry
RESOLVED FIXED
5 years ago
10 months ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 fixed, firefox27 fixed, firefox28 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

Attempting to add new histograms results in
./TelemetryHistogramData.inc:792:63: error: constant expression evaluates to 65545 which cannot be narrowed to type 'uint16_t' (aka 'unsigned short') [-Wc++11-narrowing]

We need to extend id_offset, comment_offset
Created attachment 823990 [details] [diff] [review]
Extending id_offset, comment_offset to 32 bits
Assignee: nobody → dteller
Attachment #823990 - Flags: review?(nfroyd)
Created attachment 823991 [details] [diff] [review]
Extending id_offset, comment_offset to 32 bits, v2

Ah, missed a static assert.
Attachment #823990 - Attachment is obsolete: true
Attachment #823990 - Flags: review?(nfroyd)
Attachment #823991 - Flags: review?(nfroyd)
Comment on attachment 823990 [details] [diff] [review]
Extending id_offset, comment_offset to 32 bits

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

r=me

Are we really blowing up both of those fields?  Can you file a bug on compressing the comment fields?  I doubt we can get away with compressing the histogram names in a reasonable fashion, but the comments should be accessed rarely.
Attachment #823990 - Attachment is obsolete: false
Comment on attachment 823991 [details] [diff] [review]
Extending id_offset, comment_offset to 32 bits, v2

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

Mid-air'd.  Please do file the bug from comment 3.
Attachment #823991 - Flags: review?(nfroyd) → review+
Yes, we blow up both fiels:

44:44.73 In file included from /Users/david/Documents/Code/mc-promise/toolkit/components/telemetry/Telemetry.cpp:397:
44:44.73 ./TelemetryHistogramData.inc:792:63: error: constant expression evaluates to 65545 which cannot be narrowed to type 'uint16_t' (aka 'unsigned short') [-Wc++11-narrowing]
44:44.73   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65511, 65545, true },
44:44.74                                                               ^~~~~
44:44.74 ./TelemetryHistogramData.inc:792:63: note: override this message by inserting an explicit cast
44:44.74   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65511, 65545, true },
44:44.74                                                               ^~~~~
44:44.74                                                               static_cast<uint16_t>( )
44:44.74 ./TelemetryHistogramData.inc:793:56: error: constant expression evaluates to 65594 which cannot be narrowed to type 'uint16_t' (aka 'unsigned short') [-Wc++11-narrowing]
44:44.74   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65594, 65624, true },
44:44.74                                                        ^~~~~
44:44.74 ./TelemetryHistogramData.inc:793:56: note: override this message by inserting an explicit cast
44:44.74   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65594, 65624, true },
44:44.74                                                        ^~~~~
44:44.74                                                        static_cast<uint16_t>( )
44:44.74 ./TelemetryHistogramData.inc:793:63: error: constant expression evaluates to 65624 which cannot be narrowed to type 'uint16_t' (aka 'unsigned short') [-Wc++11-narrowing]
44:44.74   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65594, 65624, true },
44:44.74                                                               ^~~~~
44:44.74 ./TelemetryHistogramData.inc:793:63: note: override this message by inserting an explicit cast
44:44.74   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65594, 65624, true },
44:44.74                                                               ^~~~~
44:44.75                                                               static_cast<uint16_t>( )
44:44.75 ./TelemetryHistogramData.inc:794:56: error: constant expression evaluates to 65672 which cannot be narrowed to type 'uint16_t' (aka 'unsigned short') [-Wc++11-narrowing]
44:44.76   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65672, 65701, true },
44:44.76                                                        ^~~~~
44:44.76 ./TelemetryHistogramData.inc:794:56: note: override this message by inserting an explicit cast
44:44.76   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65672, 65701, true },
44:44.76                                                        ^~~~~
44:44.76                                                        static_cast<uint16_t>( )
44:44.76 ./TelemetryHistogramData.inc:794:63: error: constant expression evaluates to 65701 which cannot be narrowed to type 'uint16_t' (aka 'unsigned short') [-Wc++11-narrowing]
44:44.76   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65672, 65701, true },
44:44.76                                                               ^~~~~
44:44.76 ./TelemetryHistogramData.inc:794:63: note: override this message by inserting an explicit cast
44:44.76   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65672, 65701, true },
44:44.76                                                               ^~~~~
44:44.76                                                               static_cast<uint16_t>( )
44:44.76 ./TelemetryHistogramData.inc:792:63: error: implicit conversion from 'int' to 'uint16_t' (aka 'unsigned short') changes value from 65545 to 9 [-Werror,-Wconstant-conversion]
44:44.76   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65511, 65545, true },
44:44.77   ~                                                           ^~~~~
44:44.77 ./TelemetryHistogramData.inc:793:56: error: implicit conversion from 'int' to 'uint16_t' (aka 'unsigned short') changes value from 65594 to 58 [-Werror,-Wconstant-conversion]
44:44.77   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65594, 65624, true },
44:44.77   ~                                                    ^~~~~
44:44.77 ./TelemetryHistogramData.inc:793:63: error: implicit conversion from 'int' to 'uint16_t' (aka 'unsigned short') changes value from 65624 to 88 [-Werror,-Wconstant-conversion]
44:44.80   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65594, 65624, true },
44:44.80   ~                                                           ^~~~~
44:44.80 ./TelemetryHistogramData.inc:794:56: error: implicit conversion from 'int' to 'uint16_t' (aka 'unsigned short') changes value from 65672 to 136 [-Werror,-Wconstant-conversion]
44:44.81   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65672, 65701, true },
44:44.81   ~                                                    ^~~~~
44:44.81 ./TelemetryHistogramData.inc:794:63: error: implicit conversion from 'int' to 'uint16_t' (aka 'unsigned short') changes value from 65701 to 165 [-Werror,-Wconstant-conversion]
44:44.81   { 1, 10000, 50, nsITelemetry::HISTOGRAM_EXPONENTIAL, 65672, 65701, true },
44:44.81   ~                                                           ^~~~~
44:44.81 ./TelemetryHistogramData.inc:2166:1: error: static_assert failed "index overflow"
44:44.81 static_assert(sizeof(gHistogramStringTable) <= UINT16_MAX, "index overflow");
44:44.81 ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Blocks: 935453
Attachment #823990 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/e91a4b7fbb2f
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/e91a4b7fbb2f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla28
Comment on attachment 823991 [details] [diff] [review]
Extending id_offset, comment_offset to 32 bits, v2

[Approval Request Comment] 
User impact if declined: Cannot add any any more telemetry probes to mozilla-aurora or mozilla-beta.

Testing completed (on m-c, etc.): This landed on mozilla-central on 2013-11-06.

Risk to taking this patch (and alternatives if risky): None.

String or IDL/UUID changes made by this patch: None
Attachment #823991 - Flags: approval-mozilla-beta?
Attachment #823991 - Flags: approval-mozilla-aurora?
Release drivers: I uplifted this trivial patch to mozilla-aurora to fix the bustage caused by uplifting the patches in bug 707275, since you asked me to make sure the patches for bug 707275 got into the next beta. Sorry for any inconvenience.

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a996059df97
Attachment #823991 - Flags: approval-mozilla-beta?
Attachment #823991 - Flags: approval-mozilla-beta+
Attachment #823991 - Flags: approval-mozilla-aurora?
Attachment #823991 - Flags: approval-mozilla-aurora+
Thanks for the quick approval.

https://hg.mozilla.org/releases/mozilla-beta/rev/173c00e5bc27
status-firefox26: --- → fixed
status-firefox27: --- → fixed
status-firefox28: --- → fixed
status-b2g-v1.2: --- → fixed
Whiteboard: [qa-]

Updated

10 months ago
Blocks: 1382560

Updated

10 months ago
No longer blocks: 1382560
You need to log in before you can comment on or make changes to this bug.