Page MenuHome

Fluid: Use hidden symbol visibility
ClosedPublic

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

Details

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

Event Timeline

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

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'
Ray molenkamp (LazyDodo) requested changes to this revision.Sep 24 2020, 5:42 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.

Sebastián Barschkis (sebbas) marked 2 inline comments as done.Sep 25 2020, 10:33 AM
Ray molenkamp (LazyDodo) accepted this revision.EditedSep 25 2020, 5:27 PM

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.

This revision was automatically updated to reflect the committed changes.