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

johnno1962 · 2017-07-10T00:08:31Z

As discussed in #9869, this is a temporary patch to allow stdlib to run on Android. The function getSectionInfo() in stdlib/public/runtime/ImageInspectionELF.cpp tries to dlopen() an image for which the full path is not available on Android causing a fatalError(). This PR patches this out for now, leaving a warning during compilation to indicating it should be revisted at a later date. No Swift program can run on Android without this patch.

Workaround for SR-5414.

gparker42 · 2017-07-10T22:00:51Z

I don't see any compile-time warning, but that's okay because we generally don't use warnings to track todo items.

There should be an open bug tracking the future fix, and the comment should refer to that bug. John, do you want to file a new bug for the future fix, or do you want to keep SR-5414 open and repurpose it?

johnno1962 · 2017-07-10T22:17:01Z

It will give a warning “code never executed” for the fatalError() on Android. How should I repurpose SR-5414 - did I set it up wrong? I dont know of a fix for this at present as android goes out of it’s way not to reveal the path to executables so the dlopen() (to inspect the image for protocol conformance?? etc.) will never work. I don’t know what the ramifications of this are myself. Things seem to be working!

johnno1962 · 2017-07-10T22:23:12Z

Perhaps my use of the words “Temporary” and “resolves” was in error. More like “likely work around"

gparker42 · 2017-07-10T22:37:35Z

Describing this change as a workaround for SR-5414 would be fine.

I suspect that simply giving up will break some protocol conformance checks and reflection. Are you able to run Swift's tests on Android? I hope that some of our tests are able to catch any missing registration here.

johnno1962 · 2017-07-10T22:49:33Z

I’ve never run the full test suite on Android myself though I believe it is possible. https://github.com/apple/swift/blob/master/docs/Android.md. The only known problem with Swift itself on Android I’ve encountered is that try/catch SEGFAULT’s out if you try to catch the error in a var.

johnno1962 · 2017-07-11T20:29:16Z

I ran the tests but it’s not performing them on the device so there isn’t much to learn there.

johnno1962 · 2017-07-11T21:26:12Z

I’ve created a new JIRA for the do/try/catch problems https://bugs.swift.org/browse/SR-5438. I’m running a Swift from master from about a month ago but I don’t think this is the problem. I’ll update the Jira if this goes away when I sync to master as swift4 comes close to release.

johnno1962 · 2017-07-12T14:19:06Z

Hi @gparker42, I’ve researched this problem a little more and updated the underlying Jira’s if you want more detail. I had thought the two Jiras discussed could be related somehow. After adding some logging seems that the dlopen() is successful even though the full path isn't supplied with the exception of one outlier which seems to be specific to Android. This means this PR is valid as is, simply making a failure to dlopen non-fatal for Android.

johnno1962 · 2017-07-12T23:04:39Z

I’ve done some more logging and I’m satisfied that protocol conformances are being imported correctly on Android so I don’t have an explanation why do/try/catch isn’t working at all.

johnno1962 · 2017-07-17T14:48:14Z

I’ve removed the compilation warning as I’m pretty sure this has been investigated on Android now. I’ve also synced my swift sources to master and everything apart from do/try/catch is working which I’m pretty sure is a separate issue.

jckarter · 2017-07-17T16:05:18Z

stdlib/public/runtime/ProtocolConformance.cpp

+#else
+static const char *class_getName(const ClassMetadata* type) {
+  return type->getNominalTypeDescriptor()->Name.get();
+}


Why is this necessary? We shouldn't be using ObjC functions if ObjC interop isn't enabled.

Seemed like the least intrusive change to get this code working on non-ObjC interop platforms so I could verify the protocol conformance import from ELF. It’s unfortunate it uses a function name from the runtime I suppose. I can change it to a second #if in the switch if you like or revert as it’s not directly related to this PR. Or, are you saying getNominalTypeDescriptor is an ObjC interop function and I should revert anyway?

Reverted. It’s not important to this PR.

No, using getNominalTypeDescriptor() is fine, but there's no reason to wrap it in something that looks like and could be confused for the ObjC runtime function name.

Fair enough. I’ve reverted it all anyway. Let me know if you’d like a version that doesn’t use the name class_getName back. It was handy on Linux. metadata_getClassName()?

swift_getTypeName should do what you want already more generally.

johnno1962 · 2017-07-19T17:18:00Z

Chances of a squash/merge? This is such a small change!

gonzalolarralde · 2017-07-21T14:23:41Z

The reason why this is required for bionic is because dl_iterate_phdr is not only returning the currently opened libraries, but also the executable file. When the iterator reaches the executable file path, RTLD_NOLOAD avoid loading it at all, so the return value for that call is null.

This is what glibc's dl_iterate_phdr would return for exec_test:

1: null < will return a reference to the originating executable who started the process when dlopen is called
2: loadedlib1.so
3: loadedlib2.so

Vs bionic's dl_iterate_phdr:

1: null < same as glibc, dlopen will get a referece to itself
2: exec_test < this won't count as an opened lib so RTLD_NOLOAD will make dlopen to ignore it
3: loadedlib1.so
4: loadedlib2.so

Potential solutions to this problem

Get the executable name and ignore that entry specifically.

I couldn't find a reliable way to get this. There are some SO responses suggesting calling dl_iterate_phdr and taking the first result, but I couldn't find documentation sustaining this. There may be even problems when this is running in a java environment.

Have a static variable that keeps the iteration repetition value.

But if this is called more than once then it fails. I don't think it is tho.

Ignoring any non-loaded dlopen.

The assumption here is that if something is returned by dl_iterate_phdr then it should be loaded, on bionic the executable is clearly the exception. It is loaded, you can get the reference by calling dlopen(null…, but the actual path that it is passing is not in the table of loaded libraries, so the call returns empty.

If the library is loaded, the conformances processed and then unloaded again, when the conformances are checked the memory will be deallocated and it's going to crash.

So...

Ignoring non loaded libraries sound like the safe path to me, because even if there's a conformance to be declared it will fail as soon as you want to access it. But we can totally be missing something.

@gparker42 @jckarter please let us know if any of this solutions make sense or if there's any other better alternative that we should consider.

Thanks!

jckarter · 2017-07-21T14:36:26Z

Ignoring libraries that don’t get loaded seems fine.

[stdlib] Temporary fix to getSectionInfo() for Android Update comment Conformance logging on Linux Remove compile warning Revert "Conformance logging on Linux" This reverts commit 3db6c01a017512484a4db3eeefc6afafaec4fad3.

johnno1962 · 2017-07-21T16:55:16Z

OK, I’ve succeeded in squashing the commits.

johnno1962 · 2017-07-23T17:17:17Z

Opened a new PR on swift-llvm apple/swift-llvm#55 to fix the do/try/catch problems I mentioned before on non-darwin ARM. Details are in the updated Jira: https://bugs.swift.org/browse/SR-5438

johnno1962 · 2017-07-27T22:13:57Z

HI @gparker42, Any change of getting this very minor change merged?

johnno1962 · 2017-08-30T19:09:51Z

Closed in favour of #11615. Hallelujah!

johnno1962 closed this Jul 10, 2017

johnno1962 reopened this Jul 10, 2017

johnno1962 changed the title ~~[stdlib] Temporary fix to getSectionInfo() for Android~~ [stdlib] Workaround to getSectionInfo() for Android Jul 11, 2017

johnno1962 changed the title ~~[stdlib] Workaround to getSectionInfo() for Android~~ [stdlib] Workaround to fix getSectionInfo() for Android Jul 11, 2017

jckarter reviewed Jul 17, 2017

View reviewed changes

[stdlib] Temporary fix to getSectionInfo() for Android

fbd48b8

[stdlib] Temporary fix to getSectionInfo() for Android Update comment Conformance logging on Linux Remove compile warning Revert "Conformance logging on Linux" This reverts commit 3db6c01a017512484a4db3eeefc6afafaec4fad3.

johnno1962 referenced this pull request Aug 18, 2017

Add -pie flag to clang++ linker invocation.

78938d8

gonzalolarralde mentioned this pull request Aug 25, 2017

In case of Android, give warning only and return the empty sectionInfo. #11615

Merged

johnno1962 closed this Aug 30, 2017

This was referenced Oct 19, 2017

[FreeBSD][SR-3635] Fix __gnustep_objcxx_personality_v0 symbol emitted #12510

Closed

[FreeBSD] Fix a few issues #12528

Merged

johnno1962 mentioned this pull request Jul 10, 2017

[SR-5414] ImageInspectionELF.cpp:getSectionInfo crashes on Android #47988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

johnno1962 commented Jul 10, 2017 •

edited

gparker42 commented Jul 10, 2017

johnno1962 commented Jul 10, 2017 •

edited

johnno1962 commented Jul 10, 2017 •

edited

gparker42 commented Jul 10, 2017

johnno1962 commented Jul 10, 2017

johnno1962 commented Jul 11, 2017

johnno1962 commented Jul 11, 2017 •

edited

johnno1962 commented Jul 12, 2017 •

edited

johnno1962 commented Jul 12, 2017

johnno1962 commented Jul 17, 2017

jckarter Jul 17, 2017

johnno1962 Jul 17, 2017 •

edited

johnno1962 Jul 17, 2017

jckarter Jul 18, 2017

johnno1962 Jul 18, 2017 •

edited

jckarter Jul 18, 2017

johnno1962 commented Jul 19, 2017 •

edited

gonzalolarralde commented Jul 21, 2017

jckarter commented Jul 21, 2017

johnno1962 commented Jul 21, 2017

johnno1962 commented Jul 23, 2017 •

edited

johnno1962 commented Jul 27, 2017

johnno1962 commented Aug 30, 2017

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

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

Conversation

johnno1962 commented Jul 10, 2017 • edited

gparker42 commented Jul 10, 2017

johnno1962 commented Jul 10, 2017 • edited

johnno1962 commented Jul 10, 2017 • edited

gparker42 commented Jul 10, 2017

johnno1962 commented Jul 10, 2017

johnno1962 commented Jul 11, 2017

johnno1962 commented Jul 11, 2017 • edited

johnno1962 commented Jul 12, 2017 • edited

johnno1962 commented Jul 12, 2017

johnno1962 commented Jul 17, 2017

jckarter Jul 17, 2017

Choose a reason for hiding this comment

johnno1962 Jul 17, 2017 • edited

Choose a reason for hiding this comment

johnno1962 Jul 17, 2017

Choose a reason for hiding this comment

jckarter Jul 18, 2017

Choose a reason for hiding this comment

johnno1962 Jul 18, 2017 • edited

Choose a reason for hiding this comment

jckarter Jul 18, 2017

Choose a reason for hiding this comment

johnno1962 commented Jul 19, 2017 • edited

gonzalolarralde commented Jul 21, 2017

Potential solutions to this problem

Get the executable name and ignore that entry specifically.

Have a static variable that keeps the iteration repetition value.

Ignoring any non-loaded dlopen.

So...

jckarter commented Jul 21, 2017

johnno1962 commented Jul 21, 2017

johnno1962 commented Jul 23, 2017 • edited

johnno1962 commented Jul 27, 2017

johnno1962 commented Aug 30, 2017

johnno1962 commented Jul 10, 2017 •

edited

johnno1962 commented Jul 10, 2017 •

edited

johnno1962 commented Jul 10, 2017 •

edited

johnno1962 commented Jul 11, 2017 •

edited

johnno1962 commented Jul 12, 2017 •

edited

johnno1962 Jul 17, 2017 •

edited

johnno1962 Jul 18, 2017 •

edited

johnno1962 commented Jul 19, 2017 •

edited

johnno1962 commented Jul 23, 2017 •

edited