Boost C++ Libraries: Ticket #1711: Boost Serialization export facility broken on gcc 4.1, 4.2

Sohail Somani — Mon, 24 Mar 2008 18:25:32 GMT

I believe it fails on RC2 as well.

attachment set

Sohail Somani — Tue, 25 Mar 2008 05:55:56 GMT

attachment → weird.patch

This patch shows which instantiation is causing the problem.

Wed, 26 Mar 2008 18:03:06 GMT

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

Dave Abrahams — Fri, 28 Mar 2008 20:13:19 GMT

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?

Fri, 28 Mar 2008 20:18:49 GMT

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

Fri, 28 Mar 2008 20:19:15 GMT

Also, test_exported_text_archive was the failing test.

Dave Abrahams — Fri, 28 Mar 2008 21:13:55 GMT

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.

Fri, 28 Mar 2008 21:41:46 GMT

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.

Fri, 28 Mar 2008 21:46:58 GMT

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.

Fri, 28 Mar 2008 22:18:40 GMT

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 — Fri, 28 Mar 2008 23:10:50 GMT

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.

Fri, 28 Mar 2008 23:23:29 GMT

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?

Fri, 28 Mar 2008 23:50:56 GMT

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');

}

Fri, 28 Mar 2008 23:55:08 GMT

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)
$

Sat, 29 Mar 2008 00:04:13 GMT

And of course it is 14.6.9 not 14.6.7.

Sat, 29 Mar 2008 00:28:25 GMT

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

attachment set

Sat, 29 Mar 2008 02:47:29 GMT

attachment → header_ordering_fix.patch

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!

attachment set

Sat, 29 Mar 2008 03:01:37 GMT

attachment → header_ordering_fix.2.patch

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

Sat, 29 Mar 2008 04:07:52 GMT

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

anonymous — Sun, 30 Mar 2008 13:33:00 GMT

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.

Sun, 30 Mar 2008 15:18:47 GMT

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

status changed; resolution set

Dave Abrahams — Sun, 30 Mar 2008 16:18:04 GMT

status new → closed
resolution → fixed

(In [43949]) fixes #1711

Dave Abrahams — Sun, 30 Mar 2008 16:21:15 GMT

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.

status changed; resolution deleted

Dave Abrahams — Sun, 30 Mar 2008 16:30:47 GMT

status closed → reopened
resolution fixed

owner, status changed; cc set

Dave Abrahams — Sun, 30 Mar 2008 16:32:07 GMT

cc Robert Ramey added
owner changed from Robert Ramey to Beman Dawes
status reopened → new

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

Dave Abrahams — Sun, 30 Mar 2008 17:15:11 GMT

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 ;-)

Sun, 30 Mar 2008 17:55:17 GMT

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*

:-)

Sun, 30 Mar 2008 18:27:11 GMT

All tests passing for me on trunk

Dave Abrahams — Sun, 30 Mar 2008 18:28:50 GMT

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]

Sun, 30 Mar 2008 20:02:39 GMT

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.

status changed; resolution set

Robert Ramey — Sun, 07 Sep 2008 04:50:24 GMT

status new → closed
resolution → fixed

Fixed in 1.36

Robert Ramey