Boost Serialization export facility broken on gcc 4.1, 4.2

[Sohail Somani]

The test libs/serialization/test/test_exported.cpp fails on these compilers. However, if you reorder export.hpp to be after the inclusion of the archive headers, it works. Changing base_object to void_cast_register doesn't help either (since one is syntactic sugar for the other)

This shouldn't be the case as the header-ordering requirement is no longer there but it seems that the correct code is not being instantiated on this compiler. I cannot figure out why.

Please see this thread for further information: ​http://article.gmane.org/gmane.comp.lib.boost.devel/172421

If you need me to try any changes, let me know. I think this should be fixed for 1.35 because it essentially means that a very important part of the serialization lib does not work.

If we suggest the header-ordering workaround, I would really stress that we understand *why* this is happening.

[Sohail Somani]

I believe it fails on RC2 as well.

[Sohail Somani]

This patch shows which instantiation is causing the problem.

[Sohail Somani <boost-trac@…>]

Adding a comment using ID so I get cc'ed hopefully.

[Dave Abrahams]

bjam gcc test_exported_text_archive is passing for me in the Boost trunk with gcc 4.1.3 on ubuntu 7.10

Which test exactly is failing, please?

[Sohail Somani <boost-trac@…>]

The reason the test is passing now is because Robert changed the header order to see if it would fix other failures.

See ​http://svn.boost.org/trac/boost/changeset/43806

[Sohail Somani <boost-trac@…>]

Also, test_exported_text_archive was the failing test.

[Dave Abrahams]

OK, let me have a quick look at this. If it's a hard problem it's very unlikely I'll have time for a fix that would be accepted before 1.35 goes out, if a fix is even possible. If you're still of the opinion that the code shouldn't work on any compiler as written, could you please explain in a little more detail? It's been over a year since I've thought about how this stuff works.

Thanks.

[Sohail Somani <boost-trac@…>]

I am not of the opinion that it shouldn't work on any compiler as written. I think it *should* work.

If you look at the patch that I've attached, it shows you which definition is affecting the ordering. I feel that it should work but when all the instantiation magic is happening, it is choosing the int overload rather than the Archive overload.

[Sohail Somani <boost-trac@…>]

Replying to Sohail Somani <boost-trac@taggedtype.net>:

I feel that it should work but when all the instantiation magic is happening, it is choosing the int overload rather than the Archive overload.

The int overload of instantiate_ptr_serialization, that is.

[Sohail Somani <boost-trac@…>]

Replying to Sohail Somani <boost-trac@taggedtype.net>:

Replying to Sohail Somani <boost-trac@taggedtype.net>:

I feel that it should work but when all the instantiation magic is happening, it is choosing the int overload rather than the Archive overload.

The int overload of instantiate_ptr_serialization, that is.

This is the case. If you remove the inlined empty definition of instantiate_ptr_serialization, there is a link error referencing the int instantiations. I think it is a g++ bug...

[Dave Abrahams]

Replying to Sohail Somani <boost-trac@taggedtype.net>:

Replying to Sohail Somani <boost-trac@taggedtype.net>:

Replying to Sohail Somani <boost-trac@taggedtype.net>:

I feel that it should work but when all the instantiation magic is happening, it is choosing the int overload rather than the Archive overload.

The int overload of instantiate_ptr_serialization, that is.

This is the case. If you remove the inlined empty definition of instantiate_ptr_serialization, there is a link error referencing the int instantiations. I think it is a g++ bug...

That part at least isn't a surprise. As the comment says, that version of the function is actually called.

[Sohail Somani <boost-trac@…>]

Oops... Is the int overload supposed to be called from guid_initializer<T>::export_guid?

The call to instantiate_ptr_serialization is a dependent lookup isn't it?

[Sohail Somani <boost-trac@…>]

Uh... Is this version of g++ hopelessly broken?

From the standard (section 14.6.7) #include <iostream>

using namespace std;

void f(char){cout << "f(char)" << endl;} template<class T> void g(T t) {

f(1); f(char)

dependent

f(T(1));

dependent

f(t);

}

void f(int){cout << "f(int)" << endl;}

int main() {

will cause one call of f(char) followed

g(2);

by two calls of f(int) will cause three calls of f(char)

g('a');

}

[Sohail Somani <boost-trac@…>]

Sorry my submit trigger finger went too early. Wow this is pretty messed up...

// From the standard (section 14.6.7)
#include <iostream>

using namespace std;

void f(char){cout << "f(char)" << endl;}
template<class T> void g(T t)
{
        f(1);                 // f(char)
                              
        f(T(1));              // dependent
                              // dependent
        f(t);
}

void f(int){cout << "f(int)" << endl;}

int main()
{
                              // will cause one call of f(char) followed
        g(2);                 // by two calls of f(int)
                              // will cause three calls of f(char)
        g('a');
}

$ g++ /tmp/cpp.cpp -o /tmp/wtf_broken
$ /tmp/wtf_broken 
f(char)
f(char)
f(char)
f(char)
f(char)
f(char)
$ 

[Sohail Somani <boost-trac@…>]

And of course it is 14.6.9 not 14.6.7.

[Sohail Somani <boost-trac@…>]

​http://gcc.gnu.org/bugzilla/show_bug.cgi?id=2922

[Sohail Somani <boost-trac@…>]

Here is a fix which forces dependent name lookup and uses ADL to instantiate the appropriate classes. I am very doubtful that this will work on any other compiler since I haven't tried!

[Sohail Somani <boost-trac@…>]

Doesn't use ADL. Don't know what I was thinking. Uses enable_if to instantiate the right stuff.

[Sohail Somani <boost-trac@…>]

The patch doesn't seem to work for a few of the tests :-( Ambiguous overloads and stuff...

[anonymous]

Replying to Sohail Somani <boost-trac@taggedtype.net>:

Oops... Is the int overload supposed to be called from guid_initializer<T>::export_guid?

The call to instantiate_ptr_serialization is a dependent lookup isn't it?

Yes, and yes.

See the comments at [browser:/trunk/boost/archive/detail/register_archive.hpp#L23] and [browser:/trunk/boost/archive/detail/register_archive.hpp#L29]. The problem, AFAICT, is that the 2nd overload there is not actually considered and thus its return type is never instantiated.

[Sohail Somani <boost-trac@…>]

I'm wondering if this is the reason: ​http://anubis.dkuug.dk/jtc1/sc22/wg21/docs/cwg_active.html#197

Since 14.6.4.2 temp.dep.candidate says that only Koenig lookup is done from the instantiation context, and since 3.4.2 basic.lookup.koenig says that fundamental types have no associated namespaces, either the example is incorrect (and f(int) will never be called) or the specification in 14.6.4.2 temp.dep.candidate is incorrect.

This gcc bug has a discussion about the above issue: ​http://gcc.gnu.org/bugzilla/show_bug.cgi?id=2922

[Dave Abrahams]

(In [43949]) fixes #1711

[Dave Abrahams]

Replying to dave:

(In [43949]) fixes #1711

Thanks for the inspiration, Sohail. in addition to being simpler, I believe this fix to be much, much safer than the attached patch; the chance of breaking another compiler by using it is next to nil. However, without your idea of forcing ADL I'm not sure it would have occurred to me.

[Dave Abrahams]

Reopening so Beman and Robert Ramey can decide together whether this is important enough to put in the 1.35 release.

[Dave Abrahams]

Replying to Sohail Somani <boost-trac@taggedtype.net>:

I'm wondering if this is the reason: ​http://anubis.dkuug.dk/jtc1/sc22/wg21/docs/cwg_active.html#197

Since 14.6.4.2 temp.dep.candidate says that only Koenig lookup is done from the instantiation context, and since 3.4.2 basic.lookup.koenig says that fundamental types have no associated namespaces, either the example is incorrect (and f(int) will never be called) or the specification in 14.6.4.2 temp.dep.candidate is incorrect.

Yes, I think you're right. Not having something in there to force ADL was my mistake. I'm a little surprised that I made it in the first place and also this worked anywhere before I made the patch. I guess my capacity to have issues with ADL is boundless ;-)

[Sohail Somani <boost-trac@…>]

Replying to dave:

Sometimes you have to be thankful that there is something that so anally standard conforming (g++) and an expert like yourself to fix these issues. Anyway, I'm glad I could be a muse.

The previously failing test is now passing. One other thing is that you might want to do is to revert test_exported.cpp to its prior state (where export.hpp came before anything else.)

Would be great to have this rolled into a 1.35.0 re-release.

*nudge* *nudge* *wink* *wink*

:-)

[Sohail Somani <boost-trac@…>]

All tests passing for me on trunk

[Dave Abrahams]

Replying to Sohail Somani <boost-trac@taggedtype.net>:

The previously failing test is now passing. One other thing is that you might want to do is to revert test_exported.cpp to its prior state (where export.hpp came before anything else.)

See [43953]

[Sohail Somani <boost-trac@…>]

Replying to dave:

Replying to Sohail Somani <boost-trac@taggedtype.net>:

The previously failing test is now passing. One other thing is that you might want to do is to revert test_exported.cpp to its prior state (where export.hpp came before anything else.)

See [43953]

Looks good. Thanks for looking into it.

[Robert Ramey]

Fixed in 1.36

Robert Ramey