Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VS 2010 broken: exporting std::string subclass crashes #86

Closed
sebastianelsner opened this issue Jan 4, 2014 · 14 comments
Closed

VS 2010 broken: exporting std::string subclass crashes #86

sebastianelsner opened this issue Jan 4, 2014 · 14 comments
Labels
Bug A bug in the source code
Milestone

Comments

@sebastianelsner
Copy link

The issue was already discussed on the mailing list, this is just for the sake of completion:

nick.porcino@gmail.com
Right, on windows one can't derive from standard string and then export the
class. When the exception is thrown, Iex will crash deep in heap code in
the Stdlib.

halfdan@sidefx.com
The problem is that BaseExc derives from std::string, which doesn't have a dllexport in MSVC2010 and onwards, and so in our case, actually broke the build.
We patched our version so that BaseExc uses std::string as a member variable, rather than deriving from it. This was easy enough to do, since this change only affects testBaseExc.cpp.

@meshula
Copy link
Contributor

meshula commented Jan 5, 2014

That's what I did as well. At the moment, I have no Windows machines to build on, so I can't reliably do the work and prepare a fix. I am wondering if you might submit a pull request?

@sebastianelsner
Copy link
Author

Sadly, this is out of my league. But if you could provide a patch as a starting point I'd be willing to test and try to make it work.

@meshula
Copy link
Contributor

meshula commented Jan 22, 2014

As I recall, it was really straight forward. Basically make the following modification, and everywhere a string is assigned or referenced (for example from what()), make it operate on _message instead of the this pointer. The compiler will bitch until you are done, which makes it easy :)

class BaseExc: public std::exception
{
public:
...
private:
std::string _message;
std::string _stackTrace;
};

Really, the difficulty would not be at the OpenEXR end, but at the client software end, where there is a lot of code that tries to mess with the exception on the assumption that it is actually a string. As an alternative to modifying client code, I had a go at making BaseExc fully interoperable with string without being a string, which turns out to be a freak load of work and doesn't solve the problem in the end, because the code that wants Exc to be a string has got complicated data structures that require polymorphic compatibility with string, so it's best just to do the simple thing and fix the client code wherever the issue comes up. Since this is an EXR 2.0 we're working on here, maybe we can move ahead with this fix...

@sebastianelsner
Copy link
Author

I would not worry about the client code of The Foundry, SideFX or
Autodesk since they are all using VS 2010 by now for their products. So
they must have "fixed" it already (at least SideFX has the same
workaround). I guess since this breaks a lot of things it could be
included in version 2.2?
Thanks for the starting point, I will try to make it work the way you
suggested.

On 01/22/2014 06:34 AM, Nick Porcino wrote:

As I recall, it was really straight forward. Basically make the
following modification, and everywhere a string is assigned or
referenced (for example from what()), make it operate on _message
instead of the this pointer. The compiler will bitch until you are
done, which makes it easy :)

class BaseExc: public std::exception
{
public:
...
private:
std::string _message;
std::string _stackTrace;
};

Really, the difficulty would not be at the OpenEXR end, but at the
client software end, where there is a lot of code that tries to mess
with the exception on the assumption that it is actually a string. As
an alternative to modifying client code, I had a go at making BaseExc
fully interoperable with string without being a string, which turns
out to be a freak load of work and doesn't solve the problem in the
end, because the code that wants Exc to be a string has got
complicated data structures that require polymorphic compatibility
with string, so it's best just to do the simple thing and fix the
client code wherever the issue comes up. Since this is an EXR 2.0
we're working on here, maybe we can move ahead with this fix...


Reply to this email directly or view it on GitHub
#86 (comment).

check out www.pointcloud9.com

Sebastian Elsner - Pipeline Technical Director - RISE

t: +49 30 20180300 florian@risefx.com
f: +49 30 61651074 www.risefx.com

RISE FX GmbH
Schlesische Strasse 28, Aufgang B, 10997 Berlin
c/o action concept, An der Hasenkaule 1-7, 50354 Hürth
Geschaeftsfuehrer: Sven Pannicke, Robert Pinnow
Handelsregister Berlin HRB 106667 B

@sebastianelsner
Copy link
Author

I have added all the changes you suggested, it compiles, but the warning:
warning C4275: non dll-interface class 'Iex_2_1::BaseExc' used as base for dll-interface class 'Iex_2_1::ArgExc'
(and others) still won't go away.

@meshula
Copy link
Contributor

meshula commented Jan 25, 2014

The next step is to adorn BaseExc.
Remove all of the IEX_EXPORT in IexBase on the individual members.
Add one for

class IEX_EXPORT BaseExc ...

and you should be good!

@sebastianelsner
Copy link
Author

Still: warning C4251: 'Iex_2_1::BaseExc::_message' : class 'std::basic_string<_Elem,_Traits,_Ax>' needs to have dll-interface to be used by clients of class 'Iex_2_1::BaseExc'

I read a bit about the problem, (http://www.microsoft-questions.com/microsoft/VC-Language/30952961/a-solution-to-warning-c4251--class-needs-to-have-dllinterface.aspx), but the fix is still in the dark for me.

@meshula
Copy link
Contributor

meshula commented Jan 28, 2014

In the case of std::string, this warning is harmless, because IIRC MS removed the static member variables from the string implementation in VS2008.

That said, are you compiling your project with the multithreaded dll option? That's necessary. Also, once you have made sure you are running/linking with the DLL version of the MSVC libraries, is the _DLL macro defined for you at runtime? I don't recall if the compiler sets it automatically, if it doesn't, you'll have to set it in the preprocess section of the vcproj.

If you are running MT DLL, and _DLL is set, and you get a warning, you should nonetheless be able to run successfully without crashing when an exception is thrown.

@meshula
Copy link
Contributor

meshula commented Feb 12, 2014

Here are two more reasons to finish this modification. The issue described in the alembic post is the same, as is this issue from the exr mailing list. The problem stems from the members of the base exception being exported, not the class definition.

https://groups.google.com/forum/#!searchin/alembic-discussion/Cannot$20build$20windows/alembic-discussion/OMOvhg0Nv7o/VnJRrByVv10J

Date: Wed, 12 Feb 2014 18:50:21 +1100
From: federicon@al.com.au
To: openexr-devel@nongnu.org
CC: federicon@al.com.au
Subject: [Openexr-devel] pyilmbase problems importing imath

Hi

Hope this is the forum to ask.

I'm building pyilmbase 1.0.0 under llinux Centos 6.2, with ilmbase 1.0.3, gcc4.1.2 and boost 1.44.0 (also tried all combinations of the newer version of pyilmbase 2.0.0/1 against ilmbase 2.0 with newer compiler gcc446, and several flavours boost.

In all cases I can build successfully and import iex just fine, but importing imath does SegFault, it does crashed on line 241 of PyIex.h

240│ const TypeTranslator<IEX_NAMESPACE::BaseExc>::ClassDesc *baseDesc = baseExcTranslator().template findClassDesc(baseExcTranslator().firstClassDesc());
241├> std::string baseName = baseDesc->typeName();
242│ std::string baseModule = baseDesc->moduleName();

baseDesc ends up with a nullPointer (const PyIex::TypeTranslator<Iex_2_0::BaseExc>::ClassDesc *) 0x0

As importing iex works fine, I changed the code in imathmodule.cpp to kind of copy how the Exception gets registered.

< PyIex::registerExcImath::NullVecExc,Iex::MathExc("NullVecExc","imath");
< PyIex::registerExcImath::NullQuatExc,Iex::MathExc("NullQuatExc","imath");
< PyIex::registerExcImath::SingMatrixExc,Iex::MathExc("SingMatrixExc","imath");
< PyIex::registerExcImath::ZeroScaleExc,Iex::MathExc("ZeroScaleExc","imath");

< PyIex::registerExcImath::IntVecNormalizeExc,Iex::MathExc("IntVecNormalizeExc","imath");

PyIex::registerExc<Imath::NullVecExc,Iex::BaseExc>("NullVecExc","iex");
PyIex::registerExc<Imath::NullQuatExc,Iex::BaseExc>("NullQuatExc","iex");
PyIex::registerExc<Imath::SingMatrixExc,Iex::BaseExc>("SingMatrixExc","iex");
PyIex::registerExc<Imath::ZeroScaleExc,Iex::BaseExc>("ZeroScaleExc","iex");
PyIex::registerExc<Imath::IntVecNormalizeExc,Iex::BaseExc>("IntVecNormalizeExc","iex");

This way I can import imath, and in python land I can see that the imath module contains the registered exceptions and I can raise/try/catch them. but this code change seems fishy since nothing else seems to have reported and the code in the latest version remains the same.

I saw someone else did the same change (back in 2012) https://groups.google.com/forum/#!searchin/alembic-discussion/Cannot$20build$20windows/alembic-discussion/OMOvhg0Nv7o/VnJRrByVv10J

but I'm pretty sure I am missing something more obvious

also it caught my attention that I get this warning on gdb

PyIex::TypeTranslator<Iex_2_0::BaseExc>::findClassDesc<Iex_2_0::MathExc> (this=0x6e6510, cd=0x6eaa00) at /scratch/federicon/git/rezExternalPackages/pyilmbase/2.0.0/build/2/pyilmbase-prefix/src/pyilmbase/PyIex/Py
IexTypeTranslator.h:262
(gdb) print cd->typeInfo()
warning: RTTI symbol not found for class 'PyIex::TypeTranslator<Iex_2_0::BaseExc>::ClassDescT<Iex_2_0::BaseExc>'

could it be related to the realtime type info? I made sure that ilmbase and pyilmbase were compiled wihout the -fno-rtti so it should be on by default

Hope someone can help me find the issue

Thanks in advance
Fede

@sebastianelsner
Copy link
Author

I have played around with the changes you proposed, but I do not feel confident about my c++ skills. For example there seem to be static functions left over, which I do not kown how to handle correctly....

@meshula
Copy link
Contributor

meshula commented Feb 14, 2014

I grepped the exception headers, which static functions do you mean?

@sebastianelsner
Copy link
Author

sorry for the late reply, busy times. I got the changes to work, but now openexr fails to compile because it does assumes in some places that exceptions are strings. For example:

REPLACE_EXC (e, "Cannot read image file "
            "\"" << fileName << "\". " << e);

This does not work. I tried to overload with:

std::string& operator<<(const BaseExc& rhs);
std::string& operator<<(const char *s);
std::string& operator<<(const std::string &s);
std::string& operator<<(std::stringstream &s);

but I cannot get it to work. Is this the correct approach?

@sebastianelsner
Copy link
Author

sorry for the noise, hit the wrong button.

@cary-ilm
Copy link
Member

Looking into the OpenEXR issue backlog. This appears to have been addressed in v2.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in the source code
Projects
None yet
Development

No branches or pull requests

4 participants