set_error_handler etc. in header-only mode

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

set_error_handler etc. in header-only mode

Fritz Mueller
I’m having some trouble getting set_error_handler() etc. working when using the 5.0 release in its now default header-only mode.

Looking into the sources a bit, get_static_error_handler() etc. are define in header-only mode as inlines, returning references to local statics.  I think this means each call could conceivably use different storage for those statics, and indeed, some instrumentation indicates that this is what is happening (if I log the address of _error_handler within get_standard_error_hander(), I see multiple distinct addresses during a run of my program.

This doesn’t seem like it is going to work correctly...  Is there a plan for addressing set_error_handler() et. al. in header-only mode, as non-header-only mode is now deprecated?

   thanks much,
     —FritzM.



--
You are currently subscribed to cgal-discuss.
To unsubscribe or access the archives, go to
https://sympa.inria.fr/sympa/info/cgal-discuss


Reply | Threaded
Open this post in threaded view
|

Re: set_error_handler etc. in header-only mode

Marc Glisse
On Sat, 7 Dec 2019, Fritz Mueller wrote:

> I’m having some trouble getting set_error_handler() etc. working when
> using the 5.0 release in its now default header-only mode.
>
> Looking into the sources a bit, get_static_error_handler() etc. are
> define in header-only mode as inlines, returning references to local
> statics.  I think this means each call could conceivably use different
> storage for those statics,

No. A static variable inside a function is a unique global object. It is
the usual way to simulate C++17 inline variables and get singletons.

> and indeed, some instrumentation indicates that this is what is
> happening (if I log the address of _error_handler within
> get_standard_error_hander(), I see multiple distinct addresses during a
> run of my program.

That seems very broken. I would almost suspect that you are on windows,
calling the function from several DLLs...

> This doesn’t seem like it is going to work correctly...  Is there a plan
> for addressing set_error_handler() et. al. in header-only mode, as
> non-header-only mode is now deprecated?

We need a testcase and details about your setup so we can reproduce the
issue first, as with most bugs.

--
Marc Glisse

--
You are currently subscribed to cgal-discuss.
To unsubscribe or access the archives, go to
https://sympa.inria.fr/sympa/info/cgal-discuss


Reply | Threaded
Open this post in threaded view
|

Re: set_error_handler etc. in header-only mode

Fritz Mueller
Hi Marc — thank you for the prompt reply!

>> Looking into the sources a bit, get_static_error_handler() etc. are define in header-only mode as inlines, returning references to local statics.  I think this means each call could conceivably use different storage for those statics,
>
> No. A static variable inside a function is a unique global object. It is the usual way to simulate C++17 inline variables and get singletons.

Okay, good to know this is true, even for inlines -- thanks.  I can do some experiments with a small bit of test code to verify that I understand correctly.

>> some instrumentation indicates that this is what is happening (if I log the address of _error_handler within get_standard_error_hander(), I see multiple distinct addresses during a run of my program.)
>
> That seems very broken. I would almost suspect that you are on windows, calling the function from several DLLs...

Well, OS X Catalina w/ clang 11, actually.  But my use case is somewhat complicated: compiling into a dylib which is being loaded as a module into node.js.

Thanks for the confirmation that it *should* work.  I’ll try some experiments with some small test programs, and also dig a little deeper to see if node/v8 might in fact be loading more than one instantiation of my module.

> We need a testcase and details about your setup so we can reproduce the issue first, as with most bugs.

I’ll make sure to have those in hand if I end up coming back around for another pass on this one!

        thanks again,
           —FritzM.


--
You are currently subscribed to cgal-discuss.
To unsubscribe or access the archives, go to
https://sympa.inria.fr/sympa/info/cgal-discuss


Reply | Threaded
Open this post in threaded view
|

Re: set_error_handler etc. in header-only mode

Fritz Mueller
>>> Looking into the sources a bit, get_static_error_handler() etc. are define in header-only mode as inlines, returning references to local statics.  I think this means each call could conceivably use different storage for those statics,
>>
>> No. A static variable inside a function is a unique global object. It is the usual way to simulate C++17 inline variables and get singletons.
>
> Okay, good to know this is true, even for inlines -- thanks.  I can do some experiments with a small bit of test code to verify that I understand correctly.

Well, what I’ve found with OS X clang so far is this:

Static variables inside a function are indeed unique if that function is inline, but seeming *not* if that function is static inline?  The CGAL functions in question are perhaps effectively static inline because the enclosing anonymous namespace.

Attached here is a shar archive of a small standalone test program that illustrates the issue; this calls an inline function which prints the address of a local static from two separate translation units.  Running this on my system shows two different addresses for the static, but if you edit to remove the anonymous namespace around the inline function the problem goes away.

# This is a shell archive.  Save it in a file, remove anything before
# this line, and then unpack it by entering "sh file".  Note, it may
# create directories; files and directories will be owned by you and
# have default permissions.
#
# This archive contains:
#
# get_static_int
# get_static_int/Makefile
# get_static_int/get_static_int.h
# get_static_int/main.cpp
# get_static_int/unit1.cpp
# get_static_int/unit1.h
# get_static_int/unit2.cpp
# get_static_int/unit2.h
#
echo c - get_static_int
mkdir -p get_static_int > /dev/null 2>&1
echo x - get_static_int/Makefile
sed 's/^X//' >get_static_int/Makefile << 'END-of-get_static_int/Makefile'
Xget_static_int:
X g++ main.cpp unit1.cpp unit2.cpp -o get_static_int
END-of-get_static_int/Makefile
echo x - get_static_int/get_static_int.h
sed 's/^X//' >get_static_int/get_static_int.h << 'END-of-get_static_int/get_static_int.h'
X#ifndef GET_STATIC_INT_H
X#define GET_STATIC_INT_H
X
X#include <iostream>
X
Xnamespace {
X
Xinline int& get_static_int()
X{
X    static int _static_int = 0;
X    std::cout << "&_static_int: " << &_static_int << std::endl;
X    return _static_int;
X}
X
X}
X
X#endif // !defined(GET_STATIC_INT_H)
END-of-get_static_int/get_static_int.h
echo x - get_static_int/main.cpp
sed 's/^X//' >get_static_int/main.cpp << 'END-of-get_static_int/main.cpp'
X#include "unit1.h"
X#include "unit2.h"
X
Xint main(int argc, const char *argv[])
X{
X    unit1_call_get_static_int();
X    unit2_call_get_static_int();
X    return -1;
X}
END-of-get_static_int/main.cpp
echo x - get_static_int/unit1.cpp
sed 's/^X//' >get_static_int/unit1.cpp << 'END-of-get_static_int/unit1.cpp'
X#include "get_static_int.h"
X
Xint& unit1_call_get_static_int()
X{
X    return get_static_int();
X}
END-of-get_static_int/unit1.cpp
echo x - get_static_int/unit1.h
sed 's/^X//' >get_static_int/unit1.h << 'END-of-get_static_int/unit1.h'
X#ifndef UNIT1_H
X#define UNIT1_H
X
Xint &unit1_call_get_static_int();
X
X#endif // !defined(UNIT1_H)
END-of-get_static_int/unit1.h
echo x - get_static_int/unit2.cpp
sed 's/^X//' >get_static_int/unit2.cpp << 'END-of-get_static_int/unit2.cpp'
X#include "get_static_int.h"
X
Xint& unit2_call_get_static_int()
X{
X    return get_static_int();
X}
END-of-get_static_int/unit2.cpp
echo x - get_static_int/unit2.h
sed 's/^X//' >get_static_int/unit2.h << 'END-of-get_static_int/unit2.h'
X#ifndef UNIT2_H
X#define UNIT2_H
X
Xint &unit2_call_get_static_int();
X
X#endif // !defined(UNIT2_H)
END-of-get_static_int/unit2.h
exit


--
You are currently subscribed to cgal-discuss.
To unsubscribe or access the archives, go to
https://sympa.inria.fr/sympa/info/cgal-discuss


Reply | Threaded
Open this post in threaded view
|

Re: set_error_handler etc. in header-only mode

Marc Glisse
On Sat, 7 Dec 2019, Fritz Mueller wrote:

>>>> Looking into the sources a bit, get_static_error_handler() etc. are define in header-only mode as inlines, returning references to local statics.  I think this means each call could conceivably use different storage for those statics,
>>>
>>> No. A static variable inside a function is a unique global object. It is the usual way to simulate C++17 inline variables and get singletons.
>>
>> Okay, good to know this is true, even for inlines -- thanks.  I can do some experiments with a small bit of test code to verify that I understand correctly.
>
> Well, what I’ve found with OS X clang so far is this:
>
> Static variables inside a function are indeed unique if that function is
> inline, but seeming *not* if that function is static inline?  The CGAL
> functions in question are perhaps effectively static inline because the
> enclosing anonymous namespace.

Uh, what is that anonymous namespace doing there? It defeats the purpose
of the inline functions. It seems that the namespace was there first, used
to hide some private stuff but not necessary, and people didn't notice it
when they added the header-only mode.

Thanks a lot for the detailed analysis!

If you want to make sure we don't forget to fix it, you can open an issue
in cgal's github with this information (or even better open a pull request
with a fix), although hopefully it will get fixed soon even without that.

--
Marc Glisse

--
You are currently subscribed to cgal-discuss.
To unsubscribe or access the archives, go to
https://sympa.inria.fr/sympa/info/cgal-discuss


Reply | Threaded
Open this post in threaded view
|

Re: set_error_handler etc. in header-only mode

Fritz Mueller
Hi Marc,

> If you want to make sure we don't forget to fix it, you can open an issue in cgal's github with this information (or even better open a pull request with a fix), although hopefully it will get fixed soon even without that.

I opened issue #4400 with an attached repro, and also put up PR #4401.

I just moved the handler/behaviour accessors outside the anonymous namespace in that PR for minimal impact, but sounds like maybe the anonymous namespace there should be gotten rid of entirely?

   cheers,
     --FritzM.


--
You are currently subscribed to cgal-discuss.
To unsubscribe or access the archives, go to
https://sympa.inria.fr/sympa/info/cgal-discuss