AcademySoftwareFoundation / openexr

The OpenEXR project provides the specification and reference implementation of the EXR file format, the professional-grade image storage format of the motion picture industry.
http://www.openexr.com/
BSD 3-Clause "New" or "Revised" License
1.63k stars 616 forks source link

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

Closed sebastianelsner closed 5 years ago

sebastianelsner commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 https://github.com/openexr/openexr/issues/86#issuecomment-32994772.

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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::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::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::findClassDesc (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::ClassDescT'

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 commented 10 years ago

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 commented 10 years ago

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

sebastianelsner commented 10 years ago

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 commented 10 years ago

sorry for the noise, hit the wrong button.

cary-ilm commented 5 years ago

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