Build native code for WebAssembly target by ph1ll · Pull Request #4624 · dotnet/corert

ph1ll · 2017-09-27T17:14:35Z

This is very much a work in progress, but adding the PR so that changes are visible.

At the moment, the overall wasm build will fail. See #4620 for the related build scripts.

Update cmake files for wasm target.
Build sdk\libbootstrappercpp.bc
Build sdk\libPortableRuntime.bc (Compile Runtime with Emscripten #4505)
Build sdk\libSystem.Private.CoreLib.Native.bc (Compile System.Private.CoreLib.Native for WebAssembly #4507)

* [ ] Build sdk\libbootstrapper.bc
* [ ] Build sdk\libRuntime.bc (#4505)
* [ ] Build tools\jitinterface.bc

dnfclas · 2017-09-27T17:14:39Z

@ph1ll,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

jkotas · 2017-09-27T17:19:07Z

You only need: libbootstrappercpp.bc, libPortableRuntime.bc and libSystem.Private.CoreLib.Native.bc . The rest are not applicable for the WebAssembly target.

jkotas · 2017-09-27T17:20:05Z

  elseif(CLR_CMAKE_PLATFORM_ARCH_ARM)
    set(ARCH_SOURCES_DIR arm)
    set(ASM_SUFFIX S)
+  elseif(CLR_CMAKE_PLATFORM_ARCH_WASM)


Should not be needed.

jkotas · 2017-09-27T17:21:31Z

-    SHARED
-    ${NATIVE_SOURCES}
-)
+if(CMAKE_SYSTEM_NAME STREQUAL Emscripten)


Exclude jitinterface in src\Native\CMakeLists.txt instead

ph1ll · 2017-09-27T18:04:02Z

Thanks @jkotas. I have updated the cmake files to conditionally exclude the non-applicable libraries. Should make it a bit easier to focus on getting the rest to compile.

jkotas · 2017-09-27T20:28:20Z

+#else
    uuid_generate_random(*(uuid_t*)pGuid);
-
+    


Nit: Unnecessary white space change

jkotas · 2017-09-27T20:28:52Z

+    
    // Change the byte order of the Data1, 2 and 3, since the uuid_generate_random
    // generates them with big endian while GUIDS need to have them in little endian.
    pGuid->Data1 = SWAP32(pGuid->Data1);


I would think this swapping should be done for uuid_generate as well.

jkotas · 2017-09-27T20:30:39Z

 extern "C" void CoreLibNative_CreateGuid(GUID* pGuid)
 {
 #if HAVE_LIBUUID_H
+#ifdef _WASM_


It would be nice to do this using HAVE_XXX autogenerated defines for portability, not hard coding it to WASM.

Will do.

My only concern here would be that uuid_generate_random will always provide version 4 UUIDs, where as uuid_generate will fall back to version 1 UUIDs, based on current time and MAC address. Where as we know Emscripten implements uuid_generate using version 4 UUIDs, this could result in information leakage on other platforms in the future.

jkotas · 2017-09-27T20:32:11Z

-add_subdirectory(jitinterface)
+
+# We don't need jitinterface for WASM.
+if(NOT CMAKE_SYSTEM_NAME STREQUAL Emscripten)


Should we rather use CLR_CMAKE_PLATFORM_ARCH_WASM for these checks? (It would be self-explanatory at least - no need to have We don't need jitinterface for WASM. comment)

morganbr · 2017-09-27T20:53:34Z


-add_subdirectory(base)
+# We don't need base for WASM.
+if(NOT CMAKE_SYSTEM_NAME STREQUAL Emscripten)


Do we have the WASM build variable available here? Based on some of the other comments, it's not clear if emcmake is a long term plan, so it might be better to switch based on the target rather than the cmake name

morganbr · 2017-09-27T20:55:46Z


+if(CMAKE_SYSTEM_NAME STREQUAL Emscripten)
+    set(CLR_CMAKE_PLATFORM_UNIX 1)
+    set(CLR_CMAKE_PLATFORM_UNIX_TARGET_WASM 1)


WebAssembly isn't really UNIX, would it make sense for this variable to just be called CLR_CMAKE_PLATFORM_WASM ?

ph1ll · 2017-09-30T10:59:46Z

I have rebased this on master so that the build scripts are present.

jkotas · 2017-09-30T14:26:38Z

OSX build failed with:

error: Don't know how to generate UUID on this platform

Other failures are infrastructure issues - not related to your change.

ph1ll · 2017-10-02T12:28:06Z

I dont have an OS X machine for testing on unfortunately. I have instead switched to a different method of checking for the uuid functions in the uuid/uuid.h header that doesnt require linking the right libraries for the check. Hopefully this unbreaks the build on OS X.

morganbr · 2017-10-03T00:10:52Z

@dotnet-bot build and test Windows_NT Debug and CoreCLR tests please
@dotnet-bot build and test Windows_NT Release please

morganbr · 2017-10-03T02:01:11Z

@ph1ll Looks like you're clean on all OSes now.

jkotas · 2017-10-03T05:16:59Z

+#include <uuid/uuid.h>
+int main()
+{
+#ifndef uuid_generate_random


This hack does not look like improvement. I would rather take the WASM ifdef that you had originally than this.

Were you happy with the check_function_exists usage as below? This fails on OS X but I should imagine it is case of linking the correct library for the check. I don't have a OS X machine to test with at the moment, but if you bear with me I can try and sort something out.

set(CMAKE_REQUIRED_LIBRARIES uuid) check_function_exists(uuid_generate_random HAVE_UUID_GENERATE_RANDOM) check_function_exists(uuid_generate HAVE_UUID_GENERATE) set(CMAKE_REQUIRED_LIBRARIES)

Yes, the check_function_exists usage looked much better.

@ph1ll , if you need to commit and watch the CI results a couple times to figure out OS X, that's totally fine. I'll also try to get a build on a Mac soon, but it's probably easier for you to just iterate with CI.

morganbr · 2017-10-17T20:44:02Z

@ph1ll, is there anything you need help with? I'd really love to get the runtime compiling 😄

ph1ll · 2017-10-18T07:55:31Z

@morganbr Sorry I've had a couple of busy weeks with work and haven't had a chance to do anymore yet I am afraid. I have some time next week, but I certainly do not want to hold anything up. I've added you to as a collaborator to my fork, but also feel free to take what is already here if you want to get things moving.

At the moment, there is the cmake OSX issue to look at, and about 40 or so initial build errors for libPortableRuntime. Some of the build errors relate to simple defines, but I haven't had a chance to go beyond that at the moment.

morganbr · 2017-10-29T11:47:38Z

I've got libPortableRuntime and libRuntime building using these changes and I can link it to the wasm test program. I had to touch a lot of files in the runtime, so I'll create a separate PR. I only needed a couple tiny changes to your build system changes, so hopefully we can just deal with the merge conflict and UUID issue to merge this change.

morganbr · 2017-10-30T04:51:12Z

I tried to point to libc for UUID for Mac, but that seems to have broken both Mac and Linux (Emscripten still works). Any ideas @ph1ll or @jkotas?

jkotas · 2017-10-30T06:06:11Z

The libraries are specified without libc prefix, so it would be just c, not libc.

If it still does not work, I think the following should be good enough:

if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)
   set(CMAKE_REQUIRED_LIBRARIES uuid)
endif()
check_function_exists(uuid_generate_random HAVE_UUID_GENERATE_RANDOM) 
check_function_exists(uuid_generate HAVE_UUID_GENERATE) 
set(CMAKE_REQUIRED_LIBRARIES)

morganbr · 2017-10-30T07:17:07Z

Changing libc to c didn't work, trying the conditional required libraries check now.

jkotas · 2017-10-30T13:32:58Z

+if(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)
+   set(CMAKE_REQUIRED_LIBRARIES uuid)
+endif()
+unset(HAVE_UUID_GENERATE_RANDOM CACHE)


These unset commands should not be needed.

…get.

This reverts commit 2b50344.

…erate_random

morganbr · 2017-10-31T08:17:23Z

Thanks for getting most of this together, @ph1ll! When you're less busy, there's lots more that we'd love your help with if you're interested.

ph1ll · 2017-10-31T08:59:10Z

@morganbr I think you did the hard part, sorry again for not being able to get it finished up. Will definitely try and help out where I can.

dnfclas added the cla-already-signed label Sep 27, 2017

jkotas reviewed Sep 27, 2017

View reviewed changes

jkotas requested a review from morganbr September 27, 2017 17:24

jkotas reviewed Sep 27, 2017

View reviewed changes

morganbr reviewed Sep 27, 2017

View reviewed changes

jkotas reviewed Oct 3, 2017

View reviewed changes

MichalStrehovsky added the arch-wasm label Oct 3, 2017

jkotas reviewed Oct 30, 2017

View reviewed changes

ph1ll added 6 commits October 31, 2017 00:10

Add support to cmake files for wasm build

b3e5652

Use uuid_generate in place of uuid_generate_random for wasm builds

0de6e31

Conditionally exclude non-applicable libraries when building wasm tar…

253a6a6

…get.

Use HAVE_XXX defines for portability

31ebdc9

Update platform build variables for WASM based on review

b0b770b

Fix WASM build for bootstrappercpp

e267f99

ph1ll and others added 7 commits October 31, 2017 00:13

Use alternative method to check for uuid functions

0ded0ef

Revert "Use alternative method to check for uuid functions"

9245828

This reverts commit 2b50344.

Try to use check_function_exists for Mac as well as Unix for uuid_gen…

21319ec

…erate_random

Clear CI machine cache for HAVE_UUID_GENERATE

be4a62b

Fix libc reference

46d7889

Special case OS X for UUID detection

5be1835

Remove UUID cache clearing

1f0622e

morganbr changed the title ~~WIP: Build native code for WebAssembly target~~ Build native code for WebAssembly target Oct 31, 2017

morganbr merged commit 15ce20d into dotnet:master Oct 31, 2017

vadimkantorov mentioned this pull request Sep 1, 2020

[feature request] Support uuid_generate_random (in uuid/uuid.h) emscripten-core/emscripten#12093

Open

		#else
		uuid_generate_random((uuid_t)pGuid);

Uh oh!

Conversation

ph1ll commented Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Sep 27, 2017

Uh oh!

jkotas commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ph1ll commented Sep 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ph1ll commented Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Sep 30, 2017

Uh oh!

ph1ll commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morganbr commented Oct 3, 2017

Uh oh!

morganbr commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganbr commented Oct 17, 2017

Uh oh!

ph1ll commented Oct 18, 2017

Uh oh!

morganbr commented Oct 29, 2017

Uh oh!

morganbr commented Oct 30, 2017

Uh oh!

jkotas commented Oct 30, 2017

Uh oh!

morganbr commented Oct 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morganbr commented Oct 31, 2017

Uh oh!

ph1ll commented Oct 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ph1ll commented Sep 27, 2017 •

edited

Loading

ph1ll commented Sep 30, 2017 •

edited

Loading

ph1ll commented Oct 2, 2017 •

edited

Loading