Fluid: Use hidden symbol visibility
ClosedPublic
Actions

Authored by Sebastián Barschkis (sebbas) on Sep 24 2020, 2:53 PM.

Details

Reviewers

Sergey Sharybin (sergey)
Ray Molenkamp (LazyDodo)

Commits

rBab65fe5a2dcc: Fluid: Use hidden symbol visibility

Summary

This resolves a long list of linker warnings that is currently only showing up on macOS arm builds.
The warnings themselves are of this shape (one example):

ld: warning: direct access in function 'Manta::MeshDataImpl<Manta::Vector3D<float> >::_W_39(_object, object, object*)' from file '../../lib/libextern_mantaflow.a(mesh.h.reg.cpp.o)' to global weak symbol 'typeinfo for Manta::MeshDataImpl<Manta::Vector3D<float> >' from file '../../lib/libextern_mantaflow.a(mesh.cpp.o)' means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.

Just would like to get your opinion to make sure this is an acceptable way to handle this on all platforms.

Diff Detail

Repository

rB Blender

Branch

manta_symbol_visibility (branched from master)

Build Status

Buildable 10386
Build 10386: arc lint + arc unit

Event Timeline

Sebastián Barschkis (sebbas) requested review of this revision.Sep 24 2020, 2:53 PM

Sebastián Barschkis (sebbas) created this revision.

Harbormaster completed remote builds in B10386: Diff 29231.Sep 24 2020, 2:54 PM

Using hidden visibility is fine with me, but I'm not sure if it is the solution, or whether this is just eases symptom and the root of the issue goes deeper.

Would we run into more UBSan warnings of type [1] due to this?
https://bugs.llvm.org/show_bug.cgi?id=39191
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80963
https://stackoverflow.com/questions/57294792/c-ubsan-produces-false-positives-with-derived-objects

[1]:

runtime error: member call on address 0x7ffd9212cb28 which does not point to an object of type 'Foo'
0x7ffd9212cb28: note: object is of type 'Bar'
 fd 7f 00 00  08 69 43 00 00 00 00 00  50 e1 42 00 00 00 00 00  e1 82 2b 52 9c 7f 00 00  00 1c 01 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'Bar'

Sebastián Barschkis (sebbas) edited the summary of this revision. (Show Details)Sep 24 2020, 4:41 PM

some minor nitpicking looks good otherwise.

extern/mantaflow/CMakeLists.txt
34	That only holds true if you were to build mantaflow as a shared library, given it is a static link for us `-fvisibility=hidden` controls is if these functions are exported from the blender binary or not.
35	clang on windows takes arguments in the MSVC format and will throw a fit on normal clang compiler switches, so you'd want to check for `NOT MSVC_CLANG` in this if, or wrap the whole block in a `if(NOT WIN32)` symbols are hidden by default since the dawn of time on windows, so it's safe to skip it.

This revision now requires changes to proceed.Sep 24 2020, 5:42 PM

Since I haven't seen the linker warnings on any system except Apple arm, I would like to silence them just here.
At least until there is a first production-ready arm device. Once that is out, this should be revisited.

Harbormaster completed remote builds in B10401: Diff 29255.Sep 25 2020, 10:33 AM

Sebastián Barschkis (sebbas) marked 2 inline comments as done.Sep 25 2020, 10:33 AM

I think @Brecht Van Lommel (brecht) mentioned a desire to move to -fvisibility=hidden on linux (and mac?) so for now this will do, until there is a blender wide solution in place.

This revision is now accepted and ready to land.Sep 25 2020, 5:27 PM

This is odd, just like it's odd to me that D8823 only happened on Intel and not ARM.

Maybe the default visibility is slightly different depending on the architecture and this is documented somewhere, but this may well be a compiler bug too.

Either way, seems fine to commit this.

Closed by commit rBab65fe5a2dcc: Fluid: Use hidden symbol visibility (authored by Sebastián Barschkis (sebbas)). · Explain WhyOct 12 2020, 12:45 PM

This revision was automatically updated to reflect the committed changes.

Sebastián Barschkis (sebbas) added a commit: rBab65fe5a2dcc: Fluid: Use hidden symbol visibility.

Sebastián Barschkis (sebbas) mentioned this in rBd518b0fda5c1: Fluid: Updated hidden symbol visibility comment.Mar 11 2021, 4:16 PM