In case of Android, give warning only and return the empty sectionInfo. #11615
Conversation
|
@jckarter can you review this? I'm not familiar with this part of the runtime. |
|
@swift-ci please smoke test |
|
Why is the runtime attempting dlopens that fail in the first place? Can we avoid them to begin with? |
|
@jckarter I found in https://developer.android.com/about/versions/nougat/android-7.0-changes.html
|
|
Thanks @amraboelela. The Swift runtime should only be using dlopen here to get a handle to libraries that have already been loaded into the process, either because the executable is linked against them, or because of a previous successful dlopen. It sounds like the libraries affected by this compatibility hack are things that should not be linked against to begin with. Can we find out why Swift binaries are getting linked against these libraries, and fix the compiler and/or implementation so that it doesn't have to? |
|
@jckarter yes it actually tries to open tons of libraries. Here are the warnings I get after using my patch: 08-24 14:45:14.293 7242-7242/com.example.addswift A/SwiftRuntime: dlopen() failed on |
|
If you |
|
Like @gparker42 suggests in https://twitter.com/gparker/status/901168354001436672, it might be better to have the runtime just skip trying to load any libraries under |
|
I tried in adb shell: generic:/data/local/tmp $ ldd libswiftCore.so |
|
You may need to do it on your host device instead of within Android itself. |
|
I am running it using Mac OS, but those libs are in arm v7 format. And there is no executable as it is running though a java activity. |
|
@jckarter @amraboelela it is my understanding that you're not supposed to link them, but it doesn't mean that the libraries that you can link against, can't link against any of the "prohibited" dependencies, so yeah, there's some weird logic there where you end up with a lib in memory that you're not supposed to specifically reference. In #10836 we've discussed this problem, but coming from a different issue: one of the libraries that is passed in that list as per bionic's There's a list called Sorry for the noise if any, hope this is useful. |
|
If there were an alternate way to get a |
|
I used ndk-depends which is the replacement of ldd in android idk, and I got: $ ndk-depends libswiftCore.so And I tried ndk-depends on all these libs and none of them link with libcutils.so or any of the libs mentioned in the warnings. |
|
OK, as long as we aren't inappropriately linking against anything we aren't supposed to, then it should be OK to skip over trying to open libraries in |
|
ok but some of these libs comes from system, like liblog.so, libz.so,libm.so,libdl.so,libc.so |
|
Sure, but none of those is going to have any Swift code in them. |
|
ok, good, i was also thinking about looking into the lib name path and check if it include /system/lib to ignore it. But the question is, why do we link to those libs in the first place? |
|
Those are (AFAICT) still public libraries. The Swift runtime relies on the standard C/C++ libraries as well as ICU for various things, so it's expected that it (and most executables) would link against them. |
|
Hi, Sorry to butt in, but I’m not sure there is a way to avoid just skipping shared libs that don’t load. On my device (LG Lollipop) the imageName argument to getSectionInfo does not include the full path and there is an imageName passed in that doesn't correspond to any shared library: e.g. ‘net.zhuoweizhang.swifthello' is identifier of the Java side of the app. I suppose where there is a path you could not even try to load anything that starts with /system or libs that don’t end in '.so’. How about: #ifdef __ANDROID__
llvm::StringRef imagePath = imageName;
if (imagePath.startswith("/system/lib") || (imageName && !imagePath.endswith(".so")))
return sectionInfo;
#endif
void *handle = dlopen(imageName, RTLD_LAZY | RTLD_NOLOAD);
if (!handle) {
fatalError(/* flags = */ 0, "dlopen() failed on `%s': %s", imageName, dlerror());
}This works on my LG. @amraboelela does this work on your V7 device? |
|
Hi @johnn1962 |
|
ok I changed the code to @ johnn1962 suggestion. I am currently rebuilding swift with the new code then I'll test it after. |
|
I tested it with the new change and it is working fine. |
amraboelela
force-pushed
the
android-getSectionInfo
branch
from
c260756 to
3ce27b0
Compare
|
Excellent, I’ll be happy to see this merged! Do you think the warning message is needed in the final version? |
|
@swift-ci Please smoke test |
| #ifdef __ANDROID__ | ||
| llvm::StringRef imagePath = llvm::StringRef(imageName); | ||
| if (imagePath.startswith("/system/lib") || (imageName && !imagePath.endswith(".so"))) { | ||
| warning(/* flags = */ 0, "dlopen() failed on `%s': %s", imageName, dlerror()); |
There was a problem hiding this comment.
I don't think this warning is necessary, since failure in this case may be expected. It'll just noise up logs.
There was a problem hiding this comment.
I prefer to keep the warning so hopefully somebody later would figure out the reason we are trying to link to all these libraries and try to avoid it. But I can remove it, no problem.
There was a problem hiding this comment.
I removed the warning in the final commit
There was a problem hiding this comment.
Thanks. It sounds like we're trying to load these private libraries because they're transitive dependencies of public libraries, but Android's dlopen is special-cased to reject attempts to open them explicitly, which is gross but a situation we can't do much about.
|
I fixed the indentation, even though I don't understand why don't we keep the standard indentation of the Xcode? |
amraboelela
force-pushed
the
android-getSectionInfo
branch
from
83208af to
8ca8d91
Compare
|
@amraboelela @johnno1962 question: how is this dealing with ignoring the path to self? it's my understanding that this won't work when embedding the library in an APK, as we expect it to end with |
|
Not with you. Code will still try to open it if you put a loaded library in the .apk without .so at the end. |
| #ifdef __ANDROID__ | ||
| llvm::StringRef imagePath = llvm::StringRef(imageName); | ||
| if (imagePath.startswith("/system/lib") || | ||
| (imageName && !imagePath.endswith(".so"))) { |
There was a problem hiding this comment.
One more space is required for this line.
| return sectionInfo; | ||
| } | ||
| #endif | ||
| fatalError(/* flags = */ 0, "dlopen() failed on `%s': %s", imageName, dlerror()); |
There was a problem hiding this comment.
Please restore the original line break.
|
@swift-ci Please smoke test |
…ry doesn't end with .so, we will give warning only and return empty sectionInfo.
amraboelela
force-pushed
the
android-getSectionInfo
branch
from
9f8a7c7 to
2447658
Compare
|
Is it ready to merge now? |
|
@swift-ci Please smoke test |
|
Thanks @amraboelela ! |
|
Thanks @jckarter |
This will avoid unnecessary crashes in Android
Fix for https://bugs.swift.org/browse/SR-5757
Resolves SR-5757.