Bug 171241

Summary: Unhandled enumeration values in IntlDateTimeFormat.cpp
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: JavaScriptCoreAssignee: Andy VanWagoner <andy>
Status: RESOLVED FIXED    
Severity: Normal CC: andy, bugs-noreply, buildbot, commit-queue, jfbastien, keith_miller, mark.lam, mcatanzaro, msaboff, rniwa, sbarati
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Patch none

Description Michael Catanzaro 2017-04-24 12:32:23 PDT
[925/5514] Building CXX object Source/...e.dir/runtime/IntlDateTimeFormat.cpp.o
../../Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp: In static member function ‘static const char* JSC::IntlDateTimeFormat::partTypeString(UDateFormatField)’:
../../Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:909:12: warning: enumeration value ‘UDAT_AM_PM_MIDNIGHT_NOON_FIELD’ not handled in switch [-Wswitch]
     switch (field) {
            ^
../../Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:909:12: warning: enumeration value ‘UDAT_FLEXIBLE_DAY_PERIOD_FIELD’ not handled in switch [-Wswitch]
Comment 1 Andy VanWagoner 2017-04-24 20:50:49 PDT
Created attachment 308058 [details]
Patch
Comment 2 Andy VanWagoner 2017-04-24 20:54:36 PDT
Comment on attachment 308058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308058&action=review

> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:965
> +    default:

This change will prevent us from getting future warnings. Should we not do this, so that we get warnings about cases we haven't categorized when they are added?
Comment 3 JF Bastien 2017-04-24 22:04:50 PDT
Comment on attachment 308058 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308058&action=review

>> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:965
>> +    default:
> 
> This change will prevent us from getting future warnings. Should we not do this, so that we get warnings about cases we haven't categorized when they are added?

Agreed that we probably do want to force ourselves to handle new cases instead of going to a default handler. The above comment seems to say otherwise though, but as we see from this addition that behavior would have been erroneous.

That makes we wonder though: if we build with ICU 56, and then at runtime we actually use ICU 57 which can have the above two new values, will it be possible to receive a UDateFormatField field with these new values? That would lead to UB if the range of the enum's underlying type changed (say, we go from [0,255] to more values).
Comment 4 Andy VanWagoner 2017-04-25 07:29:02 PDT
(In reply to JF Bastien from comment #3)
> Comment on attachment 308058 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308058&action=review
> 
> >> Source/JavaScriptCore/runtime/IntlDateTimeFormat.cpp:965
> >> +    default:
> > 
> > This change will prevent us from getting future warnings. Should we not do this, so that we get warnings about cases we haven't categorized when they are added?
> 
> Agreed that we probably do want to force ourselves to handle new cases
> instead of going to a default handler. The above comment seems to say
> otherwise though, but as we see from this addition that behavior would have
> been erroneous.
> 
> That makes we wonder though: if we build with ICU 56, and then at runtime we
> actually use ICU 57 which can have the above two new values, will it be
> possible to receive a UDateFormatField field with these new values? That
> would lead to UB if the range of the enum's underlying type changed (say, we
> go from [0,255] to more values).

We use udatpg_getBestPattern rather than building a pattern directly, so it's possible that the pattern we use for formatting ends up including one of the new field types. Especially since that should be looking at the icu data file at run time, which may be newer than the header version used at build time. We also dynamically link to libicucore, right? So the running icu functions would be aware of the new values, but our code won't.

Since enums are just numbers in C++, I could put in the integer cases for those new values in a #else. While those numbers can refer to UDAT_FIELD_COUNT in older icu versions, that isn't a value we should ever actually receive.

What do you think?
Comment 5 Michael Catanzaro 2017-04-25 07:33:30 PDT
(In reply to Andy VanWagoner from comment #4)
> Since enums are just numbers in C++, I could put in the integer cases for
> those new values in a #else. While those numbers can refer to
> UDAT_FIELD_COUNT in older icu versions, that isn't a value we should ever
> actually receive.
> 
> What do you think?

It seems like the best approach to me.
Comment 6 Andy VanWagoner 2017-04-25 07:42:39 PDT
Created attachment 308100 [details]
Patch
Comment 7 JF Bastien 2017-04-25 08:14:23 PDT
Comment on attachment 308100 [details]
Patch

Looks good. The problem I had in mind is actually kindof a non-problem now that I think about it: if the enum's underlying type change (say from uint8_t to uint16_t) then the mangled name of ICU functions which pass that enum in/out would change.

So I think what you had before was OK in that it would fall through to "literal", but adding the cases by number works too because it classifies the values properly.


r=me
Comment 8 JF Bastien 2017-04-25 08:15:04 PDT
Comment on attachment 308100 [details]
Patch

Actually tests seem sad?
Comment 9 Build Bot 2017-04-25 08:19:01 PDT
Comment on attachment 308100 [details]
Patch

Attachment 308100 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3601875

New failing tests:
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-ftl
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-eager-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-ftl-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-no-llint
jsc-layout-tests.yaml/js/script-tests/intl-datetimeformat.js.layout-dfg-eager-no-cjit
Comment 10 Build Bot 2017-04-25 08:49:06 PDT
Comment on attachment 308100 [details]
Patch

Attachment 308100 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3601943

New failing tests:
js/intl-datetimeformat.html
Comment 11 Build Bot 2017-04-25 08:49:07 PDT
Created attachment 308101 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Build Bot 2017-04-25 08:52:18 PDT
Comment on attachment 308100 [details]
Patch

Attachment 308100 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3601945

New failing tests:
js/intl-datetimeformat.html
Comment 13 Build Bot 2017-04-25 08:52:20 PDT
Created attachment 308102 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 14 Build Bot 2017-04-25 09:12:04 PDT
Comment on attachment 308100 [details]
Patch

Attachment 308100 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3601962

New failing tests:
js/intl-datetimeformat.html
Comment 15 Build Bot 2017-04-25 09:12:06 PDT
Created attachment 308104 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 16 Andy VanWagoner 2017-04-25 19:39:10 PDT
Looking at the test failures, it looks like a ":" between hours and minutes was miscategorized as a "dayPeriod" with the change. UDAT_TIME_SEPARATOR_FIELD = 35 makes trying to use the raw numbers incorrect.

I'm going to go back to just including the new values in an #if and remove the #else with numbers.
Comment 17 Andy VanWagoner 2017-04-25 19:53:52 PDT
Created attachment 308194 [details]
Patch
Comment 18 JF Bastien 2017-04-25 21:49:50 PDT
Comment on attachment 308194 [details]
Patch

r=me
Comment 19 WebKit Commit Bot 2017-04-25 22:18:12 PDT
Comment on attachment 308194 [details]
Patch

Clearing flags on attachment: 308194

Committed r215792: <http://trac.webkit.org/changeset/215792>
Comment 20 WebKit Commit Bot 2017-04-25 22:18:14 PDT
All reviewed patches have been landed.  Closing bug.