AcademySoftwareFoundation / MaterialX

MaterialX is an open standard for the exchange of rich material and look-development content across applications and renderers.
http://www.materialx.org/
Apache License 2.0
1.85k stars 352 forks source link

Build and runtime error on MacOS Catalina #1518

Open pablode opened 1 year ago

pablode commented 1 year ago

When trying to build MaterialXView, I encounter following error with Xcode 12.4 on Catalina 10.15.7 (sorry for the outdated setup!):

[100%] Linking CXX executable ../../bin/MaterialXView
Undefined symbols for architecture x86_64:
  "_OBJC_CLASS_$_MTLTileRenderPipelineDescriptor", referenced from:
      objc-class-ref in libMaterialXRenderMsl.a(MetalState.mm.o)
ld: symbol(s) not found for architecture x86_64
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)
make[3]: *** [bin/MaterialXView] Error 1
make[2]: *** [source/MaterialXView/CMakeFiles/MaterialXView.dir/all] Error 2
make[1]: *** [source/MaterialXView/CMakeFiles/MaterialXView.dir/rule] Error 2
make: *** [MaterialXView] Error 2

The issue can be resolved as follows

--- a/source/MaterialXRenderMsl/CMakeLists.txt
+++ b/source/MaterialXRenderMsl/CMakeLists.txt
@@ -76,6 +79,7 @@ set_target_properties(
     OUTPUT_NAME ${MATERIALX_MODULE_NAME}${MATERIALX_LIBNAME_SUFFIX}
     COMPILE_FLAGS "${EXTERNAL_COMPILE_FLAGS}"
     LINK_FLAGS "${EXTERNAL_LINK_FLAGS}"
+    INTERFACE_LINK_LIBRARIES "-undefined dynamic_lookup"
     INSTALL_RPATH "${MATERIALX_SAME_DIR_RPATH}"
     VERSION "${MATERIALX_LIBRARY_VERSION}"
     SOVERSION "${MATERIALX_MAJOR_VERSION}")

which, however, leads to a number of linker warnings.

Starting MaterialXView causes a sanitizer error

/Users/pablode/tmp/MaterialX/source/MaterialXRenderMsl/MetalState.mm:60:29: runtime error: load of value 190, which is not a valid value for type 'bool'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Users/pablode/tmp/MaterialX/source/MaterialXRenderMsl/MetalState.mm:60:29 in
[1]    48884 abort      ./bin/MaterialXView

which can be fixed using following patch:

--- a/source/MaterialXRenderMsl/MetalState.h
+++ b/source/MaterialXRenderMsl/MetalState.h
@@ -56,7 +56,7 @@ struct MetalState
     id<MTLRenderCommandEncoder>         renderCmdEncoder = nil;
     std::stack<MaterialX::MetalFramebufferPtr> framebufferStack;

-    bool supportsTiledPipeline;
+    bool supportsTiledPipeline = false;

     id<MTLDepthStencilState> opaqueDepthStencilState = nil;
     id<MTLDepthStencilState> transparentDepthStencilState = nil;
jstone-lucasfilm commented 1 year ago

Good catches, @pablode, and these would be good to address in the upcoming 1.38.8 release. Would you have the bandwidth to propose a pull request for these changes, or should we aim to address them on our side?

pablode commented 1 year ago

Unfortunately I don't have time to investigate this further right now.

I submitted this as an issue because I have concerns about the -undefined dynamic_lookup approach, which apparently could be replaced by a symbol-specific flag (ld -U, which I wasn't able to get to work, possibly due to Obj-C interop). Furthermore, a dozen warnings are thrown and possibly more on newer macOS versions.

jstone-lucasfilm commented 1 year ago

Completely understood, @pablode, and given that additional context, we'll likely want to continue the discussion about this proposed fix beyond the timeline of 1.38.8.