Git Product home page Git Product logo

Comments (9)

jstone-lucasfilm avatar jstone-lucasfilm commented on May 14, 2024

This is a great question, and the main impediment to implementing this in MaterialX was the lack of a show or client who required the feature. At Lucasfilm, our Windows projects based on MaterialX have always used static libraries, but it does seem inevitable that we'll ultimately need to support shared libraries as well.

I believe it should be straightforward to add support for shared libraries on Windows, and we've been considering using the CMake feature described here to reduce the volume of boilerplate code and maintenance burden going forward:

https://blog.kitware.com/create-dlls-on-windows-without-declspec-using-new-cmake-export-all-feature/

Historically, supporting shared libraries on Windows has required import/export declarations for every exported symbol (see earlier projects like OpenEXR for examples of this pattern), and the CMake feature above could reduce this overhead significantly.

@marktucker If this is a project that your team would have the bandwidth to take on, we would greatly appreciate the contribution! Otherwise, we'll plan to take a look at this on the Lucasfilm side.

All the best,

-Jonathan

from materialx.

lgritz avatar lgritz commented on May 14, 2024

Wow, that's really interesting, I didn't know about that feature.

But is there a downside to simply making everything exported? The "all" approach doesn't let you distinguish between true exports and symbols that aren't part of the public interface that you'd ideally like to keep from being exported. But maybe that doesn't matter compared to the simplicity?

from materialx.

marktucker avatar marktucker commented on May 14, 2024

Thanks, that's good to know. I also wasn't aware of that cmake feature. I'll give that a try and see how far it gets me.

FWIW, if I understand properly, other platforms (particularly MacOS) can greatly improve load times by restricting the number of exported symbols from shared libraries. So in the long term it might be worth the effort to explicitly decide/declare what to export (even if it's whole classes). But I'm happy to take the easy road for now. I'll report back here with any progress (which probably won't happen for a little while).

from materialx.

jstone-lucasfilm avatar jstone-lucasfilm commented on May 14, 2024

Thanks @marktucker, that contribution would be greatly appreciated! Keep us posted as your work proceeds, and make sure to fill out a CLA when you're ready to write up a pull request.

@lgritz That's a good point, and although exporting all symbols is consistent with our current approach to shared libraries on Linux and MacOS, we should definitely keep in mind the potential advantages of selective symbol visibility in the future.

from materialx.

meshula avatar meshula commented on May 14, 2024

I must point out that although cmake now includes an export-all feature, I can't help but feel, based on many years involvement with this problem, that this feature is ill-advised and a nightmare in the making. Reading the linked article does not give me confidence on robustness, or that they have hit on something new that alleviates the problems with automatic symbol export.

The Windows port of Zeno used to have an automatic symbol exporter thing, but as it turns out templated libraries can quickly cause the auto export of more symbols that are legal from a DLL (65535, as I recall.) Some libraries, such as boost, PhysBAM, and parts of the STL cause such overflows, and David Lenihan and I had built a fairly large regex-contraption of symbol exclusions. In the end, we went with manual exporting because the exclusion list was absolutely unmaintainable due to MSVC's mangling scheme not being stable between compiler revisions.

The caveats in the linked article, describing how they had to reorganize code, including banning certain structures in order to accomodate the schema, reinforcement my opinion that an automatic system is a nightmare, and a sure sign of a tail that wags that dog.

On the OpenEXR project we have gathered notes here, built in conjunction with the USD team, as we attempt to come to grips with the current SotA across compilers and build environments: https://github.com/AcademySoftwareFoundation/openexr/blob/master/docs/SymbolVisibility.md

Despite the pain, I have to say again, my advice is to use the visibility features via macros, because a short term gain with automatic export will lead to an eternal can of worms. I'm sure that a call for assistance on this front could get you community help in an adornment pass, and subsequent upkeep is easy once the first pass is done.

from materialx.

meshula avatar meshula commented on May 14, 2024

To illustrate the issues I anticipated, I did a dry run on MaterialXCore.

If anyone has any feedback on whether this was done correctly, that would be much appreciated, as I'd like a proper understanding of what's going on. First blush TL/DR, this doesn't seem to be a magic bullet.

Read on to see my observations ~

l added these lines, per the article:

diff --git a/source/MaterialXCore/CMakeLists.txt b/source/MaterialXCore/CMakeLists.txt
index 735b847..01fc885 100644
--- a/source/MaterialXCore/CMakeLists.txt
+++ b/source/MaterialXCore/CMakeLists.txt
@@ -1,7 +1,10 @@
 file(GLOB materialx_source "${CMAKE_CURRENT_SOURCE_DIR}/*.cpp")
 file(GLOB materialx_headers "${CMAKE_CURRENT_SOURCE_DIR}/*.h*")

-add_library(MaterialXCore ${materialx_source} ${materialx_headers})
+include(GenerateExportHeader)
+
+add_library(MaterialXCore SHARED ${materialx_source} ${materialx_headers})
+generate_export_header(MaterialXCore)

 add_definitions(-DMATERIALX_MAJOR_VERSION=${MATERIALX_MAJOR_VERSION})
 add_definitions(-DMATERIALX_MINOR_VERSION=${MATERIALX_MINOR_VERSION})
@@ -10,6 +13,7 @@ add_definitions(-DMATERIALX_BUILD_VERSION=${MATERIALX_BUILD_VERSION})
 set_target_properties(
     MaterialXCore PROPERTIES
     OUTPUT_NAME MaterialXCore
+    WINDOWS_EXPORT_ALL_SYMBOLS TRUE
     COMPILE_FLAGS "${EXTERNAL_COMPILE_FLAGS}"
     LINK_FLAGS "${EXTERNAL_LINK_FLAGS}"
     VERSION "${MATERIALX_LIBRARY_VERSION}"

The first issue that occurs is that as reported in the article the cmake facility does not export static const symbols, such as MaterialX::Node::CATEGORY. MaterialX::TypedElement::TYPE_ATTRIBUTE, and so on. These would have to be manually adorned in any case.

The next issue that I encountered, is that there's no facility to adapt existing code to link the new dll. Every MaterialXCore symbol generates a link error. For example,

MaterialXGenShader.lib(UnitSystem.obj) : error LNK2001: unresolved external symbol "class std::basic_string<char,
struct std::char_traits<char>,class std::allocator<char> > const MaterialX::EMPTY_STRING" (?EMPTY_STRING@Material
X@@3V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@B) [C:\Projects\Lucasfilm\MaterialX-build\sourc
e\MaterialXTest\MaterialXTest.vcxproj]

According to the article, you also need to use CMake to generate export headers that include the appropriate declspec macros, which you need to backpatch into your existing headers. I didn't attempt to resolve this problem. Here's the generated export header, materialxcore_export.h. It looks pretty traditional.

#ifndef MATERIALXCORE_EXPORT_H
#define MATERIALXCORE_EXPORT_H

#ifdef MATERIALXCORE_STATIC_DEFINE
#  define MATERIALXCORE_EXPORT
#  define MATERIALXCORE_NO_EXPORT
#else
#  ifndef MATERIALXCORE_EXPORT
#    ifdef MaterialXCore_EXPORTS
        /* We are building this library */
#      define MATERIALXCORE_EXPORT __declspec(dllexport)
#    else
        /* We are using this library */
#      define MATERIALXCORE_EXPORT __declspec(dllimport)
#    endif
#  endif

#  ifndef MATERIALXCORE_NO_EXPORT
#    define MATERIALXCORE_NO_EXPORT
#  endif
#endif

#ifndef MATERIALXCORE_DEPRECATED
#  define MATERIALXCORE_DEPRECATED __declspec(deprecated)
#endif

#ifndef MATERIALXCORE_DEPRECATED_EXPORT
#  define MATERIALXCORE_DEPRECATED_EXPORT MATERIALXCORE_EXPORT MATERIALXCORE_DEPRECATED
#endif

#ifndef MATERIALXCORE_DEPRECATED_NO_EXPORT
#  define MATERIALXCORE_DEPRECATED_NO_EXPORT MATERIALXCORE_NO_EXPORT MATERIALXCORE_DEPRECATED
#endif

#if 0 /* DEFINE_NO_DEPRECATED */
#  ifndef MATERIALXCORE_NO_DEPRECATED
#    define MATERIALXCORE_NO_DEPRECATED
#  endif
#endif

#endif /* MATERIALXCORE_EXPORT_H */

In order to import MaterialXCore, you need to redeclare the things are hoping to import with MATERIALXCORE_EXPORT in the MaterialX headers. So if that's a requirement, I don't see what the autogeneration is buying, as it becomes a meta-source generation step, that doesn't remove any manual labor. I would appreciate feedback on whether there is some step I've misunderstood here.

As I suspected, exported symbols include symbols MaterialXCore is not responsible for. I didn't check for export of private members, although a serious effort would check for those as well. I would guess, based on past experience, that private symbols are not elided, but that should be verified.

I've attached the generated dll/exp/lib in Release.zip, and the full symbol dump as symbols.txt.

Microsoft (R) COFF/PE Dumper Version 14.26.28806.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file MaterialXCore.dll

File Type: DLL

  Section contains the following exports for MaterialXCore.dll

    00000000 characteristics
    FFFFFFFF time date stamp
        0.00 version
           1 ordinal base
        2416 number of functions
        2416 number of names

    ordinal hint RVA      name

          1    0 00009900 ??$?0$$QEAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@$$Z$$V@?$pair@$$CBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@Upiecewise_construct_t@1@V?$tuple@$$QEAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@1@V?$tuple@$$V@1@@Z
          2    1 00009940 ??$?0AEAY01$$CBDAEAY01$$CBD$0A@@?$pair@$$CBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@AEAY01$$CBD0@Z
          3    2 00009940 ??$?0AEAY0BA@$$CBDAEAY0BA@$$CBD$0A@@?$pair@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@AEAY0BA@$$CBD0@Z
          4    3 00009940 ??$?0AEAY0BE@$$CBDAEAY0BE@$$CBD$0A@@?$pair@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@AEAY0BE@$$CBD0@Z
          5    4 00009940 ??$?0AEAY0BJ@$$CBDAEAY0BJ@$$CBD$0A@@?$pair@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@AEAY0BJ@$$CBD0@Z
          6    5 00009940 ??$?0AEAY0L@$$CBDAEAY0L@$$CBD$0A@@?$pair@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@AEAY0L@$$CBD0@Z
          7    6 00009940 ??$?0AEAY0N@$$CBDAEAY0BB@$$CBD$0A@@?$pair@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@AEAY0N@$$CBDAEAY0BB@$$CBD@Z
          8    7 00009940 ??$?0AEAY0N@$$CBDAEAY0BI@$$CBD$0A@@?$pair@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@AEAY0N@$$CBDAEAY0BI@$$CBD@Z
          9    8 00009940 ??$?0AEAY0P@$$CBDAEAY0P@$$CBD$0A@@?$pair@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@AEAY0P@$$CBD0@Z
         10    9 00051060 ??$?0AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@$$Z$$V@?$pair@$$CBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@H@std@@QEAA@Upiecewise_construct_t@1@V?$tuple@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@1@V?$tuple@$$V@1@@Z
         11    A 00051060 ??$?0AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@$$Z$$V@?$pair@$$CBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@M@std@@QEAA@Upiecewise_construct_t@1@V?$tuple@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@1@V?$tuple@$$V@1@@Z
 ...
         21   14 000204E0 ??$?0V?$_Vector_iterator@V?$_Vector_val@U?$_Simple_types@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@std@@@std@@@std@@@?$set@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@U?$less@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@2@V?$allocator@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@2@@std@@QEAA@V?$_Vector_iterator@V?$_Vector_val@U?$_Simple_types@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@std@@@std@@@1@0@Z
         22   15 000099D0 ??$?0V?$tuple@$$QEAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@std@@V?$tuple@$$V@1@$0A@$$Z$S@?$pair@$$CBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@V12@@std@@QEAA@AEAV?$tuple@$$QEAV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@1@AEAV?$tuple@$$V@1@U?$integer_sequence@_K$0A@@1@U?$integer_sequence@_K$S@1@@Z
         23   16 00051090 ??$?0V?$tuple@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@std@@V?$tuple@$$V@1@$0A@$$Z$S@?$pair@$$CBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@H@std@@QEAA@AEAV?$tuple@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@1@AEAV?$tuple@$$V@1@U?$integer_sequence@_K$0A@@1@U?$integer_sequence@_K$S@1@@Z
         24   17 00051090 ??$?0V?$tuple@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@std@@V?$tuple@$$V@1@$0A@$$Z$S@?$pair@$$CBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@M@std@@QEAA@AEAV?$tuple@AEBV?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@1@AEAV?$tuple@$$V@1@U?$integer_sequence@_K$0A@@1@U?$integer_sequence@_K$S@1@@Z
  ...
       2410  969 000098D0 ?what@bad_weak_ptr@std@@UEBAPEBDXZ
       2411  96A 000098E0 ?what@exception@std@@UEBAPEBDXZ
...
       2414  96D 000547F0 __local_stdio_printf_options
       2415  96E 00054800 _scprintf
       2416  96F 00054860 sprintf_s

symbols.txt

Release.zip

from materialx.

meshula avatar meshula commented on May 14, 2024

@marktucker I hope I haven't scared you off ~ clearly I've got some PTSD around dealing with windows export issues over the years!

Jonathan noticed that the HDF5 library uses this facility. Looking at the implementation it seems they use it in conjunction with their own set of H5_DLL macros that adorn their API.

https://github.com/HDFGroup/hdf5/blob/2ea165efd095d8876639814dc615dd01a2ec34e6/src/H5api_adpt.h

A worthwhile investigation would be what the difference is between a traditional export macro suite and the cmake facility, and a consideration of whether the re-export of std symbols is really a problem or just noise. The biggest question in my mind is if it's in fact necessary to add macros to support import when using the cmake facility, as appears might be the case from my brief experiment, and the HDF5 implementation.

Please consider my notes investigative groundwork, and pointers to where tigers have been encountered in the past :)

from materialx.

marktucker avatar marktucker commented on May 14, 2024

No worries @meshula, I was already scared. Thank you for scouting it out. The last time I dealt with this stuff in a serious way was in 1997 when I ported Houdini to Windows NT :) I think I briefly tried using .def files, but after a little while ended up biting the bullet and putting "LIB_API" in front of every single class definition.

@jstone-lucasfilm, would you be averse to a massive PR that added these explicit export/import declarations (and associated LIB_API.h headers to each library)? If this is the right way forward, I'd rather just bite the bullet and do it... Obviously we should agree on names and such ahead of time. But I don't want to even start on this work unless I know it can be pulled back into the main branch...

from materialx.

jstone-lucasfilm avatar jstone-lucasfilm commented on May 14, 2024

@meshula Thanks for that investigative groundwork, and I think your recommendation to use explicit macros from the start is the right approach forward. Looking more carefully at references to CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS on GitHub, it seems most common as a quick workaround for shared libraries on Windows, rather than as an enduring solution, and I don't see examples of its usage being recommended as a general pattern.

@marktucker If you're open to tackling the explicit export/import declarations, I'd be happy to work with you to prepare this work for merging to MaterialX main. It's a project that will benefit multiple teams going forward, and you're well positioned to test the new work in the context of Houdini on Windows. My initial recommendation would be to follow the high-level approach of an ASWF project such as Imath, with headers following a pattern such as MaterialXCore/Export.h, and macros following a pattern such as MX_CORE_EXPORT. Additional suggestions from @meshula, @lgritz, and others are welcome!

from materialx.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.