Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ios][wasm] Bump to ICU-72 #84849

Closed
wants to merge 5 commits into from
Closed

Conversation

steveisok
Copy link
Member

No description provided.

@steveisok steveisok added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 14, 2023
@ghost ghost assigned steveisok Apr 14, 2023
@steveisok
Copy link
Member Author

/azp run runtime-ioslike

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member

[18:12:06] info: RangeError: Maximum call stack size exceeded
[18:12:06] info:     at icu::UnicodeString::doHashCode\28\29\20const (wasm://wasm/00b2d586:wasm-function[3223]:0xe6576)
[18:12:06] info:     at uhash_hashUnicodeString (wasm://wasm/00b2d586:wasm-function[3230]:0xe6767)
[18:12:06] info:     at _uhash_put\28UHashtable*\2c\20UElement\2c\20UElement\2c\20signed\20char\2c\20UErrorCode*\29 (wasm://wasm/00b2d586:wasm-function[3261]:0xe7342)
[18:12:06] info:     at uhash_put (wasm://wasm/00b2d586:wasm-function[3260]:0xe7289)
[18:12:06] info:     at icu::Hashtable::put\28icu::UnicodeString\20const&\2c\20void*\2c\20UErrorCode&\29 (wasm://wasm/00b2d586:wasm-function[4181]:0x10be63)
[18:12:06] info:     at icu::CanonicalIterator::getEquivalents2\28icu::Hashtable*\2c\20char16_t\20const*\2c\20int\2c\20UErrorCode&\29 (wasm://wasm/00b2d586:wasm-function[6731]:0x188ee4)
[18:12:06] info:     at icu::CanonicalIterator::CanonicalIterator\28icu::UnicodeString\20const&\2c\20UErrorCode&\29 (wasm://wasm/00b2d586:wasm-function[6726]:0x188a50)
[18:12:06] info:     at icu::CollationBuilder::addOnlyClosure\28icu::UnicodeString\20const&\2c\20icu::UnicodeString\20const&\2c\20long\20long\20const*\2c\20int\2c\20unsigned\20int\2c\20UErrorCode&\29 (wasm://wasm/00b2d586:wasm-function[6753]:0x18ca27)
[18:12:06] info:     at icu::CollationBuilder::addRelation\28int\2c\20icu::UnicodeString\20const&\2c\20icu::UnicodeString\20const&\2c\20icu::UnicodeString\20const&\2c\20char\20const*&\2c\20UErrorCode&\29 (wasm://wasm/00b2d586:wasm-function[6749]:0x18c5b8)
[18:12:06] info:     at icu::CollationRuleParser::parse\28icu::UnicodeString\20const&\2c\20UErrorCode&\29 (wasm://wasm/00b2d586:wasm-function[6625]:0x1824d2)
[18:12:06] info:     at icu::CollationRuleParser::parse\28icu::UnicodeString\20const&\2c\20UErrorCode&\29 (wasm://wasm/00b2d586:wasm-function[6625]:0x1819c4)
[18:12:06] info:     at icu::CollationRuleParser::parse\28icu::UnicodeString\20const&\2c\20UErrorCode&\29 (wasm://wasm/00b2d586:wasm-function[6625]:0x1819c4)

Steve Pfister added 2 commits April 20, 2023 12:54
@steveisok
Copy link
Member Author

@lambdageek I am getting a weird crash on iossimulator when testing System.Globalization tests. The test runner runs to completion, but when the app tries to finish (or seemingly tries), it crashes.

https://gist.github.com/steveisok/ee8e7734a522154b96bf2831dab1b44d

@steveisok
Copy link
Member Author

steveisok commented Apr 21, 2023

[18:12:06] info: RangeError: Maximum call stack size exceeded

@pavelsavara I'm seeing this insanity on iOS, which wasm can't tolerate. I don't think it's wasm specific, but something icu is doing.

https://gist.github.com/steveisok/ee8e7734a522154b96bf2831dab1b44d#file-globalization-crash-txt-L156-L667

@lambdageek
Copy link
Member

@lambdageek I am getting a weird crash on iossimulator when testing System.Globalization tests. The test runner runs to completion, but when the app tries to finish (or seemingly tries), it crashes.

https://gist.github.com/steveisok/ee8e7734a522154b96bf2831dab1b44d

I don't actually see it crashing here in the runtime - all the threads (except the one with the deep recursion inside ICU) are idle and waiting for something. It seems like the app is just getting killed. Maybe from a stack overflow on that ICU thread, or maybe something is killing it from the outside?

Maybe it's some ICU bug that causes an infinite recursion?

@lambdageek
Copy link
Member

lambdageek commented Apr 21, 2023

The place where icu::CollationRuleParser::parseSetting calls icu::CollationRuleParser::parse is here

https://github.com/unicode-org/icu/blob/5be2ea84e59e63e4d68ebcd4082996d6db3af9f0/icu4c/source/i18n/collationruleparser.cpp#L656

Which looks like it's something to do with [import langTag] rules. So maybe something about an importer changed?
(I have no idea what a "collation rule" is, or an "import" or "importer". I'm just reading the code).

So maybe something in the icu data changed and the importer keeps returning the same file which has an [import langTag] in it that imports the same exact file in a loop forever?

I guess this is the spec for collation rules, and it has an [import] command http://www.unicode.org/reports/tr35/tr35-collation.html#Special_Purpose_Commands

@steveisok
Copy link
Member Author

So maybe something in the icu data changed and the importer keeps returning the same file which has an [import langTag] in it that imports the same exact file in a loop forever?

Interesting. It might have to do with our filtering. Maybe we're filtering out too much and it's trying to import non existent data?

@lambdageek
Copy link
Member

So maybe something in the icu data changed and the importer keeps returning the same file which has an [import langTag] in it that imports the same exact file in a loop forever?

Interesting. It might have to do with our filtering. Maybe we're filtering out too much and it's trying to import non existent data?

Yea, if i'm reading this stuff right, it looks like icu4c uses something called a BundleImporter which ends up calling CollationLoader::loadRules which seems like it does set errors under some conditions. But I'm not sure about this ures_getByKeyWithFallback

https://github.com/unicode-org/icu/blob/5be2ea84e59e63e4d68ebcd4082996d6db3af9f0/icu4c/source/i18n/ucol_res.cpp#L107-L135

https://github.com/unicode-org/icu/blob/5be2ea84e59e63e4d68ebcd4082996d6db3af9f0/icu4c/source/common/uresimp.h#L262

My guess is maybe we trimmed out too much from the data file and confused their loader

@steveisok
Copy link
Member Author

I enabled ICU tracing and this happens over and over:

[ICUDT] bundle-open: icudt72l-coll/ja.res
[17:04:37.7849360] [ICUDT] resc:       (get) icudt72l-coll/ja.res @ /collations
[17:04:37.7849380] [ICUDT] resc:       (get) icudt72l-coll/ja.res @ /collations/standard
[17:04:37.7849400] [ICUDT] resc:    (string) icudt72l-coll/ja.res @ /collations/standard/Sequence

I also turned off HAVE_UCOL_CLONE to see if it could be ucol_clone causing problems, but it did not. Same behavior.

What I'm going to do next is bring an unfiltered data file over and see if it works.

@steveisok
Copy link
Member Author

Pretty much confirmed it's something missing from our filters. A full data file run shows it may be expecting some additional binary data:

[ICUDT] bundle-open: icudt72l-coll/ja.res
[19:31:49.6113780] [ICUDT] resc:       (get) icudt72l-coll/ja.res @ /collations
[19:31:49.6113930] [ICUDT] resc:       (get) icudt72l-coll/root.res @ /collations/default
[19:31:49.6114070] [ICUDT] resc:    (string) icudt72l-coll/root.res @ /collations/default
[19:31:49.6114220] [ICUDT] resc:       (get) icudt72l-coll/ja.res @ /collations/standard
[19:31:49.6114370] [ICUDT] resc:       (get) icudt72l-coll/ja.res @ /collations/standard/%%CollationBin
[19:31:49.6114520] [ICUDT] resc:    (binary) icudt72l-coll/ja.res @ /collations/standard/%%CollationBin
[19:31:49.6114660] [ICUDT] resc:    (string) icudt72l-coll/ja.res @ /collations/standard/Sequence

# Ensure we aren't using deprecated ICU functions
if(STATIC_ICU)
set(ICU_FLAGS "-DHAVE_UCOL_CLONE ${ICU_FLAGS}")
endif()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this changed with Filip's PR: you also need to set this here and we need to set it to 0 for Android (since we use system ICU there), and to 1 for Apple/WASM/WASI:

if(CLR_CMAKE_TARGET_ANDROID OR CLR_CMAKE_TARGET_APPLE OR CLR_CMAKE_TARGET_BROWSER OR CLR_CMAKE_TARGET_WASI)
set(HAVE_SET_MAX_VARIABLE 1)
set(HAVE_UDAT_STANDALONE_SHORTER_WEEKDAYS 1)
set(HAVE_UCOL_CLONE 0)
else()

@ilonatommy
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member Author

Will revisit sometime in .NET 9

@steveisok steveisok closed this Aug 9, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Build-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants