Can we rely on any guarantees based on memory layout and default destructor

[Fredrik Eriksson]

(cross "posted" from Stackoverflow)

A while ago we had this declaration (simplified):

Declaration 1

struct SomeType
{
// allocates Implementation in SomeType's TU
SomeType();
// ... other stuff
struct Implementation
{
std::string someAtribute1;
std::string someAtribute2;
}
std::auto_ptr< Implementation> pimpl;
};

Some time later we changed de declaration to this (simplified):

Declaration 2


class OtherNameImplementation;

struct SomeType
{
// allocates OtherNameImplementation in SomeType's TU
SomeType();
// ... other stuff
std::auto_ptr< OtherNameImplementation> pimpl;
};

Where `OtherNameImplementation` is defined in `SomeType`'s TU, and has the exact same definition as `SomeType::Implementation` had (in Declaration 1).

Somehow we missed that `SomeType` has no defined destructor, hence the compiler generated destructor is defined in the user's TU.

The question:

For TU's that has been compiled against 'Declaration 1' Is there any form of guarantees that the behavior is "correct" when, in runtime, it uses 'Declaration 2'. Standard, implementation guarantees, I'll take anything

There is two reasons that I can think of, that it at least seems to 'work':

1. The compiler generated destructor should do the "right thing" since the memory layout is the same for both the Implementation-types, even though the type's name has changed.

2. We haven't noticed any implications during our Test period (there are several hundreds of applications that has this dependency)


Our target platform is Solaris, and we use Sunstudio 12.


Somehow we missed this during code review, and I assume this is at least UB, and in a perfect world we would define a destructor for `SomeType` and recompile every TU that is dependent of this.

 

[Alf P. Steinbach]

(cross "posted" from Stackoverflow)

You mean multi-posted.

A while ago we had this declaration (simplified):

Declaration 1

struct SomeType
{
// allocates Implementation in SomeType's TU
SomeType();
// ... other stuff
struct Implementation
{
std::string someAtribute1;
std::string someAtribute2;
}
std::auto_ptr< Implementation> pimpl;
};

This is not the PIMPL idiom. The point of PIMPL is to remove header
dependency by not defining SomeType::Implementation in the header that
defines SomeType. So, what is the point of the "pimpl" pointer here?

By the way the code above is missing a semicolon.

Some time later we changed de declaration to this (simplified):

Declaration 2


class OtherNameImplementation;

struct SomeType
{
// allocates OtherNameImplementation in SomeType's TU
SomeType();
// ... other stuff
std::auto_ptr< OtherNameImplementation> pimpl;
};

Where `OtherNameImplementation` is defined in `SomeType`'s TU, and has the exact same definition
as `SomeType::Implementation` had (in Declaration 1).

Somehow we missed that `SomeType` has no defined destructor, hence the compiler generated
destructor is defined in the user's TU.

Ah, you saw that.

The question:

For TU's that has been compiled against 'Declaration 1' Is there any form of guarantees that
the behavior is "correct" when, in runtime, it uses 'Declaration 2'. Standard, implementation
guarantees, I'll take anything

No.

It will probably work.

But as soon as those TU's are recompiled you have a problem.

There is two reasons that I can think of, that it at least seems to 'work':

1. The compiler generated destructor should do the "right thing" since the memory layout is
the same for both the Implementation-types, even though the type's name has changed.

The compiler is not formally required to generate the same memory
layout, but most likely it will.

2. We haven't noticed any implications during our Test period (there are several hundreds of
applications that has this dependency)

Apps using the new header will probably leak memory; test for that.

Our target platform is Solaris, and we use Sunstudio 12.


Somehow we missed this during code review, and I assume this is at least UB, and in a perfect
world we would define a destructor for `SomeType` and recompile every TU that is dependent
of this.

Do that.

What on Earth has prevented that from happening automatically?


Cheers & hth.,

- Alf

 

[Fredrik Eriksson]

A while ago we had this declaration (simplified):

This is not the PIMPL idiom. The point of PIMPL is to remove header
dependency by not defining SomeType::Implementation in the header that
defines SomeType. So, what is the point of the "pimpl" pointer here?

Well, there are other benifits besides header dependency, such as the ability to change the definition of implementation without braking the ABI.

No.

It will probably work.

But as soon as those TU's are recompiled you have a problem.

Why? Lets say that we recompile "everyting" (which we are trying to avoid) why would there be a problem?

Do that.

What on Earth has prevented that from happening automatically?

That's a question that I don't really have an answer to. And I totally agree whith you. Hopefully we can and will do something about it.

 

[Fredrik Eriksson]

Apps using the new header will probably leak memory; test for that.

You're right. Thanks. It seems that we are better of to bite the bullet...

 

[Alf P. Steinbach]

Well, there are other benifits besides header dependency, such as the ability to change the definition of implementation without braking the ABI.


Why? Lets say that we recompile "everyting" (which we are trying to avoid) why would there be a problem?

In practice, because those TUs may/will then use the compiler-generated
destructor for `std::auto_ptr<Implementation>`, since Implementation is
an incomplete type in the Declaration 2 (snipped in the quoting above).

Formally, because instantiating `std::auto_ptr` for incomplete type is
Undefined Behavior.

The fix of defining a SomeType destructor is to walk a very fine line
between formal UB and in-practice can't-really-fail-to-work. I wouldn't
walk that line. I would either define a destructor and replace
`std::auto_ptr` with a raw pointer, or I would not define a destructor,
but would then replace `std::auto_ptr` with `std::unique_ptr` or
`std::shared_ptr`, which both allow correct delete behavior -- the
latter is easier.

That's a question that I don't really have an answer to.

Seeing your reply now I'm pretty sure it has to do with that binary ABI
point of view. ;-)

And I totally agree whith you. Hopefully we can and will do something about it.

Cheers,

- Alf

 

[Fredrik Eriksson]

But as soon as those TU's are recompiled you have a problem.

In practice, because those TUs may/will then use the compiler-generated
destructor for `std::auto_ptr<Implementation>`, since Implementation is
an incomplete type in the Declaration 2 (snipped in the quoting above).

It didn't occur to mig until after my post (which is quite embarrassing)

Formally, because instantiating `std::auto_ptr` for incomplete type is
Undefined Behavior.

The fix of defining a SomeType destructor is to walk a very fine line
between formal UB and in-practice can't-really-fail-to-work. I wouldn't
walk that line. I would either define a destructor and replace
`std::auto_ptr` with a raw pointer, or I would not define a destructor,
but would then replace `std::auto_ptr` with `std::unique_ptr` or
`std::shared_ptr`, which both allow correct delete behavior -- the
latter is easier.

If you don't define a destructor, how would std::unique_ptr or std::shared_ptr do the right thing, in the user's TU? Do the standard require them to depend on the signatur of "value_type's" destructor? Which would require a defined destructor with external linkage for the "Implementation-type" (wihch would detect errors like these link-time)?

Sun Studio is not even standard conformant to C++98, regarding stl (RougeWave). At least not out of the box. So we can't use those features anyway.

 

[Alf P. Steinbach]

If you don't define a destructor, how would std::unique_ptr or std::shared_ptr do the
right thing, in the user's TU? Do the standard require them to depend on the signatur
of "value_type's" destructor? Which would require a defined destructor with external
linkage for the "Implementation-type" (wihch would detect errors like these link-time)?

The way it works is quite simple. The smart pointer has at least one
constructor that takes a DELETER functor as additional argument. In the
SomeType default constructor definition the Implementation type is
complete, and the deleter functor can be supplied.

boost::shared_ptr had the reasonable deleter as default. I am not so
sure about std::shared_ptr and std::unique_ptr. Maybe with those you
have to explicitly supply the deleter.

I'm sorry, but while I have used boost::shared_ptr a lot I haven't used
these newfangled C++ standard library beasties: I know what they
support, but not the practical details.

Sun Studio is not even standard conformant to C++98, regarding stl (RougeWave).
At least not out of the box. So we can't use those features anyway.

I'm pretty sure you can always use boost::shared_ptr.

But it's just as easy, and more efficient, to define destructor and use
a raw pointer. ;-)

Just remember to then disable copying by making copy constructor and
copy assignment non-public.


Cheers & hth.,

- Alf

 

[Fredrik Eriksson]

If you don't define a destructor, how would std::unique_ptr or std::shared_ptr do the

The way it works is quite simple. The smart pointer has at least one
constructor that takes a DELETER functor as additional argument. In the
SomeType default constructor definition the Implementation type is
complete, and the deleter functor can be supplied.

Ok, I see.

I'm pretty sure you can always use boost::shared_ptr.

It's pretty imposible to compile anything with boost when using Sun's RougeWave stl implementation (or Sun's default configuration of RougeWave, rather). If you're using stlport it works pretty well I understand...

But it's just as easy, and more efficient, to define destructor and use
a raw pointer. ;-)

Just remember to then disable copying by making copy constructor and
copy assignment non-public.

It is leaning towards that we revert back to the original declaration, and recompile every domain that has compiled against the newer declaration. Hence we have no header or ABI decoupling And fix this in the next release cycle.

Thanks

/Fredrik