[C++ification] Continue converting code to "proper" C++ #3759

grendello · 2019-10-09T20:41:15Z

Next commit in the series of making our native code use "proper" C++ constructs,
take advantage of the language's features etc. Changes:

Fix more compilation warnings arising from differences between platforms
Compare pointers against nullptr instead of the traditional !p C-style check
Use the simple_pointer_guard class in more places
Use references to pointers instead of pointers to pointers in a handful of
places to decrease the number of null checks
Switch nested namespace declarations to C++17 style
Make Util::ends_with slightly faster when used with literal strings,
taking advantage of C++ templates
Move variable declarations closer to places where they are actually used
Move a number of extern "C" functions to external-api.cc

jonpryor · 2019-10-11T02:48:02Z

Mono.Android_TestsAppBundle crashed:

Unhandled Exception:
System.DllNotFoundException: __Internal assembly:<unknown assembly> type:<unknown type> member:(null)
  at (wrapper managed-to-native) Android.Runtime.JNIEnv.monodroid_timing_start(string)
  at Android.Runtime.JNIEnv.Initialize (Android.Runtime.JnienvInitializeArgs* args) [0x0001c] in <1260048817b5414c97481e16c94f6621>:0

grendello · 2019-10-11T06:17:27Z

Mono.Android_TestsAppBundle crashed:

Unhandled Exception:
System.DllNotFoundException: __Internal assembly:<unknown assembly> type:<unknown type> member:(null)
  at (wrapper managed-to-native) Android.Runtime.JNIEnv.monodroid_timing_start(string)
  at Android.Runtime.JNIEnv.Initialize (Android.Runtime.JnienvInitializeArgs* args) [0x0001c] in <1260048817b5414c97481e16c94f6621>:0

Huh... This is exactly the same code as in master, don't see a reason why it would fail. We'll see if re-run helps.

grendello · 2019-10-14T11:18:18Z

The weird DllNotFoundException failure is gone on rebuild. There's one BCL failure:

10-11 09:07:48.995  3228  5104 I NUnit   : 1) MonoTests.System.GCTest.TestGetBytesAllocatedForCurrentThread (monodroid_corlib_test)
10-11 09:07:48.995  3228  5104 I NUnit   :   Expected: 219949840
10-11 09:07:48.995  3228  5104 I NUnit   :   But was:  -4075017456
10-11 09:07:48.995  3228  5104 I NUnit   : 
10-11 09:07:48.995  3228  5104 I NUnit   : 
10-11 09:07:48.995  3228  5104 I NUnit   :   at MonoTests.System.GCTest.TestGetBytesAllocatedForCurrentThread () [0x00107] in <5db7dfc9077540ba9da416882d254e81>:0 
10-11 09:07:48.995  3228  5104 I NUnit   :   at (wrapper managed-to-native) System.Reflection.RuntimeMethodInfo.InternalInvoke(System.Reflection.RuntimeMethodInfo,object,object[],System.Exception&)
10-11 09:07:48.995  3228  5104 I NUnit   :   at System.Reflection.RuntimeMethodInfo.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0006a] in <af3ab1ad8108411d85c71d327db0ec0f>:0

and one our recent favorite failure:

  /Library/Frameworks/Mono.framework/External/xbuild/Xamarin/Android/Xamarin.Android.Common.targets(3151,2): error MSB6006: "zipalign" exited with code 1. [/Users/vsts/agent/2.158.0/work/1/s/bin/TestRelease/temp/CheckSequencePointGenerationTrueTrueFalseTrueFullFalserelease/UnnamedProject.csproj]

    1 Warning(s)
    1 Error(s)

Neither of those appear to be related to this PR.

jonpryor · 2019-10-14T17:20:24Z

src/monodroid/jni/debug.cc

+	}
+
+	bool use_fd = false;
+	if (strncmp (cmd, "start debugger: ", 16) == 0) {


Question: stylistically would it be better if we removed the hardcoded length and instead went with string constants & sizeof()?

static constexpr char COMMAND_START_DEBUGGER[] = "start debugger: "; if (strncmp (cmd, COMMAND_START_DEBUGGER, sizeof (COMMAND_START_DEBUGGER)-1) == 0) { ... }

Yeah, it would be better (even better with constexpr size_t COMMAND_START_DEBUGGER_LEN = sizeof(COMMAND_START_DEBUGGER)-1 but it would blow out the source code size...)

but if you don't mind the visual bloat, I'm ok with it :)

I think the "visual bloat" would be a good idea, considering that all of these lines are changing anyway..,.

Next commit in the series of making our native code use "proper" C++ constructs, take advantage of the language's features etc. Changes: * Fix more compilation warnings arising from differences between platforms * Compare pointers against `nullptr` instead of the traditional `!p` C-style check * Use the `simple_pointer_guard` class in more places * Use references to pointers instead of pointers to pointers in a handful of places to decrease the number of null checks * Switch nested namespace declarations to C++17 style * Make `Util::ends_with` slightly faster when used with literal strings, taking advantage of C++ templates * Move variable declarations closer to places where they are actually used * Move a number of extern "C" functions to `external-api.cc`

grendello · 2019-10-15T08:22:39Z

/azp run

azure-pipelines · 2019-10-15T08:23:01Z

Azure Pipelines successfully started running 1 pipeline(s).

grendello · 2019-10-15T12:47:59Z

/azp run

azure-pipelines · 2019-10-15T12:48:21Z

Azure Pipelines successfully started running 1 pipeline(s).

jaykrell · 2019-10-20T08:29:35Z

@grendello
"• Compare pointers against nullptr instead of the traditional !p C-style check"

Why?

I think if (p) and if (!p) are both idiomatic C and even better idiomatic C++.
Because p could be a Win32Handle and it'll compare against aboth nullptr and INVALID_HANDLE_VALUE in its explicit operator bool (C++11) or its C++98 workaround.

or p would be File (perhaps from open) and if (p) would compare against -1.
etc.

It means "if valid", not "if not 0".

Context: dotnet#3759 Context: dotnet@6d85759#diff-e99739c16155208685952aa52870d4cdL33-L39 When switching from C-style `xcalloc` to C++-style `new` I failed to use the proper type to allocate array which holds directories where Android placed the APK files of the application. This mistake manifested itself only when using AppBundles, which is where we allocate more than one entry of the array. The resulting error was the following runtime exception: 11-04 12:27:55.722 3339 3339 E mono : Unhandled Exception: 11-04 12:27:55.722 3339 3339 E mono : System.DllNotFoundException: __Internal assembly:<unknown assembly> type:<unknown type> member:(null) 11-04 12:27:55.722 3339 3339 E mono : at (wrapper managed-to-native) Android.Runtime.JNIEnv.monodroid_timing_start(string) Which occurred because Xamarin.Android runtime failed to load `libmonodroid.so` from one of the directories that should have been stored in the array mentioned above - usually found in the *last* entry of the array. When iterating over the array, the XA runtime would end up with a null pointer after the end of the array instead of a pointer to the last entry. This commit uses the proper type when allocating the array.

grendello added do-not-merge PR should not be merged. full-mono-integration-build For PRs; run a full build (~6-10h for mono bumps), not the faster PR subset (~2h for mono bumps) labels Oct 9, 2019

grendello requested a review from jonpryor as a code owner October 9, 2019 20:41

grendello force-pushed the cppify branch from 8107a6c to ddc6625 Compare October 10, 2019 07:56

grendello removed the do-not-merge PR should not be merged. label Oct 10, 2019

grendello changed the title ~~[WIP, C++ification] Continue converting code to "proper" C++~~ [C++ification] Continue converting code to "proper" C++ Oct 10, 2019

grendello force-pushed the cppify branch from ddc6625 to d5e00b5 Compare October 10, 2019 21:24

grendello force-pushed the cppify branch from d5e00b5 to fd93dd7 Compare October 11, 2019 06:16

jonpryor reviewed Oct 14, 2019

View reviewed changes

grendello force-pushed the cppify branch from fd93dd7 to 03e42a5 Compare October 14, 2019 19:36

jonpryor merged commit 6d85759 into dotnet:master Oct 15, 2019

grendello deleted the cppify branch October 15, 2019 17:53

grendello mentioned this pull request Nov 7, 2019

Allocate directory array with correct type #3888

Merged

brendanzagaeski added this to the In Preview: 10.2 (d16-5) milestone Jan 23, 2020

github-actions bot locked and limited conversation to collaborators Jan 24, 2024

[C++ification] Continue converting code to "proper" C++ #3759

[C++ification] Continue converting code to "proper" C++ #3759

Uh oh!

Conversation

grendello commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonpryor commented Oct 11, 2019

Uh oh!

grendello commented Oct 11, 2019

Uh oh!

grendello commented Oct 14, 2019

Uh oh!

jonpryor Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

grendello Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grendello Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

jonpryor Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

grendello commented Oct 15, 2019

Uh oh!

azure-pipelines bot commented Oct 15, 2019

Uh oh!

grendello commented Oct 15, 2019

Uh oh!

azure-pipelines bot commented Oct 15, 2019

Uh oh!

jaykrell commented Oct 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

grendello commented Oct 9, 2019 •

edited

Loading

grendello Oct 14, 2019 •

edited

Loading