-
Notifications
You must be signed in to change notification settings - Fork 0
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
Android Build Issue Discussion #1
Comments
@cydhaselton, it would be great if you could share your changes with us. It would be helpful in figuring out issues you will be hitting. You just need to:
Share the branch name with us. That way, we can view the diff of your changes. |
@cydhaselton I am sorry, I have missed that before. Thank you! |
@cydhaselton I have thought you have said you have rebased the changes on the latest sources. But the log shows the last checkin in your branch before your changes is from July 27. |
Can you do the rebase and force push to get the stuff up to date? It is simple:
I've just tried that locally and there were no merge conflicts. |
@janvorli: This is odd…I created the fork about a week ago, maybe less, and created the feature-branch off that fork the same day. What branch should I have checked out before running the first command? EDIT: Ran the commands and got he following errors
|
FYI, here's the gdb log of the debugging recommendation (MapViewOfFileEx set as breakpoint) |
@cydhaselton as for the rebase, I've checked out the feature-android branch first (that's what I meant by saying "Make sure you are switched to the 'feature-android' branch") Regarding the gdb log, the SIGSEGV is strange, since without the debugger, we have not seen it and the strace log has shown second call to the MapViewOfFileEx. I wonder if it is some debugger quirk. |
@janvorli
Also you may be right about the compiler bug. Here's the latest output: http://pastebin.com/uSm3jkyB |
Sorry, I meant |
Here's the output requested: http://pastebin.com/qEDaxR8Y (it stops on 695) Running |
As for the rebase, it is strange, it worked ok for me when I checked out your branch and did the rebase right away. Is it possible that you have some modifications in your local tree that are not pushed to the github server? Regarding the debugging, there are two things. First, the sources don't match the binaries, as GDB reports at one place. But it still seems that GDB just cannot step and behaves erratically instead. Setting a breakpoint and running until it is hit seems to behave well. So we will need to use breakpoints for further debugging. |
@janvorli See previous message regarding pushed changes Regarding source files being newer; I'll rebuild and then debug with the breakpoints you recommended |
@cydhaselton I have rebased the changes locally and resolved all the conflicts. But I can see that there are many changes that are or seem unnecessary, changes that are wrong and also some things that reverted some changes that were made in the master during the last few months (which are probably the git rebase issues). Several CMakeLists.txt and .sh files look completely messed up probably because of the git rebase. So changes in these will likely need to be remade manually. Changes that are not neccesary:
Changes that are wrong:
Changes that I don't know why they were made
|
Given the number of issues with the sources in this fork I'm going to do a hard reset from the upstream master and start over with a new feature-android branch. |
@cydhaselton I have scanned the other thread and I don't see any recommendation to comment out parts of functions in the .S files or change the EMIT_BREAKPOINT. Can you point me to that discussion, please? |
@janvorli I updated previous message; I'm starting over with a hard reset from the upstream master. |
Here's the problem I ran into that led to this change. I'm running into it now, since I hard reset the master and feature-android branch.
|
@cydhaselton the last option is the closest to what I would recommend. You can try to modify the src/pal/inc/pal_mstypes.h to exclude the int8_t and similar types. They are already under |
@janvorli: Just saw your message after making changes. When you get a chance, could you check latest commit to see if it's appropriate? |
@cydhaselton this looks fine, only remove the include of mbusafecrt.h and remove the spaces around the parents in the prototype of PAL_wcsnlen and the parameter names in the prototype in src/pal/src/safecrt/sscanf.cpp |
@janvorli Done, I think. I'm not sure what you meant by parents of the prototype. I've pushed the change to the feature-android branch I've also pushed a set of changes to fix another conflict with |
I am sorry, I meant parens (parentheses). |
@cydhaselton regarding your fix to __reserved, I think that the defines in sortversioning.h, sal.h and specstrings_strict.h should not be needed. These files should never be included together with platform specific headers, so there should be no conflict. The only place where it can conflict is the dbgeng.h. And in this file, I would prefer solving the problem differently. I would suggest to:
That should fix the conflict. |
@janvorli When I leave sal.h, sortversioning.h and specstrings_strict.h as is, the build still fails. After changing those files the build proceeds. I can reverse the changes, file by file, until the build fails, if necessary. Will make the recommended changes to dbgeng.h and push. |
@cydhaselton it would be nice to know which source was being compiled when it failed without the change in the sal.h, sortversioning.h and specstrings_strict.h, just to let me understand when that happened. It is possible we are including the sal.h unnecessarily somewhere. |
@janvorli I'll start reversing the In the meantime, here's the link to the scanelf -qT output of the completed build: http://pastebin.com/7wpUUeZW. It would be good to come up with a plan to eliminate them; that way I can commit and push a set of changes that would be applicable for Android's -fPIC requirements, and those changes could be used as a reference point for other platforms with similar restrictions. |
@janvorli I did comment out those lines previously; I'll double check just in case it was somehow reverted. I'll also add that line and post the results. |
@janvorli Here is the link to the log post-addition of printf: http://pastebin.com/NNERUAQW As usual,
|
@cydhaselton the output is strange, maybe I've asked you to put the printf to a wrong place. Let me try it locally and see where it really belongs. |
Hmm, that's really strange. On amd64, it prints the addresses and sizes as expected while in your case, it prints always zero and 0x7f.
|
@janvorli In src/jit/codegencommon.cpp there was a printf statement at 3158 that was commented out; it's possible I commented out the line earlier instead of deleting it in anticipation for needing it later on. EDIT: Running the PAL tests throws a series of 'file not found' errors, even after fixing the hardcoded EDIT: I'm running |
@janvorli So, I found some time to try & build coreclr for Alpine on ARM. I'm building om aarch64 (so 64-bit version of ARM), and Alpine seems to have all dependencies except for libunwind. I built libunwind from source. Building coreclr via Here's (part of) the build output:
I used a Docker container for building, so the entire VM setup & build process is documented here: https://github.com/qmfrederik/dotnet-alpine-arm/blob/master/Dockerfile I'm assuming something went wrong when building libunwind. I'll dig around but if you have any pointers, they would be greatly appreciated. PS @cydhaselton I don't think e-mail notifications are sent out when you edit Github messages, so if you have news to share, perhaps best is to create a new entry :) |
@qmfrederik, aports has libunwind package in their current release, and they didn't enabled AARCH64 yet: |
@qmfrederik can you please create a separate issue for the Linux aarch64? This one is already extremely long. if(PAL_CMAKE_PLATFORM_ARCH_ARM64)
find_library(UNWIND_ARCH NAMES unwind-aarch64)
endif() in the src/pal/src/CMakeLists.txt around line 290 (I am not sure if the unwind-aarch64 is the right name, just extrapolating from the names for other architectures) |
@janvorli EDIT: Running the PAL tests throws a series of 'file not found' errors, even after fixing the hardcoded EDIT: I'm running (thanks for the tip @qmfrederik) |
@cydhaselton yes, the skiptests is the reason |
@janvorli Unfortunately unless it's possible to a) selectively build tests or b) mount external storage into the Termux fs, it won't be possible to build tests on my system; building the entire suite of tests requires more space than it has available. Unless there are other suggestions I'm going to recommend putting this on the back-burner until debugger issues are resolved. |
Closing this issue due to unresolvable/untestable build issues. Will re-open if one or more of the following conditions are met:
|
@cydhaselton can you keep this repo alive meanwhile? It will serve as a good reference for Alpine ARM64. You may also want to give it another try once Alpine ARM succeeds |
@qmfrederik have you already copied the relevant changes from this repo to your one? You will definitely need them to make stuff work. If you are unsure on which changes to bring over, I can help you to pick the right ones. |
@janvorli Not yet. If you give me commit ids I can cherry pick them into my branch. |
@qmfrederik Will keep fork and feature-android branch around. Update this issue if Alpine Linux build and tests with simple app succeed. |
@qmfrederik it seems that some of the commits are mix of the necessary changes and Android specific stuff. Let me actually cleanup the changes and commit them to main. @cydhaselton I hope you won't mind. |
Sure, ping me when you have a branch in upstream coreclr and I'll start from there. |
@janvorli Nope; if you're busy ping me with what you're seeing and I'll make changes here |
@cydhaselton I was thinking about the reasons you listed for suspending the effort. Perhaps there is an alternative way. Did you consider porting to native Android instead of Termux? Here's what that would look like:
You'll loose the Termux compatibility layer so it may be a bit more of a porting effort (although there are similarities with the Linux kernel and the BSD APIs) Do you think that would work? Is there a specific reason Termux was involved? |
@qmfrederik I started building coreclr for Android (and its .NET dependencies) with the end goal of getting Powershell running on Android. Earlier, and with an older device, I'd managed to get pash partially running on Android; when I upgraded devices I was going to resume work on pash but then I heard about PowerShell being open-sourced. Hopefully the above provides a partial explanation of why I was building coreclr on Android in Termux; it would provide the cli for running Powershell and (up until I tried building paltests) it was faster/easier to be able to build and test in one place rather than building, testing, transferring to device, then testing again. Especially since that one place was a tablet that I could carry anywhere. Couple of additional points:
I'll update my list of reasons for effort suspension. |
Additionally, I'm researching mobile docker or VM access but if anyone knows or has/is used/using particular solutions, let me know. |
Are you looking for a version of Android which runs as a VM? If so, I'd recommend android-x86. It runs on Hyper-V, VirtualBox, VMWare,... |
No. I'm looking for a hosted solution that has mobile access… via app, web interface or ssh. If possible, I'd like to access and use the VM or contajner or whatever from my tablet so I can still have the ability to work on the build from anywhere. |
UPDATE: AWS has a free 12-month VM micro-instance hosting accessible via ssh. I've spun up an Ubuntu 14.04 VM, installed all of the coreclr prereqs, downloaded the ndk and generated a cross-build android toolchain. If I can build sources tomorrow, I'll open a new issue and reference this one; this issue is becoming unwieldy, length-wise. |
There are two two kinds of transition penalties: 1.Transition from 256-bit AVX code to 128-bit legacy SSE code. 2.Transition from 128-bit legacy SSE code to either 128 or 256-bit AVX code. This only happens if there was a preceding AVX256->legacy SSE transition penalty. The primary goal is to remove the #1 AVX to SSE transition penalty. Added two emitter flags: contains256bitAVXInstruction indicates that if the JIT method contains 256-bit AVX code, containsAVXInstruction indicates that if the method contains 128-bit or 256-bit AVX code. Issue VZEROUPPER in prolog if the method contains 128-bit or 256-bit AVX code, to avoid legacy SSE to AVX transition penalty, this could happen for reverse pinvoke situation. Issue VZEROUPPER in epilog if the method contains 256-bit AVX code, to avoid AVX to legacy SSE transition penalty. To limite code size increase impact, we only issue VZEROUPPER before PInvoke call on user defined function if the JIT method contains 256-bit AVX code, assuming user defined function contains legacy SSE code. No need to issue VZEROUPPER after PInvoke call because #2 SSE to AVX transition penalty won't happen since #1 AVX to SSE transition has been taken care of before the PInvoke call. We measured ~3% to 1% performance gain on TechEmPower plaintext and verified those VTune AVX/SSE events: OTHER_ASSISTS.AVX_TO_SSE and OTHER_ASSISTS.SSE_TO_AVE have been reduced to 0. Fix #7240 move setContainsAVX flags to lower, refactor to a smaller method refactor, fix typo in comments fix format error
1. Use the LIR node dumper to display nodes to be generated by codegen, since we're in LIR form at that point. Add a new "prefix message" argument to allow "Generating: " to prefix all such lines. 2. Fix off-by-one error in LIR dump due to `#ifdef` versus `#if`. 3. Remove extra trailing line for each LIR node. This interfered with #1. But I always thought it was unnecessarily verbose; I don't believe there is any ambiguity without that extra space. 4. Add dTreeLIR()/cTreeLIR() functions for use in the debugger.
@janvorli @jkotas: Moving the discussion of Android-specific build issues to this feature-android branch as per this message.
Currently re-building coreclr; will add links to debug logs when available
The text was updated successfully, but these errors were encountered: