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

Add System.Globalization.Native support for Windows #33704

Merged
merged 7 commits into from
Mar 21, 2020

Conversation

safern
Copy link
Member

@safern safern commented Mar 18, 2020

This enables building System.Globalization.Native on Windows (just building it on coreclr) since we shouldn't care about the windows native shim being built on the libraries side until the managed code is changed. I hope that by that time we have it linked in mono as well so that we can entirely stop building the shim in libraries at all.

This also enables loading icu.dll on Windows.

Contributes to: #826

cc: @jkotas @tarekgh

@@ -403,8 +407,12 @@ static const UCollator* GetCollatorFromSortHandle(SortHandle* pSortHandle, int32
pCollator = CloneCollatorWithOptions(pSortHandle->collatorsPerOption[0], options, pErr);
UCollator* pNull = NULL;

#ifdef TARGET_UNIX
// we are not using the standard atomic_compare_exchange_strong to workaround bugs in clang 5.0 (https://bugs.llvm.org/show_bug.cgi?id=37457)
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need this workaround? I believe we are not building with clang5 anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. We now use clang9 so we can remove this workaround. I will fix it as part of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I try to use atomic_compare_exchange_strong I get an error:

/home/runtime/src/libraries/Native/Unix/System.Globalization.Native/pal_collation.c:410:46: error: _Atomic cannot be applied to incomplete type 'UCollator' (aka 'struct UCollator')
if (!atomic_compare_exchange_strong((_Atomic(UCollator) *)&pSortHandle->collatorsPerOption[options], &pNull, pCollator))
/usr/include/unicode/ucol.h:58:8: note: forward declaration of 'struct UCollator'
struct UCollator;

It seems like the declaration of UCollator in the icu headers is incomplete.

Copy link
Member

Choose a reason for hiding this comment

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

maybe you need to ensure

#include "unicode/uloc.h"

is included?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was included. I managed to do it with void* volatile*. I'll put an update soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like in gcc atomic_compare_exchange_strong uses __atomic_compare_exchange macro which expects other parameter types, if I instead directly use __atomic_compare_exchange_n in gcc builds just fine. Will leave as is in order to have a green build at the moment.

/__w/1/s/src/libraries/Native/Unix/Common/pal_atomic.h:15:12: error: size mismatch in argument 2 of '__atomic_compare_exchange'
return atomic_compare_exchange_strong_explicit((_Atomic(void*)volatile*)dest, exchange, comperand, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);

@tarekgh
Copy link
Member

tarekgh commented Mar 18, 2020

CC @janvorli

@tarekgh
Copy link
Member

tarekgh commented Mar 18, 2020

modulo the comments mentioned here, LGTM

@safern safern force-pushed the GlobalizationWindowsIcu branch from fe6c335 to ebe31fa Compare March 19, 2020 00:49
@safern safern force-pushed the GlobalizationWindowsIcu branch from ebe31fa to f2e22b4 Compare March 19, 2020 00:50
@safern safern force-pushed the GlobalizationWindowsIcu branch from 058e160 to cd46ed9 Compare March 20, 2020 05:39
@safern safern force-pushed the GlobalizationWindowsIcu branch from cd46ed9 to 7544939 Compare March 20, 2020 05:48
@safern
Copy link
Member Author

safern commented Mar 20, 2020

It seems like finally CI is almost green. Can I get another review on this please? 😄

@@ -4,7 +4,10 @@
//

#include <stdint.h>

#if defined(TARGET_UNIX)
#include <unistd.h>
Copy link
Member

Choose a reason for hiding this comment

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

Is this header actually needed on Unix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. I’ll double check and if not I’ll remove it.

@safern
Copy link
Member Author

safern commented Mar 21, 2020

It seems like the failures are: #32377 — I’ve also seen them fail in some recent PRs.

@safern safern merged commit d07a30c into dotnet:master Mar 21, 2020
@safern safern deleted the GlobalizationWindowsIcu branch March 23, 2020 21:49
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants