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

antonmes · 2017-10-19T15:50:58Z

This PR fixes __gnustep_objcxx_personality_v0 symbol being emitted:
libswiftCore.so: undefined reference to __gnustep_objcxx_personality_v0

Related issues: #4804 (comment)

Resolves SR-3635.

dcci · 2017-10-19T16:03:29Z

cmake/modules/AddSwift.cmake

@@ -348,7 +348,7 @@ function(_add_variant_link_flags)
  if("${LFLAGS_SDK}" STREQUAL "LINUX")
    list(APPEND result "-lpthread" "-latomic" "-ldl")
  elseif("${LFLAGS_SDK}" STREQUAL "FREEBSD")
-    list(APPEND result "-lpthread")
+    list(APPEND result "-lpthread" "-L/usr/local/lib" "-lobjc")


I don't think this is quite right. We shouldn't really link objC on platforms where interoperability is disabled.

Yeah, i'm trying to remove this dependency, couldn't solve this with any know flags/options, the only working solution i found is to completely avoid .mm. Couldn't we use -lobjc as a temporary fix? Isn't it required on any freebsd platform using the current build environment? Sorry, i'm not familiar with FreeBSD, just having fun 😊

I think the correct solution is that of not emitting __gnustep_objcxx_personality_v0 altogether. FWIW, I think clang is the right place to look at (but @jrose-apple / @compnerd probably know better). FWIW, I and @compnerd discussed about enabling obj-C interop on other platforms yesterday. Maybe we can consider taking a look at that on FreeBSD.

We should get a working non-ObjC build working on FreeBSD first. Getting ObjC interop on a non-Apple platform will be complicated.

dcci · 2017-10-19T17:06:30Z

stdlib/public/runtime/ImageInspectionELF.cpp

@@ -72,6 +72,13 @@ static SectionInfo getSectionInfo(const char *imageName,
  SectionInfo sectionInfo = { 0, nullptr };
  void *handle = dlopen(imageName, RTLD_LAZY | RTLD_NOLOAD);
  if (!handle) {
+#ifdef __FreeBSD__


I think this bit is fine although it duplicates the logic. Can you try to share the code between FreeBSD and android?

antonmes · 2017-10-19T18:37:29Z

I found that in swift 3.0 there were *Native.cpp wrappers for Reflection.mm and SwiftValue.mm https://github.com/apple/swift/tree/swift-3.0-branch/stdlib/public/runtime
So bringing them back should solve the issue. Shall I restore them?

Update: found where they were removed #5942

gparker42 · 2017-10-19T21:23:02Z

How is the .mm problem solved on Linux? Is there something in the build system that knows to compile .mm files as C++ on that platform?

dcci · 2017-10-19T21:23:36Z

I think you may want instead check what emits that symbol in clang/swift (and why that symbol is emitted). You can probably start running a object file dumper (objdump or readelf) on the file to find out which relocatable ELF objects contain the symbol. Then you can just grab the invocation that produces that object and stare at the intermediate (or just run the compiler under a debugger). It may be tedious but it shouldn't be impossible to track down, IMHO. If you get stuck, let me know.

dcci · 2017-10-20T03:12:25Z

stdlib/public/runtime/Reflection.mm

@@ -1100,6 +1100,7 @@ static Mirror ObjC_getMirrorForSuperclass(Class sup,
 //// define what these will be.
 /// \returns the demangled name. Returns nullptr if the input String is not a
 /// Swift mangled name.
+#ifndef __FreeBSD__


Can you explain the rationale behind this change? As far as I can tell you only commented out the demangler API. Thanks for your hard work, btw!

Thanks! It took enormous amount of time to locate this piece of code. I found that both strdup and _swift_strlcpy in this function results to objc being linked. As far as I understood from the comment it's not used anywhere directly. I will file a bug on jira.

I found that it's not the functions themselves but the combination with std:string, see my comment in the PR thread.

compnerd · 2017-10-20T04:18:36Z

Undefined references to __gnustep_personality_v0 is slightly worrisome. That implies that you are using the GNUStep runtime for the ObjC interop. Is the object layout compatible? Right now the swift ObjC interop assumes the resilient NeXTstep layout.

antonmes · 2017-10-20T16:15:05Z

That's weird, __gnustep_objcxx_personality_v0 emitted if both std::string and strdup/_swift_strlcpy are used in the swift_demangle function even though they are not connected. I don't know what causes this behavior.

SWIFT_RUNTIME_EXPORT
char *swift_demangle(const char *mangledName,
                     size_t mangledNameLength,
                     char *outputBuffer,
                     size_t *outputBufferSize,
                     uint32_t flags) {
  // __gnustep_objcxx_personality_v0
  auto result = std::string();
  return strdup("");

  // ok
  auto result = std::string();
  return ""

  // ok
  return strdup("");
}

Since it's not a part of reflection i suggest we move it to Demangle.cpp, this should solve the issue.

jrose-apple · 2017-10-20T16:47:23Z

utils/build-script-impl

@@ -3448,7 +3448,7 @@ function build_and_test_installable_package() {
              call tar -c -z -f "${package_for_host}" "${TOOLCHAIN_PREFIX/#\/}"
        else
            # tar on OS X doesn't support --owner/--group.


Nitpick: please change this comment too! It sounds like the real condition here is "BSD tar".

dcci · 2017-10-20T17:27:45Z

FWIW, I think you can probably split this in multiple pull requests and rebase.
I think your fixes to 1, 3 and 4 make sense [and I reviewed/tested them], but I still have hard time convincing myself that the undefined Obj-C symbol issue is solved properly.
As I don't want this to be blocked forever, you can probably move the other fixes and we can keep discussing here.

antonmes · 2017-10-20T17:59:34Z

@dcci yeah, there definitely is a bug somewhere in the compiler, and by moving the code we're only avoiding it, but on the other side isn't swift_demangle really belongs to Demangle.cpp?

you mean move fixes 1,2,4 to another PR? _atomics in a number 2.

dcci · 2017-10-20T18:09:17Z

I mean everything but the Obj-C issue.

Resolves https://bugs.swift.org/browse/SR-3635 std::string + strdup being used in the function causes some weird behavior on FreeBSD where __gnustep_objcxx_personality_v0 is emmitted. It's not happening if the code is in a .cpp file.

antonmes · 2017-10-20T18:42:28Z

moved to #12528

jrose-apple · 2017-10-20T23:29:58Z

If we really want to future-proof this…can we just make a policy that says "no .m/.mm files on non-Apple platforms"? And then split these files up appropriately?

(And we could take Anton's patch in the mean time, and clean it up later.)

compnerd · 2017-10-23T00:22:54Z

@jrose-apple Im not sure that restriction is really needed.

The better way to handle this is to just actually call out the fact that you need a specific ObjC ABI. We can just add -fobjc-runtime=ios to the flag and get the right behavior. @antonmes is getting a reference to __gnustep_objcxx_personality_v0 because on FreeBSD the default runtime is GNUStep, which cannot work. Switching the runtime to a NeXT family would give us __objc_personality_v0.

However, more worrisome is that somehow -fno-exceptions is being dropped. The std::string constructor is getting marked as except, which is causing the compiler to emit a catch clause which requires a personality. Because the file type is ObjC++, it is using the GNU ObjC++ exception personality __gnustep_objcxx_personality_v0 rather than the __objc_personality_v0 which is used for the NeXT family of ObjC runtimes.

jrose-apple · 2017-10-23T16:32:26Z

No, no. There should be no ObjC dependencies in the non-Darwin runtimes. We need to link to neither __gnustep_objcxx_personality_v0 nor __objc_personality_v0.

The -fexceptions thing may be deliberate, in order to make the Swift runtime slightly more capable in the face of exceptions. But that does seem unnecessary, given that you already can't throw C++ exceptions through Swift stack frames. @gparker42 (or @rjmccall) would have to say for sure.

compnerd · 2017-10-23T19:16:34Z

@jrose-apple yes, I agree that there shouldn't be a mandatory objc dependency. I was suggesting that the issue was exceptions rather than the fact that we have .m or .mm sources being used here. We should be able to keep a single path for all the targets, just need to ensure that we don't end up with ObjC runtime dependencies.

gparker42 · 2017-10-23T22:25:35Z

Can we fix this with an appropriately-placed -x c++ on non-ObjC platforms? Or conversely name the files as .cpp and add -x objective-c++ on ObjC platforms?

compnerd · 2017-10-24T00:42:51Z

@gparker42, yes, that would work. The question in my mind is why is this path trying to raise an exception? Having exceptions pop up unexpectedly would be bad. Plus, there are code-size considerations.

gparker42 · 2017-10-24T01:28:24Z

Is it trying to raise an exception? The exception personality functions are used by functions that need to catch or unwind exceptions (such as running local destructors). I don't think they are used by the throw points.

compnerd · 2017-10-24T01:36:33Z

@gparker42 right. The std::string constructor may raise an exception if built with exceptions. I suspect that the strdup is resolving to std::strdup which if not marked as noexcept would cause an implicit catch for the local destructors. If we are expecting to handle the exceptions in this case, we shouldn't let the exception propagate past this point (in which case, I agree, the -x approach is the best way to handle this).

dcci · 2017-10-27T17:12:49Z

@anton with your fixes in (including this one pending), what's the number of failures on FreeBSD x86-64 for swift?

jckarter · 2017-10-27T18:29:38Z

I don't think there's anything in the runtime that should require building with -fexceptions.

antonmes · 2017-10-27T19:52:25Z

@dcci you mean test failures? I didn't fix/run any tests and we have the same issue with __atomics* there. After I fixed them by linking libclang_rt.builtins here's what I got: Failing Tests (190)

antonmes · 2017-11-10T19:28:51Z

@compnerd is #12702 related to the issue?

Random question: what is the reason of swift_demangle being in Reflection.mm and not in Demangle.cpp?

jckarter · 2017-11-10T19:51:49Z

I think it'd be great if we can move code that isn't ObjC specific into plain .cpp files. Ideally we wouldn't have any .mm files in the no-ObjC-interop build. Can the ObjC-specific parts of Reflection.mm be broken out?

dcci · 2017-11-16T18:52:33Z

@antonmes are you still planning to make these changes? Thanks!

compnerd · 2017-11-16T21:46:35Z

So, just to ensure that this is recorded somewhere ...

Currently, there is only one viable ObjC runtime which can interop with swift: the iOS runtime (non-fragile NeXT). If/when the GNUStep becomes a viable runtime, we could expose the runtime via a switch (à la -fobjc-runtime=).

The next thing to realize is that we do not use ObjC exceptions in the runtime, but we may need the personality to provide the cleanup landing pads (anyone up for setting up MS-style exception funclets for EH? 😄) . However, the frontend will simplify the personality to prefer to use the C++ personality if possible (i.e. there are no ObjC exceptions crossing the boundary, which is the restriction that we have). So, at the end of the day, we should only really be seeing references to __gxx_personality_v0 atm even though we are using .mm files.

I think that overall, sharing the code base this way is better since you have a single implementation. The use of the .mm files effectively is meaningless as we do not use the ObjC bits unless we are enabling the ObjC interop, but the rest of the code can be structured appropriately.

antonmes · 2017-11-17T06:09:56Z

@dcci I'd love to, but I'm stuck. The current PR solves the issue but doesn't prevent it in the future.

Ideally we wouldn't have any .mm files in the no-ObjC-interop build.

@jckarter I don't see a way to split them not involving a lot of code duplication, but the previous solution should met this requirement #12510 (comment) even though .mm is still involved in #include <Reflection.mm> in the no-ObjC-interop build.

I could investigate it further (clang / freebsd sources) but I agree that we shouldn't use .mm files in the no-ObjC-interop build in the first place, so I don't think it's worth it. So the proper solutions would be:

split the code into Reflection.cpp and Reflection.mm (beyond my understanding if possible)
bring the ReflectionNative.cpp back and do #include <Reflection.mm> for the no-ObjC build.

Let's bring the *Native wrappers back?

jckarter · 2017-11-17T17:32:32Z

OK, I looked at the code and largely agree. Reflection.mm is largely legacy we want to get rid of anyway. Explicitly building it as plain C++ should be fine.

CodaFi · 2019-11-11T20:50:00Z

The core of this patch has been addressed by 44d9b2f. I'm going to close this. Please let me know if this was in error.

See swiftlang#12510 and SR-3635 for some context; OpenBSD exhibits the same problem listed here. Without this, the build fails with an undefined symbol error for __gnustep_objcxx_personality_v0. Per commentary on the aforementioned PR, forcing the language to be interpreted as C++ solves the problem. There might be something different we can do here to diagnose and fix this, but this is the simplest solution to get the build working and enabling this for all C++ files should be tautologous and therefore harmless.

dcci requested changes Oct 19, 2017

View reviewed changes

jrose-apple requested review from slavapestov and gparker42 October 19, 2017 21:01

dcci reviewed Oct 20, 2017

View reviewed changes

jrose-apple reviewed Oct 20, 2017

View reviewed changes

antonmes changed the title ~~Fix FreeBSD build~~ [FreeBSD][SR-3635] Fix __gnustep_objcxx_personality_v0 symbol emitted Oct 20, 2017

CodaFi closed this Nov 11, 2019

3405691582 mentioned this pull request Apr 16, 2020

[build] Set language flag for C++ on OpenBSD. #31057

Merged

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

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

Uh oh!

Conversation

antonmes commented Oct 19, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonmes commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gparker42 commented Oct 19, 2017

Uh oh!

dcci commented Oct 19, 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!

compnerd commented Oct 20, 2017

Uh oh!

antonmes commented Oct 20, 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!

dcci commented Oct 20, 2017

Uh oh!

antonmes commented Oct 20, 2017

Uh oh!

dcci commented Oct 20, 2017

Uh oh!

antonmes commented Oct 20, 2017

Uh oh!

jrose-apple commented Oct 20, 2017

Uh oh!

compnerd commented Oct 23, 2017

Uh oh!

jrose-apple commented Oct 23, 2017

Uh oh!

compnerd commented Oct 23, 2017

Uh oh!

gparker42 commented Oct 23, 2017

Uh oh!

compnerd commented Oct 24, 2017

Uh oh!

gparker42 commented Oct 24, 2017

Uh oh!

compnerd commented Oct 24, 2017

Uh oh!

dcci commented Oct 27, 2017

Uh oh!

jckarter commented Oct 27, 2017

Uh oh!

antonmes commented Oct 27, 2017

Uh oh!

antonmes commented Nov 10, 2017

Uh oh!

jckarter commented Nov 10, 2017

Uh oh!

dcci commented Nov 16, 2017

Uh oh!

compnerd commented Nov 16, 2017

Uh oh!

antonmes commented Nov 17, 2017

Uh oh!

jckarter commented Nov 17, 2017

Uh oh!

CodaFi commented Nov 11, 2019

antonmes commented Oct 19, 2017 •

edited

Loading

antonmes commented Oct 19, 2017 •

edited

Loading

antonmes commented Oct 20, 2017 •

edited

Loading