[corefoundation] Fix TypeInitializationException in OSLog #7962

spouliot · 2020-02-24T17:03:18Z

Default property was using a nil-handle which is incorrect since

we don't allow that (this is generally a bad sign)
it does not map to OS_LOG_DEFAULT

Since Default was assigned in the type (static) constructor then
the whole type became unusable :(

Header log.h shows that the right definition requires us to load a
field and use it.

define OS_LOG_DEFAULT OS_OBJECT_GLOBAL_OBJECT(os_log_t, _os_log_default)

While NULL can actually be used for disabled (not exposed) by this
contradicting (nullability-wise) macro

define OS_LOG_DISABLED ((os_log_t _Nonnull)NULL)

Also adds unit tests. A more general tests for .cctor will be added
to introspection tests in a separate PR.

Fixes #7959

`Default` property was using a nil-handle which is incorrect since * we don't allow that (this is generally a bad sign) * it does not map to `OS_LOG_DEFAULT` Since `Default` was assigned in the type (static) constructor then the whole type became unusable :( Header `log.h` shows that the right definition requires us to load a field and use it. ``` define OS_LOG_DEFAULT OS_OBJECT_GLOBAL_OBJECT(os_log_t, _os_log_default) ``` While `NULL` can actually be used for disabled (not exposed) by this contradicting (nullability-wise) macro ``` define OS_LOG_DISABLED ((os_log_t _Nonnull)NULL) ``` Also adds unit tests. A more general tests for `.cctor` will be added to introspection tests in a separate PR. Fixes dotnet#7959

dalexsoto

Nice catch!

mandel-macaque · 2020-02-24T17:30:52Z

tests/monotouch-test/CoreFoundation/OSLogTest.cs

+			using (var log = new OSLog ("subsystem", "category")) {
+				log.Log (OSLogLevel.Error, "monotouch-test /  custom / Debug");
+				// this will show in the application output (e.g. inside VSfM)
+				// and also inside Console.app under the simulator/device


No Asserts?

As long as it does to crash or throw here :P I think this is fine #YOLO

@mandel-macaque not quite sure on what to assert (if it does not crash, like it would without the fix). Reading console.app is a quite troublesome from simulator and would not work on devices. Got any ideas ?

I think there is a way with http://www.cplusplus.com/reference/cstdio/freopen/ freopen stderr to a file, read wha was logged in the file. Maybe too much work :/

I'm fine with "not crashing is good enough". I could see redirects being brittle maybe.

I won't stop you PR'ing that... but, for myself, messing with stderr, which is already used used by the test harness, seems more a problem than a solution :)

There's a precedent for redirecting stderr... https://github.com/xamarin/maccore/blob/4c92d05de776e6166648f57ff26a5efab8c06363/tools/mlaunch/Xamarin.Hosting/Xamarin.Launcher/InstallationProxyService.cs#L95-L109

monojenkins · 2020-02-24T18:11:12Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

3 tests failed, 86 tests passed.

Failed tests

link all/iOS Unified 64-bits - simulator/Release: Failed
link all/tvOS - simulator/Debug: Failed
link sdk/tvOS - simulator/Release: Failed

rolfbjarne · 2020-02-24T18:17:08Z

tests/monotouch-test/CoreFoundation/OSLogTest.cs

+		[Test]
+		public void Default ()
+		{
+			OSLog.Default.Log (OSLogLevel.Default, "monotouch-test / Default / Default");


You need a version check to avoid this test on older OSes.

spouliot · 2020-02-24T18:20:02Z

Unrelated failures (3) due to The remote server returned an error: (504) Gateway Timeout. on some tests

rolfbjarne · 2020-02-24T18:21:25Z

src/CoreFoundation/OSLog.cs

+
+		static OSLog ()
+		{
+			_os_log_default = Dlfcn.dlsym (Libraries.System.Handle, "_os_log_default");


Would make more sense to fetch this on demand in the Default getter? Otherwise it can't be linked away if someone doesn't use the Default property (it's possible to create custom OSLogs).

yes, another reason is that this will run into a similar issue (TLE) on older OS (even if less a problem since it would be unusable if called anyway)

* Only run tests on Xcode 8.0 and later * Remove .cctor as it will help the linker and ensure a TLE won't happen on older OS versions

src/CoreFoundation/OSLog.cs

monojenkins · 2020-02-24T20:22:41Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 88 tests passed.

Failed tests

link all/tvOS - simulator/Release: Failed

monojenkins · 2020-02-25T00:01:11Z

Build success
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
✅ Test run succeeded

spouliot · 2020-02-25T13:55:52Z

@monojenkins backport to d16-6

monojenkins · 2020-02-25T13:56:23Z

@spouliot backporting to d16-6 failed, the patch results in conflicts:

Applying: [corefoundation] Fix TypeInitializationException in OSLog
Using index info to reconstruct a base tree...
M	src/CoreFoundation/OSLog.cs
Falling back to patching base and 3-way merge...
Auto-merging src/CoreFoundation/OSLog.cs
CONFLICT (content): Merge conflict in src/CoreFoundation/OSLog.cs
error: Failed to merge in the changes.
Patch failed at 0001 [corefoundation] Fix TypeInitializationException in OSLog

Please backport manually!

…net#7962) `Default` property was using a nil-handle which is incorrect since * we don't allow that (this is generally a bad sign) * it does not map to `OS_LOG_DEFAULT` Since `Default` was assigned in the type (static) constructor then the whole type became unusable :( Header `log.h` shows that the right definition requires us to load a field and use it. ``` define OS_LOG_DEFAULT OS_OBJECT_GLOBAL_OBJECT(os_log_t, _os_log_default) ``` While `NULL` can actually be used for disabled (not exposed) by this contradicting (nullability-wise) macro ``` define OS_LOG_DISABLED ((os_log_t _Nonnull)NULL) ``` Also adds unit tests. A more general tests for `.cctor` will be added to introspection tests in a separate PR. Fixes dotnet#7959

…) (#7977) `Default` property was using a nil-handle which is incorrect since * we don't allow that (this is generally a bad sign) * it does not map to `OS_LOG_DEFAULT` Since `Default` was assigned in the type (static) constructor then the whole type became unusable :( Header `log.h` shows that the right definition requires us to load a field and use it. ``` define OS_LOG_DEFAULT OS_OBJECT_GLOBAL_OBJECT(os_log_t, _os_log_default) ``` While `NULL` can actually be used for disabled (not exposed) by this contradicting (nullability-wise) macro ``` define OS_LOG_DISABLED ((os_log_t _Nonnull)NULL) ``` Also adds unit tests. A more general tests for `.cctor` will be added to introspection tests in a separate PR. Fixes #7959

`Default` property was using a nil-handle which is incorrect since * we don't allow that (this is generally a bad sign) * it does not map to `OS_LOG_DEFAULT` Since `Default` was assigned in the type (static) constructor then the whole type became unusable :( Header `log.h` shows that the right definition requires us to load a field and use it. ``` define OS_LOG_DEFAULT OS_OBJECT_GLOBAL_OBJECT(os_log_t, _os_log_default) ``` While `NULL` can actually be used for disabled (not exposed) by this contradicting (nullability-wise) macro ``` define OS_LOG_DISABLED ((os_log_t _Nonnull)NULL) ``` Also adds unit tests. A more general tests for `.cctor` will be added to introspection tests in a separate PR. Fixes dotnet#7959

spouliot requested a review from rolfbjarne as a code owner February 24, 2020 17:03

dalexsoto approved these changes Feb 24, 2020

View reviewed changes

mandel-macaque approved these changes Feb 24, 2020

View reviewed changes

rolfbjarne requested changes Feb 24, 2020

View reviewed changes

rolfbjarne reviewed Feb 24, 2020

View reviewed changes

Apply feedback

fbd9e44

* Only run tests on Xcode 8.0 and later * Remove .cctor as it will help the linker and ensure a TLE won't happen on older OS versions

Therzok reviewed Feb 24, 2020

View reviewed changes

src/CoreFoundation/OSLog.cs Outdated Show resolved Hide resolved

Adding missing space

22ec19b

chamons approved these changes Feb 24, 2020

View reviewed changes

rolfbjarne approved these changes Feb 24, 2020

View reviewed changes

spouliot merged commit 9a88551 into dotnet:master Feb 25, 2020

spouliot deleted the gh7959 branch February 25, 2020 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[corefoundation] Fix TypeInitializationException in OSLog #7962

[corefoundation] Fix TypeInitializationException in OSLog #7962

spouliot commented Feb 24, 2020

dalexsoto left a comment

mandel-macaque Feb 24, 2020

dalexsoto Feb 24, 2020 •

edited

Loading

spouliot Feb 24, 2020

mandel-macaque Feb 24, 2020

chamons Feb 24, 2020

spouliot Feb 24, 2020

rolfbjarne Feb 24, 2020

monojenkins commented Feb 24, 2020

Failed tests

rolfbjarne Feb 24, 2020

spouliot commented Feb 24, 2020

rolfbjarne Feb 24, 2020

spouliot Feb 24, 2020

monojenkins commented Feb 24, 2020

Failed tests

monojenkins commented Feb 25, 2020

spouliot commented Feb 25, 2020

monojenkins commented Feb 25, 2020

[corefoundation] Fix TypeInitializationException in OSLog #7962

[corefoundation] Fix TypeInitializationException in OSLog #7962

Conversation

spouliot commented Feb 24, 2020

dalexsoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalexsoto Feb 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Feb 24, 2020

Test results

Failed tests

Choose a reason for hiding this comment

spouliot commented Feb 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Feb 24, 2020

Test results

Failed tests

monojenkins commented Feb 25, 2020

spouliot commented Feb 25, 2020

monojenkins commented Feb 25, 2020

dalexsoto Feb 24, 2020 •

edited

Loading