[ObjWriter/Linux] Integration into build process by BredPet · Pull Request #5091 · dotnet/corert

BredPet · 2017-12-11T09:24:38Z

- now it downloads llvm 5 release and uses it in the building objwriter
- add [objwriter] option into build.sh
- only Linux platform support
- cross building support(x86/x64 -> ARM32)

Later I'm going to add the following:

Documentation with the work's logic.
Additional trick related to the fact that the cross building for arm32 is x86, not the x64 host.
Integration ObjWriter build and general build to use the locally built ObjWriter.
Support for other platforms.

@dotnet/arm32-contrib please review

jkotas · 2017-12-11T15:19:56Z

+
+# Make sure to remove debug flags from general build flags in Release case
+if(OBJWRITER_BUILD_TYPE STREQUAL "Release")
+  list(REMOVE_ITEM CORERT_NATIVE_COMPILE_OPTIONS "-g")


Our convention has been to build with -g in Release, and then archive and strip the symbols on the binary.

Ok, just llvm picks up "-g" and binaries become very large. Of course, we can archive and strip it manually.

jkotas · 2017-12-11T15:20:51Z

+# Make sure to remove debug flags from general build flags in Release case
+if(OBJWRITER_BUILD_TYPE STREQUAL "Release")
+  list(REMOVE_ITEM CORERT_NATIVE_COMPILE_OPTIONS "-g")
+  list(REMOVE_ITEM CORERT_NATIVE_COMPILE_OPTIONS "-O0")


This should not be necessary. The compile options do not have -O0 for release build.

This is in case the flags came from Debug version, but we are building Release)
Now, by default, Debug building is going to Release ObjWriter. I did this because Debug build is needed really rare. For Debug version, it's necessary to set OBJWRITER_BUILD_TYPE manually.

jkotas · 2017-12-11T15:29:36Z

+# Build ObjWriter on Linux only
+if(CMAKE_SYSTEM_NAME STREQUAL Linux)
+  if($ENV{__ObjWriterBuild})
+    add_subdirectory(ObjWriter/llvmCap)


Is the Cap suffix a convention used for similar external projects? I do not see a problem with the name ... just wondering where it came from.

No, maybe, any suggestions?)

jkotas · 2017-12-11T15:29:46Z

cc @janvorli

jkotas · 2017-12-11T15:30:24Z

LGTM otherwise. Thank you!

janvorli · 2017-12-11T15:46:48Z

+  list(APPEND LLVM_CMAKE_EXTRA_ARGS "-DLLVM_DEFAULT_TARGET_TRIPLE=thumbv7-linux-gnueabi")
+endif()
+
+list(REMOVE_DUPLICATES CORERT_NATIVE_COMPILE_OPTIONS)


Why is this needed?

Just for ARM, it needs thumb triple, https://github.com/dotnet/corert/blob/master/src/Native/ObjWriter/objwriter.cpp#L307
As for the deletion of duplicates, in our project flags are set several times, just to simplify the transfer of arguments.
Moreover, there is a bug associated with the use of CMake toolchain file for our Native builds. For example, https://github.com/dotnet/corert/blob/master/cross/armel/toolchain.cmake#L40 here, CMAKE_EXE_LINKER_FLAGS for each rebuild will change and include itself twice, then three times and so on. Until the bash or CMake screams that the line of arguments is overflowing. This bug is repeated for other architectures too. I decided to fix it later.

Ok, thanks for the explanation. We should definitely fix it at the source of the issue, but I am ok doing that later. Could you please create an issue to track it (unless we already have it)?

janvorli · 2017-12-11T16:23:43Z

+
+if(NOT OBJWRITER_TARGET_ARCH)
+  if(CLR_CMAKE_TARGET_ARCH MATCHES "^(x64|x86)$")
+    if(NOT $ENV{__CrossBuild})


It would be cleaner to pass this as a CMake variable in gen-buildsys-clang.sh rather than using an env var. Basically, you'd just add the variable definition to the cmake_extra_defines in there based on the __CrossBuild value.

I thought about it. It seemed to me, it would complicate gen-buildsys-clang.sh logic. But if it's desirable to maintain a single style, I can do a little to change, so that there is no use of external variables from CMake.

It is a matter of single style. It seems strange to have some parameters in env vars and some passed as defined variables using the cmake -D option. I personally prefer the -D option since it is more natural to cmake and also makes the behavior of the generated make files static. So basically, you generate make files and then when you run make, it always builds the same way no matter what the env settings are.
We use cmake in a slightly twisted way, the "native" way as I understand it is to run cmake only if the CMakeLists.txt are changed. Otherwise just run make.

Ok, I'll fix.

- now it downloads llvm 5 release and uses it in the building ObjWriter - add [objwriter] option into build.sh - only Linux platform support - cross building support(x86/x64 -> ARM32) - LLVM is built in Release build type always, ObjWriter uses global project build type. Signed-off-by: Petr Bred <bredpetr@gmail.com>

jkotas

Thanks

BredPet force-pushed the objwriter_build_integration branch 2 times, most recently from cdc960d to d3e3cda Compare December 11, 2017 09:33

jkotas reviewed Dec 11, 2017

View reviewed changes

janvorli reviewed Dec 11, 2017

View reviewed changes

BredPet mentioned this pull request Dec 12, 2017

[Cross] CMake Toolchain compile flags loop #5093

Closed

BredPet force-pushed the objwriter_build_integration branch from d3e3cda to f8977b8 Compare December 12, 2017 18:37

BredPet force-pushed the objwriter_build_integration branch from f8977b8 to 20b1c3e Compare December 12, 2017 18:40

jkotas approved these changes Dec 12, 2017

View reviewed changes

jkotas merged commit 9580df7 into dotnet:master Dec 12, 2017

BredPet mentioned this pull request Apr 6, 2018

Fix cmake toolchain compile flags loop dotnet/coreclr#17380

Merged

Uh oh!

Conversation

BredPet commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

BredPet Dec 11, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Dec 11, 2017

Uh oh!

jkotas commented Dec 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BredPet Dec 11, 2017 • 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!

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!

BredPet Dec 11, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BredPet commented Dec 11, 2017 •

edited

Loading

BredPet Dec 11, 2017 •

edited

Loading

BredPet Dec 11, 2017 •

edited

Loading

BredPet Dec 11, 2017 •

edited

Loading