Export C++ library symbols for core methods

[ischoegl]

Changes proposed in this pull request

• Replace CANTERA_USE_INTERNAL by CT_DLL_EXPORT
• Add new preprocessor CT_API and use it for selected C++ objects

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#140

If applicable, provide an example illustrating new features this pull request is introducing

Checklist

• The pull request includes a clear description of this code change
• Commit messages have short titles and reference relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• Style & formatting of contributed code follows contributing guidelines
• The pull request is ready for review

[ischoegl]

🎉 ... looks like this is working

> dumpbin /exports cantera_shared.dll
Microsoft (R) COFF/PE Dumper Version 14.31.31104.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file cantera_shared.dll

File Type: DLL

  Section contains the following exports for cantera_shared.dll

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

    ordinal hint RVA      name

[...]

Main thing left is to add the CT_API decoration to some additional classes.

[ischoegl]

@speth ... I am leaving this for the moment to get some feedback.

While adding CT_API to class definitions works, it produces a large number of warnings, as dll-export needs to be enabled for everything (including inherited and private methods, see documentation). While it is more tedious, I believe adding the decorations selectively is preferable, as it allows for finer control while avoiding compiler warnings that are difficult to track.

[ischoegl]

I am assuming that this appends to CPPDEFINES.

[speth]

I think you only want to set this while compiling the library sources, e.g. from the localenv that's created in src/SConscript. Setting it here will affect other cases like the tests, which is not what you want.

[ischoegl]

I think that CT_DLL_EXPORT needs to be defined when linking statically? (Also, builds with using the flag only on src/SConscript won't compile, but I may be overlooking something)

[rwest]

I don't understand the details of what's happening here, but I have one thought: if these CT_API labels should be added to some (but not all) new code, perhaps some instructions should be added to CONTRIBUTING.md as part of this PR?

[ischoegl]

I don't understand the details of what's happening here, but I have one thought: if these CT_API labels should be added to some (but not all) new code, perhaps some instructions should be added to CONTRIBUTING.md as part of this PR?

Yes - if this is pursued, then all external facing methods should receive this decoration. I was applying this conservatively for the moment as I'd like to get feedback on the general direction.

[speth]

Have you been able to actually use this for an example, when compiling on Windows? I tried compiling a pared-down version of the sample demo.cpp using SCons (editing it to link to cantera_shared) and couldn't get it to work at all.

#include "cantera/base/Solution.h"
#include <iostream>

using namespace Cantera;

void demoprog()
{
    auto sol = newSolution("h2o2.yaml", "ohmech");
}

int main(int argc, char** argv)
{
    try {
        std::cout << "running demoprog..." << std::endl;
        std::cout.flush();
        demoprog();
        std::cout << "finished!" << std::endl;
    } catch (std::exception& err) {
        std::cout << "oops..." << std::endl;
        std::cout << err.what() << std::endl;
    }
}

This compiles, but crashes before even getting to the bit of output. Even if I comment out the call to demoprog, it still crashes. The only case that "works" is to remove the call to newSolution from demoprog.

I know I had tried doing some work in this direction quite a while back, but abandoned it because there were just so many "gotchas" to doing this for even a moderately complex C++ API that wasn't designed with the quirks of Windows DLLs in mind.

[ischoegl]

Have you been able to actually use this for an example, when compiling on Windows?

Not yet. I was mostly waiting on some initial feedback for go/no-go.

My plan for testing is to get this to run for this demo (which works without issues on Linux). Your example is certainly easier!

[speth]

For me, I think the things that are important for the go/no-go decision on this will come down to how difficult it is to actually resolve the various challenges with getting this to work reliably and without too many limitations. Otherwise, I think it's easier to just suggest that users stick to using the static library on Windows for anything more than the C API. Potentially needing to add the CT_API macro to a bunch of methods to make this work doesn't bother me at all.

I should mention, I also noticed that compiling the demo.cpp example did not work when I specified the static library, due to the CT_API macro, and I think we need to make sure there that linking statically is still an option.

[ischoegl]

[…] Potentially needing to add the CT_API macro to a bunch of methods to make this work doesn't bother me at all.

That’s good to know as it was one of my concerns that kept me from going deep into the weeds.

For me, I think the things that are important for the go/no-go decision on this will come down to how difficult it is to actually resolve the various challenges with getting this to work reliably and without too many limitations. Otherwise, I think it's easier to just suggest that users stick to using the static library on Windows for anything more than the C API.

I would agree on that.

I should mention, I also noticed that compiling the demo.cpp example did not work when I specified the static library, due to the CT_API macro, and I think we need to make sure there that linking statically is still an option.

This is odd. I was under the impression that the test suite would cover that? All tests still pass …

Regardless, based on this feedback I will pursue this idea as something that I would like to include in 3.0, but not necessarily with a high priority.

[ischoegl]

Lacking bandwidth/interest to pursue this further.

[ischoegl]

Reopening with a help wanted label. While I won’t be able to pursue this myself, it is still documenting some attempts to introduce this feature.

[speth]

I wanted to take a new look at this, since it seems that generating a more complete shared library is necessary to resolve Cantera/conda-recipes#40 cleanly. Using this PR, I tried compiling the demo program above with Visual Studio 2022, and learned a few things.

First, on why the demo program was crashing without generating output: one cause of this is not having cantera_shared.dll on the PATH, which I think was the error that I made before. If you run demo.exe from a standalone command prompt, you get an error telling you that this is what's wrong, but unfortunately running from inside VS Code gives no such useful information.

Second, when compiling the demo program with VS2022, I get the error:

include\cantera/base/ctexceptions.h(78): error C2491: 'Cantera::CanteraError::CanteraError': definition of dllimport function not allowed

If we look at the relevant lines of ctexceptions.h, we see this definition for the CanteraError constructor:

    template <typename... Args>
    CT_API CanteraError(const std::string& procedure, const std::string& msg,
                        const Args&... args)
        : procedure_(procedure)
    {
        if (sizeof...(args) == 0) {
            msg_ = msg;
        } else {
            msg_ = fmt::format(msg, args...);
        }
    }

I think there are two possible fixes to this. One is to separate the declaration and implementation (both in the header), with the latter toggled with an #ifdef:

// Inside CanteraError class
    template <typename... Args>
    CT_API CanteraError(const std::string& procedure, const std::string& msg,
                        const Args&... args);

// Outside CanteraError class, but still in ctexceptions.h
#ifdef CT_DLL_EXPORT
template <typename... Args>
CanteraError::CanteraError(const std::string& procedure, const std::string& msg,
                    const Args&... args)
    : procedure_(procedure)
{
    if (sizeof...(args) == 0) {
        msg_ = msg;
    } else {
        msg_ = fmt::format(msg, args...);
    }
}
#endif

This unfortunately seems like it will require a lot of ugly boilerplate throughout the codebase. The other option is to simply remove the CT_API declaration for this method -- In any case where it is used, the code provided in this definition can be recompiled anyway, and it's not critical to link to versions of this function from the shared library. This creates a little bit of code bloat, but nothing like linking to the static library.

Finally, I also noticed that I am able to call some methods that are not marked with CT_API, and I'm not sure why. For instance, the following definition of the demoprog function works fine:

void demoprog()
{
    auto sol = newSolution("h2o2.yaml", "ohmech");
    std::cout << sol->thermo()->report() << std::endl;
    std::cout << "phi = " << sol->thermo()->electricPotential() << std::endl;
}

despite the fact that electricPotential() (a non-virtual, inline method), and report() (a virtual method) are not marked with CT_API. If it's not actually necessary to decorate every function, this could end up being less invasive than we originally anticipated. But I think we need to better understand what the rules are for when this is and isn't required.

[speth]

I pushed a rebased version of this to my fork (https://github.com/speth/cantera/tree/export-symbols) which resolves the noted merge conflicts. I also added the fix to remove CT_API from the templated CanteraError constructor.

[ischoegl]

I pushed a rebased version of this to my fork (https://github.com/speth/cantera/tree/export-symbols) which resolves the noted merge conflicts. I also added the fix to remove CT_API from the templated CanteraError constructor.

@speth ... depending on where this is going, I can do a hard reset on my repo to your branch (edit: I just reset my branch to yours) as long as there aren't major revisions necessary; if there is substantial additional development, feel free to close this PR and start a new one.

[ischoegl]

Superseded by #1429