932281 - We blow up id_offset, comment_offset

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Extending id_offset, comment_offset to 32 bits

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Extending id_offset, comment_offset to 32 bits, v2

Ah, missed a static assert.

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Nathan Froyd [:froydnj]]

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.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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 ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/e91a4b7fbb2f

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e91a4b7fbb2f

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks for the quick approval.

https://hg.mozilla.org/releases/mozilla-beta/rev/173c00e5bc27

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/173c00e5bc27