C++ driver: strict aliasing warnings

[Pedro Larroy]

Hi

There are some strict aliasing warnings like this one:

mongo-cxx-driver/src/mongo/db/../util/optime.h:96:69: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]

        int& dataAsInt() {

            return *((int *) _data);

        }

I see this is only used in QueryMessage. 

Any reason not to return the data by value to avoid the warning and possible aliasing?

Pedro.

[Andy Schwerin]

Are you referring to the dataAsInt() in struct MsgData?  It cannot return by value, because its return value is indirectly used as an lvalue, elsewhere.  I believe that making _data in MsgData a union with int and char[4] members should make GCC aware of what's going on.  Can you file a ticket against "Core Server" (SERVER) at jira.mongodb.org?

-Andy

Pedro.

--
You received this message because you are subscribed to the Google Groups "mongodb-dev" group.
To view this discussion on the web visit https://groups.google.com/d/msg/mongodb-dev/-/8PEJEqNCSdQJ.
To post to this group, send email to mongo...@googlegroups.com.
To unsubscribe from this group, send email to mongodb-dev...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mongodb-dev?hl=en.

[Andy Schwerin]

Actually, I've filed said ticket, now.  https://jira.mongodb.org/browse/SERVER-5634.

-Andy

[Pedro Larroy]

The portable and safer alternative is doing something like this:

int asInt() {

        uint8_t* p = (uint8_t*)data;

        uint32_t res = 0;

        res = (p[3] << 24) | (p[2] << 16) | (p[1] << 8) | (p[0]);

        return static_cast<int>(res);

}

At -O2 and -O3 it gets optimized away and inlined, doesn't look like it will be slower than the previous cast.

Pedro.

-Andy

To unsubscribe from this group, send email to mongodb-dev+unsubscribe@googlegroups.com.

[Glenn Maynard]

On Tue, Apr 17, 2012 at 10:57 AM, Pedro Larroy <pedro.lar...@gmail.com> wrote:

The portable and safer alternative is doing something like this:

int asInt() {

        uint8_t* p = (uint8_t*)data;

        uint32_t res = 0;

        res = (p[3] << 24) | (p[2] << 16) | (p[1] << 8) | (p[0]);

        return static_cast<int>(res);

}

At -O2 and -O3 it gets optimized away and inlined, doesn't look like it will be slower than the previous cast.

This can't be used as an lvalue.

asInt() = 100

--
Glenn Maynard

[Per Ola Ingvarsson]

On Tue, Apr 17, 2012 at 5:39 PM, Andy Schwerin <schw...@10gen.com> wrote:
> Actually, I've filed said ticket, now.
>  https://jira.mongodb.org/browse/SERVER-5634.

I'm starting to sound like a broken record, but this is actually also
solved in https://github.com/skrabban/mongo-nonx86 .

It would be nice if the maintainers actually took a serious look at
this, because you would get the solution to SERVER-1625, SERVER-1811
and now also SERVER-5634.

Apart from that you would get a nice uniform way of casting, for example:

- int& dataAsInt() {
- return *((int *) _data);
+ little<int>& dataAsInt() {
+ return little<int>::ref( _data );
}

- return *reinterpret_cast< const int* >( value() );
+ return little<int>::ref( value() );

- return (reinterpret_cast < const PackedDouble* >(value ()))->d != 0
+ return little<double>::ref( value() ) != 0;

- *((int*)data) = size;
+ little<int>::ref( data ) = size;

- return (unsigned&) x[0];
+ return little<unsigned>::ref( x );

On x86/amd64 this will compile to the same as the original cast.

A lot of iterations have been made to make it as simple and uniform as possible.

I suggest you look at the master branch, but the v1.8 branch is the stable one.

/pi

[Pedro Larroy]

Hi Skrabban.

Thanks for your remarks. I see your repository has not been updated for a while, have you tried creating a pull request with updated contents?  If I'm not mistaken the non-x86 changes are for release v1.8 only?

I will have a look at your solution with the little template so maybe I can adapt it to the C++ driver I'm trying to use, since I'm using the one from the master branch as it has fixes that prevents me from using an older one.

We treat all warnings as errors in our project and I wouldn't like to be more liberal about it only for the mongo driver, anyway some warnings are coming also from headers, such as the strict aliasing topic discussed here. If it could actually produce a real bug by means of broken optimization etc.  Could the problem with strict aliasing really produce undefined behaviour?

Pedro.

On Tuesday, April 17, 2012 9:02:11 PM UTC+2, skrabban wrote:

[Per Ola Ingvarsson]

On Wed, Apr 18, 2012 at 12:12 PM, Pedro Larroy
<pedro.lar...@gmail.com> wrote:
> Hi Skrabban.
>
> Thanks for your remarks. I see your repository has not been updated for a
> while, have you tried creating a pull request with updated contents?  If I'm
> not mistaken the non-x86 changes are for release v1.8 only?

The master branch is currently also almost up to date, but there is
some issue in snappy.
Bson should be complete though.

> I will have a look at your solution with the little template so maybe I can
> adapt it to the C++ driver I'm trying to use, since I'm using the one from
> the master branch as it has fixes that prevents me from using an older one.
> We treat all warnings as errors in our project and I wouldn't like to be
> more liberal about it only for the mongo driver, anyway some warnings are
> coming also from headers, such as the strict aliasing topic discussed here.
> If it could actually produce a real bug by means of broken optimization etc.
>  Could the problem with strict aliasing really produce undefined behaviour?

Yes. You could get all sorts of results if you remove
-fno-strict-aliasing when the
project usually is built with that flag.

/pi

[Pedro Larroy]

Hi

Good point, so -fno-strict-aliasing could do as a workaround. Another option is using a union, and the more intrusive one is trying to port your endian agnostic changes.

Pedro.

[Per Ola Ingvarsson]

On Fri, Apr 20, 2012 at 11:42 AM, Pedro Larroy
<pedro.lar...@gmail.com> wrote:
> Hi
>

> Good point, so -fno-strict-aliasing could do as a workaround. Another option
> is using a union, and the more intrusive one is trying to port your endian
> agnostic changes.

Yes, but the union only solves this particular instance of the
warning. I think I saw more when I used -fstrict-aliasing.

/pi

[Pedro Larroy]

Is there a recommended way to test the driver after changes to see that it doesn't break it?

Pedro.

[Andy Schwerin]

On the master branch, you can run "scons clientBuild".  It will build mongod, and several client programs, and perform some basic tests.

The mongo shell, mongod and mongos instances also use the C++ client to communicate, so running the test suite (such as it is) can't hurt.  It can take a while to run everything, but a not-too-slow smoke test is to run "scons smoke smokeJs".  Might take about an hour.

-Andy

--

You received this message because you are subscribed to the Google Groups "mongodb-dev" group.

To view this discussion on the web visit https://groups.google.com/d/msg/mongodb-dev/-/ubp_GyIe1UIJ.

[Pedro Larroy]

Thanks.

I'll test next week, have a nice weekend.

Pedro.

-Andy

To unsubscribe from this group, send email to mongodb-dev+unsubscribe@googlegroups.com.