Ensure fatalError message logged on Android #9869

johnno1962 · 2017-05-23T08:46:59Z

Change to make sure the message passed to fatalError() in Swift is logged to the console on Android. Requires linking with an additional library -llog so any help on exactly where in the CMake file this should be added would certainly be appreciated.

Resolves SR-2734.

CodaFi · 2017-05-30T18:45:38Z

stdlib/public/runtime/Errors.cpp

@@ -231,6 +235,9 @@ reportNow(uint32_t flags, const char *message)
 #ifdef __APPLE__
  asl_log(nullptr, nullptr, ASL_LEVEL_ERR, "%s", message);
 #endif
+#ifdef __ANDROID__
+  __android_log_print(ANDROID_LOG_ERROR, "Swift", "%s", message);


A few quick things:

This should be ANDROID_LOG_FATAL

I'm not sure how this "Swift" tag is reported, but it seems like it should include slightly more info here from the uses I've found elsewhere

Considering that __APPLE__ and __ANDROID__ should be mutually-exclusive, I think a better structure would be #if defined(...)/#elif defined(...)

I wondered about ERROR v. FATAL. I’ve updated it and changed to a #elif. For the “tag” it is displayed as prefix Swift/F before the message. Ideally It would be the process/image name but I couldn’t find a portable way to determine it in the NDK. I’ve changed it to “SwiftRuntime” for now.

CodaFi · 2017-05-30T20:00:35Z

Because this is a runtime change, let's hit it with the full test suite

@swift-ci please test

CodaFi · 2017-05-30T20:02:11Z

Does this not require CMake changes?

johnno1962 · 2017-05-30T20:08:37Z

Yes it will for -llog and there is a new requirement for linking against the atomic library which I’ve not looked at. The closest solution I’ve seen is here https://github.com/gonzalolarralde/swift/pull/1/files

CodaFi · 2017-05-30T20:10:43Z

In the interest of not breaking the android build, could you push those changes here too?

Requires CMake changes

johnno1962 · 2017-05-30T21:29:34Z

This last commit should be what is required for -latomic and -llog.

CodaFi · 2017-05-30T22:03:33Z

cmake/modules/AddSwift.cmake

        "${SWIFT_ANDROID_NDK_PATH}/sources/cxx-stl/llvm-libc++/libs/armeabi-v7a/libc++_shared.so")
    list(APPEND library_search_directories
+        "${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/arm-linux-androideabi/lib/armv7-a"


My CMake-fu is weak, but this seems unnecessarily specific.

Yeah, the outermost CMake List uses this instead

"${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-${SWIFT_ANDROID_NDK_GCC_VERSION}/prebuilt/${_swift_android_prebuilt_suffix}"

How about the latest version? I have to admit I’m coding blind at the moment as my build is broken for completely unrelated reasons but this should be the path.

Why not just use the one I quoted above?

The previous line sets SWIFT_ANDROID_PREBUILT_PATH doesn’t it?

set(SWIFT_ANDROID_PREBUILT_PATH "${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-${SWIFT_ANDROID_NDK_GCC_VERSION}/prebuilt/${_swift_android_prebuilt_suffix}")

Ah. Well, at least the magic version constant 4.9 can be ${SWIFT_ANDROID_NDK_GCC_VERSION}

How do you mean?

I mean the 4.9 in

"${SWIFT_ANDROID_NDK_PATH}/toolchains/arm-linux-androideabi-4.9"

right?

If it can't and I'm just blowing smoke here tell me and I'll get this CI'd and merged as-is. But as it stands I really don't have a way to properly test this patch and I'm trying not to take anything that looks too user-specific.

I think that was removed in the last commit.

CodaFi · 2017-05-31T19:00:37Z

@swift-ci please smoke test

johnno1962 · 2017-05-31T21:59:10Z

@CodaFi, don’t merge this for now. I’ve fixed my toolchain ($PATH problems) am testing and the CMake magic isn’t quite happening just yet.

CodaFi · 2017-05-31T22:00:22Z

Alright. Ping me here again when you get things working

johnno1962 · 2017-05-31T22:12:00Z

Thanks. It’s an odd one. The order the libraries are specified is turning out to be important 🙁

johnno1962 · 2017-06-01T13:42:20Z

@CodaFi this last commit with the current least bad CMake change cribbed from another developer checks out in testing. This can be merged now when you’re ready.

CodaFi · 2017-06-01T15:51:55Z

@swift-ci please clean smoke test

CodaFi · 2017-06-04T23:02:47Z

stdlib/public/runtime/ImageInspectionELF.cpp

@@ -68,8 +68,12 @@ static SectionInfo getSectionInfo(const char *imageName,
  SectionInfo sectionInfo = { 0, nullptr };
  void *handle = dlopen(imageName, RTLD_LAZY | RTLD_NOLOAD);
  if (!handle) {
+#ifdef __ANDROID__
+    return sectionInfo;
+#else


Why is this relevant to the fatal error patch? I would prefer this get reviewed separately if it could.

Sure, was just something else that needs a look at.

Error seems to be due to dladdr() not returning path to executable:

I/AEE/AED (12587): *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** I/AEE/AED (12587): Build fingerprint: 'lge/me1_global_com/me1:5.1.1/LMY47V/1626817040327:user/release-keys' I/AEE/AED (12587): Revision: '1.0' I/AEE/AED (12587): ABI: 'arm' I/AEE/AED (12587): pid: 12566, tid: 12566, name: hang.swifthello >>> net.zhuoweizhang.swifthello <<< I/AEE/AED (12587): signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr -------- I/AEE/AED (12587): Abort message: 'dlopen() failed on `net.zhuoweizhang.swifthello': dlopen failed: library "net.zhuoweizhang.swifthello" not found' I/AEE/AED (12587): r0 00000000 r1 00003116 r2 00000006 r3 00000000 I/AEE/AED (12587): r4 b6fc1e38 r5 00000006 r6 00000002 r7 0000010c I/AEE/AED (12587): r8 a16ff368 r9 befbc3a0 sl a1218a88 fp befbc2e8 I/AEE/AED (12587): ip 00003116 sp befbc290 lr b6e4bf01 pc b6e71fc4 cpsr 600f0010 I/AEE/AED (12587): I/AEE/AED (12587): backtrace: I/AEE/AED (12587): #00 pc 00039fc4 /system/lib/libc.so (tgkill+12) I/AEE/AED (12587): #01 pc 00013efd /system/lib/libc.so (pthread_kill+52) I/AEE/AED (12587): #02 pc 00014b07 /system/lib/libc.so (raise+10) I/AEE/AED (12587): #03 pc 000113e7 /system/lib/libc.so (__libc_android_abort+34) I/AEE/AED (12587): #04 pc 0000f7e8 /system/lib/libc.so (abort+4) I/AEE/AED (12587): #05 pc 0041e3a8 /data/app/net.zhuoweizhang.swifthello-2/lib/arm/libswiftCore.so (_ZN5swift10fatalErrorEjPKcz+56)

This reverts commit 8248322.

CodaFi · 2017-06-05T17:04:47Z

Alright, @johnno1962, can you confirm this works for the Android build?

johnno1962 · 2017-06-05T18:43:55Z

The build in the PR generates a Swift that runs on the device (but crashes later due to the ImageInspectionELF.cpp problem - not sure what Swift is up to there)

CodaFi · 2017-06-05T19:41:36Z

That's good enough for right now if you have a patch that fixes that issue to submit too.

CodaFi · 2017-06-05T19:42:08Z

@swift-ci please smoke test

CodaFi · 2017-06-07T16:15:01Z

⛵️

Thanks for being patient

johnno1962 · 2017-06-07T16:58:53Z

np, thanks @CodaFi. I imagine you’re having a busy week/month.

amraboelela · 2017-06-08T18:44:41Z

cmake/modules/AddSwift.cmake

@@ -359,7 +359,7 @@ function(_add_variant_link_flags)
    endif()
  elseif("${LFLAGS_SDK}" STREQUAL "ANDROID")
    list(APPEND result
-        "-ldl" "-llog" "-latomic"
+        "-ldl" "-llog" "-latomic" "-licudata" "-licui18n" "-licuuc"


Is this change really necessary?
Now when I run:
$ adb shell LD_LIBRARY_PATH=/data/local/tmp /data/local/tmp/hello

I get:
CANNOT LINK EXECUTABLE: could not load library "libswiftCore.so" needed by "/data/local/tmp/hello"; caused by could not load library "libicudata.so" needed by "libswiftCore.so"; caused by library "libicudata.so" not found

My experience was these libraries are essential for doing anything with Swift Strings. Have you tried copying them across? There is also a bit of a story behind these libraries before you can use them as they are newer versions than the android ones. In a final app it best to rename them and use the trick: "rpl -R -e libicu libscu lib*.so && rm -f Unittest &&” on an app’s shared libraries. I can revert this if you like if it’s causing you problems.

I'll try to copy those libraries to Android device, and let you know. But the Hello World used to work before without copying those libraries!

I tried it with this order: "-ldl" "-llog" "-latomic" "-licui18n" "-licudata" "-licuuc"
$ adb shell LD_LIBRARY_PATH=/data/local/tmp /data/local/tmp/hello
And I got: dlopen() failed on `/data/local/tmp/hello': dlopen failed: Aborted

I’m not sure linking with those libraries would be the problem. I’ve followed the instructions https://github.com/apple/swift/blob/master/docs/Android.md to run a command line program and though I had to add a “-Xlinker -pie” to the compile command for hello.swift I can get it to work (with a few warnings on my device):

$ adb shell LD_LIBRARY_PATH=/data/local/tmp /data/local/tmp/hello
WARNING: linker: /data/local/tmp/hello: unused DT entry: type 0x6ffffffe arg 0xcb8
WARNING: linker: /data/local/tmp/hello: unused DT entry: type 0x6fffffff arg 0x1
WARNING: linker: libswiftCore.so: unused DT entry: type 0x6ffffffe arg 0xa55c8
WARNING: linker: libswiftCore.so: unused DT entry: type 0x6fffffff arg 0x3
WARNING: linker: libswiftSwiftOnoneSupport.so: unused DT entry: type 0x6ffffffe arg 0x2074c
WARNING: linker: libswiftSwiftOnoneSupport.so: unused DT entry: type 0x6fffffff arg 0x1
WARNING: linker: libicui18n.so: unused DT entry: type 0x6ffffffe arg 0x85e84
WARNING: linker: libicui18n.so: unused DT entry: type 0x6fffffff arg 0x2
WARNING: linker: libicuuc.so: unused DT entry: type 0x6ffffffe arg 0x31c10
WARNING: linker: libicuuc.so: unused DT entry: type 0x6fffffff arg 0x3
WARNING: linker: libc++_shared.so: unused DT entry: type 0x6ffffffe arg 0x2adb0
WARNING: linker: libc++_shared.so: unused DT entry: type 0x6fffffff arg 0x3
Hello, Android

Normally I run an app using gradle, for example the project: https://github.com/SwiftJava/swift-android-samples and it’s associated plugin https://github.com/SwiftJava/swift-android-gradle and things seem to be working with the extra libraries.

Ensure fatalError message logged on Android

e45ddc8

CodaFi requested changes May 30, 2017

View reviewed changes

Use #elif

d600e75

CodaFi previously approved these changes May 30, 2017

View reviewed changes

Cmake changes for -llog and -latomic

c7ea92a

CodaFi reviewed May 30, 2017

View reviewed changes

Cmake changes for -llog and -latomic II

0ab8788

johnno1962 added 3 commits June 1, 2017 01:25

Cmake changes for -llog and -latomic III

32bd9f1

Cmake changes for -llog and -latomic IV

b5bd098

Cmake changes for -llog and -latomic V

304f556

Remove cause of crash on Android

8248322

CodaFi reviewed Jun 4, 2017

View reviewed changes

Revert "Remove cause of crash on Android"

ed38b57

This reverts commit 8248322.

CodaFi merged commit b9836e6 into swiftlang:master Jun 7, 2017

amraboelela reviewed Jun 8, 2017

View reviewed changes

johnno1962 mentioned this pull request Jul 10, 2017

[stdlib] Workaround to fix getSectionInfo() for Android #10836

Closed

finagolfin mentioned this pull request Mar 5, 2020

[build] rewrite #21707 so that it doesn't break cross-compilation #30170

Open

johnno1962 mentioned this pull request Jun 7, 2017

[SR-2734] fatalError does not log on Android #45338

Closed

swift-ci mentioned this pull request Jun 8, 2017

[SR-4443] Android: Cannot run swift executable on Android 7.0 emulator (armv7) as it is not a PIE #47020

Closed

Ensure fatalError message logged on Android #9869

Ensure fatalError message logged on Android #9869

Uh oh!

Conversation

johnno1962 commented May 23, 2017

Uh oh!

CodaFi May 30, 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!

johnno1962 May 30, 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!

CodaFi commented May 30, 2017

Uh oh!

CodaFi commented May 30, 2017

Uh oh!

johnno1962 commented May 30, 2017

Uh oh!

CodaFi commented May 30, 2017

Uh oh!

johnno1962 commented May 30, 2017

Uh oh!

CodaFi May 30, 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!

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!

johnno1962 May 31, 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!

CodaFi commented May 31, 2017

Uh oh!

johnno1962 commented May 31, 2017

Uh oh!

CodaFi commented May 31, 2017

Uh oh!

johnno1962 commented May 31, 2017

Uh oh!

johnno1962 commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Jun 1, 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!

CodaFi commented Jun 5, 2017

Uh oh!

johnno1962 commented Jun 5, 2017

Uh oh!

CodaFi commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Jun 5, 2017

Uh oh!

CodaFi commented Jun 7, 2017

Uh oh!

johnno1962 commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi May 30, 2017 •

edited

Loading

johnno1962 May 30, 2017 •

edited

Loading

CodaFi May 30, 2017 •

edited

Loading

johnno1962 May 31, 2017 •

edited

Loading

johnno1962 commented Jun 1, 2017 •

edited

Loading

CodaFi commented Jun 5, 2017 •

edited

Loading

johnno1962 Jun 8, 2017 •

edited

Loading

johnno1962 Jun 13, 2017 •

edited

Loading