Unable to catch RDKit exceptions in linked library when compiling with fvisibility=hidden

[cdvonbargen]

For example, the following won't get caught if this is in a library that compiles with -fvisibility=hidden

#include <GraphMol/SanitException.h>
...
try {
    RDKit::MolOps::sanitizeMol(*returnMol);
} catch (const RDKit::MolSanitizeException& e) {
    std::cerr << "Sanitization failed: " << e.message() << "\n";
}

because RDKIT_GRAPHMOL_BUILD is not defined in this external library, and so the macro is defined by empty and falls back to hidden visibility:

// RDKIT_GRAPHMOL_EXPORT definitions
#if defined(BOOST_HAS_DECLSPEC) && defined(RDKIT_DYN_LINK) && !defined(SWIG)
#ifdef RDKIT_GRAPHMOL_BUILD
#define RDKIT_GRAPHMOL_EXPORT __declspec(dllexport)
#else
#define RDKIT_GRAPHMOL_EXPORT __declspec(dllimport)
#endif
#endif
#ifndef RDKIT_GRAPHMOL_EXPORT
#define RDKIT_GRAPHMOL_EXPORT
#endif
// RDKIT_GRAPHMOL_EXPORT end definitions

See some discussion of "Problems with C++ exceptions" in https://gcc.gnu.org/wiki/Visibility

[bp-kelley]

RDKIT_GRAPHMOL_BUILD is defined in RDGeneral/export.h which SanitException.h includes.

The relevant bit is here:

// RDKIT_GRAPHMOL_EXPORT definitions
#if defined(BOOST_HAS_DECLSPEC) && defined(RDKIT_DYN_LINK) && !defined(SWIG)
#ifdef RDKIT_GRAPHMOL_BUILD
#define RDKIT_GRAPHMOL_EXPORT __declspec(dllexport)
#else
#define RDKIT_GRAPHMOL_EXPORT __declspec(dllimport)
#endif
#endif
#ifndef RDKIT_GRAPHMOL_EXPORT
#define RDKIT_GRAPHMOL_EXPORT
#endif
// RDKIT_GRAPHMOL_EXPORT end definitions

Could you try defining RDKIT_DYN_LINK in your build?

[d-b-w]

So we should be defining RDKIT_DYN_LINK when building any library that links against RDKit?

[bp-kelley]

It certainly looks that way (for dynamic linking). Did it end up working for you? I'm going to ping Greg on this one. Cheers, Brian

…

On Thu, Oct 31, 2019 at 12:18 PM Dan N ***@***.***> wrote: So we should be defining RDKIT_DYN_LINK when building any library that links against RDKit? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2753?email_source=notifications&email_token=ACLOFIASWETUJZNNPKPAWWDQRMAN3A5CNFSM4JHL36O2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECYL5SA#issuecomment-548454088>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACLOFIG75Z62W52RVFHV6M3QRMAN3ANCNFSM4JHL36OQ> .

[bp-kelley]

The plot thickens. Here is my test case:

#include <RDGeneral/test.h>
#include <iostream>
#include <string>
#include <GraphMol/RDKitBase.h>
#include <GraphMol/SmilesParse/SmilesParse.h>
#include <GraphMol/FileParsers/FileParsers.h>
#include <RDGeneral/RDLog.h>
#include <GraphMol/SanitException.h>

using namespace RDKit;

int main(int, char *argv[]) {
  std::string smi = "N(=O)(=O)=O";
  ROMol *mol;
    try {
      mol = SmilesToMol(smi);
      std::cerr << "Did not catch exception" << std::endl;
      return 1;
    } catch (MolSanitizeException &) {
      std::cerr << "Caught exception" << std::endl;
      mol = (ROMol *)nullptr;
      return 0;
    }
}

I'm using rdkit as installed via conda.

> c++ doit.cpp -I /usr/prog/anaconda3/include/rdkit/ -I /usr/prog/anaconda3/include/ -L /usr/prog/anaconda3/lib -lRDKitGraphMol -lRDKitSmilesParse
> LD_LIBRARY_PATH=/usr/prog/anaconda3/lib ./a.out
Caught exception

Which works ok. Let's try -fvisibility=hidden

> c++ doit.cpp -fvisibility=hidden -I /usr/prog/anaconda3/include/rdkit/ -I /usr/prog/anaconda3/include/ -L /usr/prog/anaconda3/lib -lRDKitGraphMol -lRDKitSmilesParse
> LD_LIBRARY_PATH=/usr/prog/anaconda3/lib ./a.out
Caught exception

which also works.

It also appears that the symbols are certainly exported properly:

> nm -C -D /usr/prog/anaconda3/lib/libRDKitGraphMol.so | grep MolSanitizeException
000000000016b480 V typeinfo for RDKit::MolSanitizeException
000000000012d7e0 V typeinfo name for RDKit::MolSanitizeException
000000000016b608 V vtable for RDKit::MolSanitizeException

Could you run the later on your libRDKitGraphMol?

[ricrogz]

Sorry for coming back to this after so long, but yesterday I hit an issue like this one (but with a different library), and I found some interesting information in https://stackoverflow.com/questions/14268736/symbol-visibility-exceptions-runtime-error.

I think the second paragraph in the answer gives a good explanation about what is causing the problem: classes with different visibility cannot be matched as the same. Apparently some compilers do not have this problem, e.g., the post mentions MSVC comparing the exception classes by literal name, and I couldn't reproduce my problem when using gcc 6.4 on linux, but clang 11.0 on Mac did definitely hit it.

What happens both in @cdvonbargen's case, and in the one I had yesterday, is that the libraries we were using were built with "default" visibility for the exported functions (the libraries would be useless otherwise), but the code that uses them is building with the "hidden" visibility flag, triggering the mismatch. The problem can be addressed by hardcoding the "default" visibility attribute into the RDKIT_*_EXPORT definitions for non-windows builds, so that we force the functions to be visible both when building RDKit and also when building any other code using it, e.g.:

// RDKIT_GRAPHMOL_EXPORT definitions
#if defined(BOOST_HAS_DECLSPEC) && defined(RDKIT_DYN_LINK) && !defined(SWIG)
#ifdef WIN32
#ifdef RDKIT_GRAPHMOL_BUILD
#define RDKIT_GRAPHMOL_EXPORT __declspec(dllexport)
#else
#define RDKIT_GRAPHMOL_EXPORT __declspec(dllimport)
#endif
#else
#define RDKIT_GRAPHMOL_EXPORT __attribute__((visibility("default")))
#endif
#endif
#ifndef RDKIT_GRAPHMOL_EXPORT
#define RDKIT_GRAPHMOL_EXPORT
#endif
// RDKIT_GRAPHMOL_EXPORT end definitions

[ricrogz]

I just pushed a branch with a "proof of concept" to my fork of this repo. The differences with the current master are this executable and the CMakeLists update that builds 2 copies of the executable, one with 'hidden' and one with 'default' visibility:

https://github.com/ricrogz/rdkit/blob/82f299356c35e5f49bb41a769767e28a72c2f58d/Code/GraphMol/CMakeLists.txt#L166-L176

Looking at the CI build, we can see that the one with hidden visibility fails on Mac (the test failure in Ubuntu 16.04 py27 looks like a random artifact). It seems that clang doesn't do a good job matching object types with different visibilities: when the RDKit library is built, the header which declares AtomValenceException is used with default visibility, while in the exe it is built with hidden visibility. This causes a mismatch between the types of the exception object being thrown and the type we expect to catch, which leads to the wrong exception bubbling up, and failing the test.

There's a way to work around this if you want to link RDKit to something that is build with hidden visibility, which consists in wrapping RDKit's #includes in #pragma GCC visibility / #pragma GCC visibility pop to force default visibility, but that's prone to error, since you have to remember doing it for every file.