[Open Babel] Critical problem with openbabel-2

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[Open Babel] Critical problem with openbabel-2

Jean Bréfort-3
Hi,

I am currently writing a module using openbabel-2 for the Gnome Office
applications. My module is loaded with dlopen and openbabel fails
loading it's format files.
This is because, in such a case, resolution of symbols might come after
the calls of the constructors of static variables.
Attached patch solves the problem.

Best regards,
Jean Bréfort

obconversion.cpp.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Open Babel] Critical problem with openbabel-2

Geoffrey Hutchison
Hi Jean,

Thanks for the patch. Looks good to me -- hopefully we haven't killed  
the Windows build. Getting the module loading right for all platforms  
has been tricky. Unfortunately, static builds on UNIX and Cygwin  
still seem broken, but those seem to be small problems for most  
people. (I never received any sort of reply about fixing the bug on  
Cygwin and it's been around for months.)

Thanks
-Geoff

On Nov 14, 2005, at 1:54 PM, Jean Bréfort wrote:

> Hi,
>
> I am currently writing a module using openbabel-2 for the Gnome Office
> applications. My module is loaded with dlopen and openbabel fails
> loading it's format files.
> This is because, in such a case, resolution of symbols might come  
> after
> the calls of the constructors of static variables.
> Attached patch solves the problem.
>
> Best regards,
> Jean Bréfort
> <obconversion.cpp.patch>



-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
OpenBabel-discuss mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Open Babel] Critical problem with openbabel-2

Chris Morley-3
Geoffrey Hutchison wrote:
> Thanks for the patch. Looks good to me -- hopefully we haven't killed  
> the Windows build.
I'm afraid it has.

Before Jean's alteration the format files were loaded
during the initialisation of a static variable
OBConversion::FormatFilesLoaded which called a static
function OBConversion::LoadFormatFiles().
Jean has moved the call of this function to the
OBConversion constructor. (Which in fact was the way it
was until a couple of months ago.)

Unfortunately, the Windows GUI accesses the static members
of OBConversion before an instance has been constructed,
in order to populate its list of available formats, so
that no formats are seen with the altered version.
I think setting the static members during initialisation
rather than during instance construction is a cleaner
design because each instance contains specific information
(in and out formats and streams etc.) A meaningless
dummy instance would have to be made in order to get a
list of the formats.

I don't recognize the header file <dlfcn.h> but it
shouldn't be in obconversion.cpp. If it is a UNIX system
header it should be in dlhandler_unix.cpp and if it
connected with the program calling OB it should be there.
The new OB framework was designed to keep the dependencies
between parts as small as possible. (The keyboard and GUI
interfaces could be used to convert any kind of files
(image, audio?) if somebody could be bothered to write an
API and some formats.) I don't know how OB how Jean is
using OB as a shared library, but let me describe how it
is used with Windows DLLs; forgive me if this is not relevant.

There are two DLLs made from obconversion.cpp +
dlhandler_xxx.cpp and the rest of the API respectively.
Each has no dependencies on any other part of OB. The
formats are in several DLLs, either singly or in groups,
which depend on the Conversion and API DLLs. The calling
program, which might be a keyboard or other interface, or
a more substantial program can be aware of only the
Conversion DLL (i.e. #include only obconversion.h) if its
only purpose is conversion. If it needs the API it can
#include mol.h and call the API DLL. The Conversion and
API DLLs are loaded implicitly at startup and the format
DLLs are loaded explicitly as discussed above.

This has not caused any difficulties during loading in
Windows and I would have expected that a UNIX system would
have been better designed. However there is a well-known
problem in C++ in that the initialisation of static
variables is done in an arbitary order and this can bite
unexpectedly. Maybe we've been lucky with the Windows
system and/or maybe this is connected with Jean's problem.

We could live with the kludge of a dummy OBConversion
object, but it would be better to keep the structure of
the program clean if we can.

Chris

 > Getting the module loading right for all platforms

> has been tricky. Unfortunately, static builds on UNIX and Cygwin  still
> seem broken, but those seem to be small problems for most  people. (I
> never received any sort of reply about fixing the bug on  Cygwin and
> it's been around for months.)
>
> On Nov 14, 2005, at 1:54 PM, Jean Bréfort wrote:
>> I am currently writing a module using openbabel-2 for the Gnome Office
>> applications. My module is loaded with dlopen and openbabel fails
>> loading it's format files.
>> This is because, in such a case, resolution of symbols might come  after
>> the calls of the constructors of static variables.
>> Attached patch solves the problem.
>>
>> Best regards,
>> Jean Bréfort
>> <obconversion.cpp.patch>
>
>
>
>
> -------------------------------------------------------
> SF.Net email is sponsored by:
> Tame your development challenges with Apache's Geronimo App Server.
> Download
> it for free - -and be entered to win a 42" plasma tv or your very own
> Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
> _______________________________________________
> OpenBabel-discuss mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
>



-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
<a href="http://ads.osdn.com/?ad_idv28&alloc_id845&op=click">http://ads.osdn.com/?ad_idv28&alloc_id845&op=click
_______________________________________________
OpenBabel-discuss mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Open Babel] Critical problem with openbabel-2

Jean Bréfort-3
Le mardi 15 novembre 2005 à 09:18 +0000, Chris Morley a écrit :

> Geoffrey Hutchison wrote:
> > Thanks for the patch. Looks good to me -- hopefully we haven't killed  
> > the Windows build.
> I'm afraid it has.
>
> Before Jean's alteration the format files were loaded
> during the initialisation of a static variable
> OBConversion::FormatFilesLoaded which called a static
> function OBConversion::LoadFormatFiles().
> Jean has moved the call of this function to the
> OBConversion constructor. (Which in fact was the way it
> was until a couple of months ago.)
>
> Unfortunately, the Windows GUI accesses the static members
> of OBConversion before an instance has been constructed,
> in order to populate its list of available formats, so
> that no formats are seen with the altered version.
> I think setting the static members during initialisation
> rather than during instance construction is a cleaner
> design because each instance contains specific information
> (in and out formats and streams etc.) A meaningless
> dummy instance would have to be made in order to get a
> list of the formats.

I proposed the change only because it did NOT work for me in some
instances. Loading modules during static variable initialization is not
a good idea on linux at least. The initialization takes place before
resolution of all symbols and even with RTLD_LAZY, things fail
(unresolved symbols). This might be a glibc bug, but we wmust live with
it.

If you really need an initialization of formats before creating an
OBConversion instance, it would be more protable to add an explicit
initialization function. Something as:

void Openbabel::Initialize ()
{
        OBConversion::LoadFormatFiles();
}

so that applications needing these before the creation of the first
OBConversion instance might work too.

> I don't recognize the header file <dlfcn.h> but it
> shouldn't be in obconversion.cpp. If it is a UNIX system
> header it should be in dlhandler_unix.cpp and if it
> connected with the program calling OB it should be there.
> The new OB framework was designed to keep the dependencies
> between parts as small as possible. (The keyboard and GUI
> interfaces could be used to convert any kind of files
> (image, audio?) if somebody could be bothered to write an
> API and some formats.) I don't know how OB how Jean is
> using OB as a shared library, but let me describe how it
> is used with Windows DLLs; forgive me if this is not relevant.
>
> There are two DLLs made from obconversion.cpp +
> dlhandler_xxx.cpp and the rest of the API respectively.
> Each has no dependencies on any other part of OB. The
> formats are in several DLLs, either singly or in groups,
> which depend on the Conversion and API DLLs. The calling
> program, which might be a keyboard or other interface, or
> a more substantial program can be aware of only the
> Conversion DLL (i.e. #include only obconversion.h) if its
> only purpose is conversion. If it needs the API it can
> #include mol.h and call the API DLL. The Conversion and
> API DLLs are loaded implicitly at startup and the format
> DLLs are loaded explicitly as discussed above.
>
> This has not caused any difficulties during loading in
> Windows and I would have expected that a UNIX system would
> have been better designed. However there is a well-known
> problem in C++ in that the initialisation of static
> variables is done in an arbitary order and this can bite
> unexpectedly. Maybe we've been lucky with the Windows
> system and/or maybe this is connected with Jean's problem.
>
> We could live with the kludge of a dummy OBConversion
> object, but it would be better to keep the structure of
> the program clean if we can.
>
> Chris
>
>  > Getting the module loading right for all platforms
> > has been tricky. Unfortunately, static builds on UNIX and Cygwin  still
> > seem broken, but those seem to be small problems for most  people. (I
> > never received any sort of reply about fixing the bug on  Cygwin and
> > it's been around for months.)
> >
> > On Nov 14, 2005, at 1:54 PM, Jean Bréfort wrote:
> >> I am currently writing a module using openbabel-2 for the Gnome Office
> >> applications. My module is loaded with dlopen and openbabel fails
> >> loading it's format files.
> >> This is because, in such a case, resolution of symbols might come  after
> >> the calls of the constructors of static variables.
> >> Attached patch solves the problem.
> >>
> >> Best regards,
> >> Jean Bréfort
> >> <obconversion.cpp.patch>
> >
> >
> >
> >
> > -------------------------------------------------------
> > SF.Net email is sponsored by:
> > Tame your development challenges with Apache's Geronimo App Server.
> > Download
> > it for free - -and be entered to win a 42" plasma tv or your very own
> > Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
> > _______________________________________________
> > OpenBabel-discuss mailing list
> > [hidden email]
> > https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
> >
>
>
>
> -------------------------------------------------------
> This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
> Register for a JBoss Training Course.  Free Certification Exam
> for All Training Attendees Through End of 2005. For more info visit:
> <a href="http://ads.osdn.com/?ad_idv28&alloc_id845&op=click">http://ads.osdn.com/?ad_idv28&alloc_id845&op=click
> _______________________________________________
> OpenBabel-discuss mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
>



-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
<a href="http://ads.osdn.com/?ad_idv28&alloc_id845&op=click">http://ads.osdn.com/?ad_idv28&alloc_id845&op=click
_______________________________________________
OpenBabel-discuss mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Open Babel] Critical problem with openbabel-2

Jean Bréfort-3
Le mardi 15 novembre 2005 à 11:57 +0100, Jean Bréfort a écrit :

> Le mardi 15 novembre 2005 à 09:18 +0000, Chris Morley a écrit :
> > Geoffrey Hutchison wrote:
> > > Thanks for the patch. Looks good to me -- hopefully we haven't killed  
> > > the Windows build.
> > I'm afraid it has.
> >
> > Before Jean's alteration the format files were loaded
> > during the initialisation of a static variable
> > OBConversion::FormatFilesLoaded which called a static
> > function OBConversion::LoadFormatFiles().
> > Jean has moved the call of this function to the
> > OBConversion constructor. (Which in fact was the way it
> > was until a couple of months ago.)
> >
> > Unfortunately, the Windows GUI accesses the static members
> > of OBConversion before an instance has been constructed,
> > in order to populate its list of available formats, so
> > that no formats are seen with the altered version.
> > I think setting the static members during initialisation
> > rather than during instance construction is a cleaner
> > design because each instance contains specific information
> > (in and out formats and streams etc.) A meaningless
> > dummy instance would have to be made in order to get a
> > list of the formats.
>
> I proposed the change only because it did NOT work for me in some
> instances. Loading modules during static variable initialization is not
> a good idea on linux at least. The initialization takes place before
> resolution of all symbols and even with RTLD_LAZY, things fail
> (unresolved symbols). This might be a glibc bug, but we wmust live with
> it.
>
> If you really need an initialization of formats before creating an
> OBConversion instance, it would be more protable to add an explicit
> initialization function. Something as:
>
> void Openbabel::Initialize ()
> {
> OBConversion::LoadFormatFiles();
> }
>
> so that applications needing these before the creation of the first
> OBConversion instance might work too.

There is of course another way to do things:

#ifdef _WIN32
  int OBConversion::FormatFilesLoaded = OBConversion::LoadFormatFiles();
#else
  int OBConversion::FormatFilesLoaded = 0;
#endif

OBFormat* OBConversion::pDefaultFormat=NULL;

OBConversion::OBConversion(istream* is, ostream* os) :
        pInFormat(NULL),pOutFormat(NULL), Index(0), StartNumber(1),
        EndNumber(0), Count(-1), m_IsLast(true), MoreFilesToCome(false),
        OneObjectOnly(false), pOb1(NULL), pAuxConv(NULL)
{
        pInStream=is;
        pOutStream=os;

#ifndef _WIN32
        if (FormatFilesLoaded == 0)
                FormatFilesLoaded = LoadFormatFiles();
#endif
       
        //These options take a parameter
        RegisterOptionParam("f", NULL, 1,GENOPTIONS);
        RegisterOptionParam("l", NULL, 1,GENOPTIONS);
}

Cheers,
Jean




-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
<a href="http://ads.osdn.com/?ad_idv28&alloc_id845&op=click">http://ads.osdn.com/?ad_idv28&alloc_id845&op=click
_______________________________________________
OpenBabel-discuss mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Open Babel] Critical problem with openbabel-2

Geoffrey Hutchison

On Nov 15, 2005, at 6:58 AM, Jean Bréfort wrote:

> There is of course another way to do things:
>
> #ifdef _WIN32
>   int OBConversion::FormatFilesLoaded =  
> OBConversion::LoadFormatFiles();
> #else
>   int OBConversion::FormatFilesLoaded = 0;
> #endif

I think this may be the best near-term solution. Chris, is this OK  
with you? We can sort out a cleaner solution after 2.0 if needed.  
(For example, I'd like to figure out why the static builds are dying.)

-Geoff

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
<a href="http://ads.osdn.com/?ad_idv28&alloc_id845&op=click">http://ads.osdn.com/?ad_idv28&alloc_id845&op=click
_______________________________________________
OpenBabel-discuss mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [Open Babel] Critical problem with openbabel-2

Chris Morley-3
Geoffrey Hutchison wrote:

>
> On Nov 15, 2005, at 6:58 AM, Jean Bréfort wrote:
>
>> There is of course another way to do things:
>>
>> #ifdef _WIN32
>>   int OBConversion::FormatFilesLoaded =  OBConversion::LoadFormatFiles();
>> #else
>>   int OBConversion::FormatFilesLoaded = 0;
>> #endif
>
>
> I think this may be the best near-term solution. Chris, is this OK  with
> you? We can sort out a cleaner solution after 2.0 if needed.  (For
> example, I'd like to figure out why the static builds are dying.)
>
It would be better to reduce the amount of platform
dependent code. Let's go with Jean's altered version
(without the #include < dflcn.h>) and I'll add a dummy
OBConversion object to the Windows GUI.

Chris


-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.  Get Certified Today
Register for a JBoss Training Course.  Free Certification Exam
for All Training Attendees Through End of 2005. For more info visit:
<a href="http://ads.osdn.com/?ad_idv28&alloc_id845&op=click">http://ads.osdn.com/?ad_idv28&alloc_id845&op=click
_______________________________________________
OpenBabel-discuss mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
Loading...